From b5b2c95c7a6f8736f8b9d258f60103b780d3e62c Mon Sep 17 00:00:00 2001 From: Povilas Liubauskas Date: Mon, 18 Nov 2024 12:13:19 +0200 Subject: [PATCH] Aggregate superset check fix: support Electra aggregates in superset checking --- fork_choice_control/src/mutator.rs | 8 +- fork_choice_store/src/misc.rs | 2 +- fork_choice_store/src/store.rs | 4 +- fork_choice_store/src/supersets.rs | 120 +++++++++++++++++++++++++---- 4 files changed, 110 insertions(+), 24 deletions(-) diff --git a/fork_choice_control/src/mutator.rs b/fork_choice_control/src/mutator.rs index f3bca16a..11f0f652 100644 --- a/fork_choice_control/src/mutator.rs +++ b/fork_choice_control/src/mutator.rs @@ -650,7 +650,7 @@ where Ok(AggregateAndProofAction::Accept { aggregate_and_proof, attesting_indices, - is_superset, + is_subset_aggregate, }) => { if let Some(metrics) = self.metrics.as_ref() { metrics.register_mutator_aggregate_and_proof(&["accepted"]); @@ -671,10 +671,10 @@ where let (gossip_id, sender) = origin.split(); if let Some(gossip_id) = gossip_id { - if is_superset { - P2pMessage::Accept(gossip_id).send(&self.p2p_tx); - } else { + if is_subset_aggregate { P2pMessage::Ignore(gossip_id).send(&self.p2p_tx); + } else { + P2pMessage::Accept(gossip_id).send(&self.p2p_tx); } } diff --git a/fork_choice_store/src/misc.rs b/fork_choice_store/src/misc.rs index 63ee2b2e..479e4710 100644 --- a/fork_choice_store/src/misc.rs +++ b/fork_choice_store/src/misc.rs @@ -548,7 +548,7 @@ pub enum AggregateAndProofAction { Accept { aggregate_and_proof: Arc>, attesting_indices: AttestingIndices

, - is_superset: bool, + is_subset_aggregate: bool, }, Ignore, DelayUntilBlock(Arc>, H256), diff --git a/fork_choice_store/src/store.rs b/fork_choice_store/src/store.rs index 42b0d872..76d7f7b9 100644 --- a/fork_choice_store/src/store.rs +++ b/fork_choice_store/src/store.rs @@ -1316,12 +1316,12 @@ impl Store

{ )?; // https://github.com/ethereum/consensus-specs/pull/2847 - let is_superset = self.aggregate_and_proof_supersets.check(&aggregate); + let is_subset_aggregate = !self.aggregate_and_proof_supersets.check(&aggregate); Ok(AggregateAndProofAction::Accept { aggregate_and_proof, attesting_indices, - is_superset, + is_subset_aggregate, }) } diff --git a/fork_choice_store/src/supersets.rs b/fork_choice_store/src/supersets.rs index 70150c87..7340d1cb 100644 --- a/fork_choice_store/src/supersets.rs +++ b/fork_choice_store/src/supersets.rs @@ -1,4 +1,5 @@ -use crossbeam_skiplist::SkipMap; +use crossbeam_skiplist::{SkipMap, SkipSet}; +use helper_functions::misc; use ssz::{BitList, SszHash as _, H256}; use types::{ combined::Attestation, @@ -6,7 +7,7 @@ use types::{ preset::Preset, }; -type AggregateEpochSupersets = SkipMap>; +type AggregateEpochSupersets = SkipMap>>; #[derive(Default)] pub struct AggregateAndProofSets { @@ -22,18 +23,29 @@ impl AggregateAndProofSets { let supersets = supersets.value(); - let is_superset = if let Some(existing) = supersets.get(&attestation_data_root) { - let existing = existing.value(); - aggregation_bits.any_not_in(existing) && !existing.any_not_in(aggregation_bits) + let has_unseen_bits = if let Some(existing_sets) = supersets.get(&attestation_data_root) { + existing_sets + .value() + .iter() + .all(|entry| aggregation_bits.any_not_in(entry.value())) } else { true }; - if is_superset { - supersets.insert(attestation_data_root, aggregation_bits.clone()); + if has_unseen_bits { + let existing_sets = supersets.get_or_insert(attestation_data_root, SkipSet::new()); + let existing_sets = existing_sets.value(); + + for entry in existing_sets { + if !entry.value().any_not_in(aggregation_bits) { + entry.remove(); + } + } + + existing_sets.insert(aggregation_bits.clone()); } - is_superset + has_unseen_bits } pub fn prune(&self, finalized_epoch: Epoch) { @@ -44,7 +56,16 @@ impl AggregateAndProofSets { #[cfg(test)] pub fn len(&self) -> usize { - self.supersets.len() + self.supersets + .iter() + .map(|epoch_entry| { + epoch_entry + .value() + .iter() + .map(|entry| entry.value().len()) + .sum::() + }) + .sum() } } @@ -65,13 +86,21 @@ impl MultiPhaseAggregateAndProofSets

{ } pub fn check(&self, attestation: &Attestation

) -> bool { + let committee_index = misc::committee_index(attestation); + match attestation { Attestation::Phase0(attestation) => self .phase0_supersets .check(&attestation.data, &attestation.aggregation_bits), - Attestation::Electra(attestation) => self - .electra_supersets - .check(&attestation.data, &attestation.aggregation_bits), + Attestation::Electra(attestation) => { + let data = AttestationData { + index: committee_index, + ..attestation.data + }; + + self.electra_supersets + .check(&data, &attestation.aggregation_bits) + } } } @@ -83,16 +112,73 @@ impl MultiPhaseAggregateAndProofSets

{ #[cfg(test)] mod tests { + use ssz::BitVector; + use types::{ + electra::containers::Attestation as ElectraAttestation, phase0::primitives::CommitteeIndex, + preset::Minimal, + }; + use super::*; - use types::preset::Minimal; + + fn aggregate( + committee_index: CommitteeIndex, + attester_bits: impl IntoIterator, + ) -> Attestation { + let mut aggregation_bits = BitList::with_length(3); + let mut committee_bits = BitVector::default(); + + for (index, bit) in attester_bits.into_iter().enumerate() { + aggregation_bits.set(index, bit == 1); + } + + committee_bits.set( + committee_index + .try_into() + .expect("committee index should fit in usize"), + true, + ); + + Attestation::Electra(ElectraAttestation { + aggregation_bits, + committee_bits, + ..ElectraAttestation::default() + }) + } #[test] - fn check_first_insert_is_superset() { + fn test_superset_check() { + let agg_0_1 = aggregate(0, [0, 0, 1]); + let agg_0_2 = aggregate(0, [1, 0, 1]); + let agg_0_3 = aggregate(0, [0, 1, 1]); + let agg_0_4 = aggregate(0, [1, 0, 0]); + let agg_0_5 = aggregate(0, [0, 0, 1]); + let agg_1_1 = aggregate(1, [0, 0, 1]); + let supersets = MultiPhaseAggregateAndProofSets::::new(); assert_eq!(supersets.len(), 0); - // 1 0 1 1 - // let superset = BitList::::new(true, 1); - // assert!(superset.check()) + assert!(supersets.check(&agg_0_1)); + // Adds a new superset + assert_eq!(supersets.len(), 1); + + assert!(supersets.check(&agg_0_2)); + // Overrides existing superset + assert_eq!(supersets.len(), 1); + + assert!(supersets.check(&agg_0_3)); + // Adds a new superset + assert_eq!(supersets.len(), 2); + + assert!(!supersets.check(&agg_0_4)); + // Does nothing + assert_eq!(supersets.len(), 2); + + assert!(!supersets.check(&agg_0_5)); + // Does nothing + assert_eq!(supersets.len(), 2); + + assert!(supersets.check(&agg_1_1)); + // Adds a new superset + assert_eq!(supersets.len(), 3); } }