Aggregate superset check fix: support Electra aggregates in superset …

…checking

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);
                     }
                 }
 

fork_choice_store/src/misc.rs

@@ -548,7 +548,7 @@ pub enum AggregateAndProofAction<P: Preset> {
     Accept {
         aggregate_and_proof: Arc<SignedAggregateAndProof<P>>,
         attesting_indices: AttestingIndices<P>,
-        is_superset: bool,
+        is_subset_aggregate: bool,
     },
     Ignore,
     DelayUntilBlock(Arc<SignedAggregateAndProof<P>>, H256),

fork_choice_store/src/store.rs

@@ -1316,12 +1316,12 @@ impl<P: Preset> Store<P> {
         )?;
 
         // 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,
         })
     }
 

fork_choice_store/src/supersets.rs

@@ -1,12 +1,13 @@
-use crossbeam_skiplist::SkipMap;
+use crossbeam_skiplist::{SkipMap, SkipSet};
+use helper_functions::misc;
 use ssz::{BitList, SszHash as _, H256};
 use types::{
     combined::Attestation,
     phase0::{containers::AttestationData, primitives::Epoch},
     preset::Preset,
 };
 
-type AggregateEpochSupersets<N> = SkipMap<H256, BitList<N>>;
+type AggregateEpochSupersets<N> = SkipMap<H256, SkipSet<BitList<N>>>;
 
 #[derive(Default)]
 pub struct AggregateAndProofSets<N> {
@@ -22,18 +23,29 @@ impl<N: Send + Sync + 'static> AggregateAndProofSets<N> {
 
         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<N: Send + Sync + 'static> AggregateAndProofSets<N> {
 
     #[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::<usize>()
+            })
+            .sum()
     }
 }
 
@@ -65,13 +86,21 @@ impl<P: Preset> MultiPhaseAggregateAndProofSets<P> {
     }
 
     pub fn check(&self, attestation: &Attestation<P>) -> 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<P: Preset> MultiPhaseAggregateAndProofSets<P> {
 
 #[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<Item = u8>,
+    ) -> Attestation<Minimal> {
+        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::<Minimal>::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);
     }
 }