Single attestation "Full" implementation #7444
Conversation
eserilev
added
backwards-incompat
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
mergify
bot
added
the
waiting-on-author
…ngle-attestation-full-implementation
|
All required checks have passed and there are no merge conflicts. This pull request may now be ready for another review. |
mergify
bot
added
ready-for-review
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
|
This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏 |
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
|
This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
mergify
bot
added
ready-for-review
…ngle-attestation-full-implementation
| // Check to ensure that the attestation is "unaggregated". I.e., it has exactly one | ||
| // aggregation bit set. | ||
| let num_aggregation_bits = attestation.num_set_aggregation_bits(); | ||
| if num_aggregation_bits != 1 { | ||
| return Err(Error::NotExactlyOneAggregationBitSet(num_aggregation_bits)); |
There was a problem hiding this comment.
SingleAttestation objects are guaranteed to be unaggregated so this check is not needed
There was a problem hiding this comment.
| // [New in Electra:EIP7549] | ||
| verify_committee_index(attestation)?; | ||
|
|
There was a problem hiding this comment.
SingleAttestation objects are guaranteed to be unaggregated so no need for this check
There was a problem hiding this comment.
This check was initially introduced in Electra, but is removed for SingleAttestation
| // Check that the attester is a member of the committee and return `committees_per_slot` which | ||
| // is used in `verify_middle_checks` to calculate the expected `subnet_id` |
There was a problem hiding this comment.
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
| fn verify_late_checks( | ||
| attestation: AttestationRef<T::EthSpec>, | ||
| attestation: &'a SingleAttestation, | ||
| validator_index: u64, | ||
| subnet_id: Option<SubnetId>, | ||
| chain: &BeaconChain<T>, | ||
| ) -> Result<(), Error> { | ||
| ) -> Result<(Attestation<T::EthSpec>, SubnetId), Error> { | ||
| // Check that the attester is a member of the committee | ||
| let (committee_opt, committees_per_slot) = chain.with_committee_cache( | ||
| attestation.data.target.root, | ||
| attestation.data.slot.epoch(T::EthSpec::slots_per_epoch()), | ||
| |committee_cache, _| { | ||
| let committee_opt = committee_cache | ||
| .get_beacon_committee(attestation.data.slot, attestation.committee_index) | ||
| .map(|beacon_committee| beacon_committee.committee.to_vec()); | ||
|
|
||
| Ok((committee_opt, committee_cache.committees_per_slot())) | ||
| }, |
There was a problem hiding this comment.
In the spirit of the SingleAttestation changes, I've moved all checks that require access to the shuffling to after signature verification
There was a problem hiding this comment.
These checks are
- Committee exists for slot and index
- Attester is in the committee
- expected subnet id == provided subnet id
| if committee.contains(&(attestation.attester_index as usize)) { | ||
| return Err(Error::AttesterNotInCommittee { | ||
| attester_index: attestation.attester_index, | ||
| committee_index: attestation.committee_index, | ||
| slot: attestation.data.slot, | ||
| }); |
There was a problem hiding this comment.
| .inspect_unaggregate_err( | ||
| "attestation without any aggregation bits set", | ||
| |tester, mut a, _| { |
There was a problem hiding this comment.
not a valid test case for SingleAttestation
| ) | ||
| .inspect_unaggregate_err( | ||
| "attestation with two aggregation bits set", | ||
| |tester, mut a, _| { | ||
| match &mut a { |
There was a problem hiding this comment.
not a valid test case for SingleAttestation
| * The number of aggregation bits matches the committee size -- i.e. | ||
| * `len(attestation.aggregation_bits) == len(get_beacon_committee(state, data.slot, | ||
| * data.index))`. | ||
| */ | ||
| .inspect_unaggregate_err( | ||
| "attestation with invalid bitfield", | ||
| |_, mut a, _| { |
There was a problem hiding this comment.
not a valid test case for SingleAttestation
|
We now move checks that require access to the shuffling to AFTER signature verification. This seems in line with the spirit of the |
eserilev
added
ready-for-review
Issue Addressed
#6970
Proposed Changes
This allows for us to receive
SingleAttestationover gossip and process it without converting. There is still a conversion toAttestationas a final step in the attestation verification process, but by then theSingleAttestationis fully verified.I've also fully removed the
submitPoolAttestationsV1endpoint as its been deprecatedI've also pre-emptively deprecated supporting
AttestationinsubmitPoolAttestationsV2endpoint. See here for more info: ethereum/beacon-APIs#531I tried to the minimize the diff here by only making the "required" changes. There are some unnecessary complexities with the way we manage the different attestation verification wrapper types. We could probably consolidate this to one wrapper type and refactor this even further. We could leave that to a separate PR if we feel like cleaning things up in the future.
Note that I've also updated the test harness to always submit
SingleAttestationregardless of fork variant. I don't see a problem in that approach and it allows us to delete more code :)