Electra attestation changes from Lions review (#5971)

* dedup/cleanup and remove unneeded hashset use * remove irrelevant TODOs
sigp · Jun 20, 2024 · f85a124 · f85a124
1 parent 0e2add2
commit f85a124
Show file tree

Hide file tree

Showing 5 changed files with 11 additions and 29 deletions.
diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs
@@ -1397,8 +1397,7 @@ pub fn obtain_indexed_attestation_and_committees_per_slot<T: BeaconChainTypes>(
                 attesting_indices_electra::get_indexed_attestation(&committees, att)
                     .map(|attestation| (attestation, committees_per_slot))
                     .map_err(|e| {
-                        let index = att.committee_index().unwrap_or(0);
-                        if e == BlockOperationError::BeaconStateError(NoCommitteeFound(index)) {
+                        if let BlockOperationError::BeaconStateError(NoCommitteeFound(index)) = e {
                             Error::NoCommitteeForSlotAndIndex {
                                 slot: att.data.slot,
                                 index,

diff --git a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs
@@ -241,24 +241,12 @@ impl<E: EthSpec> AggregateMap for AggregatedAttestationMap<E> {
     fn insert(&mut self, a: AttestationRef<E>) -> Result<InsertOutcome, Error> {
         let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_CORE_INSERT);
 
-        let aggregation_bit = match a {
-            AttestationRef::Base(att) => att
-                .aggregation_bits
-                .iter()
-                .enumerate()
-                .filter_map(|(i, bit)| if bit { Some(i) } else { None })
-                .at_most_one()
-                .map_err(|iter| Error::MoreThanOneAggregationBitSet(iter.count()))?
-                .ok_or(Error::NoAggregationBitsSet)?,
-            AttestationRef::Electra(att) => att
-                .aggregation_bits
-                .iter()
-                .enumerate()
-                .filter_map(|(i, bit)| if bit { Some(i) } else { None })
-                .at_most_one()
-                .map_err(|iter| Error::MoreThanOneAggregationBitSet(iter.count()))?
-                .ok_or(Error::NoAggregationBitsSet)?,
-        };
+        let aggregation_bit = *a
+            .set_aggregation_bits()
+            .iter()
+            .at_most_one()
+            .map_err(|iter| Error::MoreThanOneAggregationBitSet(iter.count()))?
+            .ok_or(Error::NoAggregationBitsSet)?;
 
         let attestation_key = AttestationKey::from_attestation_ref(a)?;
         let attestation_key_root = attestation_key.tree_hash_root();

diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs
@@ -233,7 +233,6 @@ fn get_non_aggregator<T: BeaconChainTypes>(
     let state = &head.beacon_state;
     let current_slot = chain.slot().expect("should get slot");
 
-    // TODO(electra) make fork-agnostic
     let committee = state
         .get_beacon_committee(
             current_slot,

diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs
@@ -47,7 +47,6 @@ pub mod attesting_indices_electra {
     use std::collections::HashSet;
 
     use crate::per_block_processing::errors::{AttestationInvalid as Invalid, BlockOperationError};
-    use itertools::Itertools;
     use safe_arith::SafeArith;
     use types::*;
 
@@ -96,7 +95,7 @@ pub mod attesting_indices_electra {
         aggregation_bits: &BitList<E::MaxValidatorsPerSlot>,
         committee_bits: &BitVector<E::MaxCommitteesPerSlot>,
     ) -> Result<Vec<u64>, BeaconStateError> {
-        let mut output: HashSet<u64> = HashSet::new();
+        let mut attesting_indices = vec![];
 
         let committee_indices = get_committee_indices::<E>(committee_bits);
 
@@ -128,8 +127,7 @@ pub mod attesting_indices_electra {
                 })
                 .collect::<HashSet<u64>>();
 
-            output.extend(committee_attesters);
-
+            attesting_indices.extend(committee_attesters);
             committee_offset.safe_add_assign(beacon_committee.committee.len())?;
         }
 
@@ -138,10 +136,9 @@ pub mod attesting_indices_electra {
             return Err(BeaconStateError::InvalidBitfield);
         }
 
-        let mut indices = output.into_iter().collect_vec();
-        indices.sort_unstable();
+        attesting_indices.sort_unstable();
 
-        Ok(indices)
+        Ok(attesting_indices)
     }
 
     pub fn get_committee_indices<E: EthSpec>(

diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs
@@ -326,7 +326,6 @@ where
         genesis_validators_root,
     );
 
-    // TODO(electra), signing root isnt unique in the case of electra
     let message = indexed_attestation.data().signing_root(domain);
 
     Ok(SignatureSet::multiple_pubkeys(signature, pubkeys, message))