From 7cb7653d3614e1455e97cd5a6192671e1c465aa0 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 9 May 2024 17:45:52 +1000 Subject: [PATCH 1/7] Sketch op pool changes --- .../operation_pool/src/attestation_storage.rs | 97 +++++++++++++------ beacon_node/operation_pool/src/lib.rs | 27 ++++-- consensus/types/src/attestation.rs | 3 + 3 files changed, 91 insertions(+), 36 deletions(-) diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 00fbcbe4b01..5cf93e642c9 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -1,6 +1,6 @@ use crate::AttestationStats; use itertools::Itertools; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use types::{ attestation::{AttestationBase, AttestationElectra}, superstruct, AggregateSignature, Attestation, AttestationData, BeaconState, BitList, BitVector, @@ -42,6 +42,7 @@ pub struct SplitAttestation { pub indexed: CompactIndexedAttestation, } +// TODO(electra): rename this type #[derive(Debug, Clone)] pub struct AttestationRef<'a, E: EthSpec> { pub checkpoint: &'a CheckpointKey, @@ -159,15 +160,15 @@ impl CheckpointKey { } impl CompactIndexedAttestation { - pub fn signers_disjoint_from(&self, other: &Self) -> bool { + pub fn should_aggregate(&self, other: &Self) -> bool { match (self, other) { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { - this.signers_disjoint_from(other) + this.should_aggregate(other) } ( CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other), - ) => this.signers_disjoint_from(other), + ) => this.should_aggregate(other), // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => false, } @@ -189,7 +190,7 @@ impl CompactIndexedAttestation { } impl CompactIndexedAttestationBase { - pub fn signers_disjoint_from(&self, other: &Self) -> bool { + pub fn should_aggregate(&self, other: &Self) -> bool { self.aggregation_bits .intersection(&other.aggregation_bits) .is_zero() @@ -208,14 +209,15 @@ impl CompactIndexedAttestationBase { } impl CompactIndexedAttestationElectra { - // TODO(electra) update to match spec requirements - pub fn signers_disjoint_from(&self, other: &Self) -> bool { - self.aggregation_bits - .intersection(&other.aggregation_bits) - .is_zero() + pub fn should_aggregate(&self, other: &Self) -> bool { + // For Electra, only aggregate attestations in the same committee. + self.committee_bits == other.committee_bits + && self + .aggregation_bits + .intersection(&other.aggregation_bits) + .is_zero() } - // TODO(electra) update to match spec requirements pub fn aggregate(&mut self, other: &Self) { self.attesting_indices = self .attesting_indices @@ -226,6 +228,18 @@ impl CompactIndexedAttestationElectra { self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.signature.add_assign_aggregate(&other.signature); } + + pub fn committee_index(&self) -> u64 { + *self.get_committee_indices().first().unwrap_or(&0u64) + } + + pub fn get_committee_indices(&self) -> Vec { + self.committee_bits + .iter() + .enumerate() + .filter_map(|(index, bit)| if bit { Some(index as u64) } else { None }) + .collect() + } } impl AttestationMap { @@ -239,34 +253,63 @@ impl AttestationMap { let attestation_map = self.checkpoint_map.entry(checkpoint).or_default(); let attestations = attestation_map.attestations.entry(data).or_default(); - // TODO(electra): // Greedily aggregate the attestation with all existing attestations. // NOTE: this is sub-optimal and in future we will remove this in favour of max-clique // aggregation. let mut aggregated = false; - match attestation { - Attestation::Base(_) => { - for existing_attestation in attestations.iter_mut() { - if existing_attestation.signers_disjoint_from(&indexed) { - existing_attestation.aggregate(&indexed); - aggregated = true; - } else if *existing_attestation == indexed { - aggregated = true; - } - } + for existing_attestation in attestations.iter_mut() { + if existing_attestation.should_aggregate(&indexed) { + existing_attestation.aggregate(&indexed); + aggregated = true; + } else if *existing_attestation == indexed { + aggregated = true; } - // TODO(electra) in order to be devnet ready, we can skip - // aggregating here for now. this will result in "poorly" - // constructed blocks, but that should be fine for devnet - Attestation::Electra(_) => (), - }; + } if !aggregated { attestations.push(indexed); } } + pub fn aggregate_across_committees(&mut self, checkpoint_key: CheckpointKey) { + let Some(attestation_map) = self.checkpoint_map.get_mut(&checkpoint_key) else { + return; + }; + for (compact_attestation_data, compact_indexed_attestations) in + attestation_map.attestations.iter_mut() + { + let unaggregated_attestations = std::mem::take(compact_indexed_attestations); + let mut aggregated_attestations = vec![]; + + // Aggregate the best attestations for each committee and leave the rest. + let mut best_attestations_by_committee = BTreeMap::new(); + + for committee_attestation in unaggregated_attestations { + // TODO(electra) + // compare to best attestations by committee + // could probably use `.entry` here + if let Some(existing_attestation) = + best_attestations_by_committee.get_mut(committee_attestation.committee_index()) + { + // compare and swap, put the discarded one straight into + // `aggregated_attestations` in case we have room to pack it without + // cross-committee aggregation + } else { + best_attestations_by_committee.insert( + committee_attestation.committee_index(), + committee_attestation, + ); + } + } + + // TODO(electra): aggregate all the best attestations by committee + // (use btreemap sort order to get order by committee index) + + *compact_indexed_attestations = aggregated_attestations; + } + } + /// Iterate all attestations matching the given `checkpoint_key`. pub fn get_attestations<'a>( &'a self, diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 6645416d4b0..1d83ecd4651 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -40,7 +40,7 @@ use std::ptr; use types::{ sync_aggregate::Error as SyncAggregateError, typenum::Unsigned, AbstractExecPayload, Attestation, AttestationData, AttesterSlashing, BeaconState, BeaconStateError, ChainSpec, - Epoch, EthSpec, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange, + Epoch, EthSpec, ForkName, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange, SignedVoluntaryExit, Slot, SyncAggregate, SyncCommitteeContribution, Validator, }; @@ -256,6 +256,7 @@ impl OperationPool { curr_epoch_validity_filter: impl for<'a> FnMut(&AttestationRef<'a, E>) -> bool + Send, spec: &ChainSpec, ) -> Result>, OpPoolError> { + let fork_name = state.fork_name_unchecked(); if !matches!(state, BeaconState::Base(_)) { // Epoch cache must be initialized to fetch base reward values in the max cover `score` // function. Currently max cover ignores items on errors. If epoch cache is not @@ -267,7 +268,6 @@ impl OperationPool { // Attestations for the current fork, which may be from the current or previous epoch. let (prev_epoch_key, curr_epoch_key) = CheckpointKey::keys_for_state(state); - let all_attestations = self.attestations.read(); let total_active_balance = state .get_total_active_balance() .map_err(OpPoolError::GetAttestationsTotalBalanceError)?; @@ -284,6 +284,14 @@ impl OperationPool { let mut num_prev_valid = 0_i64; let mut num_curr_valid = 0_i64; + // TODO(electra): Work out how to do this more elegantly. This is a bit of a hack. + let mut all_attestations = self.attestations.write(); + + all_attestations.aggregate_across_committees(prev_epoch_key); + all_attestations.aggregate_across_committees(curr_epoch_key); + + let all_attestations = parking_lot::RwLockWriteGuard::downgrade(all_attestations); + let prev_epoch_att = self .get_valid_attestations_for_epoch( &prev_epoch_key, @@ -307,6 +315,11 @@ impl OperationPool { ) .inspect(|_| num_curr_valid += 1); + let curr_epoch_limit = if fork_name < ForkName::Electra { + E::MaxAttestations::to_usize() + } else { + E::MaxAttestationsElectra::to_usize() + }; let prev_epoch_limit = if let BeaconState::Base(base_state) = state { std::cmp::min( E::MaxPendingAttestations::to_usize() @@ -314,7 +327,7 @@ impl OperationPool { E::MaxAttestations::to_usize(), ) } else { - E::MaxAttestations::to_usize() + curr_epoch_limit }; let (prev_cover, curr_cover) = rayon::join( @@ -329,11 +342,7 @@ impl OperationPool { }, move || { let _timer = metrics::start_timer(&metrics::ATTESTATION_CURR_EPOCH_PACKING_TIME); - maximum_cover( - curr_epoch_att, - E::MaxAttestations::to_usize(), - "curr_epoch_attestations", - ) + maximum_cover(curr_epoch_att, curr_epoch_limit, "curr_epoch_attestations") }, ); @@ -343,7 +352,7 @@ impl OperationPool { Ok(max_cover::merge_solutions( curr_cover, prev_cover, - E::MaxAttestations::to_usize(), + curr_epoch_limit, )) } diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index bcecfde10e4..0f7e8468488 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -247,6 +247,9 @@ impl<'a, E: EthSpec> AttestationRef<'a, E> { impl AttestationElectra { /// Are the aggregation bitfields of these attestations disjoint? + // TODO(electra): check whether the definition from CompactIndexedAttestation::should_aggregate + // is useful where this is used, i.e. only consider attestations disjoint when their committees + // match AND their aggregation bits do not intersect. pub fn signers_disjoint_from(&self, other: &Self) -> bool { self.aggregation_bits .intersection(&other.aggregation_bits) From ab9e58aa3d0e6fe2175a4996a5de710e81152896 Mon Sep 17 00:00:00 2001 From: ethDreamer <37123614+ethDreamer@users.noreply.github.com> Date: Thu, 9 May 2024 16:59:39 -0500 Subject: [PATCH 2/7] Get `electra_op_pool` up to date (#5756) * fix get attesting indices (#5742) * fix get attesting indices * better errors * fix compile * only get committee index once * Ef test fixes (#5753) * attestation related ef test fixes * delete commented out stuff * Fix Aggregation Pool for Electra (#5754) * Fix Aggregation Pool for Electra * Remove Outdated Interface * fix ssz (#5755) --------- Co-authored-by: realbigsean --- .../src/attestation_verification.rs | 5 +- beacon_node/beacon_chain/src/beacon_chain.rs | 38 ++- .../src/naive_aggregation_pool.rs | 282 ++++++++++++++---- beacon_node/beacon_chain/src/test_utils.rs | 2 +- .../tests/payload_invalidation.rs | 5 +- beacon_node/http_api/src/lib.rs | 2 +- .../src/common/get_attesting_indices.rs | 15 +- consensus/types/src/attestation.rs | 3 +- consensus/types/src/beacon_state.rs | 3 +- consensus/types/src/eth_spec.rs | 4 +- testing/ef_tests/src/type_name.rs | 7 +- testing/ef_tests/tests/tests.rs | 49 +-- 12 files changed, 307 insertions(+), 108 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 62e65d5f87a..3d722a534be 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -1309,10 +1309,11 @@ 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| { - if e == BlockOperationError::BeaconStateError(NoCommitteeFound) { + let index = att.committee_index(); + if e == BlockOperationError::BeaconStateError(NoCommitteeFound(index)) { Error::NoCommitteeForSlotAndIndex { slot: att.data.slot, - index: att.committee_index(), + index, } } else { Error::Invalid(e) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 973dcaadb47..331e04069fd 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1612,11 +1612,28 @@ impl BeaconChain { /// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`. /// /// The attestation will be obtained from `self.naive_aggregation_pool`. - pub fn get_aggregated_attestation( + pub fn get_aggregated_attestation_base( &self, data: &AttestationData, ) -> Result>, Error> { - if let Some(attestation) = self.naive_aggregation_pool.read().get(data) { + let attestation_key = crate::naive_aggregation_pool::AttestationKey::new_base(data); + if let Some(attestation) = self.naive_aggregation_pool.read().get(&attestation_key) { + self.filter_optimistic_attestation(attestation) + .map(Option::Some) + } else { + Ok(None) + } + } + + // TODO(electra): call this function from the new beacon API method + pub fn get_aggregated_attestation_electra( + &self, + data: &AttestationData, + committee_index: CommitteeIndex, + ) -> Result>, Error> { + let attestation_key = + crate::naive_aggregation_pool::AttestationKey::new_electra(data, committee_index); + if let Some(attestation) = self.naive_aggregation_pool.read().get(&attestation_key) { self.filter_optimistic_attestation(attestation) .map(Option::Some) } else { @@ -1628,16 +1645,21 @@ impl BeaconChain { /// `attestation.data.tree_hash_root()`. /// /// The attestation will be obtained from `self.naive_aggregation_pool`. - pub fn get_aggregated_attestation_by_slot_and_root( + /// + /// NOTE: This function will *only* work with pre-electra attestations and it only + /// exists to support the pre-electra validator API method. + pub fn get_pre_electra_aggregated_attestation_by_slot_and_root( &self, slot: Slot, attestation_data_root: &Hash256, ) -> Result>, Error> { - if let Some(attestation) = self - .naive_aggregation_pool - .read() - .get_by_slot_and_root(slot, attestation_data_root) - { + let attestation_key = + crate::naive_aggregation_pool::AttestationKey::new_base_from_slot_and_root( + slot, + *attestation_data_root, + ); + + if let Some(attestation) = self.naive_aggregation_pool.read().get(&attestation_key) { self.filter_optimistic_attestation(attestation) .map(Option::Some) } else { diff --git a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs index a12521cd171..6c4f7cdae72 100644 --- a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs +++ b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs @@ -1,7 +1,9 @@ use crate::metrics; use crate::observed_aggregates::AsReference; +use itertools::Itertools; +use smallvec::SmallVec; use std::collections::HashMap; -use tree_hash::TreeHash; +use tree_hash::{MerkleHasher, TreeHash, TreeHashType}; use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; use types::slot_data::SlotData; use types::sync_committee_contribution::SyncContributionData; @@ -9,9 +11,114 @@ use types::{ Attestation, AttestationData, AttestationRef, EthSpec, Hash256, Slot, SyncCommitteeContribution, }; -type AttestationDataRoot = Hash256; +type AttestationKeyRoot = Hash256; type SyncDataRoot = Hash256; +/// Post-Electra, we need a new key for Attestations that includes the committee index +#[derive(Debug, Clone, PartialEq)] +pub struct AttestationKey { + data_root: Hash256, + committee_index: Option, + slot: Slot, +} + +// A custom implementation of `TreeHash` such that: +// AttestationKey(data, None).tree_hash_root() == data.tree_hash_root() +// AttestationKey(data, Some(index)).tree_hash_root() == (data, index).tree_hash_root() +// This is necessary because pre-Electra, the validator will ask for the tree_hash_root() +// of the `AttestationData` +impl TreeHash for AttestationKey { + fn tree_hash_type() -> TreeHashType { + TreeHashType::Container + } + + fn tree_hash_packed_encoding(&self) -> SmallVec<[u8; 32]> { + unreachable!("AttestationKey should never be packed.") + } + + fn tree_hash_packing_factor() -> usize { + unreachable!("AttestationKey should never be packed.") + } + + fn tree_hash_root(&self) -> Hash256 { + match self.committee_index { + None => self.data_root, // Return just the data root if no committee index is present + Some(index) => { + // Combine the hash of the data with the hash of the index + let mut hasher = MerkleHasher::with_leaves(2); + hasher + .write(self.data_root.as_bytes()) + .expect("should write data hash"); + hasher + .write(&index.to_le_bytes()) + .expect("should write index"); + hasher.finish().expect("should give tree hash") + } + } + } +} + +impl AttestationKey { + pub fn from_attestation_ref(attestation: AttestationRef) -> Result { + let slot = attestation.data().slot; + match attestation { + AttestationRef::Base(att) => Ok(Self { + data_root: att.data.tree_hash_root(), + committee_index: None, + slot, + }), + AttestationRef::Electra(att) => { + let committee_index = att + .committee_bits + .iter() + .enumerate() + .filter_map(|(i, bit)| if bit { Some(i) } else { None }) + .at_most_one() + .map_err(|_| Error::MoreThanOneCommitteeBitSet)? + .ok_or(Error::NoCommitteeBitSet)?; + + Ok(Self { + data_root: att.data.tree_hash_root(), + committee_index: Some(committee_index as u64), + slot, + }) + } + } + } + + pub fn new_base(data: &AttestationData) -> Self { + let slot = data.slot; + Self { + data_root: data.tree_hash_root(), + committee_index: None, + slot, + } + } + + pub fn new_electra(data: &AttestationData, committee_index: u64) -> Self { + let slot = data.slot; + Self { + data_root: data.tree_hash_root(), + committee_index: Some(committee_index), + slot, + } + } + + pub fn new_base_from_slot_and_root(slot: Slot, data_root: Hash256) -> Self { + Self { + data_root, + committee_index: None, + slot, + } + } +} + +impl SlotData for AttestationKey { + fn get_slot(&self) -> Slot { + self.slot + } +} + /// 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 items @@ -49,6 +156,10 @@ pub enum Error { /// The given `aggregation_bits` field had more than one signature. The number of /// signatures found is included. MoreThanOneAggregationBitSet(usize), + /// The electra attestation has more than one committee bit set + MoreThanOneCommitteeBitSet, + /// The electra attestation has NO committee bit set + NoCommitteeBitSet, /// We have reached the maximum number of unique items that can be stored in a /// slot. This is a DoS protection function. ReachedMaxItemsPerSlot(usize), @@ -90,9 +201,6 @@ where /// Get a reference to the inner `HashMap`. fn get_map(&self) -> &HashMap; - /// 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; @@ -112,13 +220,13 @@ where /// A collection of `Attestation` objects, keyed by their `attestation.data`. Enforces that all /// `attestation` are from the same slot. pub struct AggregatedAttestationMap { - map: HashMap>, + map: HashMap>, } impl AggregateMap for AggregatedAttestationMap { - type Key = AttestationDataRoot; + type Key = AttestationKeyRoot; type Value = Attestation; - type Data = AttestationData; + type Data = AttestationKey; /// Create an empty collection with the given `initial_capacity`. fn new(initial_capacity: usize) -> Self { @@ -133,45 +241,43 @@ impl AggregateMap for AggregatedAttestationMap { fn insert(&mut self, a: AttestationRef) -> Result { let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_CORE_INSERT); - let set_bits = match a { + let aggregation_bit = match a { AttestationRef::Base(att) => att .aggregation_bits .iter() .enumerate() - .filter(|(_i, bit)| *bit) - .map(|(i, _bit)| i) - .collect::>(), + .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(|(_i, bit)| *bit) - .map(|(i, _bit)| i) - .collect::>(), + .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 committee_index = set_bits - .first() - .copied() - .ok_or(Error::NoAggregationBitsSet)?; - - if set_bits.len() > 1 { - return Err(Error::MoreThanOneAggregationBitSet(set_bits.len())); - } - - let attestation_data_root = a.data().tree_hash_root(); + let attestation_key = AttestationKey::from_attestation_ref(a)?; + let attestation_data_root = attestation_key.tree_hash_root(); if let Some(existing_attestation) = self.map.get_mut(&attestation_data_root) { if existing_attestation - .get_aggregation_bit(committee_index) + .get_aggregation_bit(aggregation_bit) .map_err(|_| Error::InconsistentBitfieldLengths)? { - Ok(InsertOutcome::SignatureAlreadyKnown { committee_index }) + Ok(InsertOutcome::SignatureAlreadyKnown { + committee_index: aggregation_bit, + }) } else { let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_AGGREGATION); existing_attestation.aggregate(a); - Ok(InsertOutcome::SignatureAggregated { committee_index }) + Ok(InsertOutcome::SignatureAggregated { + committee_index: aggregation_bit, + }) } } else { if self.map.len() >= MAX_ATTESTATIONS_PER_SLOT { @@ -180,7 +286,9 @@ impl AggregateMap for AggregatedAttestationMap { self.map .insert(attestation_data_root, a.clone_as_attestation()); - Ok(InsertOutcome::NewItemInserted { committee_index }) + Ok(InsertOutcome::NewItemInserted { + committee_index: aggregation_bit, + }) } } @@ -195,11 +303,6 @@ impl AggregateMap for AggregatedAttestationMap { &self.map } - /// Returns an aggregated `Attestation` with the given `root`, if any. - fn get_by_root(&self, root: &Self::Key) -> Option<&Self::Value> { - self.map.get(root) - } - fn len(&self) -> usize { self.map.len() } @@ -306,11 +409,6 @@ impl AggregateMap for SyncContributionAggregateMap { &self.map } - /// Returns an aggregated `SyncCommitteeContribution` with the given `root`, if any. - fn get_by_root(&self, root: &SyncDataRoot) -> Option<&SyncCommitteeContribution> { - self.map.get(root) - } - fn len(&self) -> usize { self.map.len() } @@ -445,13 +543,6 @@ where .and_then(|map| map.get(data)) } - /// 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 { - self.maps - .get(&slot) - .and_then(|map| map.get_by_root(root).cloned()) - } - /// Iterate all items in all slots of `self`. pub fn iter(&self) -> impl Iterator { self.maps.values().flat_map(|map| map.get_map().values()) @@ -500,19 +591,30 @@ mod tests { use super::*; use ssz_types::BitList; use store::BitVector; + use tree_hash::TreeHash; use types::{ test_utils::{generate_deterministic_keypair, test_random_instance}, - Fork, Hash256, SyncCommitteeMessage, + Attestation, AttestationBase, AttestationElectra, Fork, Hash256, SyncCommitteeMessage, }; type E = types::MainnetEthSpec; - fn get_attestation(slot: Slot) -> Attestation { - let mut a: Attestation = test_random_instance(); - a.data_mut().slot = slot; - *a.aggregation_bits_base_mut().unwrap() = - BitList::with_capacity(4).expect("should create bitlist"); - a + fn get_attestation_base(slot: Slot) -> Attestation { + let mut a: AttestationBase = test_random_instance(); + a.data.slot = slot; + a.aggregation_bits = BitList::with_capacity(4).expect("should create bitlist"); + Attestation::Base(a) + } + + fn get_attestation_electra(slot: Slot) -> Attestation { + let mut a: AttestationElectra = test_random_instance(); + a.data.slot = slot; + a.aggregation_bits = BitList::with_capacity(4).expect("should create bitlist"); + a.committee_bits = BitVector::new(); + a.committee_bits + .set(0, true) + .expect("should set committee bit"); + Attestation::Electra(a) } fn get_sync_contribution(slot: Slot) -> SyncCommitteeContribution { @@ -555,10 +657,16 @@ mod tests { } fn unset_attestation_bit(a: &mut Attestation, i: usize) { - a.aggregation_bits_base_mut() - .unwrap() - .set(i, false) - .expect("should unset aggregation bit") + match a { + Attestation::Base(ref mut att) => att + .aggregation_bits + .set(i, false) + .expect("should unset aggregation bit"), + Attestation::Electra(ref mut att) => att + .aggregation_bits + .set(i, false) + .expect("should unset aggregation bit"), + } } fn unset_sync_contribution_bit(a: &mut SyncCommitteeContribution, i: usize) { @@ -579,8 +687,8 @@ mod tests { a.data().beacon_block_root == block_root } - fn key_from_attestation(a: &Attestation) -> AttestationData { - a.data().clone() + fn key_from_attestation(a: &Attestation) -> AttestationKey { + AttestationKey::from_attestation_ref(a.to_ref()).expect("should create attestation key") } fn mutate_sync_contribution_block_root( @@ -605,6 +713,45 @@ mod tests { SyncContributionData::from_contribution(a) } + #[test] + fn attestation_key_tree_hash_tests() { + let attestation_base = get_attestation_base(Slot::new(42)); + // for a base attestation, the tree_hash_root() of the key should be the same as the tree_hash_root() of the data + let attestation_key_base = AttestationKey::from_attestation_ref(attestation_base.to_ref()) + .expect("should create attestation key"); + assert_eq!( + attestation_key_base.tree_hash_root(), + attestation_base.data().tree_hash_root() + ); + let mut attestation_electra = get_attestation_electra(Slot::new(42)); + // for an electra attestation, the tree_hash_root() of the key should be different from the tree_hash_root() of the data + let attestation_key_electra = + AttestationKey::from_attestation_ref(attestation_electra.to_ref()) + .expect("should create attestation key"); + assert_ne!( + attestation_key_electra.tree_hash_root(), + attestation_electra.data().tree_hash_root() + ); + // for an electra attestation, the tree_hash_root() of the key should be dependent on which committee bit is set + let committe_bits = attestation_electra + .committee_bits_mut() + .expect("should get committee bits"); + committe_bits + .set(0, false) + .expect("should set committee bit"); + committe_bits + .set(1, true) + .expect("should set committee bit"); + let new_attestation_key_electra = + AttestationKey::from_attestation_ref(attestation_electra.to_ref()) + .expect("should create attestation key"); + // this new key should have a different tree_hash_root() than the previous key + assert_ne!( + attestation_key_electra.tree_hash_root(), + new_attestation_key_electra.tree_hash_root() + ); + } + macro_rules! test_suite { ( $mod_name: ident, @@ -800,8 +947,21 @@ mod tests { } test_suite! { - attestation_tests, - get_attestation, + attestation_tests_base, + get_attestation_base, + sign_attestation, + unset_attestation_bit, + mutate_attestation_block_root, + mutate_attestation_slot, + attestation_block_root_comparator, + key_from_attestation, + AggregatedAttestationMap, + MAX_ATTESTATIONS_PER_SLOT + } + + test_suite! { + attestation_tests_electra, + get_attestation_electra, sign_attestation, unset_attestation_bit, mutate_attestation_block_root, diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index edfff4bf81e..bcf7582ebfd 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1374,7 +1374,7 @@ where // aggregate locally. let aggregate = self .chain - .get_aggregated_attestation(attestation.data()) + .get_aggregated_attestation_base(attestation.data()) .unwrap() .unwrap_or_else(|| { committee_attestations.iter().skip(1).fold( diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 8c9957db169..594872e2fff 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1228,10 +1228,7 @@ async fn attesting_to_optimistic_head() { let get_aggregated_by_slot_and_root = || { rig.harness .chain - .get_aggregated_attestation_by_slot_and_root( - attestation.data().slot, - &attestation.data().tree_hash_root(), - ) + .get_aggregated_attestation_base(attestation.data()) }; /* diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 6cb8f6fe0b9..838f7233052 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3191,7 +3191,7 @@ pub fn serve( task_spawner.blocking_json_task(Priority::P0, move || { not_synced_filter?; chain - .get_aggregated_attestation_by_slot_and_root( + .get_pre_electra_aggregated_attestation_by_slot_and_root( query.slot, &query.attestation_data_root, ) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 595cc69f87c..9848840e96d 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -113,11 +113,15 @@ pub mod attesting_indices_electra { .map(|committee| (committee.index, committee)) .collect(); + let committee_count_per_slot = committees.len() as u64; + let mut participant_count = 0; for index in committee_indices { if let Some(&beacon_committee) = committees_map.get(&index) { - if aggregation_bits.len() != beacon_committee.committee.len() { - return Err(BeaconStateError::InvalidBitfield); + // This check is new to the spec's `process_attestation` in Electra. + if index >= committee_count_per_slot { + return Err(BeaconStateError::InvalidCommitteeIndex(index)); } + participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; let committee_attesters = beacon_committee .committee .iter() @@ -136,10 +140,13 @@ pub mod attesting_indices_electra { committee_offset.safe_add(beacon_committee.committee.len())?; } else { - return Err(Error::NoCommitteeFound); + return Err(Error::NoCommitteeFound(index)); } + } - // TODO(electra) what should we do when theres no committee found for a given index? + // This check is new to the spec's `process_attestation` in Electra. + if participant_count as usize != aggregation_bits.len() { + return Err(BeaconStateError::InvalidBitfield); } let mut indices = output.into_iter().collect_vec(); diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 0f7e8468488..8c8a81b90f2 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -67,9 +67,9 @@ pub struct Attestation { #[superstruct(only(Electra), partial_getter(rename = "aggregation_bits_electra"))] pub aggregation_bits: BitList, pub data: AttestationData, - pub signature: AggregateSignature, #[superstruct(only(Electra))] pub committee_bits: BitVector, + pub signature: AggregateSignature, } impl Decode for Attestation { @@ -92,6 +92,7 @@ impl Decode for Attestation { } } +// TODO(electra): think about how to handle fork variants here impl TestRandom for Attestation { fn random_for_test(rng: &mut impl RngCore) -> Self { let aggregation_bits: BitList = BitList::random_for_test(rng); diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 599c0bfc39c..d9c7a78537a 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -159,7 +159,8 @@ pub enum Error { IndexNotSupported(usize), InvalidFlagIndex(usize), MerkleTreeError(merkle_proof::MerkleTreeError), - NoCommitteeFound, + NoCommitteeFound(CommitteeIndex), + InvalidCommitteeIndex(CommitteeIndex), } /// Control whether an epoch-indexed field can be indexed at the next epoch or not. diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index cec4db2da51..14949e67531 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -408,6 +408,8 @@ impl EthSpec for MainnetEthSpec { pub struct MinimalEthSpec; impl EthSpec for MinimalEthSpec { + type MaxCommitteesPerSlot = U4; + type MaxValidatorsPerSlot = U8192; type SlotsPerEpoch = U8; type EpochsPerEth1VotingPeriod = U4; type SlotsPerHistoricalRoot = U64; @@ -432,8 +434,6 @@ impl EthSpec for MinimalEthSpec { SubnetBitfieldLength, SyncCommitteeSubnetCount, MaxValidatorsPerCommittee, - MaxCommitteesPerSlot, - MaxValidatorsPerSlot, GenesisEpoch, HistoricalRootsLimit, ValidatorRegistryLimit, diff --git a/testing/ef_tests/src/type_name.rs b/testing/ef_tests/src/type_name.rs index 30db5c0e4a9..cbea78dabfc 100644 --- a/testing/ef_tests/src/type_name.rs +++ b/testing/ef_tests/src/type_name.rs @@ -111,11 +111,8 @@ type_name_generic!(LightClientUpdateDeneb, "LightClientUpdate"); type_name_generic!(PendingAttestation); type_name!(ProposerSlashing); type_name_generic!(SignedAggregateAndProof); -type_name_generic!(SignedAggregateAndProofBase, "SignedAggregateAndProofBase"); -type_name_generic!( - SignedAggregateAndProofElectra, - "SignedAggregateAndProofElectra" -); +type_name_generic!(SignedAggregateAndProofBase, "SignedAggregateAndProof"); +type_name_generic!(SignedAggregateAndProofElectra, "SignedAggregateAndProof"); type_name_generic!(SignedBeaconBlock); type_name!(SignedBeaconBlockHeader); type_name_generic!(SignedContributionAndProof); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 85d9362aaeb..fb8bdfcae71 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -219,7 +219,6 @@ mod ssz_static { use types::historical_summary::HistoricalSummary; use types::{AttesterSlashingBase, AttesterSlashingElectra, LightClientBootstrapAltair, *}; - ssz_static_test!(aggregate_and_proof, AggregateAndProof<_>); ssz_static_test!(attestation, Attestation<_>); ssz_static_test!(attestation_data, AttestationData); ssz_static_test!(beacon_block, SszStaticWithSpecHandler, BeaconBlock<_>); @@ -249,7 +248,7 @@ mod ssz_static { ssz_static_test!(voluntary_exit, VoluntaryExit); #[test] - fn signed_aggregate_and_proof() { + fn attester_slashing() { SszStaticHandler::, MinimalEthSpec>::pre_electra() .run(); SszStaticHandler::, MainnetEthSpec>::pre_electra() @@ -260,6 +259,36 @@ mod ssz_static { .run(); } + #[test] + fn signed_aggregate_and_proof() { + SszStaticHandler::, MinimalEthSpec>::pre_electra( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::pre_electra( + ) + .run(); + SszStaticHandler::, MinimalEthSpec>::electra_only( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::electra_only( + ) + .run(); + } + + #[test] + fn aggregate_and_proof() { + SszStaticHandler::, MinimalEthSpec>::pre_electra() + .run(); + SszStaticHandler::, MainnetEthSpec>::pre_electra() + .run(); + SszStaticHandler::, MinimalEthSpec>::electra_only( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::electra_only( + ) + .run(); + } + // BeaconBlockBody has no internal indicator of which fork it is for, so we test it separately. #[test] fn beacon_block_body() { @@ -283,22 +312,6 @@ mod ssz_static { .run(); } - #[test] - fn signed_aggregate_and_proof() { - SszStaticHandler::, MinimalEthSpec>::pre_electra( - ) - .run(); - SszStaticHandler::, MainnetEthSpec>::pre_electra( - ) - .run(); - SszStaticHandler::, MinimalEthSpec>::electra_only( - ) - .run(); - SszStaticHandler::, MainnetEthSpec>::electra_only( - ) - .run(); - } - // Altair and later #[test] fn contribution_and_proof() { From ca0967119b168fa1fd05f35d3961e7447446cbf6 Mon Sep 17 00:00:00 2001 From: ethDreamer <37123614+ethDreamer@users.noreply.github.com> Date: Thu, 9 May 2024 17:10:04 -0500 Subject: [PATCH 3/7] Revert "Get `electra_op_pool` up to date (#5756)" (#5757) This reverts commit ab9e58aa3d0e6fe2175a4996a5de710e81152896. --- .../src/attestation_verification.rs | 5 +- beacon_node/beacon_chain/src/beacon_chain.rs | 38 +-- .../src/naive_aggregation_pool.rs | 282 ++++-------------- beacon_node/beacon_chain/src/test_utils.rs | 2 +- .../tests/payload_invalidation.rs | 5 +- beacon_node/http_api/src/lib.rs | 2 +- .../src/common/get_attesting_indices.rs | 15 +- consensus/types/src/attestation.rs | 3 +- consensus/types/src/beacon_state.rs | 3 +- consensus/types/src/eth_spec.rs | 4 +- testing/ef_tests/src/type_name.rs | 7 +- testing/ef_tests/tests/tests.rs | 49 ++- 12 files changed, 108 insertions(+), 307 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 3d722a534be..62e65d5f87a 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -1309,11 +1309,10 @@ 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(); - if e == BlockOperationError::BeaconStateError(NoCommitteeFound(index)) { + if e == BlockOperationError::BeaconStateError(NoCommitteeFound) { Error::NoCommitteeForSlotAndIndex { slot: att.data.slot, - index, + index: att.committee_index(), } } else { Error::Invalid(e) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 331e04069fd..973dcaadb47 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1612,28 +1612,11 @@ impl BeaconChain { /// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`. /// /// The attestation will be obtained from `self.naive_aggregation_pool`. - pub fn get_aggregated_attestation_base( + pub fn get_aggregated_attestation( &self, data: &AttestationData, ) -> Result>, Error> { - let attestation_key = crate::naive_aggregation_pool::AttestationKey::new_base(data); - if let Some(attestation) = self.naive_aggregation_pool.read().get(&attestation_key) { - self.filter_optimistic_attestation(attestation) - .map(Option::Some) - } else { - Ok(None) - } - } - - // TODO(electra): call this function from the new beacon API method - pub fn get_aggregated_attestation_electra( - &self, - data: &AttestationData, - committee_index: CommitteeIndex, - ) -> Result>, Error> { - let attestation_key = - crate::naive_aggregation_pool::AttestationKey::new_electra(data, committee_index); - if let Some(attestation) = self.naive_aggregation_pool.read().get(&attestation_key) { + if let Some(attestation) = self.naive_aggregation_pool.read().get(data) { self.filter_optimistic_attestation(attestation) .map(Option::Some) } else { @@ -1645,21 +1628,16 @@ impl BeaconChain { /// `attestation.data.tree_hash_root()`. /// /// The attestation will be obtained from `self.naive_aggregation_pool`. - /// - /// NOTE: This function will *only* work with pre-electra attestations and it only - /// exists to support the pre-electra validator API method. - pub fn get_pre_electra_aggregated_attestation_by_slot_and_root( + pub fn get_aggregated_attestation_by_slot_and_root( &self, slot: Slot, attestation_data_root: &Hash256, ) -> Result>, Error> { - let attestation_key = - crate::naive_aggregation_pool::AttestationKey::new_base_from_slot_and_root( - slot, - *attestation_data_root, - ); - - if let Some(attestation) = self.naive_aggregation_pool.read().get(&attestation_key) { + if let Some(attestation) = self + .naive_aggregation_pool + .read() + .get_by_slot_and_root(slot, attestation_data_root) + { self.filter_optimistic_attestation(attestation) .map(Option::Some) } else { diff --git a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs index 6c4f7cdae72..a12521cd171 100644 --- a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs +++ b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs @@ -1,9 +1,7 @@ use crate::metrics; use crate::observed_aggregates::AsReference; -use itertools::Itertools; -use smallvec::SmallVec; use std::collections::HashMap; -use tree_hash::{MerkleHasher, TreeHash, TreeHashType}; +use tree_hash::TreeHash; use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT; use types::slot_data::SlotData; use types::sync_committee_contribution::SyncContributionData; @@ -11,114 +9,9 @@ use types::{ Attestation, AttestationData, AttestationRef, EthSpec, Hash256, Slot, SyncCommitteeContribution, }; -type AttestationKeyRoot = Hash256; +type AttestationDataRoot = Hash256; type SyncDataRoot = Hash256; -/// Post-Electra, we need a new key for Attestations that includes the committee index -#[derive(Debug, Clone, PartialEq)] -pub struct AttestationKey { - data_root: Hash256, - committee_index: Option, - slot: Slot, -} - -// A custom implementation of `TreeHash` such that: -// AttestationKey(data, None).tree_hash_root() == data.tree_hash_root() -// AttestationKey(data, Some(index)).tree_hash_root() == (data, index).tree_hash_root() -// This is necessary because pre-Electra, the validator will ask for the tree_hash_root() -// of the `AttestationData` -impl TreeHash for AttestationKey { - fn tree_hash_type() -> TreeHashType { - TreeHashType::Container - } - - fn tree_hash_packed_encoding(&self) -> SmallVec<[u8; 32]> { - unreachable!("AttestationKey should never be packed.") - } - - fn tree_hash_packing_factor() -> usize { - unreachable!("AttestationKey should never be packed.") - } - - fn tree_hash_root(&self) -> Hash256 { - match self.committee_index { - None => self.data_root, // Return just the data root if no committee index is present - Some(index) => { - // Combine the hash of the data with the hash of the index - let mut hasher = MerkleHasher::with_leaves(2); - hasher - .write(self.data_root.as_bytes()) - .expect("should write data hash"); - hasher - .write(&index.to_le_bytes()) - .expect("should write index"); - hasher.finish().expect("should give tree hash") - } - } - } -} - -impl AttestationKey { - pub fn from_attestation_ref(attestation: AttestationRef) -> Result { - let slot = attestation.data().slot; - match attestation { - AttestationRef::Base(att) => Ok(Self { - data_root: att.data.tree_hash_root(), - committee_index: None, - slot, - }), - AttestationRef::Electra(att) => { - let committee_index = att - .committee_bits - .iter() - .enumerate() - .filter_map(|(i, bit)| if bit { Some(i) } else { None }) - .at_most_one() - .map_err(|_| Error::MoreThanOneCommitteeBitSet)? - .ok_or(Error::NoCommitteeBitSet)?; - - Ok(Self { - data_root: att.data.tree_hash_root(), - committee_index: Some(committee_index as u64), - slot, - }) - } - } - } - - pub fn new_base(data: &AttestationData) -> Self { - let slot = data.slot; - Self { - data_root: data.tree_hash_root(), - committee_index: None, - slot, - } - } - - pub fn new_electra(data: &AttestationData, committee_index: u64) -> Self { - let slot = data.slot; - Self { - data_root: data.tree_hash_root(), - committee_index: Some(committee_index), - slot, - } - } - - pub fn new_base_from_slot_and_root(slot: Slot, data_root: Hash256) -> Self { - Self { - data_root, - committee_index: None, - slot, - } - } -} - -impl SlotData for AttestationKey { - fn get_slot(&self) -> Slot { - self.slot - } -} - /// 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 items @@ -156,10 +49,6 @@ pub enum Error { /// The given `aggregation_bits` field had more than one signature. The number of /// signatures found is included. MoreThanOneAggregationBitSet(usize), - /// The electra attestation has more than one committee bit set - MoreThanOneCommitteeBitSet, - /// The electra attestation has NO committee bit set - NoCommitteeBitSet, /// We have reached the maximum number of unique items that can be stored in a /// slot. This is a DoS protection function. ReachedMaxItemsPerSlot(usize), @@ -201,6 +90,9 @@ where /// Get a reference to the inner `HashMap`. fn get_map(&self) -> &HashMap; + /// 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; @@ -220,13 +112,13 @@ where /// A collection of `Attestation` objects, keyed by their `attestation.data`. Enforces that all /// `attestation` are from the same slot. pub struct AggregatedAttestationMap { - map: HashMap>, + map: HashMap>, } impl AggregateMap for AggregatedAttestationMap { - type Key = AttestationKeyRoot; + type Key = AttestationDataRoot; type Value = Attestation; - type Data = AttestationKey; + type Data = AttestationData; /// Create an empty collection with the given `initial_capacity`. fn new(initial_capacity: usize) -> Self { @@ -241,43 +133,45 @@ 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 { + let set_bits = 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)?, + .filter(|(_i, bit)| *bit) + .map(|(i, _bit)| i) + .collect::>(), 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)?, + .filter(|(_i, bit)| *bit) + .map(|(i, _bit)| i) + .collect::>(), }; - let attestation_key = AttestationKey::from_attestation_ref(a)?; - let attestation_data_root = attestation_key.tree_hash_root(); + let committee_index = set_bits + .first() + .copied() + .ok_or(Error::NoAggregationBitsSet)?; + + if set_bits.len() > 1 { + return Err(Error::MoreThanOneAggregationBitSet(set_bits.len())); + } + + let attestation_data_root = a.data().tree_hash_root(); if let Some(existing_attestation) = self.map.get_mut(&attestation_data_root) { if existing_attestation - .get_aggregation_bit(aggregation_bit) + .get_aggregation_bit(committee_index) .map_err(|_| Error::InconsistentBitfieldLengths)? { - Ok(InsertOutcome::SignatureAlreadyKnown { - committee_index: aggregation_bit, - }) + Ok(InsertOutcome::SignatureAlreadyKnown { committee_index }) } else { let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_AGGREGATION); existing_attestation.aggregate(a); - Ok(InsertOutcome::SignatureAggregated { - committee_index: aggregation_bit, - }) + Ok(InsertOutcome::SignatureAggregated { committee_index }) } } else { if self.map.len() >= MAX_ATTESTATIONS_PER_SLOT { @@ -286,9 +180,7 @@ impl AggregateMap for AggregatedAttestationMap { self.map .insert(attestation_data_root, a.clone_as_attestation()); - Ok(InsertOutcome::NewItemInserted { - committee_index: aggregation_bit, - }) + Ok(InsertOutcome::NewItemInserted { committee_index }) } } @@ -303,6 +195,11 @@ impl AggregateMap for AggregatedAttestationMap { &self.map } + /// Returns an aggregated `Attestation` with the given `root`, if any. + fn get_by_root(&self, root: &Self::Key) -> Option<&Self::Value> { + self.map.get(root) + } + fn len(&self) -> usize { self.map.len() } @@ -409,6 +306,11 @@ impl AggregateMap for SyncContributionAggregateMap { &self.map } + /// Returns an aggregated `SyncCommitteeContribution` with the given `root`, if any. + fn get_by_root(&self, root: &SyncDataRoot) -> Option<&SyncCommitteeContribution> { + self.map.get(root) + } + fn len(&self) -> usize { self.map.len() } @@ -543,6 +445,13 @@ where .and_then(|map| map.get(data)) } + /// 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 { + self.maps + .get(&slot) + .and_then(|map| map.get_by_root(root).cloned()) + } + /// Iterate all items in all slots of `self`. pub fn iter(&self) -> impl Iterator { self.maps.values().flat_map(|map| map.get_map().values()) @@ -591,30 +500,19 @@ mod tests { use super::*; use ssz_types::BitList; use store::BitVector; - use tree_hash::TreeHash; use types::{ test_utils::{generate_deterministic_keypair, test_random_instance}, - Attestation, AttestationBase, AttestationElectra, Fork, Hash256, SyncCommitteeMessage, + Fork, Hash256, SyncCommitteeMessage, }; type E = types::MainnetEthSpec; - fn get_attestation_base(slot: Slot) -> Attestation { - let mut a: AttestationBase = test_random_instance(); - a.data.slot = slot; - a.aggregation_bits = BitList::with_capacity(4).expect("should create bitlist"); - Attestation::Base(a) - } - - fn get_attestation_electra(slot: Slot) -> Attestation { - let mut a: AttestationElectra = test_random_instance(); - a.data.slot = slot; - a.aggregation_bits = BitList::with_capacity(4).expect("should create bitlist"); - a.committee_bits = BitVector::new(); - a.committee_bits - .set(0, true) - .expect("should set committee bit"); - Attestation::Electra(a) + fn get_attestation(slot: Slot) -> Attestation { + let mut a: Attestation = test_random_instance(); + a.data_mut().slot = slot; + *a.aggregation_bits_base_mut().unwrap() = + BitList::with_capacity(4).expect("should create bitlist"); + a } fn get_sync_contribution(slot: Slot) -> SyncCommitteeContribution { @@ -657,16 +555,10 @@ mod tests { } fn unset_attestation_bit(a: &mut Attestation, i: usize) { - match a { - Attestation::Base(ref mut att) => att - .aggregation_bits - .set(i, false) - .expect("should unset aggregation bit"), - Attestation::Electra(ref mut att) => att - .aggregation_bits - .set(i, false) - .expect("should unset aggregation bit"), - } + a.aggregation_bits_base_mut() + .unwrap() + .set(i, false) + .expect("should unset aggregation bit") } fn unset_sync_contribution_bit(a: &mut SyncCommitteeContribution, i: usize) { @@ -687,8 +579,8 @@ mod tests { a.data().beacon_block_root == block_root } - fn key_from_attestation(a: &Attestation) -> AttestationKey { - AttestationKey::from_attestation_ref(a.to_ref()).expect("should create attestation key") + fn key_from_attestation(a: &Attestation) -> AttestationData { + a.data().clone() } fn mutate_sync_contribution_block_root( @@ -713,45 +605,6 @@ mod tests { SyncContributionData::from_contribution(a) } - #[test] - fn attestation_key_tree_hash_tests() { - let attestation_base = get_attestation_base(Slot::new(42)); - // for a base attestation, the tree_hash_root() of the key should be the same as the tree_hash_root() of the data - let attestation_key_base = AttestationKey::from_attestation_ref(attestation_base.to_ref()) - .expect("should create attestation key"); - assert_eq!( - attestation_key_base.tree_hash_root(), - attestation_base.data().tree_hash_root() - ); - let mut attestation_electra = get_attestation_electra(Slot::new(42)); - // for an electra attestation, the tree_hash_root() of the key should be different from the tree_hash_root() of the data - let attestation_key_electra = - AttestationKey::from_attestation_ref(attestation_electra.to_ref()) - .expect("should create attestation key"); - assert_ne!( - attestation_key_electra.tree_hash_root(), - attestation_electra.data().tree_hash_root() - ); - // for an electra attestation, the tree_hash_root() of the key should be dependent on which committee bit is set - let committe_bits = attestation_electra - .committee_bits_mut() - .expect("should get committee bits"); - committe_bits - .set(0, false) - .expect("should set committee bit"); - committe_bits - .set(1, true) - .expect("should set committee bit"); - let new_attestation_key_electra = - AttestationKey::from_attestation_ref(attestation_electra.to_ref()) - .expect("should create attestation key"); - // this new key should have a different tree_hash_root() than the previous key - assert_ne!( - attestation_key_electra.tree_hash_root(), - new_attestation_key_electra.tree_hash_root() - ); - } - macro_rules! test_suite { ( $mod_name: ident, @@ -947,21 +800,8 @@ mod tests { } test_suite! { - attestation_tests_base, - get_attestation_base, - sign_attestation, - unset_attestation_bit, - mutate_attestation_block_root, - mutate_attestation_slot, - attestation_block_root_comparator, - key_from_attestation, - AggregatedAttestationMap, - MAX_ATTESTATIONS_PER_SLOT - } - - test_suite! { - attestation_tests_electra, - get_attestation_electra, + attestation_tests, + get_attestation, sign_attestation, unset_attestation_bit, mutate_attestation_block_root, diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index bcf7582ebfd..edfff4bf81e 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1374,7 +1374,7 @@ where // aggregate locally. let aggregate = self .chain - .get_aggregated_attestation_base(attestation.data()) + .get_aggregated_attestation(attestation.data()) .unwrap() .unwrap_or_else(|| { committee_attestations.iter().skip(1).fold( diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index 594872e2fff..8c9957db169 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1228,7 +1228,10 @@ async fn attesting_to_optimistic_head() { let get_aggregated_by_slot_and_root = || { rig.harness .chain - .get_aggregated_attestation_base(attestation.data()) + .get_aggregated_attestation_by_slot_and_root( + attestation.data().slot, + &attestation.data().tree_hash_root(), + ) }; /* diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 838f7233052..6cb8f6fe0b9 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3191,7 +3191,7 @@ pub fn serve( task_spawner.blocking_json_task(Priority::P0, move || { not_synced_filter?; chain - .get_pre_electra_aggregated_attestation_by_slot_and_root( + .get_aggregated_attestation_by_slot_and_root( query.slot, &query.attestation_data_root, ) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 9848840e96d..595cc69f87c 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -113,15 +113,11 @@ pub mod attesting_indices_electra { .map(|committee| (committee.index, committee)) .collect(); - let committee_count_per_slot = committees.len() as u64; - let mut participant_count = 0; for index in committee_indices { if let Some(&beacon_committee) = committees_map.get(&index) { - // This check is new to the spec's `process_attestation` in Electra. - if index >= committee_count_per_slot { - return Err(BeaconStateError::InvalidCommitteeIndex(index)); + if aggregation_bits.len() != beacon_committee.committee.len() { + return Err(BeaconStateError::InvalidBitfield); } - participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; let committee_attesters = beacon_committee .committee .iter() @@ -140,13 +136,10 @@ pub mod attesting_indices_electra { committee_offset.safe_add(beacon_committee.committee.len())?; } else { - return Err(Error::NoCommitteeFound(index)); + return Err(Error::NoCommitteeFound); } - } - // This check is new to the spec's `process_attestation` in Electra. - if participant_count as usize != aggregation_bits.len() { - return Err(BeaconStateError::InvalidBitfield); + // TODO(electra) what should we do when theres no committee found for a given index? } let mut indices = output.into_iter().collect_vec(); diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 8c8a81b90f2..0f7e8468488 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -67,9 +67,9 @@ pub struct Attestation { #[superstruct(only(Electra), partial_getter(rename = "aggregation_bits_electra"))] pub aggregation_bits: BitList, pub data: AttestationData, + pub signature: AggregateSignature, #[superstruct(only(Electra))] pub committee_bits: BitVector, - pub signature: AggregateSignature, } impl Decode for Attestation { @@ -92,7 +92,6 @@ impl Decode for Attestation { } } -// TODO(electra): think about how to handle fork variants here impl TestRandom for Attestation { fn random_for_test(rng: &mut impl RngCore) -> Self { let aggregation_bits: BitList = BitList::random_for_test(rng); diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index d9c7a78537a..599c0bfc39c 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -159,8 +159,7 @@ pub enum Error { IndexNotSupported(usize), InvalidFlagIndex(usize), MerkleTreeError(merkle_proof::MerkleTreeError), - NoCommitteeFound(CommitteeIndex), - InvalidCommitteeIndex(CommitteeIndex), + NoCommitteeFound, } /// Control whether an epoch-indexed field can be indexed at the next epoch or not. diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index 14949e67531..cec4db2da51 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -408,8 +408,6 @@ impl EthSpec for MainnetEthSpec { pub struct MinimalEthSpec; impl EthSpec for MinimalEthSpec { - type MaxCommitteesPerSlot = U4; - type MaxValidatorsPerSlot = U8192; type SlotsPerEpoch = U8; type EpochsPerEth1VotingPeriod = U4; type SlotsPerHistoricalRoot = U64; @@ -434,6 +432,8 @@ impl EthSpec for MinimalEthSpec { SubnetBitfieldLength, SyncCommitteeSubnetCount, MaxValidatorsPerCommittee, + MaxCommitteesPerSlot, + MaxValidatorsPerSlot, GenesisEpoch, HistoricalRootsLimit, ValidatorRegistryLimit, diff --git a/testing/ef_tests/src/type_name.rs b/testing/ef_tests/src/type_name.rs index cbea78dabfc..30db5c0e4a9 100644 --- a/testing/ef_tests/src/type_name.rs +++ b/testing/ef_tests/src/type_name.rs @@ -111,8 +111,11 @@ type_name_generic!(LightClientUpdateDeneb, "LightClientUpdate"); type_name_generic!(PendingAttestation); type_name!(ProposerSlashing); type_name_generic!(SignedAggregateAndProof); -type_name_generic!(SignedAggregateAndProofBase, "SignedAggregateAndProof"); -type_name_generic!(SignedAggregateAndProofElectra, "SignedAggregateAndProof"); +type_name_generic!(SignedAggregateAndProofBase, "SignedAggregateAndProofBase"); +type_name_generic!( + SignedAggregateAndProofElectra, + "SignedAggregateAndProofElectra" +); type_name_generic!(SignedBeaconBlock); type_name!(SignedBeaconBlockHeader); type_name_generic!(SignedContributionAndProof); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index fb8bdfcae71..85d9362aaeb 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -219,6 +219,7 @@ mod ssz_static { use types::historical_summary::HistoricalSummary; use types::{AttesterSlashingBase, AttesterSlashingElectra, LightClientBootstrapAltair, *}; + ssz_static_test!(aggregate_and_proof, AggregateAndProof<_>); ssz_static_test!(attestation, Attestation<_>); ssz_static_test!(attestation_data, AttestationData); ssz_static_test!(beacon_block, SszStaticWithSpecHandler, BeaconBlock<_>); @@ -248,7 +249,7 @@ mod ssz_static { ssz_static_test!(voluntary_exit, VoluntaryExit); #[test] - fn attester_slashing() { + fn signed_aggregate_and_proof() { SszStaticHandler::, MinimalEthSpec>::pre_electra() .run(); SszStaticHandler::, MainnetEthSpec>::pre_electra() @@ -259,36 +260,6 @@ mod ssz_static { .run(); } - #[test] - fn signed_aggregate_and_proof() { - SszStaticHandler::, MinimalEthSpec>::pre_electra( - ) - .run(); - SszStaticHandler::, MainnetEthSpec>::pre_electra( - ) - .run(); - SszStaticHandler::, MinimalEthSpec>::electra_only( - ) - .run(); - SszStaticHandler::, MainnetEthSpec>::electra_only( - ) - .run(); - } - - #[test] - fn aggregate_and_proof() { - SszStaticHandler::, MinimalEthSpec>::pre_electra() - .run(); - SszStaticHandler::, MainnetEthSpec>::pre_electra() - .run(); - SszStaticHandler::, MinimalEthSpec>::electra_only( - ) - .run(); - SszStaticHandler::, MainnetEthSpec>::electra_only( - ) - .run(); - } - // BeaconBlockBody has no internal indicator of which fork it is for, so we test it separately. #[test] fn beacon_block_body() { @@ -312,6 +283,22 @@ mod ssz_static { .run(); } + #[test] + fn signed_aggregate_and_proof() { + SszStaticHandler::, MinimalEthSpec>::pre_electra( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::pre_electra( + ) + .run(); + SszStaticHandler::, MinimalEthSpec>::electra_only( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::electra_only( + ) + .run(); + } + // Altair and later #[test] fn contribution_and_proof() { From 411fcee2ac879533870c4116b28e569a0012020c Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 10 May 2024 02:56:20 +0200 Subject: [PATCH 4/7] Compute on chain aggregate impl (#5752) * add compute_on_chain_agg impl to op pool changes * fmt * get op pool tests to pass --- .../operation_pool/src/attestation_storage.rs | 79 +++++++++++++++++-- beacon_node/operation_pool/src/lib.rs | 59 ++++++++++++-- 2 files changed, 124 insertions(+), 14 deletions(-) diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 5cf93e642c9..6e04af01b9c 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -187,6 +187,13 @@ impl CompactIndexedAttestation { _ => (), } } + + pub fn committee_index(&self) -> u64 { + match self { + CompactIndexedAttestation::Base(att) => att.index, + CompactIndexedAttestation::Electra(att) => att.committee_index(), + } + } } impl CompactIndexedAttestationBase { @@ -276,25 +283,34 @@ impl AttestationMap { let Some(attestation_map) = self.checkpoint_map.get_mut(&checkpoint_key) else { return; }; - for (compact_attestation_data, compact_indexed_attestations) in - attestation_map.attestations.iter_mut() - { + for (_, compact_indexed_attestations) in attestation_map.attestations.iter_mut() { let unaggregated_attestations = std::mem::take(compact_indexed_attestations); - let mut aggregated_attestations = vec![]; + let mut aggregated_attestations: Vec> = vec![]; // Aggregate the best attestations for each committee and leave the rest. - let mut best_attestations_by_committee = BTreeMap::new(); + let mut best_attestations_by_committee: BTreeMap> = + BTreeMap::new(); for committee_attestation in unaggregated_attestations { // TODO(electra) // compare to best attestations by committee // could probably use `.entry` here if let Some(existing_attestation) = - best_attestations_by_committee.get_mut(committee_attestation.committee_index()) + best_attestations_by_committee.get_mut(&committee_attestation.committee_index()) { // compare and swap, put the discarded one straight into // `aggregated_attestations` in case we have room to pack it without // cross-committee aggregation + if existing_attestation.should_aggregate(&committee_attestation) { + existing_attestation.aggregate(&committee_attestation); + + best_attestations_by_committee.insert( + committee_attestation.committee_index(), + committee_attestation, + ); + } else { + aggregated_attestations.push(committee_attestation); + } } else { best_attestations_by_committee.insert( committee_attestation.committee_index(), @@ -305,11 +321,62 @@ impl AttestationMap { // TODO(electra): aggregate all the best attestations by committee // (use btreemap sort order to get order by committee index) + aggregated_attestations.extend(Self::compute_on_chain_aggregate( + best_attestations_by_committee, + )); *compact_indexed_attestations = aggregated_attestations; } } + // TODO(electra) unwraps in this function should be cleaned up + // also in general this could be a bit more elegant + pub fn compute_on_chain_aggregate( + mut attestations_by_committee: BTreeMap>, + ) -> Vec> { + let mut aggregated_attestations = vec![]; + if let Some((_, on_chain_aggregate)) = attestations_by_committee.pop_first() { + match on_chain_aggregate { + CompactIndexedAttestation::Base(a) => { + aggregated_attestations.push(CompactIndexedAttestation::Base(a)); + aggregated_attestations.extend( + attestations_by_committee + .values() + .map(|a| { + CompactIndexedAttestation::Base(CompactIndexedAttestationBase { + attesting_indices: a.attesting_indices().clone(), + aggregation_bits: a.aggregation_bits_base().unwrap().clone(), + signature: a.signature().clone(), + index: *a.index(), + }) + }) + .collect::>>(), + ); + } + CompactIndexedAttestation::Electra(mut a) => { + for (_, attestation) in attestations_by_committee.iter_mut() { + let new_committee_bits = a + .committee_bits + .union(attestation.committee_bits().unwrap()); + a.aggregate(attestation.as_electra().unwrap()); + + a = CompactIndexedAttestationElectra { + attesting_indices: a.attesting_indices.clone(), + aggregation_bits: a.aggregation_bits.clone(), + signature: a.signature.clone(), + index: a.index, + committee_bits: new_committee_bits, + }; + } + + aggregated_attestations.push(CompactIndexedAttestation::Electra(a)); + } + } + } + + aggregated_attestations + } + /// Iterate all attestations matching the given `checkpoint_key`. pub fn get_attestations<'a>( &'a self, diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 1d83ecd4651..4f247e6bf20 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -1246,7 +1246,17 @@ mod release_tests { let num_big = target_committee_size / big_step_size; let stats = op_pool.attestation_stats(); - assert_eq!(stats.num_attestation_data, committees.len()); + let fork_name = state.fork_name_unchecked(); + + match fork_name { + ForkName::Electra => { + assert_eq!(stats.num_attestation_data, 1); + } + _ => { + assert_eq!(stats.num_attestation_data, committees.len()); + } + }; + assert_eq!( stats.num_attestations, (num_small + num_big) * committees.len() @@ -1257,11 +1267,27 @@ mod release_tests { let best_attestations = op_pool .get_attestations(&state, |_| true, |_| true, spec) .expect("should have best attestations"); - assert_eq!(best_attestations.len(), max_attestations); + match fork_name { + ForkName::Electra => { + assert_eq!(best_attestations.len(), 8); + } + _ => { + assert_eq!(best_attestations.len(), max_attestations); + } + }; // All the best attestations should be signed by at least `big_step_size` (4) validators. for att in &best_attestations { - assert!(att.num_set_aggregation_bits() >= big_step_size); + match fork_name { + ForkName::Electra => { + // TODO(electra) some attestations only have 2 or 3 agg bits set + // others have 5 + assert!(att.num_set_aggregation_bits() >= 2); + } + _ => { + assert!(att.num_set_aggregation_bits() >= big_step_size); + } + }; } } @@ -1340,11 +1366,20 @@ mod release_tests { let num_small = target_committee_size / small_step_size; let num_big = target_committee_size / big_step_size; + let fork_name = state.fork_name_unchecked(); + + match fork_name { + ForkName::Electra => { + assert_eq!(op_pool.attestation_stats().num_attestation_data, 1); + } + _ => { + assert_eq!( + op_pool.attestation_stats().num_attestation_data, + committees.len() + ); + } + }; - assert_eq!( - op_pool.attestation_stats().num_attestation_data, - committees.len() - ); assert_eq!( op_pool.num_attestations(), (num_small + num_big) * committees.len() @@ -1355,7 +1390,15 @@ mod release_tests { let best_attestations = op_pool .get_attestations(&state, |_| true, |_| true, spec) .expect("should have valid best attestations"); - assert_eq!(best_attestations.len(), max_attestations); + + match fork_name { + ForkName::Electra => { + assert_eq!(best_attestations.len(), 8); + } + _ => { + assert_eq!(best_attestations.len(), max_attestations); + } + }; let total_active_balance = state.get_total_active_balance().unwrap(); From 437e8516cd949481f374fbbc83e7ea6bf8236f62 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 10 May 2024 12:29:57 +1000 Subject: [PATCH 5/7] Fix bugs in cross-committee aggregation --- .../operation_pool/src/attestation_storage.rs | 193 ++++++++++-------- beacon_node/operation_pool/src/lib.rs | 6 +- 2 files changed, 111 insertions(+), 88 deletions(-) diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 6e04af01b9c..4cc783e64f2 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -4,7 +4,7 @@ use std::collections::{BTreeMap, HashMap}; use types::{ attestation::{AttestationBase, AttestationElectra}, superstruct, AggregateSignature, Attestation, AttestationData, BeaconState, BitList, BitVector, - Checkpoint, Epoch, EthSpec, Hash256, Slot, + Checkpoint, Epoch, EthSpec, Hash256, Slot, Unsigned, }; #[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)] @@ -30,7 +30,6 @@ pub struct CompactIndexedAttestation { #[superstruct(only(Electra), partial_getter(rename = "aggregation_bits_electra"))] pub aggregation_bits: BitList, pub signature: AggregateSignature, - pub index: u64, #[superstruct(only(Electra))] pub committee_bits: BitVector, } @@ -79,7 +78,6 @@ impl SplitAttestation { attesting_indices, aggregation_bits: attn.aggregation_bits, signature: attestation.signature().clone(), - index: data.index, }) } Attestation::Electra(attn) => { @@ -87,7 +85,6 @@ impl SplitAttestation { attesting_indices, aggregation_bits: attn.aggregation_bits, signature: attestation.signature().clone(), - index: data.index, committee_bits: attn.committee_bits, }) } @@ -182,18 +179,11 @@ impl CompactIndexedAttestation { ( CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other), - ) => this.aggregate(other), + ) => this.aggregate_same_committee(other), // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => (), } } - - pub fn committee_index(&self) -> u64 { - match self { - CompactIndexedAttestation::Base(att) => att.index, - CompactIndexedAttestation::Electra(att) => att.committee_index(), - } - } } impl CompactIndexedAttestationBase { @@ -225,14 +215,43 @@ impl CompactIndexedAttestationElectra { .is_zero() } - pub fn aggregate(&mut self, other: &Self) { + pub fn aggregate_same_committee(&mut self, other: &Self) { + // TODO(electra): remove assert in favour of Result + assert_eq!(self.committee_bits, other.committee_bits); + self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); + self.attesting_indices = self + .attesting_indices + .drain(..) + .merge(other.attesting_indices.iter().copied()) + .dedup() + .collect(); + self.signature.add_assign_aggregate(&other.signature); + } + + pub fn aggregate_with_disjoint_committees(&mut self, other: &Self) { + // TODO(electra): remove asserts or use Result + assert!( + self.committee_bits + .intersection(&other.committee_bits) + .is_zero(), + self.committee_bits, + other.committee_bits + ); + // The attestation being aggregated in must only have 1 committee bit set. + assert_eq!(other.committee_bits.num_set_bits(), 1); + // Check we are aggregating in increasing committee index order (so we can append + // aggregation bits). + assert!(self.committee_bits.highest_set_bit() < other.committee_bits.highest_set_bit()); + + self.committee_bits = self.committee_bits.union(&other.committee_bits); + self.aggregation_bits = + bitlist_extend(&self.aggregation_bits, &other.aggregation_bits).unwrap(); self.attesting_indices = self .attesting_indices .drain(..) .merge(other.attesting_indices.iter().copied()) .dedup() .collect(); - self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.signature.add_assign_aggregate(&other.signature); } @@ -249,6 +268,25 @@ impl CompactIndexedAttestationElectra { } } +// TODO(electra): upstream this or a more efficient implementation +fn bitlist_extend(list1: &BitList, list2: &BitList) -> Option> { + let new_length = list1.len() + list2.len(); + let mut list = BitList::::with_capacity(new_length).ok()?; + + // Copy bits from list1. + for (i, bit) in list1.iter().enumerate() { + list.set(i, bit).ok()?; + } + + // Copy bits from list2, starting from the end of list1. + let offset = list1.len(); + for (i, bit) in list2.iter().enumerate() { + list.set(offset + i, bit).ok()?; + } + + Some(list) +} + impl AttestationMap { pub fn insert(&mut self, attestation: Attestation, attesting_indices: Vec) { let SplitAttestation { @@ -279,102 +317,85 @@ impl AttestationMap { } } + /// Aggregate Electra attestations for the same attestation data signed by different + /// committees. + /// + /// Non-Electra attestations are left as-is. pub fn aggregate_across_committees(&mut self, checkpoint_key: CheckpointKey) { let Some(attestation_map) = self.checkpoint_map.get_mut(&checkpoint_key) else { return; }; - for (_, compact_indexed_attestations) in attestation_map.attestations.iter_mut() { + for compact_indexed_attestations in attestation_map.attestations.values_mut() { let unaggregated_attestations = std::mem::take(compact_indexed_attestations); let mut aggregated_attestations: Vec> = vec![]; // Aggregate the best attestations for each committee and leave the rest. - let mut best_attestations_by_committee: BTreeMap> = - BTreeMap::new(); + let mut best_attestations_by_committee: BTreeMap< + u64, + CompactIndexedAttestationElectra, + > = BTreeMap::new(); for committee_attestation in unaggregated_attestations { - // TODO(electra) - // compare to best attestations by committee - // could probably use `.entry` here + let mut electra_attestation = match committee_attestation { + CompactIndexedAttestation::Electra(att) + if att.committee_bits.num_set_bits() == 1 => + { + att + } + CompactIndexedAttestation::Electra(att) => { + // Aggregate already covers multiple committees, leave it as-is. + aggregated_attestations.push(CompactIndexedAttestation::Electra(att)); + continue; + } + CompactIndexedAttestation::Base(att) => { + // Leave as-is. + aggregated_attestations.push(CompactIndexedAttestation::Base(att)); + continue; + } + }; + let committee_index = electra_attestation.committee_index(); if let Some(existing_attestation) = - best_attestations_by_committee.get_mut(&committee_attestation.committee_index()) + best_attestations_by_committee.get_mut(&committee_index) { - // compare and swap, put the discarded one straight into - // `aggregated_attestations` in case we have room to pack it without - // cross-committee aggregation - if existing_attestation.should_aggregate(&committee_attestation) { - existing_attestation.aggregate(&committee_attestation); - - best_attestations_by_committee.insert( - committee_attestation.committee_index(), - committee_attestation, - ); - } else { - aggregated_attestations.push(committee_attestation); + // Search for the best (most aggregation bits) attestation for this committee + // index. + if electra_attestation.aggregation_bits.num_set_bits() + > existing_attestation.aggregation_bits.num_set_bits() + { + // New attestation is better than the previously known one for this + // committee. Replace it. + std::mem::swap(existing_attestation, &mut electra_attestation); } + // Put the inferior attestation into the list of aggregated attestations + // without performing any cross-committee aggregation. + aggregated_attestations + .push(CompactIndexedAttestation::Electra(electra_attestation)); } else { - best_attestations_by_committee.insert( - committee_attestation.committee_index(), - committee_attestation, - ); + // First attestation seen for this committee. Place it in the map + // provisionally. + best_attestations_by_committee.insert(committee_index, electra_attestation); } } - // TODO(electra): aggregate all the best attestations by committee - // (use btreemap sort order to get order by committee index) - aggregated_attestations.extend(Self::compute_on_chain_aggregate( - best_attestations_by_committee, - )); + if let Some(on_chain_aggregate) = + Self::compute_on_chain_aggregate(best_attestations_by_committee) + { + aggregated_attestations + .push(CompactIndexedAttestation::Electra(on_chain_aggregate)); + } *compact_indexed_attestations = aggregated_attestations; } } - // TODO(electra) unwraps in this function should be cleaned up - // also in general this could be a bit more elegant pub fn compute_on_chain_aggregate( - mut attestations_by_committee: BTreeMap>, - ) -> Vec> { - let mut aggregated_attestations = vec![]; - if let Some((_, on_chain_aggregate)) = attestations_by_committee.pop_first() { - match on_chain_aggregate { - CompactIndexedAttestation::Base(a) => { - aggregated_attestations.push(CompactIndexedAttestation::Base(a)); - aggregated_attestations.extend( - attestations_by_committee - .values() - .map(|a| { - CompactIndexedAttestation::Base(CompactIndexedAttestationBase { - attesting_indices: a.attesting_indices().clone(), - aggregation_bits: a.aggregation_bits_base().unwrap().clone(), - signature: a.signature().clone(), - index: *a.index(), - }) - }) - .collect::>>(), - ); - } - CompactIndexedAttestation::Electra(mut a) => { - for (_, attestation) in attestations_by_committee.iter_mut() { - let new_committee_bits = a - .committee_bits - .union(attestation.committee_bits().unwrap()); - a.aggregate(attestation.as_electra().unwrap()); - - a = CompactIndexedAttestationElectra { - attesting_indices: a.attesting_indices.clone(), - aggregation_bits: a.aggregation_bits.clone(), - signature: a.signature.clone(), - index: a.index, - committee_bits: new_committee_bits, - }; - } - - aggregated_attestations.push(CompactIndexedAttestation::Electra(a)); - } - } + mut attestations_by_committee: BTreeMap>, + ) -> Option> { + let (_, mut on_chain_aggregate) = attestations_by_committee.pop_first()?; + for (_, attestation) in attestations_by_committee { + on_chain_aggregate.aggregate_with_disjoint_committees(&attestation); } - - aggregated_attestations + Some(on_chain_aggregate) } /// Iterate all attestations matching the given `checkpoint_key`. diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 4f247e6bf20..daddbf76652 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -287,8 +287,10 @@ impl OperationPool { // TODO(electra): Work out how to do this more elegantly. This is a bit of a hack. let mut all_attestations = self.attestations.write(); - all_attestations.aggregate_across_committees(prev_epoch_key); - all_attestations.aggregate_across_committees(curr_epoch_key); + if fork_name >= ForkName::Electra { + all_attestations.aggregate_across_committees(prev_epoch_key); + all_attestations.aggregate_across_committees(curr_epoch_key); + } let all_attestations = parking_lot::RwLockWriteGuard::downgrade(all_attestations); From 16265ef455ca8c0f4c303bce89733bbd56646d1b Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 10 May 2024 12:44:18 +1000 Subject: [PATCH 6/7] Add comment to max cover optimisation --- beacon_node/operation_pool/src/attestation.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 207e2c65e45..91fd00a3979 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -144,6 +144,13 @@ impl<'a, E: EthSpec> MaxCover for AttMaxCover<'a, E> { /// because including two attestations on chain to satisfy different participation bits is /// impossible without the validator double voting. I.e. it is only suboptimal in the presence /// of slashable voting, which is rare. + /// + /// Post-Electra this optimisation is still OK. The `self.att.data.index` will always be 0 for + /// all Electra attestations, so when a new attestation is added to the solution, we will + /// remove its validators from all attestations at the same slot. It may happen that the + /// included attestation and the attestation being updated have no validators in common, in + /// which case the `retain` will be a no-op. We could consider optimising this in future by only + /// executing the `retain` when the `committee_bits` of the two attestations intersect. fn update_covering_set( &mut self, best_att: &AttestationRef<'a, E>, From 72548cb54e4a95f800e3b0591dba68ca0af7c6d5 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 10 May 2024 12:49:15 +1000 Subject: [PATCH 7/7] Fix assert --- beacon_node/operation_pool/src/attestation_storage.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 4cc783e64f2..f06da2afb17 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -230,13 +230,10 @@ impl CompactIndexedAttestationElectra { pub fn aggregate_with_disjoint_committees(&mut self, other: &Self) { // TODO(electra): remove asserts or use Result - assert!( - self.committee_bits - .intersection(&other.committee_bits) - .is_zero(), - self.committee_bits, - other.committee_bits - ); + assert!(self + .committee_bits + .intersection(&other.committee_bits) + .is_zero(),); // The attestation being aggregated in must only have 1 committee bit set. assert_eq!(other.committee_bits.num_set_bits(), 1); // Check we are aggregating in increasing committee index order (so we can append