Skip to content

Improve comptests fork choice generator#5147

Draft
ericsson49 wants to merge 18 commits intoethereum:masterfrom
ericsson49:comptests/testgen-improvements
Draft

Improve comptests fork choice generator#5147
ericsson49 wants to merge 18 commits intoethereum:masterfrom
ericsson49:comptests/testgen-improvements

Conversation

@ericsson49
Copy link
Copy Markdown
Contributor

The PR adds various improvements to the comptests Fork Choice generator:

  • randomization improvements for block_tree and block_cover tests
  • implemented more consistency checks for block_tree and block_cover tests
  • gloas support for block_cover tests
  • more robust seed generation scheme
  • refactoring of the block_tree main generation loop

Related to #5107

@github-actions github-actions Bot added the testing CI, actions, tests, testing infra label Apr 23, 2026
@ericsson49 ericsson49 changed the title Comptests Fork Choice generator improvements Improve comptests Fork Choice generator Apr 23, 2026
Copy link
Copy Markdown
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- the refactor makes _generate_block_tree much easier to reason about. 👍 Scanned given the size, left a few nits, nothing blocking.

)


def _generate_block_tree(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup of the god‑function in _generate_block_tree. 👍

tips = sorted(block_tree_tips)
block_index_voters = {}
for validator_index in attesting_committee:
attesting_block_index = choose_attested_block_index(tips, block_parents)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an improvement to (the old)

attesters = split_list([i for i in range(validator_count)], len(attesting_tips))

That is, before: arbitrary partition of all validators across tips -- after: real beacon committees, then each member picks a random tip.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Additionally, a parent of a tip can be chosen by coin flip.

try:
run_on_attestation(spec, store, attestation, is_from_block=True, valid=True)
except AssertionError:
pass
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to catch all exceptions here and continue? Perhaps catch stale slot, already‑known, etc., and re‑raise everything else(?)

-- or log what was passed when debug=True?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped try/catch.
Generally, no exception should be raised here, or they'll manifest themselves in other part of the test.
But it won't hurt to let them be raised earlier.

return rnd.randint(0, max_count)

if mode == "edge":
candidates = [count for count in {threshold, threshold + 1} if 0 <= count <= max_voters]
Copy link
Copy Markdown
Member

@moodmosaic moodmosaic Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps worth swapping the {...} for (...) or [...]?

  • {threshold, threshold + 1} -- set, iteration order not guaranteed by the language.
  • (threshold, threshold + 1) -- tuple, order is exactly as written.
  • [threshold, threshold + 1] -- list, same.

so (in principle) this could affect which value rnd.choice returns for a given seed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Fixed

@jtraglia jtraglia changed the title Improve comptests Fork Choice generator Improve comptests fork choice generator Apr 24, 2026
@ericsson49
Copy link
Copy Markdown
Contributor Author

Thanks for the review, @moodmosaic !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing CI, actions, tests, testing infra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants