From f85a1243623f27f228525277f8fb4d9f04276a52 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 20 Jun 2024 15:36:43 +0200 Subject: [PATCH] Electra attestation changes from Lions review (#5971) * dedup/cleanup and remove unneeded hashset use * remove irrelevant TODOs --- .../src/attestation_verification.rs | 3 +-- .../src/naive_aggregation_pool.rs | 24 +++++-------------- .../tests/attestation_verification.rs | 1 - .../src/common/get_attesting_indices.rs | 11 ++++----- .../per_block_processing/signature_sets.rs | 1 - 5 files changed, 11 insertions(+), 29 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 6044cafd6e..32c33f3e61 100644 --- 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( 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 index b16ced789f..211aecfe63 100644 --- 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 AggregateMap for AggregatedAttestationMap { fn insert(&mut self, a: AttestationRef) -> Result { 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 index 3e35e58296..19efe10c6d 100644 --- 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( 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 index 457a526104..b131f7679a 100644 --- 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, committee_bits: &BitVector, ) -> Result, BeaconStateError> { - let mut output: HashSet = HashSet::new(); + let mut attesting_indices = vec![]; let committee_indices = get_committee_indices::(committee_bits); @@ -128,8 +127,7 @@ pub mod attesting_indices_electra { }) .collect::>(); - 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( diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs index 8cf39abf43..2e00ee0341 100644 --- 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))