Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EIP7549 compute_on_chain_aggregate #5685

Conversation

eserilev
Copy link
Collaborator

@eserilev eserilev commented May 1, 2024

Issue Addressed

#5605

Proposed Changes

Electra Attestations that share equal AttestationData will now be aggregated before being submitted on chain. See spec for implementation details

Additional Info

Running through several test cases, I found situations where we had duplicate attestations being included in a beacon block. Aggregating duplicate attestations will cause InvalidSignature errors, so I added an additional check to skip duplicate attestations during the aggregation process.

@eserilev eserilev changed the title EIP7549 compute_on_chain_aggreagte EIP7549 compute_on_chain_aggregate May 1, 2024
@eserilev eserilev added electra Required for the Electra/Prague fork ready-for-review The code is ready for review labels May 1, 2024
@michaelsproul
Copy link
Member

I think this needs to consider the committee_bits:

/// Are the aggregation bitfields of these attestations disjoint?
pub fn signers_disjoint_from(&self, other: &Self) -> bool {
self.aggregation_bits
.intersection(&other.aggregation_bits)
.is_zero()
}

I think the logic for disjointness should be:

  • committee_bits are disjoint (fast check), OR
  • committee_bits overlap, but at all overlapping committees, the aggregation_bits for those committees are disjoint

Similarly this aggregate function should build the new list of committee bits and append all of the aggregation_bits in order:

pub fn aggregate(&mut self, other: &Self) {
debug_assert_eq!(self.data, other.data);
debug_assert!(self.signers_disjoint_from(other));
self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits);
self.signature.add_assign_aggregate(&other.signature);
}

@eserilev eserilev added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels May 3, 2024
@ethDreamer ethDreamer force-pushed the electra_attestation_changes branch from cd0f08b to 3a41e13 Compare May 3, 2024 18:08
@eserilev eserilev force-pushed the eip7549-compute-on-chain-aggregate branch from d8c1a3f to 494d7ad Compare May 5, 2024 12:19
@eserilev eserilev force-pushed the eip7549-compute-on-chain-aggregate branch from 5a647d0 to d455c1d Compare May 7, 2024 11:35
@eserilev eserilev closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electra Required for the Electra/Prague fork work-in-progress PR is a work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants