From 1af04b38e16f304e4a5432cfc97ac95120fcd32f Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 6 May 2024 18:40:59 +0200 Subject: [PATCH 01/10] get attesting indices electra impl --- .../src/attestation_verification.rs | 139 ++++++++++---- .../http_api/src/block_packing_efficiency.rs | 21 ++- beacon_node/http_api/tests/tests.rs | 1 + .../src/sync/block_lookups/parent_chain.rs | 2 +- beacon_node/operation_pool/src/attestation.rs | 2 +- .../operation_pool/src/attestation_storage.rs | 11 +- beacon_node/operation_pool/src/lib.rs | 4 +- .../src/common/get_attesting_indices.rs | 175 ++++++++++++++++-- .../src/common/get_indexed_attestation.rs | 25 --- consensus/state_processing/src/common/mod.rs | 6 +- .../state_processing/src/consensus_context.rs | 56 +++--- .../base/validator_statuses.rs | 2 +- .../state_processing/src/upgrade/altair.rs | 4 +- consensus/types/src/attestation.rs | 19 ++ consensus/types/src/beacon_state.rs | 1 + lcli/src/indexed_attestations.rs | 13 +- 16 files changed, 361 insertions(+), 120 deletions(-) delete mode 100644 consensus/state_processing/src/common/get_indexed_attestation.rs diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index dff3fd6f2dd..28091847683 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -40,12 +40,13 @@ use crate::{ BeaconChain, BeaconChainError, BeaconChainTypes, }; use bls::verify_signature_sets; +use itertools::Itertools; use proto_array::Block as ProtoBlock; use slog::debug; use slot_clock::SlotClock; use state_processing::{ - common::get_indexed_attestation, - per_block_processing::errors::AttestationValidationError, + common::{attesting_indices_base, attesting_indices_electra}, + per_block_processing::errors::{AttestationValidationError, BlockOperationError}, signature_sets::{ indexed_attestation_signature_set_from_pubkeys, signed_aggregate_selection_proof_signature_set, signed_aggregate_signature_set, @@ -55,8 +56,9 @@ use std::borrow::Cow; use strum::AsRefStr; use tree_hash::TreeHash; use types::{ - Attestation, AttestationRef, BeaconCommittee, ChainSpec, CommitteeIndex, Epoch, EthSpec, - ForkName, Hash256, IndexedAttestation, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, + Attestation, AttestationRef, BeaconCommittee, BeaconStateError::NoCommitteeFound, ChainSpec, + CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, SelectionProof, + SignedAggregateAndProof, Slot, SubnetId, }; pub use batch::{batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations}; @@ -545,32 +547,59 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }; let get_indexed_attestation_with_committee = - |(committee, _): (BeaconCommittee, CommitteesPerSlot)| { - // Note: this clones the signature which is known to be a relatively slow operation. - // - // Future optimizations should remove this clone. - let selection_proof = - SelectionProof::from(signed_aggregate.message().selection_proof().clone()); - - if !selection_proof - .is_aggregator(committee.committee.len(), &chain.spec) - .map_err(|e| Error::BeaconChainError(e.into()))? - { - return Err(Error::InvalidSelectionProof { aggregator_index }); - } - - // Ensure the aggregator is a member of the committee for which it is aggregating. - if !committee.committee.contains(&(aggregator_index as usize)) { - return Err(Error::AggregatorNotInCommittee { aggregator_index }); + |(committees, _): (Vec, CommitteesPerSlot)| { + match attestation { + AttestationRef::Base(att) => { + let committee = committees + .iter() + .filter(|&committee| committee.index == att.data.index) + .at_most_one() + .map_err(|_| Error::NoCommitteeForSlotAndIndex { + slot: att.data.slot, + index: att.data.index, + })?; + + if let Some(committee) = committee { + // Note: this clones the signature which is known to be a relatively slow operation. + // + // Future optimizations should remove this clone. + let selection_proof = SelectionProof::from( + signed_aggregate.message().selection_proof().clone(), + ); + + if !selection_proof + .is_aggregator(committee.committee.len(), &chain.spec) + .map_err(|e| Error::BeaconChainError(e.into()))? + { + return Err(Error::InvalidSelectionProof { aggregator_index }); + } + + // Ensure the aggregator is a member of the committee for which it is aggregating. + if !committee.committee.contains(&(aggregator_index as usize)) { + return Err(Error::AggregatorNotInCommittee { aggregator_index }); + } + attesting_indices_base::get_indexed_attestation( + committee.committee, + att, + ) + .map_err(|e| BeaconChainError::from(e).into()) + } else { + Err(Error::NoCommitteeForSlotAndIndex { + slot: att.data.slot, + index: att.data.index, + }) + } + } + AttestationRef::Electra(att) => { + attesting_indices_electra::get_indexed_attestation(&committees, att) + .map_err(|e| BeaconChainError::from(e).into()) + } } - - get_indexed_attestation(committee.committee, attestation) - .map_err(|e| BeaconChainError::from(e).into()) }; - let indexed_attestation = match map_attestation_committee( + let indexed_attestation = match map_attestation_committees( chain, - attestation, + &attestation, get_indexed_attestation_with_committee, ) { Ok(indexed_attestation) => indexed_attestation, @@ -1252,13 +1281,49 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( chain: &BeaconChain, attestation: AttestationRef, ) -> Result<(IndexedAttestation, CommitteesPerSlot), Error> { - map_attestation_committee(chain, attestation, |(committee, committees_per_slot)| { - get_indexed_attestation(committee.committee, attestation) - .map(|attestation| (attestation, committees_per_slot)) - .map_err(Error::Invalid) + map_attestation_committees(chain, &attestation, |(committees, committees_per_slot)| { + match attestation { + AttestationRef::Base(att) => { + let committee = committees + .iter() + .filter(|&committee| committee.index == att.data.index) + .at_most_one() + .map_err(|_| Error::NoCommitteeForSlotAndIndex { + slot: att.data.slot, + index: att.data.index, + })?; + + if let Some(committee) = committee { + attesting_indices_base::get_indexed_attestation(committee.committee, att) + .map(|attestation| (attestation, committees_per_slot)) + .map_err(Error::Invalid) + } else { + Err(Error::NoCommitteeForSlotAndIndex { + slot: att.data.slot, + index: att.data.index, + }) + } + } + AttestationRef::Electra(att) => { + attesting_indices_electra::get_indexed_attestation(&committees, att) + .map(|attestation| (attestation, committees_per_slot)) + .map_err(|e| { + if e == BlockOperationError::BeaconStateError(NoCommitteeFound) { + Error::NoCommitteeForSlotAndIndex { + slot: att.data.slot, + index: att.committee_index(), + } + } else { + Error::Invalid(e) + } + }) + } + } }) } +// TODO(electra) update comments below to reflect logic changes +// i.e. this now runs the map_fn on a list of committees for the slot of the provided attestation /// Runs the `map_fn` with the committee and committee count per slot for the given `attestation`. /// /// This function exists in this odd "map" pattern because efficiently obtaining the committee for @@ -1268,14 +1333,14 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( /// /// If the committee for `attestation` isn't found in the `shuffling_cache`, we will read a state /// from disk and then update the `shuffling_cache`. -fn map_attestation_committee( +fn map_attestation_committees( chain: &BeaconChain, - attestation: AttestationRef, + attestation: &AttestationRef, map_fn: F, ) -> Result where T: BeaconChainTypes, - F: Fn((BeaconCommittee, CommitteesPerSlot)) -> Result, + F: Fn((Vec, CommitteesPerSlot)) -> Result, { let attestation_epoch = attestation.data().slot.epoch(T::EthSpec::slots_per_epoch()); let target = &attestation.data().target; @@ -1301,12 +1366,12 @@ where let committees_per_slot = committee_cache.committees_per_slot(); Ok(committee_cache - .get_beacon_committee(attestation.data().slot, attestation.data().index) - .map(|committee| map_fn((committee, committees_per_slot))) - .unwrap_or_else(|| { + .get_beacon_committees_at_slot(attestation.data().slot) + .map(|committees| map_fn((committees, committees_per_slot))) + .unwrap_or_else(|_| { Err(Error::NoCommitteeForSlotAndIndex { slot: attestation.data().slot, - index: attestation.data().index, + index: attestation.committee_index(), }) })) }) diff --git a/beacon_node/http_api/src/block_packing_efficiency.rs b/beacon_node/http_api/src/block_packing_efficiency.rs index 43dee5b6eb4..66c71872786 100644 --- a/beacon_node/http_api/src/block_packing_efficiency.rs +++ b/beacon_node/http_api/src/block_packing_efficiency.rs @@ -132,9 +132,24 @@ impl PackingEfficiencyHandler { } } } - // TODO(electra) implement electra variant - AttestationRef::Electra(_) => { - todo!() + AttestationRef::Electra(attn) => { + for (position, voted) in attn.aggregation_bits.iter().enumerate() { + if voted { + let unique_attestation = UniqueAttestation { + slot: attn.data.slot, + committee_index: attn.data.index, + committee_position: position, + }; + let inclusion_distance: u64 = block + .slot() + .as_u64() + .checked_sub(attn.data.slot.as_u64()) + .ok_or(PackingEfficiencyError::InvalidAttestationError)?; + + self.available_attestations.remove(&unique_attestation); + attestations_in_block.insert(unique_attestation, inclusion_distance); + } + } } } } diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 3653929bdab..ea1529f7178 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -3285,6 +3285,7 @@ impl ApiTester { .unwrap() .data; + // TODO(electra) make fork-agnostic let mut attestation = Attestation::Base(AttestationBase { aggregation_bits: BitList::with_capacity(duty.committee_length as usize).unwrap(), data: attestation_data, diff --git a/beacon_node/network/src/sync/block_lookups/parent_chain.rs b/beacon_node/network/src/sync/block_lookups/parent_chain.rs index 55f2cfe1292..7f4fe5119f6 100644 --- a/beacon_node/network/src/sync/block_lookups/parent_chain.rs +++ b/beacon_node/network/src/sync/block_lookups/parent_chain.rs @@ -55,7 +55,7 @@ pub(crate) fn compute_parent_chains(nodes: &[Node]) -> Vec { // Iterate blocks with no children for tip in nodes { let mut block_root = tip.block_root; - if parent_to_child.get(&block_root).is_none() { + if !parent_to_child.contains_key(&block_root) { let mut chain = vec![]; // Resolve chain of blocks diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 60074cde740..c0bfcab1561 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -2,7 +2,7 @@ use crate::attestation_storage::{AttestationRef, CompactIndexedAttestation}; use crate::max_cover::MaxCover; use crate::reward_cache::RewardCache; use state_processing::common::{ - base, get_attestation_participation_flag_indices, get_attesting_indices, + base, get_attestation_participation_flag_indices, attesting_indices_base::get_attesting_indices, }; use std::collections::HashMap; use types::{ diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 927e8c9b12c..d1b228fb5a6 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -81,8 +81,15 @@ impl SplitAttestation { index: data.index, }) } - // TODO(electra) implement electra variant - Attestation::Electra(_) => todo!(), + Attestation::Electra(attn) => { + CompactIndexedAttestation::Electra(CompactIndexedAttestationElectra { + attesting_indices, + aggregation_bits: attn.aggregation_bits, + signature: attestation.signature().clone(), + index: data.index, + committee_bits: attn.committee_bits, + }) + }, }; Self { diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index c2dc2732020..6645416d4b0 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -1060,7 +1060,9 @@ mod release_tests { op_pool .insert_attestation(att.clone_as_attestation(), attesting_indices.clone()) .unwrap(); - op_pool.insert_attestation(att.clone_as_attestation(), attesting_indices).unwrap(); + op_pool + .insert_attestation(att.clone_as_attestation(), attesting_indices) + .unwrap(); } assert_eq!(op_pool.num_attestations(), committees.len()); diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index e798f55f844..998559caa67 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -1,25 +1,162 @@ use types::*; -/// Returns validator indices which participated in the attestation, sorted by increasing index. -pub fn get_attesting_indices( - committee: &[usize], - bitlist: &BitList, -) -> Result, BeaconStateError> { - if bitlist.len() != committee.len() { - return Err(BeaconStateError::InvalidBitfield); +pub mod attesting_indices_base { + use crate::per_block_processing::errors::{AttestationInvalid as Invalid, BlockOperationError}; + use types::*; + + /// Convert `attestation` to (almost) indexed-verifiable form. + /// + /// Spec v0.12.1 + pub fn get_indexed_attestation( + committee: &[usize], + attestation: &AttestationBase, + ) -> Result, BlockOperationError> { + let attesting_indices = + get_attesting_indices::(committee, &attestation.aggregation_bits)?; + + Ok(IndexedAttestation::Base(IndexedAttestationBase { + attesting_indices: VariableList::new(attesting_indices)?, + data: attestation.data.clone(), + signature: attestation.signature.clone(), + })) } - let mut indices = Vec::with_capacity(bitlist.num_set_bits()); + /// Returns validator indices which participated in the attestation, sorted by increasing index. + pub fn get_attesting_indices( + committee: &[usize], + bitlist: &BitList, + ) -> Result, BeaconStateError> { + if bitlist.len() != committee.len() { + return Err(BeaconStateError::InvalidBitfield); + } + + let mut indices = Vec::with_capacity(bitlist.num_set_bits()); - for (i, validator_index) in committee.iter().enumerate() { - if let Ok(true) = bitlist.get(i) { - indices.push(*validator_index as u64) + for (i, validator_index) in committee.iter().enumerate() { + if let Ok(true) = bitlist.get(i) { + indices.push(*validator_index as u64) + } } + + indices.sort_unstable(); + + Ok(indices) + } +} + +pub mod attesting_indices_electra { + use std::collections::{HashMap, HashSet}; + + use crate::per_block_processing::errors::{AttestationInvalid as Invalid, BlockOperationError}; + use itertools::Itertools; + use safe_arith::SafeArith; + use types::*; + + pub fn get_indexed_attestation( + committees: &[BeaconCommittee], + attestation: &AttestationElectra, + ) -> Result, BlockOperationError> { + let attesting_indices = get_attesting_indices::( + committees, + &attestation.aggregation_bits, + &attestation.committee_bits, + )?; + + Ok(IndexedAttestation::Electra(IndexedAttestationElectra { + attesting_indices: VariableList::new(attesting_indices)?, + data: attestation.data.clone(), + signature: attestation.signature.clone(), + })) } - indices.sort_unstable(); + pub fn get_indexed_attestation_from_state( + beacon_state: &BeaconState, + attestation: &AttestationElectra, + ) -> Result, BlockOperationError> { + let committees = beacon_state.get_beacon_committees_at_slot(attestation.data.slot)?; + let attesting_indices = get_attesting_indices::( + &committees, + &attestation.aggregation_bits, + &attestation.committee_bits, + )?; + + Ok(IndexedAttestation::Electra(IndexedAttestationElectra { + attesting_indices: VariableList::new(attesting_indices)?, + data: attestation.data.clone(), + signature: attestation.signature.clone(), + })) + } - Ok(indices) + /// Shortcut for getting the attesting indices while fetching the committee from the state's cache. + pub fn get_attesting_indices_from_state( + state: &BeaconState, + att: &AttestationElectra, + ) -> Result, BeaconStateError> { + let committees = state.get_beacon_committees_at_slot(att.data.slot)?; + get_attesting_indices::(&committees, &att.aggregation_bits, &att.committee_bits) + } + + /// Returns validator indices which participated in the attestation, sorted by increasing index. + pub fn get_attesting_indices( + committees: &[BeaconCommittee], + aggregation_bits: &BitList, + committee_bits: &BitVector, + ) -> Result, BeaconStateError> { + let mut output: HashSet = HashSet::new(); + + let committee_indices = get_committee_indices::(committee_bits); + + let committee_offset = 0; + + let committees_map: HashMap = committees + .iter() + .map(|committee| (committee.index, committee)) + .collect(); + + 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); + } + let committee_attesters = beacon_committee + .committee + .iter() + .enumerate() + .filter_map(|(i, &index)| { + if let Ok(aggregation_bit_index) = committee_offset.safe_add(i) { + if aggregation_bits.get(aggregation_bit_index).unwrap_or(false) { + return Some(index as u64); + } + } + None + }) + .collect::>(); + + output.extend(committee_attesters); + + committee_offset.safe_add(beacon_committee.committee.len())?; + } else { + return Err(Error::NoCommitteeFound); + } + + // TODO(electra) what should we do when theres no committee found for a given index? + } + + let mut indices = output.into_iter().collect_vec(); + indices.sort_unstable(); + + Ok(indices) + } + + fn get_committee_indices( + committee_bits: &BitVector, + ) -> Vec { + committee_bits + .iter() + .enumerate() + .filter_map(|(index, bit)| if bit { Some(index as u64) } else { None }) + .collect() + } } /// Shortcut for getting the attesting indices while fetching the committee from the state's cache. @@ -27,12 +164,16 @@ pub fn get_attesting_indices_from_state( state: &BeaconState, att: AttestationRef, ) -> Result, BeaconStateError> { - let committee = state.get_beacon_committee(att.data().slot, att.data().index)?; match att { AttestationRef::Base(att) => { - get_attesting_indices::(committee.committee, &att.aggregation_bits) + let committee = state.get_beacon_committee(att.data.slot, att.data.index)?; + attesting_indices_base::get_attesting_indices::( + committee.committee, + &att.aggregation_bits, + ) + } + AttestationRef::Electra(att) => { + attesting_indices_electra::get_attesting_indices_from_state::(state, att) } - // TODO(electra) implement get_attesting_indices for electra - AttestationRef::Electra(_) => todo!(), } } diff --git a/consensus/state_processing/src/common/get_indexed_attestation.rs b/consensus/state_processing/src/common/get_indexed_attestation.rs deleted file mode 100644 index a04d9198c68..00000000000 --- a/consensus/state_processing/src/common/get_indexed_attestation.rs +++ /dev/null @@ -1,25 +0,0 @@ -use super::get_attesting_indices; -use crate::per_block_processing::errors::{AttestationInvalid as Invalid, BlockOperationError}; -use types::{indexed_attestation::IndexedAttestationBase, *}; - -type Result = std::result::Result>; - -/// Convert `attestation` to (almost) indexed-verifiable form. -/// -/// Spec v0.12.1 -pub fn get_indexed_attestation( - committee: &[usize], - attestation: AttestationRef, -) -> Result> { - let attesting_indices = match attestation { - AttestationRef::Base(att) => get_attesting_indices::(committee, &att.aggregation_bits)?, - // TODO(electra) implement get_attesting_indices for electra - AttestationRef::Electra(_) => todo!(), - }; - - Ok(IndexedAttestation::Base(IndexedAttestationBase { - attesting_indices: VariableList::new(attesting_indices)?, - data: attestation.data().clone(), - signature: attestation.signature().clone(), - })) -} diff --git a/consensus/state_processing/src/common/mod.rs b/consensus/state_processing/src/common/mod.rs index cefc47b0235..0287748fd04 100644 --- a/consensus/state_processing/src/common/mod.rs +++ b/consensus/state_processing/src/common/mod.rs @@ -1,7 +1,6 @@ mod deposit_data_tree; mod get_attestation_participation; mod get_attesting_indices; -mod get_indexed_attestation; mod initiate_validator_exit; mod slash_validator; @@ -11,8 +10,9 @@ pub mod update_progressive_balances_cache; pub use deposit_data_tree::DepositDataTree; pub use get_attestation_participation::get_attestation_participation_flag_indices; -pub use get_attesting_indices::{get_attesting_indices, get_attesting_indices_from_state}; -pub use get_indexed_attestation::get_indexed_attestation; +pub use get_attesting_indices::{ + attesting_indices_base, attesting_indices_electra, get_attesting_indices_from_state, +}; pub use initiate_validator_exit::initiate_validator_exit; pub use slash_validator::slash_validator; diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index b28f218fc8a..c3e92ddb341 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -1,4 +1,4 @@ -use crate::common::get_indexed_attestation; +use crate::common::{attesting_indices_base, attesting_indices_electra}; use crate::per_block_processing::errors::{AttestationInvalid, BlockOperationError}; use crate::EpochCacheError; use std::collections::{hash_map::Entry, HashMap}; @@ -159,32 +159,40 @@ impl ConsensusContext { state: &BeaconState, attestation: AttestationRef<'a, E>, ) -> Result, BlockOperationError> { - let aggregation_bits = match attestation { + match attestation { AttestationRef::Base(attn) => { - let mut extended_aggregation_bits: BitList = - BitList::with_capacity(attn.aggregation_bits.len()) - .map_err(BeaconStateError::from)?; - - for (i, bit) in attn.aggregation_bits.iter().enumerate() { - extended_aggregation_bits - .set(i, bit) - .map_err(BeaconStateError::from)?; + let extended_aggregation_bits = attn + .extend_aggregation_bits() + .map_err(BeaconStateError::from)?; + + let key = (attn.data.clone(), extended_aggregation_bits); + + match self.indexed_attestations.entry(key) { + Entry::Occupied(occupied) => Ok(occupied.into_mut()), + Entry::Vacant(vacant) => { + let committee = + state.get_beacon_committee(attn.data.slot, attn.data.index)?; + let indexed_attestation = attesting_indices_base::get_indexed_attestation( + committee.committee, + attn, + )?; + Ok(vacant.insert(indexed_attestation)) + } } - extended_aggregation_bits } - AttestationRef::Electra(attn) => attn.aggregation_bits.clone(), - }; - - let key = (attestation.data().clone(), aggregation_bits); - - match self.indexed_attestations.entry(key) { - Entry::Occupied(occupied) => Ok(occupied.into_mut()), - Entry::Vacant(vacant) => { - let committee = state - .get_beacon_committee(attestation.data().slot, attestation.data().index)?; - let indexed_attestation = - get_indexed_attestation(committee.committee, attestation)?; - Ok(vacant.insert(indexed_attestation)) + AttestationRef::Electra(attn) => { + let key = (attn.data.clone(), attn.aggregation_bits.clone()); + + match self.indexed_attestations.entry(key) { + Entry::Occupied(occupied) => Ok(occupied.into_mut()), + Entry::Vacant(vacant) => { + let indexed_attestation = + attesting_indices_electra::get_indexed_attestation_from_state( + state, attn, + )?; + Ok(vacant.insert(indexed_attestation)) + } + } } } .map(|indexed_attestation| (*indexed_attestation).to_ref()) diff --git a/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs b/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs index 7e244058038..edf75e7c5eb 100644 --- a/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs +++ b/consensus/state_processing/src/per_epoch_processing/base/validator_statuses.rs @@ -1,4 +1,4 @@ -use crate::common::get_attesting_indices; +use crate::common::attesting_indices_base::get_attesting_indices; use safe_arith::SafeArith; use types::{BeaconState, BeaconStateError, ChainSpec, Epoch, EthSpec, PendingAttestation}; diff --git a/consensus/state_processing/src/upgrade/altair.rs b/consensus/state_processing/src/upgrade/altair.rs index 872560db3df..3006da25ae7 100644 --- a/consensus/state_processing/src/upgrade/altair.rs +++ b/consensus/state_processing/src/upgrade/altair.rs @@ -1,5 +1,7 @@ use crate::common::update_progressive_balances_cache::initialize_progressive_balances_cache; -use crate::common::{get_attestation_participation_flag_indices, get_attesting_indices}; +use crate::common::{ + attesting_indices_base::get_attesting_indices, get_attestation_participation_flag_indices, +}; use std::mem; use std::sync::Arc; use types::{ diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 4676a100be6..d479ca9fc1c 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -236,6 +236,13 @@ impl<'a, E: EthSpec> AttestationRef<'a, E> { Self::Electra(att) => att.aggregation_bits.num_set_bits(), } } + + pub fn committee_index(&self) -> u64 { + match self { + AttestationRef::Base(att) => att.data.index, + AttestationRef::Electra(att) => att.committee_index(), + } + } } impl AttestationElectra { @@ -381,6 +388,18 @@ impl AttestationBase { Ok(()) } } + + pub fn extend_aggregation_bits( + &self, + ) -> Result, ssz_types::Error> { + let mut extended_aggregation_bits: BitList = + BitList::with_capacity(self.aggregation_bits.len())?; + + for (i, bit) in self.aggregation_bits.iter().enumerate() { + extended_aggregation_bits.set(i, bit)?; + } + Ok(extended_aggregation_bits) + } } impl SlotData for Attestation { diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 577f282a556..599c0bfc39c 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -159,6 +159,7 @@ pub enum Error { IndexNotSupported(usize), InvalidFlagIndex(usize), MerkleTreeError(merkle_proof::MerkleTreeError), + NoCommitteeFound, } /// Control whether an epoch-indexed field can be indexed at the next epoch or not. diff --git a/lcli/src/indexed_attestations.rs b/lcli/src/indexed_attestations.rs index 8d932ba7600..ccc14171128 100644 --- a/lcli/src/indexed_attestations.rs +++ b/lcli/src/indexed_attestations.rs @@ -1,6 +1,6 @@ use clap::ArgMatches; use clap_utils::parse_required; -use state_processing::common::get_indexed_attestation; +use state_processing::common::{attesting_indices_base, attesting_indices_electra}; use std::fs::File; use std::io::Read; use std::path::{Path, PathBuf}; @@ -33,9 +33,14 @@ pub fn run(matches: &ArgMatches) -> Result<(), String> { let indexed_attestations = attestations .into_iter() - .map(|att| { - let committee = state.get_beacon_committee(att.data().slot, att.data().index)?; - get_indexed_attestation(committee.committee, att.to_ref()) + .map(|att| match att { + Attestation::Base(att) => { + let committee = state.get_beacon_committee(att.data.slot, att.data.index)?; + attesting_indices_base::get_indexed_attestation(committee.committee, &att) + } + Attestation::Electra(att) => { + attesting_indices_electra::get_indexed_attestation_from_state(&state, &att) + } }) .collect::, _>>() .map_err(|e| format!("Error constructing indexed attestation: {:?}", e))?; From ed1cfcf5f8634ce6f28c81913bbbc098013bed78 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 6 May 2024 18:44:52 +0200 Subject: [PATCH 02/10] fmt --- beacon_node/operation_pool/src/attestation.rs | 2 +- beacon_node/operation_pool/src/attestation_storage.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index c0bfcab1561..207e2c65e45 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -2,7 +2,7 @@ use crate::attestation_storage::{AttestationRef, CompactIndexedAttestation}; use crate::max_cover::MaxCover; use crate::reward_cache::RewardCache; use state_processing::common::{ - base, get_attestation_participation_flag_indices, attesting_indices_base::get_attesting_indices, + attesting_indices_base::get_attesting_indices, base, get_attestation_participation_flag_indices, }; use std::collections::HashMap; use types::{ diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index d1b228fb5a6..0dde36f0b52 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -89,7 +89,7 @@ impl SplitAttestation { index: data.index, committee_bits: attn.committee_bits, }) - }, + } }; Self { From d455c1d29f25d827255e9506892fe0e12689bcd2 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 13:34:16 +0200 Subject: [PATCH 03/10] get tests to pass --- beacon_node/beacon_chain/src/beacon_chain.rs | 42 ++++++++---- .../beacon_chain/src/early_attester_cache.rs | 46 +++++++++---- .../beacon_chain/src/observed_aggregates.rs | 32 +++++---- beacon_node/beacon_chain/src/test_utils.rs | 66 +++++++++++++------ .../operation_pool/src/attestation_storage.rs | 59 +++++++++++++---- beacon_node/store/src/consensus_context.rs | 2 +- consensus/fork_choice/tests/tests.rs | 9 ++- .../src/common/get_attesting_indices.rs | 2 +- .../state_processing/src/consensus_context.rs | 4 +- consensus/types/src/attestation.rs | 6 +- consensus/types/src/beacon_block.rs | 5 +- consensus/types/src/eth_spec.rs | 8 +-- consensus/types/src/indexed_attestation.rs | 2 +- slasher/src/attester_record.rs | 2 +- 14 files changed, 199 insertions(+), 86 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index cd47068977b..973dcaadb47 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1917,18 +1917,36 @@ impl BeaconChain { }; drop(cache_timer); - // TODO(electra) implement electra variant - Ok(Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(committee_len)?, - data: AttestationData { - slot: request_slot, - index: request_index, - beacon_block_root, - source: justified_checkpoint, - target, - }, - signature: AggregateSignature::empty(), - })) + if self.spec.fork_name_at_slot::(request_slot) >= ForkName::Electra { + let mut committee_bits = BitVector::default(); + if committee_len > 0 { + committee_bits.set(request_index as usize, true)?; + } + Ok(Attestation::Electra(AttestationElectra { + aggregation_bits: BitList::with_capacity(committee_len)?, + data: AttestationData { + slot: request_slot, + index: 0u64, + beacon_block_root, + source: justified_checkpoint, + target, + }, + committee_bits, + signature: AggregateSignature::empty(), + })) + } else { + Ok(Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(committee_len)?, + data: AttestationData { + slot: request_slot, + index: request_index, + beacon_block_root, + source: justified_checkpoint, + target, + }, + signature: AggregateSignature::empty(), + })) + } } /// Performs the same validation as `Self::verify_unaggregated_attestation_for_gossip`, but for diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 595f941235c..8ed4e5db40b 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -124,18 +124,40 @@ impl EarlyAttesterCache { .get_committee_length::(request_slot, request_index, spec)?; // TODO(electra) make fork-agnostic - let attestation = Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(committee_len) - .map_err(BeaconStateError::from)?, - data: AttestationData { - slot: request_slot, - index: request_index, - beacon_block_root: item.beacon_block_root, - source: item.source, - target: item.target, - }, - signature: AggregateSignature::empty(), - }); + let attestation = if spec.fork_name_at_slot::(request_slot) >= ForkName::Electra { + let mut committee_bits = BitVector::default(); + if committee_len > 0 { + committee_bits + .set(request_index as usize, true) + .map_err(BeaconStateError::from)?; + } + Attestation::Electra(AttestationElectra { + aggregation_bits: BitList::with_capacity(committee_len) + .map_err(BeaconStateError::from)?, + committee_bits, + data: AttestationData { + slot: request_slot, + index: 0u64, + beacon_block_root: item.beacon_block_root, + source: item.source, + target: item.target, + }, + signature: AggregateSignature::empty(), + }) + } else { + Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(committee_len) + .map_err(BeaconStateError::from)?, + data: AttestationData { + slot: request_slot, + index: request_index, + beacon_block_root: item.beacon_block_root, + source: item.source, + target: item.target, + }, + signature: AggregateSignature::empty(), + }) + }; metrics::inc_counter(&metrics::BEACON_EARLY_ATTESTER_CACHE_HITS); diff --git a/beacon_node/beacon_chain/src/observed_aggregates.rs b/beacon_node/beacon_chain/src/observed_aggregates.rs index 857b0edb348..1f14fe62570 100644 --- a/beacon_node/beacon_chain/src/observed_aggregates.rs +++ b/beacon_node/beacon_chain/src/observed_aggregates.rs @@ -20,7 +20,7 @@ pub type ObservedSyncContributions = ObservedAggregates< pub type ObservedAggregateAttestations = ObservedAggregates< Attestation, E, - BitList<::MaxValidatorsPerCommittee>, + BitList<::MaxValidatorsPerSlot>, >; /// A trait use to associate capacity constants with the type being stored in `ObservedAggregates`. @@ -103,29 +103,39 @@ pub trait SubsetItem { } impl<'a, E: EthSpec> SubsetItem for AttestationRef<'a, E> { - type Item = BitList; + type Item = BitList; fn is_subset(&self, other: &Self::Item) -> bool { match self { - Self::Base(att) => att.aggregation_bits.is_subset(other), - // TODO(electra) implement electra variant - Self::Electra(_) => todo!(), + Self::Base(att) => { + if let Ok(extended_aggregation_bits) = att.extend_aggregation_bits() { + return extended_aggregation_bits.is_subset(other); + } + false + }, + Self::Electra(att) => att.aggregation_bits.is_subset(other), } } fn is_superset(&self, other: &Self::Item) -> bool { match self { - Self::Base(att) => other.is_subset(&att.aggregation_bits), - // TODO(electra) implement electra variant - Self::Electra(_) => todo!(), + Self::Base(att) => { + if let Ok(extended_aggregation_bits) = att.extend_aggregation_bits() { + return other.is_subset(&extended_aggregation_bits); + } + false + }, + Self::Electra(att) => other.is_subset(&att.aggregation_bits), } } /// Returns the sync contribution aggregation bits. fn get_item(&self) -> Self::Item { match self { - Self::Base(att) => att.aggregation_bits.clone(), - // TODO(electra) implement electra variant - Self::Electra(_) => todo!(), + Self::Base(att) => { + // TODO(electra) fix unwrap + att.extend_aggregation_bits().unwrap() + } + Self::Electra(att) => att.aggregation_bits.clone(), } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index ff439742d11..6c096d8bbdc 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1032,21 +1032,40 @@ where *state.get_block_root(target_slot)? }; - // TODO(electra) make test fork-agnostic - Ok(Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(committee_len)?, - data: AttestationData { - slot, - index, - beacon_block_root, - source: state.current_justified_checkpoint(), - target: Checkpoint { - epoch, - root: target_root, + if self.spec.fork_name_at_slot::(slot) >= ForkName::Electra { + let mut committee_bits = BitVector::default(); + committee_bits.set(index as usize, true)?; + Ok(Attestation::Electra(AttestationElectra { + aggregation_bits: BitList::with_capacity(committee_len)?, + committee_bits, + data: AttestationData { + slot, + index: 0u64, + beacon_block_root, + source: state.current_justified_checkpoint(), + target: Checkpoint { + epoch, + root: target_root, + }, }, - }, - signature: AggregateSignature::empty(), - })) + signature: AggregateSignature::empty(), + })) + } else { + Ok(Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(committee_len)?, + data: AttestationData { + slot, + index, + beacon_block_root, + source: state.current_justified_checkpoint(), + target: Checkpoint { + epoch, + root: target_root, + }, + }, + signature: AggregateSignature::empty(), + })) + } } /// A list of attestations for each committee for the given slot. @@ -1121,11 +1140,20 @@ where ) .unwrap(); - attestation - .aggregation_bits_base_mut() - .unwrap() - .set(i, true) - .unwrap(); + match attestation { + Attestation::Base(ref mut att) => { + att + .aggregation_bits + .set(i, true) + .unwrap() + }, + Attestation::Electra(ref mut att) => { + att + .aggregation_bits + .set(i, true) + .unwrap() + }, + } *attestation.signature_mut() = { let domain = self.spec.get_domain( diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 0dde36f0b52..e6a3dc2162d 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -28,7 +28,7 @@ pub struct CompactIndexedAttestation { #[superstruct(only(Base), partial_getter(rename = "aggregation_bits_base"))] pub aggregation_bits: BitList, #[superstruct(only(Electra), partial_getter(rename = "aggregation_bits_electra"))] - pub aggregation_bits: BitList, + pub aggregation_bits: BitList, pub signature: AggregateSignature, pub index: u64, #[superstruct(only(Electra))] @@ -164,8 +164,8 @@ impl CompactIndexedAttestation { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { this.signers_disjoint_from(other) } - (CompactIndexedAttestation::Electra(_), CompactIndexedAttestation::Electra(_)) => { - todo!() + (CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other)) => { + this.signers_disjoint_from(other) } // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => false, @@ -177,8 +177,8 @@ impl CompactIndexedAttestation { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { this.aggregate(other) } - (CompactIndexedAttestation::Electra(_), CompactIndexedAttestation::Electra(_)) => { - todo!() + (CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other)) => { + this.aggregate(other) } // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => (), @@ -205,13 +205,34 @@ 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() + } + + // TODO(electra) update to match spec requirements + pub fn aggregate(&mut self, other: &Self) { + 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); + } +} + impl AttestationMap { pub fn insert(&mut self, attestation: Attestation, attesting_indices: Vec) { let SplitAttestation { checkpoint, data, indexed, - } = SplitAttestation::new(attestation, attesting_indices); + } = SplitAttestation::new(attestation.clone(), attesting_indices); let attestation_map = self.checkpoint_map.entry(checkpoint).or_default(); let attestations = attestation_map.attestations.entry(data).or_default(); @@ -220,14 +241,24 @@ impl AttestationMap { // NOTE: this is sub-optimal and in future we will remove this in favour of max-clique // aggregation. let mut aggregated = false; - 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; - } - } + + 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; + } + } + }, + // 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); diff --git a/beacon_node/store/src/consensus_context.rs b/beacon_node/store/src/consensus_context.rs index 727423d172f..a1d2e674526 100644 --- a/beacon_node/store/src/consensus_context.rs +++ b/beacon_node/store/src/consensus_context.rs @@ -24,7 +24,7 @@ pub struct OnDiskConsensusContext { indexed_attestations: HashMap< ( AttestationData, - BitList, + BitList, ), IndexedAttestation, >, diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 63bf3b51bd0..3b05cfab1fd 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -830,8 +830,13 @@ async fn invalid_attestation_empty_bitfield() { .await .apply_attestation_to_chain( MutationDelay::NoDelay, - |attestation, _| { - *attestation.attesting_indices_base_mut().unwrap() = vec![].into(); + |attestation, _| match attestation { + IndexedAttestation::Base(ref mut att) => { + att.attesting_indices = vec![].into(); + } + IndexedAttestation::Electra(ref mut att) => { + att.attesting_indices = vec![].into(); + } }, |result| { assert_invalid_attestation!(result, InvalidAttestation::EmptyAggregationBitfield) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 998559caa67..595cc69f87c 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -99,7 +99,7 @@ pub mod attesting_indices_electra { /// Returns validator indices which participated in the attestation, sorted by increasing index. pub fn get_attesting_indices( committees: &[BeaconCommittee], - aggregation_bits: &BitList, + aggregation_bits: &BitList, committee_bits: &BitVector, ) -> Result, BeaconStateError> { let mut output: HashSet = HashSet::new(); diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index c3e92ddb341..3f18acb7921 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -25,7 +25,7 @@ pub struct ConsensusContext { pub indexed_attestations: HashMap< ( AttestationData, - BitList, + BitList, ), IndexedAttestation, >, @@ -208,7 +208,7 @@ impl ConsensusContext { attestations: HashMap< ( AttestationData, - BitList, + BitList, ), IndexedAttestation, >, diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index d479ca9fc1c..feee4f66879 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -65,7 +65,7 @@ pub struct Attestation { #[superstruct(only(Base), partial_getter(rename = "aggregation_bits_base"))] pub aggregation_bits: BitList, #[superstruct(only(Electra), partial_getter(rename = "aggregation_bits_electra"))] - pub aggregation_bits: BitList, + pub aggregation_bits: BitList, pub data: AttestationData, pub signature: AggregateSignature, #[superstruct(only(Electra))] @@ -391,8 +391,8 @@ impl AttestationBase { pub fn extend_aggregation_bits( &self, - ) -> Result, ssz_types::Error> { - let mut extended_aggregation_bits: BitList = + ) -> Result, ssz_types::Error> { + let mut extended_aggregation_bits: BitList = BitList::with_capacity(self.aggregation_bits.len())?; for (i, bit) in self.aggregation_bits.iter().enumerate() { diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index ac0525a3d84..a711a528067 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -610,7 +610,7 @@ impl> BeaconBlockElectra let indexed_attestation: IndexedAttestationElectra = IndexedAttestationElectra { attesting_indices: VariableList::new(vec![ 0_u64; - E::MaxValidatorsPerCommitteePerSlot::to_usize( + E::MaxValidatorsPerSlot::to_usize( ) ]) .unwrap(), @@ -626,10 +626,9 @@ impl> BeaconBlockElectra E::max_attester_slashings_electra() ] .into(); - // TODO(electra): check this let attestation = AttestationElectra { aggregation_bits: BitList::with_capacity( - E::MaxValidatorsPerCommitteePerSlot::to_usize(), + E::MaxValidatorsPerSlot::to_usize(), ) .unwrap(), data: AttestationData::default(), diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index e39b0dc9cf6..cec4db2da51 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -63,7 +63,7 @@ pub trait EthSpec: * Misc */ type MaxValidatorsPerCommittee: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; - type MaxValidatorsPerCommitteePerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; + type MaxValidatorsPerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; type MaxCommitteesPerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; /* * Time parameters @@ -352,7 +352,7 @@ impl EthSpec for MainnetEthSpec { type SubnetBitfieldLength = U64; type MaxValidatorsPerCommittee = U2048; type MaxCommitteesPerSlot = U64; - type MaxValidatorsPerCommitteePerSlot = U131072; + type MaxValidatorsPerSlot = U131072; type GenesisEpoch = U0; type SlotsPerEpoch = U32; type EpochsPerEth1VotingPeriod = U64; @@ -433,7 +433,7 @@ impl EthSpec for MinimalEthSpec { SyncCommitteeSubnetCount, MaxValidatorsPerCommittee, MaxCommitteesPerSlot, - MaxValidatorsPerCommitteePerSlot, + MaxValidatorsPerSlot, GenesisEpoch, HistoricalRootsLimit, ValidatorRegistryLimit, @@ -475,7 +475,7 @@ impl EthSpec for GnosisEthSpec { type SubnetBitfieldLength = U64; type MaxValidatorsPerCommittee = U2048; type MaxCommitteesPerSlot = U64; - type MaxValidatorsPerCommitteePerSlot = U131072; + type MaxValidatorsPerSlot = U131072; type GenesisEpoch = U0; type SlotsPerEpoch = U16; type EpochsPerEth1VotingPeriod = U64; diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index d17d00a3fa8..4e7e061b45f 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -59,7 +59,7 @@ pub struct IndexedAttestation { pub attesting_indices: VariableList, #[superstruct(only(Electra), partial_getter(rename = "attesting_indices_electra"))] #[serde(with = "quoted_variable_list_u64")] - pub attesting_indices: VariableList, + pub attesting_indices: VariableList, pub data: AttestationData, pub signature: AggregateSignature, } diff --git a/slasher/src/attester_record.rs b/slasher/src/attester_record.rs index 8de25160725..faba6efce89 100644 --- a/slasher/src/attester_record.rs +++ b/slasher/src/attester_record.rs @@ -80,7 +80,7 @@ impl IndexedAttesterRecord { #[derive(Debug, Clone, Encode, Decode, TreeHash)] struct IndexedAttestationHeader { - pub attesting_indices: VariableList, + pub attesting_indices: VariableList, pub data_root: Hash256, pub signature: AggregateSignature, } From 1a7854e2cd8d7c9c55b09a733e421e3457c7f468 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 13:40:14 +0200 Subject: [PATCH 04/10] fmt --- .../beacon_chain/src/observed_aggregates.rs | 11 ++++------- beacon_node/beacon_chain/src/test_utils.rs | 14 ++++---------- .../operation_pool/src/attestation_storage.rs | 17 +++++++++-------- beacon_node/store/src/consensus_context.rs | 9 ++------- .../state_processing/src/consensus_context.rs | 14 +++----------- consensus/types/src/beacon_block.rs | 13 +++---------- 6 files changed, 25 insertions(+), 53 deletions(-) diff --git a/beacon_node/beacon_chain/src/observed_aggregates.rs b/beacon_node/beacon_chain/src/observed_aggregates.rs index 1f14fe62570..5e161a998b5 100644 --- a/beacon_node/beacon_chain/src/observed_aggregates.rs +++ b/beacon_node/beacon_chain/src/observed_aggregates.rs @@ -17,11 +17,8 @@ pub type ObservedSyncContributions = ObservedAggregates< E, BitVector<::SyncSubcommitteeSize>, >; -pub type ObservedAggregateAttestations = ObservedAggregates< - Attestation, - E, - BitList<::MaxValidatorsPerSlot>, ->; +pub type ObservedAggregateAttestations = + ObservedAggregates, E, BitList<::MaxValidatorsPerSlot>>; /// A trait use to associate capacity constants with the type being stored in `ObservedAggregates`. pub trait Consts { @@ -111,7 +108,7 @@ impl<'a, E: EthSpec> SubsetItem for AttestationRef<'a, E> { return extended_aggregation_bits.is_subset(other); } false - }, + } Self::Electra(att) => att.aggregation_bits.is_subset(other), } } @@ -123,7 +120,7 @@ impl<'a, E: EthSpec> SubsetItem for AttestationRef<'a, E> { return other.is_subset(&extended_aggregation_bits); } false - }, + } Self::Electra(att) => other.is_subset(&att.aggregation_bits), } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 6c096d8bbdc..417774706db 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1142,17 +1142,11 @@ where match attestation { Attestation::Base(ref mut att) => { - att - .aggregation_bits - .set(i, true) - .unwrap() - }, + att.aggregation_bits.set(i, true).unwrap() + } Attestation::Electra(ref mut att) => { - att - .aggregation_bits - .set(i, true) - .unwrap() - }, + att.aggregation_bits.set(i, true).unwrap() + } } *attestation.signature_mut() = { diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index e6a3dc2162d..7d7a72704d9 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -164,9 +164,10 @@ impl CompactIndexedAttestation { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { this.signers_disjoint_from(other) } - (CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other)) => { - this.signers_disjoint_from(other) - } + ( + CompactIndexedAttestation::Electra(this), + CompactIndexedAttestation::Electra(other), + ) => this.signers_disjoint_from(other), // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => false, } @@ -177,9 +178,10 @@ impl CompactIndexedAttestation { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { this.aggregate(other) } - (CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other)) => { - this.aggregate(other) - } + ( + CompactIndexedAttestation::Electra(this), + CompactIndexedAttestation::Electra(other), + ) => this.aggregate(other), // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => (), } @@ -252,13 +254,12 @@ impl AttestationMap { 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); diff --git a/beacon_node/store/src/consensus_context.rs b/beacon_node/store/src/consensus_context.rs index a1d2e674526..409b364992d 100644 --- a/beacon_node/store/src/consensus_context.rs +++ b/beacon_node/store/src/consensus_context.rs @@ -21,13 +21,8 @@ pub struct OnDiskConsensusContext { /// /// They are not part of the on-disk format. #[ssz(skip_serializing, skip_deserializing)] - indexed_attestations: HashMap< - ( - AttestationData, - BitList, - ), - IndexedAttestation, - >, + indexed_attestations: + HashMap<(AttestationData, BitList), IndexedAttestation>, } impl OnDiskConsensusContext { diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index 3f18acb7921..6b0f2eb8fee 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -22,13 +22,8 @@ pub struct ConsensusContext { /// Block root of the block at `slot`. pub current_block_root: Option, /// Cache of indexed attestations constructed during block processing. - pub indexed_attestations: HashMap< - ( - AttestationData, - BitList, - ), - IndexedAttestation, - >, + pub indexed_attestations: + HashMap<(AttestationData, BitList), IndexedAttestation>, } #[derive(Debug, PartialEq, Clone)] @@ -206,10 +201,7 @@ impl ConsensusContext { pub fn set_indexed_attestations( mut self, attestations: HashMap< - ( - AttestationData, - BitList, - ), + (AttestationData, BitList), IndexedAttestation, >, ) -> Self { diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index a711a528067..09f3306c73d 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -608,12 +608,8 @@ impl> BeaconBlockElectra let base_block: BeaconBlockBase<_, Payload> = BeaconBlockBase::full(spec); // TODO(electra): check this let indexed_attestation: IndexedAttestationElectra = IndexedAttestationElectra { - attesting_indices: VariableList::new(vec![ - 0_u64; - E::MaxValidatorsPerSlot::to_usize( - ) - ]) - .unwrap(), + attesting_indices: VariableList::new(vec![0_u64; E::MaxValidatorsPerSlot::to_usize()]) + .unwrap(), data: AttestationData::default(), signature: AggregateSignature::empty(), }; @@ -627,10 +623,7 @@ impl> BeaconBlockElectra ] .into(); let attestation = AttestationElectra { - aggregation_bits: BitList::with_capacity( - E::MaxValidatorsPerSlot::to_usize(), - ) - .unwrap(), + aggregation_bits: BitList::with_capacity(E::MaxValidatorsPerSlot::to_usize()).unwrap(), data: AttestationData::default(), signature: AggregateSignature::empty(), // TODO(electra): does this actually allocate the size correctly? From 67157918219765db5d10af0e1d72c38a82fbddae Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 17:07:44 +0200 Subject: [PATCH 05/10] fix some beacon chain tests --- beacon_node/beacon_chain/src/test_utils.rs | 2 +- .../tests/attestation_verification.rs | 226 +++++++++++++----- .../beacon_chain/tests/block_verification.rs | 18 +- .../tests/payload_invalidation.rs | 2 +- consensus/types/src/aggregate_and_proof.rs | 12 +- consensus/types/src/attestation.rs | 1 - .../types/src/signed_aggregate_and_proof.rs | 4 +- 7 files changed, 192 insertions(+), 73 deletions(-) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 417774706db..edfff4bf81e 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1388,7 +1388,7 @@ where let signed_aggregate = SignedAggregateAndProof::from_aggregate( aggregator_index as u64, - aggregate, + aggregate.to_ref(), None, &self.validator_keypairs[aggregator_index].sk, &fork, diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 1327348713c..4c6a1f3f0c8 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -20,9 +20,10 @@ use state_processing::{ use tree_hash::TreeHash; use types::{ test_utils::generate_deterministic_keypair, Address, AggregateSignature, Attestation, - BeaconStateError, BitList, ChainSpec, Epoch, EthSpec, ForkName, Hash256, Keypair, + BeaconStateError, BitList, ChainSpec, Epoch, EthSpec, ForkName, Hash256, Keypair, signed_aggregate_and_proof::SignedAggregateAndProofRefMut, AttestationRef, AttestationRefMut, MainnetEthSpec, SecretKey, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, Unsigned, }; +use ssz_types::BitVector; pub type E = MainnetEthSpec; @@ -198,7 +199,7 @@ fn get_valid_aggregated_attestation( let signed_aggregate = SignedAggregateAndProof::from_aggregate( aggregator_index as u64, - aggregate, + aggregate.to_ref(), None, &aggregator_sk, &state.fork(), @@ -213,12 +214,13 @@ fn get_valid_aggregated_attestation( /// attestation. fn get_non_aggregator( chain: &BeaconChain, - aggregate: &Attestation, + aggregate: &AttestationRef, ) -> (usize, SecretKey) { let head = chain.head_snapshot(); 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, aggregate.data().index) .expect("should get committees"); @@ -305,11 +307,23 @@ impl GossipTester { let (mut invalid_aggregate, _, _) = get_valid_aggregated_attestation(&harness.chain, invalid_attestation.clone()); - invalid_aggregate.message.aggregator_index = invalid_aggregate - .message - .aggregator_index - .checked_sub(1) - .unwrap(); + + match invalid_aggregate.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregator_index = att + .message + .aggregator_index + .checked_sub(1) + .unwrap(); + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregator_index = att + .message + .aggregator_index + .checked_sub(1) + .unwrap(); + } + } Self { harness, @@ -361,7 +375,7 @@ impl GossipTester { } pub fn non_aggregator(&self) -> (usize, SecretKey) { - get_non_aggregator(&self.harness.chain, &self.valid_aggregate.message.aggregate) + get_non_aggregator(&self.harness.chain, &self.valid_aggregate.message().aggregate()) } pub fn import_valid_aggregate(self) -> Self { @@ -490,7 +504,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate from future slot", - |tester, a| a.message.aggregate.data_mut().slot = tester.slot() + 1, + |tester, a| { + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.slot = tester.slot() + 1, + SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.slot = tester.slot() + 1, + } + }, |tester, err| { assert!(matches!( err, @@ -504,9 +523,19 @@ async fn aggregated_gossip_verification() { "aggregate from past slot", |tester, a| { let too_early_slot = tester.earliest_valid_attestation_slot() - 1; - a.message.aggregate.data_mut().slot = too_early_slot; - a.message.aggregate.data_mut().target.epoch = - too_early_slot.epoch(E::slots_per_epoch()); + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregate.data.slot = too_early_slot; + att.message.aggregate.data.target.epoch = + too_early_slot.epoch(E::slots_per_epoch()); + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregate.data.slot = too_early_slot; + att.message.aggregate.data.target.epoch = + too_early_slot.epoch(E::slots_per_epoch()); + } + } + }, |tester, err| { let valid_early_slot = tester.earliest_valid_attestation_slot(); @@ -530,7 +559,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "attestation with invalid target epoch", - |_, a| a.message.aggregate.data_mut().target.epoch += 1, + |_, a| { + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.target.epoch += 1, + SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.target.epoch += 1, + } + }, |_, err| assert!(matches!(err, AttnError::InvalidTargetEpoch { .. })), ) /* @@ -539,7 +573,10 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "attestation with invalid target root", - |_, a| a.message.aggregate.data_mut().target.root = Hash256::repeat_byte(42), + |_, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.target.root = Hash256::repeat_byte(42), + SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.target.root = Hash256::repeat_byte(42) + }, |_, err| assert!(matches!(err, AttnError::InvalidTargetRoot { .. })), ) /* @@ -549,7 +586,13 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate with unknown head block", - |_, a| a.message.aggregate.data_mut().beacon_block_root = Hash256::repeat_byte(42), + |_, a| { + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42), + SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42) + } + + }, |_, err| { assert!(matches!( err, @@ -568,19 +611,20 @@ async fn aggregated_gossip_verification() { .inspect_aggregate_err( "aggregate with no participants", |_, a| { - match &mut a.message.aggregate { - Attestation::Base(ref mut att) => { - let aggregation_bits = &mut att.aggregation_bits; + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + let aggregation_bits = &mut att.message.aggregate.aggregation_bits; aggregation_bits.difference_inplace(&aggregation_bits.clone()); assert!(aggregation_bits.is_zero()); - } - Attestation::Electra(ref mut att) => { - let aggregation_bits = &mut att.aggregation_bits; + att.message.aggregate.signature = AggregateSignature::infinity() + }, + SignedAggregateAndProofRefMut::Electra(att) => { + let aggregation_bits = &mut att.message.aggregate.aggregation_bits; aggregation_bits.difference_inplace(&aggregation_bits.clone()); assert!(aggregation_bits.is_zero()); + att.message.aggregate.signature = AggregateSignature::infinity() } } - *a.message.aggregate.signature_mut() = AggregateSignature::infinity(); }, |_, err| assert!(matches!(err, AttnError::EmptyAggregationBitfield)), ) @@ -591,7 +635,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate with bad signature", - |tester, a| a.signature = tester.aggregator_sk.sign(Hash256::repeat_byte(42)), + |tester, a| { + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => att.signature = tester.aggregator_sk.sign(Hash256::repeat_byte(42)), + SignedAggregateAndProofRefMut::Electra(att) => att.signature = tester.aggregator_sk.sign(Hash256::repeat_byte(42)) + } + }, |_, err| assert!(matches!(err, AttnError::InvalidSignature)), ) /* @@ -608,7 +657,7 @@ async fn aggregated_gossip_verification() { .chain .head_snapshot() .beacon_state - .get_beacon_committee(tester.slot(), a.message.aggregate.data().index) + .get_beacon_committee(tester.slot(), a.message().aggregate().data().index) .expect("should get committees") .committee .len(); @@ -618,19 +667,39 @@ async fn aggregated_gossip_verification() { // // Could run for ever, but that seems _really_ improbable. let mut i: u64 = 0; - a.message.selection_proof = loop { - i += 1; - let proof: SelectionProof = tester - .aggregator_sk - .sign(Hash256::from_slice(&int_to_bytes32(i))) - .into(); - if proof - .is_aggregator(committee_len, &tester.harness.chain.spec) - .unwrap() - { - break proof.into(); + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.selection_proof = loop { + i += 1; + let proof: SelectionProof = tester + .aggregator_sk + .sign(Hash256::from_slice(&int_to_bytes32(i))) + .into(); + if proof + .is_aggregator(committee_len, &tester.harness.chain.spec) + .unwrap() + { + break proof.into(); + } + }; } - }; + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.selection_proof = loop { + i += 1; + let proof: SelectionProof = tester + .aggregator_sk + .sign(Hash256::from_slice(&int_to_bytes32(i))) + .into(); + if proof + .is_aggregator(committee_len, &tester.harness.chain.spec) + .unwrap() + { + break proof.into(); + } + }; + } + } + }, |_, err| assert!(matches!(err, AttnError::InvalidSignature)), ) @@ -644,7 +713,14 @@ async fn aggregated_gossip_verification() { |tester, a| { let mut agg_sig = AggregateSignature::infinity(); agg_sig.add_assign(&tester.aggregator_sk.sign(Hash256::repeat_byte(42))); - *a.message.aggregate.signature_mut() = agg_sig; + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregate.signature = agg_sig; + }, + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregate.signature = agg_sig; + } + } }, |_, err| assert!(matches!(err, AttnError::InvalidSignature)), ) @@ -653,8 +729,13 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate with too-high aggregator index", - |_, a| { - a.message.aggregator_index = ::ValidatorRegistryLimit::to_u64() + 1 + |_, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregator_index = ::ValidatorRegistryLimit::to_u64() + 1 + }, + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregator_index = ::ValidatorRegistryLimit::to_u64() + 1 + } }, |_, err| { assert!(matches!( @@ -673,7 +754,14 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate with unknown aggregator index", - |_, a| a.message.aggregator_index = VALIDATOR_COUNT as u64, + |_, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregator_index = VALIDATOR_COUNT as u64 + }, + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregator_index = VALIDATOR_COUNT as u64 + } + }, |_, err| { assert!(matches!( err, @@ -681,10 +769,15 @@ async fn aggregated_gossip_verification() { // // AttnError::AggregatorPubkeyUnknown(unknown_validator) // - // However the following error is triggered first: + // However, the following error is triggered first: AttnError::AggregatorNotInCommittee { aggregator_index - } + } | + // unless were working with electra attestations + // in which case this error is triggered instead: + AttnError::AggregatorPubkeyUnknown( + aggregator_index + ) if aggregator_index == VALIDATOR_COUNT as u64 )) }, @@ -703,7 +796,7 @@ async fn aggregated_gossip_verification() { let (index, sk) = tester.non_aggregator(); *a = SignedAggregateAndProof::from_aggregate( index as u64, - tester.valid_aggregate.message.aggregate.clone(), + tester.valid_aggregate.message().aggregate().clone(), None, &sk, &chain.canonical_head.cached_head().head_fork(), @@ -739,7 +832,7 @@ async fn aggregated_gossip_verification() { assert!(matches!( err, AttnError::AttestationSupersetKnown(hash) - if hash == tester.valid_aggregate.message.aggregate.data().tree_hash_root() + if hash == tester.valid_aggregate.message().aggregate().data().tree_hash_root() )) }, ) @@ -751,7 +844,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate from aggregator that has already been seen", - |_, a| a.message.aggregate.data_mut().beacon_block_root = Hash256::repeat_byte(42), + |_, a| { + match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42), + SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42), + } + }, |tester, err| { assert!(matches!( err, @@ -775,14 +873,30 @@ async fn unaggregated_gossip_verification() { */ .inspect_unaggregate_err( "attestation with invalid committee index", - |tester, a, _| { - a.data_mut().index = tester - .harness - .chain - .head_snapshot() - .beacon_state - .get_committee_count_at_slot(a.data().slot) - .unwrap() + |tester, a, _| { + match a.to_mut() { + AttestationRefMut::Base(attn) => { + attn.data.index = tester + .harness + .chain + .head_snapshot() + .beacon_state + .get_committee_count_at_slot(attn.data.slot) + .unwrap(); + } + AttestationRefMut::Electra(attn) => { + let committee_index = tester + .harness + .chain + .head_snapshot() + .beacon_state + .get_committee_count_at_slot(attn.data.slot) + .unwrap(); + // overwrite the existing committee bits before setting + attn.committee_bits = BitVector::default(); + attn.committee_bits.set(committee_index as usize, true).unwrap(); + } + } }, |_, err| assert!(matches!(err, AttnError::NoCommitteeForSlotAndIndex { .. })), ) @@ -1325,8 +1439,8 @@ async fn verify_aggregate_for_gossip_doppelganger_detection() { .verify_aggregated_attestation_for_gossip(&valid_aggregate) .expect("should verify aggregate attestation"); - let epoch = valid_aggregate.message.aggregate.data().target.epoch; - let index = valid_aggregate.message.aggregator_index as usize; + let epoch = valid_aggregate.message().aggregate().data().target.epoch; + let index = valid_aggregate.message().aggregator_index() as usize; assert!(harness.chain.validator_seen_at_epoch(index, epoch)); // Check the correct beacon cache is populated diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index e24894b242c..949a75ce298 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -13,7 +13,7 @@ use lazy_static::lazy_static; use logging::test_logger; use slasher::{Config as SlasherConfig, Slasher}; use state_processing::{ - common::get_indexed_attestation, + common::{attesting_indices_base, attesting_indices_electra}, per_block_processing::{per_block_processing, BlockSignatureStrategy}, per_slot_processing, BlockProcessingError, ConsensusContext, VerifyBlockRoot, }; @@ -1258,11 +1258,17 @@ async fn verify_block_for_gossip_doppelganger_detection() { for att in attestations.iter() { let epoch = att.data().target.epoch; - let committee = state - .get_beacon_committee(att.data().slot, att.data().index) - .unwrap(); - let indexed_attestation = - get_indexed_attestation(committee.committee, att.to_ref()).unwrap(); + let indexed_attestation = match att { + Attestation::Base(att) => { + let committee = state + .get_beacon_committee(att.data.slot, att.data.index) + .unwrap(); + attesting_indices_base::get_indexed_attestation(committee.committee, att).unwrap() + }, + Attestation::Electra(att) => { + attesting_indices_electra::get_indexed_attestation_from_state(&state, att).unwrap() + } + }; match indexed_attestation { IndexedAttestation::Base(indexed_attestation) => { diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index c1c770cfe4e..8c9957db169 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1207,7 +1207,7 @@ async fn attesting_to_optimistic_head() { .chain .naive_aggregation_pool .write() - .insert(&attestation) + .insert(attestation.to_ref()) .unwrap(); attestation diff --git a/consensus/types/src/aggregate_and_proof.rs b/consensus/types/src/aggregate_and_proof.rs index b0dbdadb952..005922667be 100644 --- a/consensus/types/src/aggregate_and_proof.rs +++ b/consensus/types/src/aggregate_and_proof.rs @@ -1,4 +1,4 @@ -use super::{Attestation, AttestationBase, AttestationElectra, AttestationRef}; +use super::{AttestationRef, AttestationBase, AttestationElectra}; use super::{ ChainSpec, Domain, EthSpec, Fork, Hash256, PublicKey, SecretKey, SelectionProof, Signature, SignedRoot, @@ -81,7 +81,7 @@ impl AggregateAndProof { /// If `selection_proof.is_none()` it will be computed locally. pub fn from_aggregate( aggregator_index: u64, - aggregate: Attestation, + aggregate: AttestationRef<'_, E>, selection_proof: Option, secret_key: &SecretKey, fork: &Fork, @@ -101,14 +101,14 @@ impl AggregateAndProof { .into(); match aggregate { - Attestation::Base(attestation) => Self::Base(AggregateAndProofBase { + AttestationRef::Base(attestation) => Self::Base(AggregateAndProofBase { aggregator_index, - aggregate: attestation, + aggregate: attestation.clone(), selection_proof, }), - Attestation::Electra(attestation) => Self::Electra(AggregateAndProofElectra { + AttestationRef::Electra(attestation) => Self::Electra(AggregateAndProofElectra { aggregator_index, - aggregate: attestation, + aggregate: attestation.clone(), selection_proof, }), } diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index feee4f66879..2d88bdb55f8 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -337,7 +337,6 @@ impl AttestationBase { pub fn aggregate(&mut self, other: &Self) { debug_assert_eq!(self.data, other.data); debug_assert!(self.signers_disjoint_from(other)); - self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.signature.add_assign_aggregate(&other.signature); } diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index 649b5fc6fac..9f6d3234529 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -2,7 +2,7 @@ use super::{ AggregateAndProof, AggregateAndProofBase, AggregateAndProofElectra, AggregateAndProofRef, }; use super::{ - Attestation, ChainSpec, Domain, EthSpec, Fork, Hash256, SecretKey, SelectionProof, Signature, + AttestationRef, ChainSpec, Domain, EthSpec, Fork, Hash256, SecretKey, SelectionProof, Signature, SignedRoot, }; use crate::test_utils::TestRandom; @@ -58,7 +58,7 @@ impl SignedAggregateAndProof { /// If `selection_proof.is_none()` it will be computed locally. pub fn from_aggregate( aggregator_index: u64, - aggregate: Attestation, + aggregate: AttestationRef<'_, E>, selection_proof: Option, secret_key: &SecretKey, fork: &Fork, From 0d481b347714aed3264a695939ea3459eeba5b5d Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 17:10:22 +0200 Subject: [PATCH 06/10] fmt --- .../tests/attestation_verification.rs | 130 ++++++++++-------- .../beacon_chain/tests/block_verification.rs | 6 +- consensus/types/src/aggregate_and_proof.rs | 2 +- .../types/src/signed_aggregate_and_proof.rs | 4 +- 4 files changed, 75 insertions(+), 67 deletions(-) diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 4c6a1f3f0c8..d132b92a5c2 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -14,16 +14,18 @@ use beacon_chain::{ use genesis::{interop_genesis_state, DEFAULT_ETH1_BLOCK_HASH}; use int_to_bytes::int_to_bytes32; use lazy_static::lazy_static; +use ssz_types::BitVector; use state_processing::{ per_block_processing::errors::AttestationValidationError, per_slot_processing, }; use tree_hash::TreeHash; use types::{ + signed_aggregate_and_proof::SignedAggregateAndProofRefMut, test_utils::generate_deterministic_keypair, Address, AggregateSignature, Attestation, - BeaconStateError, BitList, ChainSpec, Epoch, EthSpec, ForkName, Hash256, Keypair, signed_aggregate_and_proof::SignedAggregateAndProofRefMut, AttestationRef, AttestationRefMut, - MainnetEthSpec, SecretKey, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, Unsigned, + AttestationRef, AttestationRefMut, BeaconStateError, BitList, ChainSpec, Epoch, EthSpec, + ForkName, Hash256, Keypair, MainnetEthSpec, SecretKey, SelectionProof, SignedAggregateAndProof, + Slot, SubnetId, Unsigned, }; -use ssz_types::BitVector; pub type E = MainnetEthSpec; @@ -310,19 +312,11 @@ impl GossipTester { match invalid_aggregate.to_mut() { SignedAggregateAndProofRefMut::Base(att) => { - att.message.aggregator_index = att - .message - .aggregator_index - .checked_sub(1) - .unwrap(); + att.message.aggregator_index = att.message.aggregator_index.checked_sub(1).unwrap(); } SignedAggregateAndProofRefMut::Electra(att) => { - att.message.aggregator_index = att - .message - .aggregator_index - .checked_sub(1) - .unwrap(); - } + att.message.aggregator_index = att.message.aggregator_index.checked_sub(1).unwrap(); + } } Self { @@ -375,7 +369,10 @@ impl GossipTester { } pub fn non_aggregator(&self) -> (usize, SecretKey) { - get_non_aggregator(&self.harness.chain, &self.valid_aggregate.message().aggregate()) + get_non_aggregator( + &self.harness.chain, + &self.valid_aggregate.message().aggregate(), + ) } pub fn import_valid_aggregate(self) -> Self { @@ -504,10 +501,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate from future slot", - |tester, a| { - match a.to_mut() { - SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.slot = tester.slot() + 1, - SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.slot = tester.slot() + 1, + |tester, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregate.data.slot = tester.slot() + 1 + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregate.data.slot = tester.slot() + 1 } }, |tester, err| { @@ -535,7 +534,6 @@ async fn aggregated_gossip_verification() { too_early_slot.epoch(E::slots_per_epoch()); } } - }, |tester, err| { let valid_early_slot = tester.earliest_valid_attestation_slot(); @@ -559,10 +557,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "attestation with invalid target epoch", - |_, a| { - match a.to_mut() { - SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.target.epoch += 1, - SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.target.epoch += 1, + |_, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregate.data.target.epoch += 1 + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregate.data.target.epoch += 1 } }, |_, err| assert!(matches!(err, AttnError::InvalidTargetEpoch { .. })), @@ -574,8 +574,12 @@ async fn aggregated_gossip_verification() { .inspect_aggregate_err( "attestation with invalid target root", |_, a| match a.to_mut() { - SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.target.root = Hash256::repeat_byte(42), - SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.target.root = Hash256::repeat_byte(42) + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregate.data.target.root = Hash256::repeat_byte(42) + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregate.data.target.root = Hash256::repeat_byte(42) + } }, |_, err| assert!(matches!(err, AttnError::InvalidTargetRoot { .. })), ) @@ -586,12 +590,13 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate with unknown head block", - |_, a| { - match a.to_mut() { - SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42), - SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42) + |_, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42) + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42) } - }, |_, err| { assert!(matches!( @@ -610,20 +615,18 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate with no participants", - |_, a| { - match a.to_mut() { - SignedAggregateAndProofRefMut::Base(att) => { - let aggregation_bits = &mut att.message.aggregate.aggregation_bits; - aggregation_bits.difference_inplace(&aggregation_bits.clone()); - assert!(aggregation_bits.is_zero()); - att.message.aggregate.signature = AggregateSignature::infinity() - }, - SignedAggregateAndProofRefMut::Electra(att) => { - let aggregation_bits = &mut att.message.aggregate.aggregation_bits; - aggregation_bits.difference_inplace(&aggregation_bits.clone()); - assert!(aggregation_bits.is_zero()); - att.message.aggregate.signature = AggregateSignature::infinity() - } + |_, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + let aggregation_bits = &mut att.message.aggregate.aggregation_bits; + aggregation_bits.difference_inplace(&aggregation_bits.clone()); + assert!(aggregation_bits.is_zero()); + att.message.aggregate.signature = AggregateSignature::infinity() + } + SignedAggregateAndProofRefMut::Electra(att) => { + let aggregation_bits = &mut att.message.aggregate.aggregation_bits; + aggregation_bits.difference_inplace(&aggregation_bits.clone()); + assert!(aggregation_bits.is_zero()); + att.message.aggregate.signature = AggregateSignature::infinity() } }, |_, err| assert!(matches!(err, AttnError::EmptyAggregationBitfield)), @@ -635,10 +638,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate with bad signature", - |tester, a| { - match a.to_mut() { - SignedAggregateAndProofRefMut::Base(att) => att.signature = tester.aggregator_sk.sign(Hash256::repeat_byte(42)), - SignedAggregateAndProofRefMut::Electra(att) => att.signature = tester.aggregator_sk.sign(Hash256::repeat_byte(42)) + |tester, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.signature = tester.aggregator_sk.sign(Hash256::repeat_byte(42)) + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.signature = tester.aggregator_sk.sign(Hash256::repeat_byte(42)) } }, |_, err| assert!(matches!(err, AttnError::InvalidSignature)), @@ -699,7 +704,6 @@ async fn aggregated_gossip_verification() { }; } } - }, |_, err| assert!(matches!(err, AttnError::InvalidSignature)), ) @@ -716,7 +720,7 @@ async fn aggregated_gossip_verification() { match a.to_mut() { SignedAggregateAndProofRefMut::Base(att) => { att.message.aggregate.signature = agg_sig; - }, + } SignedAggregateAndProofRefMut::Electra(att) => { att.message.aggregate.signature = agg_sig; } @@ -731,10 +735,12 @@ async fn aggregated_gossip_verification() { "aggregate with too-high aggregator index", |_, a| match a.to_mut() { SignedAggregateAndProofRefMut::Base(att) => { - att.message.aggregator_index = ::ValidatorRegistryLimit::to_u64() + 1 - }, + att.message.aggregator_index = + ::ValidatorRegistryLimit::to_u64() + 1 + } SignedAggregateAndProofRefMut::Electra(att) => { - att.message.aggregator_index = ::ValidatorRegistryLimit::to_u64() + 1 + att.message.aggregator_index = + ::ValidatorRegistryLimit::to_u64() + 1 } }, |_, err| { @@ -757,7 +763,7 @@ async fn aggregated_gossip_verification() { |_, a| match a.to_mut() { SignedAggregateAndProofRefMut::Base(att) => { att.message.aggregator_index = VALIDATOR_COUNT as u64 - }, + } SignedAggregateAndProofRefMut::Electra(att) => { att.message.aggregator_index = VALIDATOR_COUNT as u64 } @@ -772,7 +778,7 @@ async fn aggregated_gossip_verification() { // However, the following error is triggered first: AttnError::AggregatorNotInCommittee { aggregator_index - } | + } | // unless were working with electra attestations // in which case this error is triggered instead: AttnError::AggregatorPubkeyUnknown( @@ -844,10 +850,12 @@ async fn aggregated_gossip_verification() { */ .inspect_aggregate_err( "aggregate from aggregator that has already been seen", - |_, a| { - match a.to_mut() { - SignedAggregateAndProofRefMut::Base(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42), - SignedAggregateAndProofRefMut::Electra(att) => att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42), + |_, a| match a.to_mut() { + SignedAggregateAndProofRefMut::Base(att) => { + att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42) + } + SignedAggregateAndProofRefMut::Electra(att) => { + att.message.aggregate.data.beacon_block_root = Hash256::repeat_byte(42) } }, |tester, err| { @@ -873,7 +881,7 @@ async fn unaggregated_gossip_verification() { */ .inspect_unaggregate_err( "attestation with invalid committee index", - |tester, a, _| { + |tester, a, _| { match a.to_mut() { AttestationRefMut::Base(attn) => { attn.data.index = tester diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 949a75ce298..257b6d53463 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -1263,10 +1263,10 @@ async fn verify_block_for_gossip_doppelganger_detection() { let committee = state .get_beacon_committee(att.data.slot, att.data.index) .unwrap(); - attesting_indices_base::get_indexed_attestation(committee.committee, att).unwrap() - }, + attesting_indices_base::get_indexed_attestation(committee.committee, att).unwrap() + } Attestation::Electra(att) => { - attesting_indices_electra::get_indexed_attestation_from_state(&state, att).unwrap() + attesting_indices_electra::get_indexed_attestation_from_state(&state, att).unwrap() } }; diff --git a/consensus/types/src/aggregate_and_proof.rs b/consensus/types/src/aggregate_and_proof.rs index 005922667be..e8fa01c143e 100644 --- a/consensus/types/src/aggregate_and_proof.rs +++ b/consensus/types/src/aggregate_and_proof.rs @@ -1,4 +1,4 @@ -use super::{AttestationRef, AttestationBase, AttestationElectra}; +use super::{AttestationBase, AttestationElectra, AttestationRef}; use super::{ ChainSpec, Domain, EthSpec, Fork, Hash256, PublicKey, SecretKey, SelectionProof, Signature, SignedRoot, diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index 9f6d3234529..57a2ce5babe 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -2,8 +2,8 @@ use super::{ AggregateAndProof, AggregateAndProofBase, AggregateAndProofElectra, AggregateAndProofRef, }; use super::{ - AttestationRef, ChainSpec, Domain, EthSpec, Fork, Hash256, SecretKey, SelectionProof, Signature, - SignedRoot, + AttestationRef, ChainSpec, Domain, EthSpec, Fork, Hash256, SecretKey, SelectionProof, + Signature, SignedRoot, }; use crate::test_utils::TestRandom; use serde::{Deserialize, Serialize}; From 20c93c91338802fdb7ebdc96163beec815a1bc54 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 17:24:41 +0200 Subject: [PATCH 07/10] fix slasher test --- slasher/src/attester_record.rs | 8 ++++---- slasher/src/test_utils.rs | 28 +++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/slasher/src/attester_record.rs b/slasher/src/attester_record.rs index faba6efce89..1cd4ba7d4e0 100644 --- a/slasher/src/attester_record.rs +++ b/slasher/src/attester_record.rs @@ -106,15 +106,15 @@ impl From> for AttesterRecord { #[cfg(test)] mod test { use super::*; - use crate::test_utils::indexed_att; + use crate::test_utils::indexed_att_electra; // Check correctness of fast hashing #[test] fn fast_hash() { let data = vec![ - indexed_att(vec![], 0, 0, 0), - indexed_att(vec![1, 2, 3], 12, 14, 1), - indexed_att(vec![4], 0, 5, u64::MAX), + indexed_att_electra(vec![], 0, 0, 0), + indexed_att_electra(vec![1, 2, 3], 12, 14, 1), + indexed_att_electra(vec![4], 0, 5, u64::MAX), ]; for att in data { assert_eq!( diff --git a/slasher/src/test_utils.rs b/slasher/src/test_utils.rs index 634b4d52113..36da40ce7fb 100644 --- a/slasher/src/test_utils.rs +++ b/slasher/src/test_utils.rs @@ -1,12 +1,38 @@ use std::collections::HashSet; use types::{ - indexed_attestation::IndexedAttestationBase, AggregateSignature, AttestationData, + indexed_attestation::{IndexedAttestationBase, IndexedAttestationElectra}, AggregateSignature, AttestationData, AttesterSlashing, AttesterSlashingBase, AttesterSlashingElectra, BeaconBlockHeader, Checkpoint, Epoch, Hash256, IndexedAttestation, MainnetEthSpec, Signature, SignedBeaconBlockHeader, Slot, }; pub type E = MainnetEthSpec; + +pub fn indexed_att_electra( + attesting_indices: impl AsRef<[u64]>, + source_epoch: u64, + target_epoch: u64, + target_root: u64, +) -> IndexedAttestation { + IndexedAttestation::Electra(IndexedAttestationElectra { + attesting_indices: attesting_indices.as_ref().to_vec().into(), + data: AttestationData { + slot: Slot::new(0), + index: 0, + beacon_block_root: Hash256::zero(), + source: Checkpoint { + epoch: Epoch::new(source_epoch), + root: Hash256::from_low_u64_be(0), + }, + target: Checkpoint { + epoch: Epoch::new(target_epoch), + root: Hash256::from_low_u64_be(target_root), + }, + }, + signature: AggregateSignature::empty(), + }) +} + pub fn indexed_att( attesting_indices: impl AsRef<[u64]>, source_epoch: u64, From 71c1dcf86c2ce4ad6f5b83878ab46bbbf7b18483 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 17:29:30 +0200 Subject: [PATCH 08/10] fmt got me again --- slasher/src/test_utils.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/slasher/src/test_utils.rs b/slasher/src/test_utils.rs index 36da40ce7fb..8bbf042c2dd 100644 --- a/slasher/src/test_utils.rs +++ b/slasher/src/test_utils.rs @@ -1,13 +1,13 @@ use std::collections::HashSet; use types::{ - indexed_attestation::{IndexedAttestationBase, IndexedAttestationElectra}, AggregateSignature, AttestationData, - AttesterSlashing, AttesterSlashingBase, AttesterSlashingElectra, BeaconBlockHeader, Checkpoint, - Epoch, Hash256, IndexedAttestation, MainnetEthSpec, Signature, SignedBeaconBlockHeader, Slot, + indexed_attestation::{IndexedAttestationBase, IndexedAttestationElectra}, + AggregateSignature, AttestationData, AttesterSlashing, AttesterSlashingBase, + AttesterSlashingElectra, BeaconBlockHeader, Checkpoint, Epoch, Hash256, IndexedAttestation, + MainnetEthSpec, Signature, SignedBeaconBlockHeader, Slot, }; pub type E = MainnetEthSpec; - pub fn indexed_att_electra( attesting_indices: impl AsRef<[u64]>, source_epoch: u64, From b57c4320a96913564147c523ce73d06ebcf01f38 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 17:52:24 +0200 Subject: [PATCH 09/10] fix more tests --- beacon_node/http_api/tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index ea1529f7178..315df8fa9c5 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -3304,7 +3304,7 @@ impl ApiTester { SignedAggregateAndProof::from_aggregate( i as u64, - attestation, + attestation.to_ref(), Some(proof), &kp.sk, &fork, From 0be9f6792136c100ee9f7780790d1318a61cd5c7 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Wed, 8 May 2024 12:49:01 +0200 Subject: [PATCH 10/10] fix tests --- consensus/types/src/attestation.rs | 3 +++ testing/ef_tests/src/handler.rs | 8 +++++++ testing/ef_tests/src/type_name.rs | 7 +++++- testing/ef_tests/tests/tests.rs | 38 +++++++++++++++++++++++++++--- 4 files changed, 52 insertions(+), 4 deletions(-) diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 2d88bdb55f8..61e63e11e22 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -437,6 +437,9 @@ mod tests { assert_eq!(signature, 288 + 16); let attestation_expected = aggregation_bits + attestation_data + signature; + // TODO(electra) since we've removed attestation aggregation for electra variant + // i've updated the attestation value expected from 488 544 + // assert_eq!(attestation_expected, 488); assert_eq!(attestation_expected, 488); assert_eq!( size_of::>(), diff --git a/testing/ef_tests/src/handler.rs b/testing/ef_tests/src/handler.rs index 2d5ea4149ef..c1e7386bc45 100644 --- a/testing/ef_tests/src/handler.rs +++ b/testing/ef_tests/src/handler.rs @@ -229,6 +229,10 @@ impl SszStaticHandler { Self::for_forks(vec![ForkName::Deneb]) } + pub fn electra_only() -> Self { + Self::for_forks(vec![ForkName::Electra]) + } + pub fn altair_and_later() -> Self { Self::for_forks(ForkName::list_all()[1..].to_vec()) } @@ -240,6 +244,10 @@ impl SszStaticHandler { pub fn capella_and_later() -> Self { Self::for_forks(ForkName::list_all()[3..].to_vec()) } + + pub fn pre_electra() -> Self { + Self::for_forks(ForkName::list_all()[0..5].to_vec()) + } } /// Handler for SSZ types that implement `CachedTreeHash`. diff --git a/testing/ef_tests/src/type_name.rs b/testing/ef_tests/src/type_name.rs index 49ebbe81909..96eaffc75b5 100644 --- a/testing/ef_tests/src/type_name.rs +++ b/testing/ef_tests/src/type_name.rs @@ -37,11 +37,14 @@ macro_rules! type_name_generic { type_name!(MinimalEthSpec, "minimal"); type_name!(MainnetEthSpec, "mainnet"); - type_name_generic!(AggregateAndProof); +type_name_generic!(AggregateAndProofBase, "AggregateAndProof"); +type_name_generic!(AggregateAndProofElectra, "AggregateAndProof"); type_name_generic!(Attestation); type_name!(AttestationData); type_name_generic!(AttesterSlashing); +type_name_generic!(AttesterSlashingBase, "AttesterSlashing"); +type_name_generic!(AttesterSlashingElectra, "AttesterSlashing"); type_name_generic!(BeaconBlock); type_name_generic!(BeaconBlockBody); type_name_generic!(BeaconBlockBodyBase, "BeaconBlockBody"); @@ -108,6 +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!(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 5226c7ac2b0..669c7721748 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -217,12 +217,11 @@ mod ssz_static { use ef_tests::{Handler, SszStaticHandler, SszStaticTHCHandler, SszStaticWithSpecHandler}; use types::blob_sidecar::BlobIdentifier; use types::historical_summary::HistoricalSummary; - use types::{LightClientBootstrapAltair, *}; + 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!(attester_slashing, AttesterSlashing<_>); ssz_static_test!(beacon_block, SszStaticWithSpecHandler, BeaconBlock<_>); ssz_static_test!(beacon_block_header, BeaconBlockHeader); ssz_static_test!(beacon_state, SszStaticTHCHandler, BeaconState<_>); @@ -238,7 +237,6 @@ mod ssz_static { ssz_static_test!(indexed_attestation, IndexedAttestation<_>); ssz_static_test!(pending_attestation, PendingAttestation<_>); ssz_static_test!(proposer_slashing, ProposerSlashing); - ssz_static_test!(signed_aggregate_and_proof, SignedAggregateAndProof<_>); ssz_static_test!( signed_beacon_block, SszStaticWithSpecHandler, @@ -249,6 +247,24 @@ mod ssz_static { ssz_static_test!(signing_data, SigningData); ssz_static_test!(validator, Validator); ssz_static_test!(voluntary_exit, VoluntaryExit); + + + #[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(); + } + // BeaconBlockBody has no internal indicator of which fork it is for, so we test it separately. #[test] fn beacon_block_body() { @@ -272,6 +288,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() {