Improve comptests fork choice generator#5147
Improve comptests fork choice generator#5147ericsson49 wants to merge 18 commits intoethereum:masterfrom
Conversation
Fix debug helper failure
moodmosaic
left a comment
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nice catch! Fixed
|
Thanks for the review, @moodmosaic ! |
The PR adds various improvements to the comptests Fork Choice generator:
block_treeandblock_covertestsblock_treeandblock_covertestsblock_covertestsblock_treemain generation loopRelated to #5107