[Altair] Sync committee pools (#2321)

Add pools supporting sync committees:
- naive sync aggregation pool
- observed sync contributions pool
- observed sync contributors pool
- observed sync aggregators pool

Add SSZ types and tests related to sync committee signatures.

Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: realbigsean <seananderson33@gmail.com>

beacon_node/beacon_chain/src/naive_aggregation_pool.rs

@@ -1,13 +1,18 @@
 use crate::metrics;
 use std::collections::HashMap;
 use tree_hash::TreeHash;
-use types::{Attestation, AttestationData, EthSpec, Hash256, Slot};
+use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT;
+use types::slot_data::SlotData;
+use types::sync_committee_contribution::SyncContributionData;
+use types::{Attestation, AttestationData, EthSpec, Hash256, Slot, SyncCommitteeContribution};
 
 type AttestationDataRoot = Hash256;
+type SyncDataRoot = Hash256;
+
 /// The number of slots that will be stored in the pool.
 ///
-/// For example, if `SLOTS_RETAINED == 3` and the pool is pruned at slot `6`, then all attestations
-/// at slots less than `4` will be dropped and any future attestation with a slot less than `4`
+/// For example, if `SLOTS_RETAINED == 3` and the pool is pruned at slot `6`, then all items
+/// at slots less than `4` will be dropped and any future item with a slot less than `4`
 /// will be refused.
 const SLOTS_RETAINED: usize = 3;
 
@@ -16,51 +21,98 @@ const SLOTS_RETAINED: usize = 3;
 /// This is a DoS protection measure.
 const MAX_ATTESTATIONS_PER_SLOT: usize = 16_384;
 
-/// Returned upon successfully inserting an attestation into the pool.
+/// Returned upon successfully inserting an item into the pool.
 #[derive(Debug, PartialEq)]
 pub enum InsertOutcome {
-    /// The `attestation.data` had not been seen before and was added to the pool.
-    NewAttestationData { committee_index: usize },
-    /// A validator signature for the given `attestation.data` was already known. No changes were
+    /// The item had not been seen before and was added to the pool.
+    NewItemInserted { committee_index: usize },
+    /// A validator signature for the given item's `Data` was already known. No changes were
     /// made.
     SignatureAlreadyKnown { committee_index: usize },
-    /// The `attestation.data` was known, but a signature for the given validator was not yet
+    /// The item's `Data` was known, but a signature for the given validator was not yet
     /// known. The signature was aggregated into the pool.
     SignatureAggregated { committee_index: usize },
 }
 
 #[derive(Debug, PartialEq)]
 pub enum Error {
-    /// The given `attestation.data.slot` was too low to be stored. No changes were made.
+    /// The given `data.slot` was too low to be stored. No changes were made.
     SlotTooLow {
         slot: Slot,
         lowest_permissible_slot: Slot,
     },
-    /// The given `attestation.aggregation_bits` field was empty.
+    /// The given `aggregation_bits` field was empty.
     NoAggregationBitsSet,
-    /// The given `attestation.aggregation_bits` field had more than one signature. The number of
+    /// The given `aggregation_bits` field had more than one signature. The number of
     /// signatures found is included.
     MoreThanOneAggregationBitSet(usize),
-    /// We have reached the maximum number of unique `AttestationData` that can be stored in a
+    /// We have reached the maximum number of unique items that can be stored in a
     /// slot. This is a DoS protection function.
-    ReachedMaxAttestationsPerSlot(usize),
-    /// The given `attestation.aggregation_bits` field had a different length to the one currently
+    ReachedMaxItemsPerSlot(usize),
+    /// The given `aggregation_bits` field had a different length to the one currently
     /// stored. This indicates a fairly serious error somewhere in the code that called this
     /// function.
     InconsistentBitfieldLengths,
-    /// The given `attestation` was for the incorrect slot. This is an internal error.
-    IncorrectSlot { expected: Slot, attestation: Slot },
+    /// The given item was for the incorrect slot. This is an internal error.
+    IncorrectSlot { expected: Slot, actual: Slot },
+}
+
+/// Implemented for items in the `NaiveAggregationPool`. Requires that items implement `SlotData`,
+/// which means they have an associated slot. This handles aggregation of items that are inserted.
+pub trait AggregateMap {
+    /// `Key` should be a hash of `Data`.
+    type Key;
+
+    /// The item stored in the map
+    type Value: Clone + SlotData;
+
+    /// The unique fields of `Value`, hashed to create `Key`.
+    type Data: SlotData;
+
+    /// Create a new `AggregateMap` with capacity `initial_capacity`.
+    fn new(initial_capacity: usize) -> Self;
+
+    /// Insert a `Value` into `Self`, returning a result.
+    fn insert(&mut self, value: &Self::Value) -> Result<InsertOutcome, Error>;
+
+    /// Get a `Value` from `Self` based on `Data`.
+    fn get(&self, data: &Self::Data) -> Option<Self::Value>;
+
+    /// Get a reference to the inner `HashMap`.
+    fn get_map(&self) -> &HashMap<Self::Key, Self::Value>;
+
+    /// Get a `Value` from `Self` based on `Key`, which is a hash of `Data`.
+    fn get_by_root(&self, root: &Self::Key) -> Option<&Self::Value>;
+
+    /// The number of items store in `Self`.
+    fn len(&self) -> usize;
+
+    /// Start a timer observing inserts.
+    fn start_insert_timer() -> Option<metrics::HistogramTimer>;
+
+    /// Start a timer observing the time it takes to create a new map for a new slot.
+    fn start_create_map_timer() -> Option<metrics::HistogramTimer>;
+
+    /// Start a timer observing the time it takes to prune the pool.
+    fn start_prune_timer() -> Option<metrics::HistogramTimer>;
+
+    /// The default capacity of `Self`.
+    fn default_capacity() -> usize;
 }
 
 /// A collection of `Attestation` objects, keyed by their `attestation.data`. Enforces that all
 /// `attestation` are from the same slot.
-struct AggregatedAttestationMap<E: EthSpec> {
+pub struct AggregatedAttestationMap<E: EthSpec> {
     map: HashMap<AttestationDataRoot, Attestation<E>>,
 }
 
-impl<E: EthSpec> AggregatedAttestationMap<E> {
+impl<E: EthSpec> AggregateMap for AggregatedAttestationMap<E> {
+    type Key = AttestationDataRoot;
+    type Value = Attestation<E>;
+    type Data = AttestationData;
+
     /// Create an empty collection with the given `initial_capacity`.
-    pub fn new(initial_capacity: usize) -> Self {
+    fn new(initial_capacity: usize) -> Self {
         Self {
             map: HashMap::with_capacity(initial_capacity),
         }
@@ -69,7 +121,7 @@ impl<E: EthSpec> AggregatedAttestationMap<E> {
     /// Insert an attestation into `self`, aggregating it into the pool.
     ///
     /// The given attestation (`a`) must only have one signature.
-    pub fn insert(&mut self, a: &Attestation<E>) -> Result<InsertOutcome, Error> {
+    fn insert(&mut self, a: &Self::Value) -> Result<InsertOutcome, Error> {
         let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_CORE_INSERT);
 
         let set_bits = a
@@ -106,65 +158,190 @@ impl<E: EthSpec> AggregatedAttestationMap<E> {
             }
         } else {
             if self.map.len() >= MAX_ATTESTATIONS_PER_SLOT {
-                return Err(Error::ReachedMaxAttestationsPerSlot(
-                    MAX_ATTESTATIONS_PER_SLOT,
-                ));
+                return Err(Error::ReachedMaxItemsPerSlot(MAX_ATTESTATIONS_PER_SLOT));
             }
 
             self.map.insert(attestation_data_root, a.clone());
-            Ok(InsertOutcome::NewAttestationData { committee_index })
+            Ok(InsertOutcome::NewItemInserted { committee_index })
         }
     }
 
     /// Returns an aggregated `Attestation` with the given `data`, if any.
     ///
     /// The given `a.data.slot` must match the slot that `self` was initialized with.
-    pub fn get(&self, data: &AttestationData) -> Option<Attestation<E>> {
+    fn get(&self, data: &Self::Data) -> Option<Self::Value> {
         self.map.get(&data.tree_hash_root()).cloned()
     }
 
+    fn get_map(&self) -> &HashMap<Self::Key, Self::Value> {
+        &self.map
+    }
+
     /// Returns an aggregated `Attestation` with the given `root`, if any.
-    pub fn get_by_root(&self, root: &AttestationDataRoot) -> Option<&Attestation<E>> {
+    fn get_by_root(&self, root: &Self::Key) -> Option<&Self::Value> {
         self.map.get(root)
     }
 
-    /// Iterate all attestations in `self`.
-    pub fn iter(&self) -> impl Iterator<Item = &Attestation<E>> {
-        self.map.iter().map(|(_key, attestation)| attestation)
+    fn len(&self) -> usize {
+        self.map.len()
     }
 
-    pub fn len(&self) -> usize {
-        self.map.len()
+    fn start_insert_timer() -> Option<metrics::HistogramTimer> {
+        metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_INSERT)
+    }
+
+    fn start_create_map_timer() -> Option<metrics::HistogramTimer> {
+        metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_CREATE_MAP)
+    }
+
+    fn start_prune_timer() -> Option<metrics::HistogramTimer> {
+        metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_PRUNE)
+    }
+
+    /// Use the `TARGET_COMMITTEE_SIZE`.
+    ///
+    /// Note: hard-coded until `TARGET_COMMITTEE_SIZE` is available via `EthSpec`.
+    fn default_capacity() -> usize {
+        128
     }
 }
 
-/// A pool of `Attestation` that is specially designed to store "unaggregated" attestations from
-/// the native aggregation scheme.
+/// A collection of `SyncCommitteeContribution`, keyed by their `SyncContributionData`. Enforces that all
+/// contributions are from the same slot.
+pub struct SyncContributionAggregateMap<E: EthSpec> {
+    map: HashMap<SyncDataRoot, SyncCommitteeContribution<E>>,
+}
+
+impl<E: EthSpec> AggregateMap for SyncContributionAggregateMap<E> {
+    type Key = SyncDataRoot;
+    type Value = SyncCommitteeContribution<E>;
+    type Data = SyncContributionData;
+
+    /// Create an empty collection with the given `initial_capacity`.
+    fn new(initial_capacity: usize) -> Self {
+        Self {
+            map: HashMap::with_capacity(initial_capacity),
+        }
+    }
+
+    /// Insert a sync committee contribution into `self`, aggregating it into the pool.
+    ///
+    /// The given sync contribution must only have one signature.
+    fn insert(
+        &mut self,
+        contribution: &SyncCommitteeContribution<E>,
+    ) -> Result<InsertOutcome, Error> {
+        let _timer =
+            metrics::start_timer(&metrics::SYNC_CONTRIBUTION_PROCESSING_AGG_POOL_CORE_INSERT);
+
+        let set_bits = contribution
+            .aggregation_bits
+            .iter()
+            .enumerate()
+            .filter(|(_i, bit)| *bit)
+            .map(|(i, _bit)| i)
+            .collect::<Vec<_>>();
+
+        let committee_index = set_bits
+            .first()
+            .copied()
+            .ok_or(Error::NoAggregationBitsSet)?;
+
+        if set_bits.len() > 1 {
+            return Err(Error::MoreThanOneAggregationBitSet(set_bits.len()));
+        }
+
+        let sync_data_root = SyncContributionData::from_contribution(contribution).tree_hash_root();
+
+        if let Some(existing_contribution) = self.map.get_mut(&sync_data_root) {
+            if existing_contribution
+                .aggregation_bits
+                .get(committee_index)
+                .map_err(|_| Error::InconsistentBitfieldLengths)?
+            {
+                Ok(InsertOutcome::SignatureAlreadyKnown { committee_index })
+            } else {
+                let _timer = metrics::start_timer(
+                    &metrics::SYNC_CONTRIBUTION_PROCESSING_AGG_POOL_AGGREGATION,
+                );
+                existing_contribution.aggregate(contribution);
+                Ok(InsertOutcome::SignatureAggregated { committee_index })
+            }
+        } else {
+            if self.map.len() >= E::sync_committee_size() {
+                return Err(Error::ReachedMaxItemsPerSlot(E::sync_committee_size()));
+            }
+
+            self.map.insert(sync_data_root, contribution.clone());
+            Ok(InsertOutcome::NewItemInserted { committee_index })
+        }
+    }
+
+    /// Returns an aggregated `SyncCommitteeContribution` with the given `data`, if any.
+    ///
+    /// The given `data.slot` must match the slot that `self` was initialized with.
+    fn get(&self, data: &SyncContributionData) -> Option<SyncCommitteeContribution<E>> {
+        self.map.get(&data.tree_hash_root()).cloned()
+    }
+
+    fn get_map(&self) -> &HashMap<SyncDataRoot, SyncCommitteeContribution<E>> {
+        &self.map
+    }
+
+    /// Returns an aggregated `SyncCommitteeContribution` with the given `root`, if any.
+    fn get_by_root(&self, root: &SyncDataRoot) -> Option<&SyncCommitteeContribution<E>> {
+        self.map.get(root)
+    }
+
+    fn len(&self) -> usize {
+        self.map.len()
+    }
+
+    fn start_insert_timer() -> Option<metrics::HistogramTimer> {
+        metrics::start_timer(&metrics::SYNC_CONTRIBUTION_PROCESSING_AGG_POOL_INSERT)
+    }
+
+    fn start_create_map_timer() -> Option<metrics::HistogramTimer> {
+        metrics::start_timer(&metrics::SYNC_CONTRIBUTION_PROCESSING_AGG_POOL_CREATE_MAP)
+    }
+
+    fn start_prune_timer() -> Option<metrics::HistogramTimer> {
+        metrics::start_timer(&metrics::SYNC_CONTRIBUTION_PROCESSING_AGG_POOL_PRUNE)
+    }
+
+    /// Default to `SYNC_COMMITTEE_SUBNET_COUNT`.
+    fn default_capacity() -> usize {
+        SYNC_COMMITTEE_SUBNET_COUNT as usize
+    }
+}
+
+/// A pool of `Attestation` or `SyncCommitteeContribution` that is specially designed to store
+/// "unaggregated" messages from the native aggregation scheme.
 ///
-/// **The `NaiveAggregationPool` does not do any signature or attestation verification. It assumes
-/// that all `Attestation` objects provided are valid.**
+/// **The `NaiveAggregationPool` does not do any verification. It assumes that all `Attestation`
+/// or `SyncCommitteeContribution` objects provided are valid.**
 ///
 /// ## Details
 ///
-/// The pool sorts the `Attestation` by `attestation.data.slot`, then by `attestation.data`.
+/// The pool sorts the items by `slot`, then by `Data`.
 ///
-/// As each unaggregated attestation is added it is aggregated with any existing `attestation` with
-/// the same `AttestationData`. Considering that the pool only accepts attestations with a single
+/// As each item is added it is aggregated with any existing item with the same `Data`. Considering
+/// that the pool only accepts attestations or sync contributions with a single
 /// signature, there should only ever be a single aggregated `Attestation` for any given
-/// `AttestationData`.
+/// `AttestationData` or a single `SyncCommitteeContribution` for any given `SyncContributionData`.
 ///
-/// The pool has a capacity for `SLOTS_RETAINED` slots, when a new `attestation.data.slot` is
+/// The pool has a capacity for `SLOTS_RETAINED` slots, when a new `slot` is
 /// provided, the oldest slot is dropped and replaced with the new slot. The pool can also be
-/// pruned by supplying a `current_slot`; all existing attestations with a slot lower than
-/// `current_slot - SLOTS_RETAINED` will be removed and any future attestation with a slot lower
-/// than that will also be refused. Pruning is done automatically based upon the attestations it
+/// pruned by supplying a `current_slot`; all existing items with a slot lower than
+/// `current_slot - SLOTS_RETAINED` will be removed and any future item with a slot lower
+/// than that will also be refused. Pruning is done automatically based upon the items it
 /// receives and it can be triggered manually.
-pub struct NaiveAggregationPool<E: EthSpec> {
+pub struct NaiveAggregationPool<T: AggregateMap> {
     lowest_permissible_slot: Slot,
-    maps: HashMap<Slot, AggregatedAttestationMap<E>>,
+    maps: HashMap<Slot, T>,
 }
 
-impl<E: EthSpec> Default for NaiveAggregationPool<E> {
+impl<T: AggregateMap> Default for NaiveAggregationPool<T> {
     fn default() -> Self {
         Self {
             lowest_permissible_slot: Slot::new(0),
@@ -173,20 +350,20 @@ impl<E: EthSpec> Default for NaiveAggregationPool<E> {
     }
 }
 
-impl<E: EthSpec> NaiveAggregationPool<E> {
-    /// Insert an attestation into `self`, aggregating it into the pool.
+impl<T: AggregateMap> NaiveAggregationPool<T> {
+    /// Insert an item into `self`, aggregating it into the pool.
     ///
-    /// The given attestation (`a`) must only have one signature and have an
-    /// `attestation.data.slot` that is not lower than `self.lowest_permissible_slot`.
+    /// The given item must only have one signature and have an
+    /// `slot` that is not lower than `self.lowest_permissible_slot`.
     ///
-    /// The pool may be pruned if the given `attestation.data` has a slot higher than any
+    /// The pool may be pruned if the given item has a slot higher than any
     /// previously seen.
-    pub fn insert(&mut self, attestation: &Attestation<E>) -> Result<InsertOutcome, Error> {
-        let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_INSERT);
-        let slot = attestation.data.slot;
+    pub fn insert(&mut self, item: &T::Value) -> Result<InsertOutcome, Error> {
+        let _timer = T::start_insert_timer();
+        let slot = item.get_slot();
         let lowest_permissible_slot = self.lowest_permissible_slot;
 
-        // Reject any attestations that are too old.
+        // Reject any items that are too old.
         if slot < lowest_permissible_slot {
             return Err(Error::SlotTooLow {
                 slot,
@@ -194,14 +371,10 @@ impl<E: EthSpec> NaiveAggregationPool<E> {
             });
         }
 
-        let lock_timer =
-            metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_MAPS_WRITE_LOCK);
-        drop(lock_timer);
-
         let outcome = if let Some(map) = self.maps.get_mut(&slot) {
-            map.insert(attestation)
+            map.insert(item)
         } else {
-            let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_CREATE_MAP);
+            let _timer = T::start_create_map_timer();
             // To avoid re-allocations, try and determine a rough initial capacity for the new item
             // by obtaining the mean size of all items in earlier epoch.
             let (count, sum) = self
@@ -213,12 +386,11 @@ impl<E: EthSpec> NaiveAggregationPool<E> {
                 .map(|(_slot, map)| map.len())
                 .fold((0, 0), |(count, sum), len| (count + 1, sum + len));
 
-            // Use the mainnet default committee size if we can't determine an average.
-            let initial_capacity = sum.checked_div(count).unwrap_or(128);
+            let initial_capacity = sum.checked_div(count).unwrap_or_else(T::default_capacity);
 
-            let mut item = AggregatedAttestationMap::new(initial_capacity);
-            let outcome = item.insert(attestation);
-            self.maps.insert(slot, item);
+            let mut aggregate_map = T::new(initial_capacity);
+            let outcome = aggregate_map.insert(item);
+            self.maps.insert(slot, aggregate_map);
 
             outcome
         };
@@ -228,39 +400,39 @@ impl<E: EthSpec> NaiveAggregationPool<E> {
         outcome
     }
 
-    /// Returns the total number of attestations stored in `self`.
-    pub fn num_attestations(&self) -> usize {
+    /// Returns the total number of items stored in `self`.
+    pub fn num_items(&self) -> usize {
         self.maps.iter().map(|(_, map)| map.len()).sum()
     }
 
-    /// Returns an aggregated `Attestation` with the given `data`, if any.
-    pub fn get(&self, data: &AttestationData) -> Option<Attestation<E>> {
-        self.maps.get(&data.slot).and_then(|map| map.get(data))
+    /// Returns an aggregated `T::Value` with the given `T::Data`, if any.
+    pub fn get(&self, data: &T::Data) -> Option<T::Value> {
+        self.maps
+            .get(&data.get_slot())
+            .and_then(|map| map.get(data))
     }
 
-    /// Returns an aggregated `Attestation` with the given `data`, if any.
-    pub fn get_by_slot_and_root(
-        &self,
-        slot: Slot,
-        root: &AttestationDataRoot,
-    ) -> Option<Attestation<E>> {
+    /// Returns an aggregated `T::Value` with the given `slot` and `root`, if any.
+    pub fn get_by_slot_and_root(&self, slot: Slot, root: &T::Key) -> Option<T::Value> {
         self.maps
             .get(&slot)
             .and_then(|map| map.get_by_root(root).cloned())
     }
 
-    /// Iterate all attestations in all slots of `self`.
-    pub fn iter(&self) -> impl Iterator<Item = &Attestation<E>> {
-        self.maps.iter().map(|(_slot, map)| map.iter()).flatten()
+    /// Iterate all items in all slots of `self`.
+    pub fn iter(&self) -> impl Iterator<Item = &T::Value> {
+        self.maps
+            .iter()
+            .map(|(_slot, map)| map.get_map().iter().map(|(_key, value)| value))
+            .flatten()
     }
 
-    /// Removes any attestations with a slot lower than `current_slot` and bars any future
-    /// attestations with a slot lower than `current_slot - SLOTS_RETAINED`.
+    /// Removes any items with a slot lower than `current_slot` and bars any future
+    /// items with a slot lower than `current_slot - SLOTS_RETAINED`.
     pub fn prune(&mut self, current_slot: Slot) {
-        let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_PRUNE);
+        let _timer = T::start_prune_timer();
 
-        // Taking advantage of saturating subtraction on `Slot`.
-        let lowest_permissible_slot = current_slot - Slot::from(SLOTS_RETAINED);
+        let lowest_permissible_slot = current_slot.saturating_sub(Slot::from(SLOTS_RETAINED));
 
         // No need to prune if the lowest permissible slot has not changed and the queue length is
         // less than the maximum
@@ -301,9 +473,10 @@ impl<E: EthSpec> NaiveAggregationPool<E> {
 mod tests {
     use super::*;
     use ssz_types::BitList;
+    use store::BitVector;
     use types::{
         test_utils::{generate_deterministic_keypair, test_random_instance},
-        Fork, Hash256,
+        Fork, Hash256, SyncCommitteeMessage,
     };
 
     type E = types::MainnetEthSpec;
@@ -315,7 +488,14 @@ mod tests {
         a
     }
 
-    fn sign(a: &mut Attestation<E>, i: usize, genesis_validators_root: Hash256) {
+    fn get_sync_contribution(slot: Slot) -> SyncCommitteeContribution<E> {
+        let mut a: SyncCommitteeContribution<E> = test_random_instance();
+        a.slot = slot;
+        a.aggregation_bits = BitVector::new();
+        a
+    }
+
+    fn sign_attestation(a: &mut Attestation<E>, i: usize, genesis_validators_root: Hash256) {
         a.sign(
             &generate_deterministic_keypair(i).sk,
             i,
@@ -326,187 +506,294 @@ mod tests {
         .expect("should sign attestation");
     }
 
-    fn unset_bit(a: &mut Attestation<E>, i: usize) {
+    fn sign_sync_contribution(
+        a: &mut SyncCommitteeContribution<E>,
+        i: usize,
+        genesis_validators_root: Hash256,
+    ) {
+        let sync_message = SyncCommitteeMessage::new::<E>(
+            a.slot,
+            a.beacon_block_root,
+            i as u64,
+            &generate_deterministic_keypair(i).sk,
+            &Fork::default(),
+            genesis_validators_root,
+            &E::default_spec(),
+        );
+        let signed_contribution: SyncCommitteeContribution<E> =
+            SyncCommitteeContribution::from_message(&sync_message, a.subcommittee_index, i)
+                .unwrap();
+
+        a.aggregate(&signed_contribution);
+    }
+
+    fn unset_attestation_bit(a: &mut Attestation<E>, i: usize) {
         a.aggregation_bits
             .set(i, false)
             .expect("should unset aggregation bit")
     }
 
-    #[test]
-    fn single_attestation() {
-        let mut a = get_attestation(Slot::new(0));
-
-        let mut pool = NaiveAggregationPool::default();
-
-        assert_eq!(
-            pool.insert(&a),
-            Err(Error::NoAggregationBitsSet),
-            "should not accept attestation without any signatures"
-        );
-
-        sign(&mut a, 0, Hash256::random());
-
-        assert_eq!(
-            pool.insert(&a),
-            Ok(InsertOutcome::NewAttestationData { committee_index: 0 }),
-            "should accept new attestation"
-        );
-        assert_eq!(
-            pool.insert(&a),
-            Ok(InsertOutcome::SignatureAlreadyKnown { committee_index: 0 }),
-            "should acknowledge duplicate signature"
-        );
-
-        let retrieved = pool
-            .get(&a.data)
-            .expect("should not error while getting attestation");
-        assert_eq!(
-            retrieved, a,
-            "retrieved attestation should equal the one inserted"
-        );
-
-        sign(&mut a, 1, Hash256::random());
-
-        assert_eq!(
-            pool.insert(&a),
-            Err(Error::MoreThanOneAggregationBitSet(2)),
-            "should not accept attestation with multiple signatures"
-        );
+    fn unset_sync_contribution_bit(a: &mut SyncCommitteeContribution<E>, i: usize) {
+        a.aggregation_bits
+            .set(i, false)
+            .expect("should unset aggregation bit")
     }
 
-    #[test]
-    fn multiple_attestations() {
-        let mut a_0 = get_attestation(Slot::new(0));
-        let mut a_1 = a_0.clone();
-
-        let genesis_validators_root = Hash256::random();
-        sign(&mut a_0, 0, genesis_validators_root);
-        sign(&mut a_1, 1, genesis_validators_root);
-
-        let mut pool = NaiveAggregationPool::default();
-
-        assert_eq!(
-            pool.insert(&a_0),
-            Ok(InsertOutcome::NewAttestationData { committee_index: 0 }),
-            "should accept a_0"
-        );
-        assert_eq!(
-            pool.insert(&a_1),
-            Ok(InsertOutcome::SignatureAggregated { committee_index: 1 }),
-            "should accept a_1"
-        );
-
-        let retrieved = pool
-            .get(&a_0.data)
-            .expect("should not error while getting attestation");
-
-        let mut a_01 = a_0.clone();
-        a_01.aggregate(&a_1);
-
-        assert_eq!(
-            retrieved, a_01,
-            "retrieved attestation should be aggregated"
-        );
-
-        /*
-         * Throw a different attestation data in there and ensure it isn't aggregated
-         */
-
-        let mut a_different = a_0.clone();
-        let different_root = Hash256::from_low_u64_be(1337);
-        unset_bit(&mut a_different, 0);
-        sign(&mut a_different, 2, genesis_validators_root);
-        assert_ne!(a_different.data.beacon_block_root, different_root);
-        a_different.data.beacon_block_root = different_root;
-
-        assert_eq!(
-            pool.insert(&a_different),
-            Ok(InsertOutcome::NewAttestationData { committee_index: 2 }),
-            "should accept a_different"
-        );
-
-        assert_eq!(
-            pool.get(&a_0.data)
-                .expect("should not error while getting attestation"),
-            retrieved,
-            "should not have aggregated different attestation data"
-        );
+    fn mutate_attestation_block_root(a: &mut Attestation<E>, block_root: Hash256) {
+        a.data.beacon_block_root = block_root
     }
 
-    #[test]
-    fn auto_pruning() {
-        let mut base = get_attestation(Slot::new(0));
-        sign(&mut base, 0, Hash256::random());
+    fn mutate_attestation_slot(a: &mut Attestation<E>, slot: Slot) {
+        a.data.slot = slot
+    }
 
-        let mut pool = NaiveAggregationPool::default();
+    fn attestation_block_root_comparator(a: &Attestation<E>, block_root: Hash256) -> bool {
+        a.data.beacon_block_root == block_root
+    }
 
-        for i in 0..SLOTS_RETAINED * 2 {
-            let slot = Slot::from(i);
-            let mut a = base.clone();
-            a.data.slot = slot;
+    fn key_from_attestation(a: &Attestation<E>) -> AttestationData {
+        a.data.clone()
+    }
 
-            assert_eq!(
-                pool.insert(&a),
-                Ok(InsertOutcome::NewAttestationData { committee_index: 0 }),
-                "should accept new attestation"
-            );
+    fn mutate_sync_contribution_block_root(
+        a: &mut SyncCommitteeContribution<E>,
+        block_root: Hash256,
+    ) {
+        a.beacon_block_root = block_root
+    }
 
-            if i < SLOTS_RETAINED {
-                let len = i + 1;
-                assert_eq!(pool.maps.len(), len, "the pool should have length {}", len);
-            } else {
-                assert_eq!(
-                    pool.maps.len(),
-                    SLOTS_RETAINED,
-                    "the pool should have length SLOTS_RETAINED"
-                );
+    fn mutate_sync_contribution_slot(a: &mut SyncCommitteeContribution<E>, slot: Slot) {
+        a.slot = slot
+    }
 
-                let mut pool_slots = pool
-                    .maps
-                    .iter()
-                    .map(|(slot, _map)| *slot)
-                    .collect::<Vec<_>>();
+    fn sync_contribution_block_root_comparator(
+        a: &SyncCommitteeContribution<E>,
+        block_root: Hash256,
+    ) -> bool {
+        a.beacon_block_root == block_root
+    }
 
-                pool_slots.sort_unstable();
+    fn key_from_sync_contribution(a: &SyncCommitteeContribution<E>) -> SyncContributionData {
+        SyncContributionData::from_contribution(&a)
+    }
+
+    macro_rules! test_suite {
+        (
+            $mod_name: ident,
+            $get_method_name: ident,
+            $sign_method_name: ident,
+            $unset_method_name: ident,
+            $block_root_mutator: ident,
+            $slot_mutator: ident,
+            $block_root_comparator: ident,
+            $key_getter: ident,
+            $map_type: ident,
+            $item_limit: expr
+        ) => {
+            #[cfg(test)]
+            mod $mod_name {
+                use super::*;
+
+                #[test]
+                fn single_item() {
+                    let mut a = $get_method_name(Slot::new(0));
+
+                    let mut pool: NaiveAggregationPool<$map_type<E>> =
+                        NaiveAggregationPool::default();
 
-                for (j, pool_slot) in pool_slots.iter().enumerate() {
-                    let expected_slot = slot - (SLOTS_RETAINED - 1 - j) as u64;
                     assert_eq!(
-                        *pool_slot, expected_slot,
-                        "the slot of the map should be {}",
-                        expected_slot
-                    )
+                        pool.insert(&a),
+                        Err(Error::NoAggregationBitsSet),
+                        "should not accept item without any signatures"
+                    );
+
+                    $sign_method_name(&mut a, 0, Hash256::random());
+
+                    assert_eq!(
+                        pool.insert(&a),
+                        Ok(InsertOutcome::NewItemInserted { committee_index: 0 }),
+                        "should accept new item"
+                    );
+                    assert_eq!(
+                        pool.insert(&a),
+                        Ok(InsertOutcome::SignatureAlreadyKnown { committee_index: 0 }),
+                        "should acknowledge duplicate signature"
+                    );
+
+                    let retrieved = pool
+                        .get(&$key_getter(&a))
+                        .expect("should not error while getting item");
+                    assert_eq!(retrieved, a, "retrieved item should equal the one inserted");
+
+                    $sign_method_name(&mut a, 1, Hash256::random());
+
+                    assert_eq!(
+                        pool.insert(&a),
+                        Err(Error::MoreThanOneAggregationBitSet(2)),
+                        "should not accept item with multiple signatures"
+                    );
+                }
+
+                #[test]
+                fn multiple_items() {
+                    let mut a_0 = $get_method_name(Slot::new(0));
+                    let mut a_1 = a_0.clone();
+
+                    let genesis_validators_root = Hash256::random();
+                    $sign_method_name(&mut a_0, 0, genesis_validators_root);
+                    $sign_method_name(&mut a_1, 1, genesis_validators_root);
+
+                    let mut pool: NaiveAggregationPool<$map_type<E>> =
+                        NaiveAggregationPool::default();
+
+                    assert_eq!(
+                        pool.insert(&a_0),
+                        Ok(InsertOutcome::NewItemInserted { committee_index: 0 }),
+                        "should accept a_0"
+                    );
+                    assert_eq!(
+                        pool.insert(&a_1),
+                        Ok(InsertOutcome::SignatureAggregated { committee_index: 1 }),
+                        "should accept a_1"
+                    );
+
+                    let retrieved = pool
+                        .get(&$key_getter(&a_0))
+                        .expect("should not error while getting attestation");
+
+                    let mut a_01 = a_0.clone();
+                    a_01.aggregate(&a_1);
+
+                    assert_eq!(retrieved, a_01, "retrieved item should be aggregated");
+
+                    /*
+                     * Throw different data in there and ensure it isn't aggregated
+                     */
+
+                    let mut a_different = a_0.clone();
+                    let different_root = Hash256::from_low_u64_be(1337);
+                    $unset_method_name(&mut a_different, 0);
+                    $sign_method_name(&mut a_different, 2, genesis_validators_root);
+                    assert!(!$block_root_comparator(&a_different, different_root));
+                    $block_root_mutator(&mut a_different, different_root);
+
+                    assert_eq!(
+                        pool.insert(&a_different),
+                        Ok(InsertOutcome::NewItemInserted { committee_index: 2 }),
+                        "should accept a_different"
+                    );
+
+                    assert_eq!(
+                        pool.get(&$key_getter(&a_0))
+                            .expect("should not error while getting item"),
+                        retrieved,
+                        "should not have aggregated different items with different data"
+                    );
+                }
+
+                #[test]
+                fn auto_pruning_item() {
+                    let mut base = $get_method_name(Slot::new(0));
+                    $sign_method_name(&mut base, 0, Hash256::random());
+
+                    let mut pool: NaiveAggregationPool<$map_type<E>> =
+                        NaiveAggregationPool::default();
+
+                    for i in 0..SLOTS_RETAINED * 2 {
+                        let slot = Slot::from(i);
+                        let mut a = base.clone();
+                        $slot_mutator(&mut a, slot);
+
+                        assert_eq!(
+                            pool.insert(&a),
+                            Ok(InsertOutcome::NewItemInserted { committee_index: 0 }),
+                            "should accept new item"
+                        );
+
+                        if i < SLOTS_RETAINED {
+                            let len = i + 1;
+                            assert_eq!(pool.maps.len(), len, "the pool should have length {}", len);
+                        } else {
+                            assert_eq!(
+                                pool.maps.len(),
+                                SLOTS_RETAINED,
+                                "the pool should have length SLOTS_RETAINED"
+                            );
+
+                            let mut pool_slots = pool
+                                .maps
+                                .iter()
+                                .map(|(slot, _map)| *slot)
+                                .collect::<Vec<_>>();
+
+                            pool_slots.sort_unstable();
+
+                            for (j, pool_slot) in pool_slots.iter().enumerate() {
+                                let expected_slot = slot - (SLOTS_RETAINED - 1 - j) as u64;
+                                assert_eq!(
+                                    *pool_slot, expected_slot,
+                                    "the slot of the map should be {}",
+                                    expected_slot
+                                )
+                            }
+                        }
+                    }
+                }
+
+                #[test]
+                fn max_items() {
+                    let mut base = $get_method_name(Slot::new(0));
+                    $sign_method_name(&mut base, 0, Hash256::random());
+
+                    let mut pool: NaiveAggregationPool<$map_type<E>> =
+                        NaiveAggregationPool::default();
+
+                    for i in 0..=$item_limit {
+                        let mut a = base.clone();
+                        $block_root_mutator(&mut a, Hash256::from_low_u64_be(i as u64));
+
+                        if i < $item_limit {
+                            assert_eq!(
+                                pool.insert(&a),
+                                Ok(InsertOutcome::NewItemInserted { committee_index: 0 }),
+                                "should accept item below limit"
+                            );
+                        } else {
+                            assert_eq!(
+                                pool.insert(&a),
+                                Err(Error::ReachedMaxItemsPerSlot($item_limit)),
+                                "should not accept item above limit"
+                            );
+                        }
+                    }
                 }
             }
-        }
+        };
     }
 
-    #[test]
-    fn max_attestations() {
-        let mut base = get_attestation(Slot::new(0));
-        sign(&mut base, 0, Hash256::random());
+    test_suite! {
+        attestation_tests,
+        get_attestation,
+        sign_attestation,
+        unset_attestation_bit,
+        mutate_attestation_block_root,
+        mutate_attestation_slot,
+        attestation_block_root_comparator,
+        key_from_attestation,
+        AggregatedAttestationMap,
+        MAX_ATTESTATIONS_PER_SLOT
+    }
 
-        let mut pool = NaiveAggregationPool::default();
-
-        for i in 0..=MAX_ATTESTATIONS_PER_SLOT {
-            let mut a = base.clone();
-            a.data.beacon_block_root = Hash256::from_low_u64_be(i as u64);
-
-            if i < MAX_ATTESTATIONS_PER_SLOT {
-                assert_eq!(
-                    pool.insert(&a),
-                    Ok(InsertOutcome::NewAttestationData { committee_index: 0 }),
-                    "should accept attestation below limit"
-                );
-            } else {
-                assert_eq!(
-                    pool.insert(&a),
-                    Err(Error::ReachedMaxAttestationsPerSlot(
-                        MAX_ATTESTATIONS_PER_SLOT
-                    )),
-                    "should not accept attestation above limit"
-                );
-            }
-        }
+    test_suite! {
+        sync_contribution_tests,
+        get_sync_contribution,
+        sign_sync_contribution,
+        unset_sync_contribution_bit,
+        mutate_sync_contribution_block_root,
+        mutate_sync_contribution_slot,
+        sync_contribution_block_root_comparator,
+        key_from_sync_contribution,
+        SyncContributionAggregateMap,
+        E::sync_committee_size()
     }
 }