fix: validate gossip aggregated attestations before storing#487
Conversation
There was a problem hiding this comment.
Looking pretty good!
One extra ask in addition to the 2 comments: The test you added is a unit test used for internal testing. It'd be great if you could add test vectors (the generated test vectors that are used by clients) as well. You can check out examples at https://github.com/leanEthereum/leanSpec/tree/main/tests/consensus/devnet. There should be 1 test vector per each of the validation rule.
Thanks a lot!
| data = signed_attestation.data | ||
| proof = signed_attestation.proof | ||
|
|
||
| self.validate_attestation(Attestation(validator_id=ValidatorIndex(0), data=data)) |
There was a problem hiding this comment.
We shouldn't be assuming the validator_id value in the spec here.
We can edit validate_attestation() so that it takes in AttestationData instead of Attestation since it's not using the validator_id anyway.
unnawut
added
specs
|
@unnawut kindly review the implemented changes |
| def _base_blocks() -> list[BlockStep]: | ||
| return [ | ||
| BlockStep( | ||
| block=BlockSpec(slot=Slot(1), label="block_1"), | ||
| checks=StoreChecks(head_slot=Slot(1)), | ||
| ), | ||
| BlockStep( | ||
| block=BlockSpec(slot=Slot(2), label="block_2"), | ||
| checks=StoreChecks(head_slot=Slot(2)), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
I think this is more readable for the reader is you put this directly into each test without a util function like this, even if this is more verbose, we love readability for the tests.
tests/consensus/devnet/fc/test_gossip_aggregated_attestation_validation.py
Outdated
Show resolved
Hide resolved
tests/consensus/devnet/fc/test_gossip_aggregated_attestation_validation.py
Outdated
Show resolved
Hide resolved
packages/testing/src/consensus_testing/test_fixtures/fork_choice.py
Outdated
Show resolved
Hide resolved
|
@tcoratger kindly review the updated changes |
|
@shaaibu7 Just one error from the newly added test and I think we're good to go! |
|
@unnawut made the fix kindly review |
|
thank you! |
3 participants
🗒️ Description
on_gossip_aggregated_attestation() now validates the aggregated data via validate_attestation() before validator lookup, leanVM proof verification, or payload storage, ensuring the gossip aggregate path enforces the same availability/topology/time invariants as individual attestations.
Added test_invalid_attestation_data_rejected to prove aggregated attestations that violate validate_attestation() (e.g., source.slot > target.slot) raise before latest_new_aggregated_payloads is mutated.
✅ Checklist
fixes #473