fix: validate gossip aggregated attestations before storing#487
fix: validate gossip aggregated attestations before storing#487shaaibu7 wants to merge 7 commits intoleanEthereum:mainfrom
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.
| ), | ||
| ] | ||
|
|
||
| fork_choice_test(steps=steps) |
There was a problem hiding this comment.
No need for this, see the other tests, you can do directly something like this (and this should be the same for all the tests below):
| fork_choice_test(steps=steps) | |
| fork_choice_test(steps= | |
| [ | |
| [ | |
| 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)), | |
| ), | |
| ], | |
| GossipAggregatedAttestationStep( | |
| attestation=GossipAggregatedAttestationSpec( | |
| validator_ids=[ValidatorIndex(1)], | |
| slot=Slot(2), | |
| target_slot=Slot(2), | |
| target_root_label="block_2", | |
| ), | |
| checks=StoreChecks(head_slot=Slot(2)), | |
| ), | |
| ]) |
| source_root=Bytes32(b"\xff" * 32), | ||
| source_slot=Slot(999), | ||
| ), | ||
| valid=False, |
There was a problem hiding this comment.
See #484 (comment), I think this is nice to add the expected failure message that we get to be sure that we are testing the right thing and that the test is not failing for something else that we don't expect to happen.
| def test_invalid_attestation_data_rejected(self, key_manager: XmssKeyManager) -> None: | ||
| """ | ||
| Aggregated attestations fail if their data violates validate_attestation(). | ||
| """ | ||
| store, attestation_data = make_store_with_attestation_data( | ||
| key_manager, num_validators=4, validator_id=ValidatorIndex(0) | ||
| ) | ||
|
|
||
| invalid_source = attestation_data.source.model_copy( | ||
| update={"slot": Slot(int(attestation_data.target.slot) + 1)} | ||
| ) | ||
| invalid_data = attestation_data.model_copy(update={"source": invalid_source}) | ||
|
|
||
| proof = make_aggregated_proof(key_manager, [ValidatorIndex(1)], invalid_data) | ||
|
|
||
| signed_aggregated = SignedAggregatedAttestation( | ||
| data=invalid_data, | ||
| proof=proof, | ||
| ) | ||
|
|
||
| with pytest.raises(AssertionError, match="Source checkpoint slot must not exceed target"): | ||
| store.on_gossip_aggregated_attestation(signed_aggregated) |
There was a problem hiding this comment.
Since you have a test vector for each case, I think that adding this act just as duplicate no? Especially since I guess that here we are not testing all the scenarios but probably just one and so to test all the scenarios, we would need multiple unit tests which will be a real duplicate of the behavior of the test vectors you added.
| def test_invalid_attestation_data_rejected(self, key_manager: XmssKeyManager) -> None: | |
| """ | |
| Aggregated attestations fail if their data violates validate_attestation(). | |
| """ | |
| store, attestation_data = make_store_with_attestation_data( | |
| key_manager, num_validators=4, validator_id=ValidatorIndex(0) | |
| ) | |
| invalid_source = attestation_data.source.model_copy( | |
| update={"slot": Slot(int(attestation_data.target.slot) + 1)} | |
| ) | |
| invalid_data = attestation_data.model_copy(update={"source": invalid_source}) | |
| proof = make_aggregated_proof(key_manager, [ValidatorIndex(1)], invalid_data) | |
| signed_aggregated = SignedAggregatedAttestation( | |
| data=invalid_data, | |
| proof=proof, | |
| ) | |
| with pytest.raises(AssertionError, match="Source checkpoint slot must not exceed target"): | |
| store.on_gossip_aggregated_attestation(signed_aggregated) |
| # Generate signature or use dummy. | ||
| if spec.valid_signature: | ||
| signature = key_manager.sign_attestation_data( | ||
| spec.validator_id, | ||
| attestation_data, | ||
| ) | ||
| else: | ||
| signature = create_dummy_signature() | ||
|
|
||
| return SignedAttestation( | ||
| validator_id=spec.validator_id, | ||
| data=attestation_data, | ||
| signature=signature, | ||
| ) |
There was a problem hiding this comment.
I've checked locally and that is dead code no?
| # Generate signature or use dummy. | |
| if spec.valid_signature: | |
| signature = key_manager.sign_attestation_data( | |
| spec.validator_id, | |
| attestation_data, | |
| ) | |
| else: | |
| signature = create_dummy_signature() | |
| return SignedAttestation( | |
| validator_id=spec.validator_id, | |
| data=attestation_data, | |
| signature=signature, | |
| ) |
|
@tcoratger kindly review the updated changes |
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