test: add consensus-critical justification test vectors#509
Conversation
Add 9 new justification tests covering previously untested consensus rules. All tests use the StateTransitionTestFiller pattern to generate JSON fixtures for client implementations. New tests: - Split aggregate merging within a single block - Same-block duplicate validator dedup - Odd validator set threshold boundary (4/5 pass, 3/5 fail) - Mismatched target root rejection - Isolated vote cleanup (only resolved target cleared) - Pending vote survival across finalization rebase - Stale vote pruning on finalization advance - Backward vote rejection (target <= source) Also enhances test_votes_accumulate_across_blocks with pending vote cleanup assertions, and renames test_exact_two_thirds_threshold_justifies to test_even_validator_threshold_boundary.
|
Hi @tcoratger and @unnawut, let me know if you have any feedback on this, and I can make adjustments to the changes. |
tcoratger
left a comment
tcoratger
left a comment
There was a problem hiding this comment.
Overall looks good, just some small comments! Thanks a lot!
| slot=Slot(2), | ||
| latest_justified_slot=Slot(0), | ||
| latest_finalized_slot=Slot(0), | ||
| justified_slots=JustifiedSlots(data=[]).model_copy(update={"data": [Boolean(False)]}), |
There was a problem hiding this comment.
We could have this no?
| justified_slots=JustifiedSlots(data=[]).model_copy(update={"data": [Boolean(False)]}), | |
| justified_slots=JustifiedSlots(data=[Boolean(False)]), |
There was a problem hiding this comment.
Thanks for flagging this @tcoratger.
Regarding the model_copy simplification, I realised that when I replace it with JustifiedSlots(data=[Boolean(False)]) the constructor ends up converting the list into a tuple, while the actual state keeps it as a list. The equality check then fails when I run the state transition tests using uv run pytest:
AssertionError: State validation failed: justified_slots = data=[Boolean(False)], expected data=(Boolean(False),)
To fix this, I can submit a separate PR, however, I was wondering which approach should be taken as the fix could either go in BaseBitlist._coerce_and_validate (changing return tuple(...) to return list(...) to match what model_copy produces), or in validate_against_state. Do you have a preference on which approach?
| justified_slots=JustifiedSlots(data=[]).model_copy( | ||
| update={ | ||
| "data": [ | ||
| Boolean(True), | ||
| Boolean(False), | ||
| Boolean(False), | ||
| ] | ||
| } | ||
| ), |
There was a problem hiding this comment.
Same thing here we can simplify this and I guess this is the case for other places as well
|
|
||
| Expected Behavior | ||
| ----------------- | ||
| 1. The two attestations do not merge because their attestation-data slots differ |
There was a problem hiding this comment.
We should remove this point because we never test it no?
| post=StateExpectation( | ||
| slot=Slot(2), | ||
| latest_justified_slot=Slot(1), | ||
| latest_finalized_slot=Slot(0), | ||
| ), |
There was a problem hiding this comment.
Why don't we test justifications_validators and justified_slots as well?
There was a problem hiding this comment.
Thanks @tcoratger, I have added post state checks for justifications_validators and justified_slots fields. This has been done for the tests that were missing them, as flagged in the review.
| post=StateExpectation( | ||
| slot=Slot(2), | ||
| latest_justified_slot=Slot(1), | ||
| latest_finalized_slot=Slot(0), | ||
| ), |
There was a problem hiding this comment.
Same here why don't we test justifications_validators and justified_slots as well?
| post=StateExpectation( | ||
| slot=Slot(2), | ||
| latest_justified_slot=Slot(0), | ||
| latest_finalized_slot=Slot(0), | ||
| ), |
There was a problem hiding this comment.
Same here? Why don't we test justifications_validators and justified_slots as well?
| post=StateExpectation( | ||
| slot=Slot(3), | ||
| latest_justified_slot=Slot(0), | ||
| latest_finalized_slot=Slot(0), | ||
| ), |
There was a problem hiding this comment.
Same here we should probably check the other stuffs right?
| justified_slots=JustifiedSlots(data=[]).model_copy( | ||
| update={"data": [Boolean(True), Boolean(False), Boolean(False)]} |
There was a problem hiding this comment.
Same here, no need to model copy
| slot=Slot(6), | ||
| latest_justified_slot=Slot(5), | ||
| latest_finalized_slot=Slot(4), | ||
| justified_slots=JustifiedSlots(data=[]).model_copy(update={"data": [Boolean(True)]}), |
| justified_slots=JustifiedSlots(data=[]).model_copy( | ||
| update={ | ||
| "data": [ | ||
| Boolean(True), | ||
| Boolean(False), | ||
| Boolean(False), | ||
| Boolean(True), | ||
| Boolean(False), | ||
| ] | ||
| } |
- Remove untested docstring claim about attestation-data slot merging - Add justified_slots, justifications_roots, and justifications_validators assertions to split aggregations, odd threshold, and mismatched root tests
2 participants
🗒️ Description
Add 9 new consensus-critical justification tests to
test_justification.py. All tests use theStateTransitionTestFillerpattern, generating JSON fixtures for client implementations.New tests:
test_repeated_validator_does_not_double_count_within_same_blocktest_pending_justification_survives_finalization_rebasetest_split_supermajority_aggregations_in_same_block_justifytest_odd_validator_threshold_boundary_justifiestest_odd_validator_threshold_boundary_does_not_justifytest_supermajority_with_mismatched_target_root_is_ignoredtest_justification_clears_only_the_resolved_target_votestest_finalization_prunes_stale_pending_votes_and_rebases_windowtest_target_at_or_before_source_is_ignoredAdditional changes:
test_votes_accumulate_across_blockswith pending vote cleanup assertions (justifications_roots,justifications_validators)test_exact_two_thirds_threshold_justifiestotest_even_validator_threshold_boundaryto pair with the new odd-validator counterpart🔗 Related Issues or PRs
N/A
✅ Checklist
toxchecks to avoid unnecessary CI fails:uvx tox