From 9e6e76fb898ad9349a2bd383fbf8543ffc915299 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 19 Jun 2024 12:47:38 +0200 Subject: [PATCH] Remove get_indexed_attestation_from_signed_aggregate --- .../src/attestation_verification.rs | 103 +++++++++--------- .../src/common/get_attesting_indices.rs | 103 ++---------------- 2 files changed, 62 insertions(+), 144 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 221d9114b1..a5635a3392 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -61,9 +61,10 @@ use std::borrow::Cow; use strum::AsRefStr; use tree_hash::TreeHash; use types::{ - Attestation, AttestationRef, BeaconCommittee, BeaconStateError, - BeaconStateError::NoCommitteeFound, ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, - Hash256, IndexedAttestation, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, + Attestation, AttestationRef, BeaconCommittee, + BeaconStateError::{self, NoCommitteeFound}, + ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, + SelectionProof, SignedAggregateAndProof, Slot, SubnetId, }; pub use batch::{batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations}; @@ -598,57 +599,61 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }; let get_indexed_attestation_with_committee = |(committees, _): (Vec, CommitteesPerSlot)| { + let (index, aggregator_index, selection_proof, data) = match signed_aggregate { + SignedAggregateAndProof::Base(signed_aggregate) => ( + signed_aggregate.message.aggregate.data.index, + signed_aggregate.message.aggregator_index, + // Note: this clones the signature which is known to be a relatively slow operation. + // Future optimizations should remove this clone. + signed_aggregate.message.selection_proof.clone(), + signed_aggregate.message.aggregate.data.clone(), + ), + SignedAggregateAndProof::Electra(signed_aggregate) => ( + signed_aggregate + .message + .aggregate + .committee_index() + .ok_or(Error::NotExactlyOneCommitteeBitSet(0))?, + signed_aggregate.message.aggregator_index, + signed_aggregate.message.selection_proof.clone(), + signed_aggregate.message.aggregate.data.clone(), + ), + }; + let slot = data.slot; + + let committee = committees + .iter() + .filter(|&committee| committee.index == index) + .at_most_one() + .map_err(|_| Error::NoCommitteeForSlotAndIndex { slot, index })? + .ok_or(Error::NoCommitteeForSlotAndIndex { slot, index })?; + + if !SelectionProof::from(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 }); + } + + // p2p aggregates have a single committee, we can assert that aggregation_bits is always + // less then MaxValidatorsPerCommittee match signed_aggregate { SignedAggregateAndProof::Base(signed_aggregate) => { - let att = &signed_aggregate.message.aggregate; - let aggregator_index = signed_aggregate.message.aggregator_index; - 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, - })?; - - // TODO(electra): - // Note: this clones the signature which is known to be a relatively slow operation. - // - // Future optimizations should remove this clone. - if let Some(committee) = committee { - 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, - }) - } + attesting_indices_base::get_indexed_attestation( + committee.committee, + &signed_aggregate.message.aggregate, + ) + .map_err(|e| BeaconChainError::from(e).into()) } SignedAggregateAndProof::Electra(signed_aggregate) => { - attesting_indices_electra::get_indexed_attestation_from_signed_aggregate( + attesting_indices_electra::get_indexed_attestation_from_committees( &committees, - signed_aggregate, - &chain.spec, + &signed_aggregate.message.aggregate, ) .map_err(|e| BeaconChainError::from(e).into()) } diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index a79604003e..f24b003665 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -51,100 +51,6 @@ pub mod attesting_indices_electra { use safe_arith::SafeArith; use types::*; - // TODO(electra) remove duplicate code - // get_indexed_attestation is almost an exact duplicate - // the only differences are the invalid selection proof - // and aggregator not in committee checks - pub fn get_indexed_attestation_from_signed_aggregate( - committees: &[BeaconCommittee], - signed_aggregate: &SignedAggregateAndProofElectra, - spec: &ChainSpec, - ) -> Result, BeaconStateError> { - let mut output: HashSet = HashSet::new(); - - let committee_bits = &signed_aggregate.message.aggregate.committee_bits; - let aggregation_bits = &signed_aggregate.message.aggregate.aggregation_bits; - let aggregator_index = signed_aggregate.message.aggregator_index; - let attestation = &signed_aggregate.message.aggregate; - - let committee_indices = get_committee_indices::(committee_bits); - - let mut committee_offset = 0; - - let committees_map: HashMap = committees - .iter() - .map(|committee| (committee.index, committee)) - .collect(); - - let committee_count_per_slot = committees.len() as u64; - let mut participant_count = 0; - - // TODO(electra): - // 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()); - - for index in committee_indices { - if let Some(&beacon_committee) = committees_map.get(&index) { - if !selection_proof - .is_aggregator(beacon_committee.committee.len(), spec) - .map_err(BeaconStateError::ArithError)? - { - return Err(BeaconStateError::InvalidSelectionProof { aggregator_index }); - } - - if !beacon_committee - .committee - .contains(&(aggregator_index as usize)) - { - return Err(BeaconStateError::AggregatorNotInCommittee { aggregator_index }); - } - - // This check is new to the spec's `process_attestation` in Electra. - if index >= committee_count_per_slot { - return Err(BeaconStateError::InvalidCommitteeIndex(index)); - } - - participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; - let committee_attesters = beacon_committee - .committee - .iter() - .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_assign(beacon_committee.committee.len())?; - } else { - return Err(Error::NoCommitteeFound(index)); - } - } - - // This check is new to the spec's `process_attestation` in Electra. - if participant_count as usize != aggregation_bits.len() { - return Err(Error::InvalidBitfield); - } - - let mut indices = output.into_iter().collect_vec(); - indices.sort_unstable(); - - Ok(IndexedAttestation::Electra(IndexedAttestationElectra { - attesting_indices: VariableList::new(indices)?, - data: attestation.data.clone(), - signature: attestation.signature.clone(), - })) - } - pub fn get_indexed_attestation( committees: &[BeaconCommittee], attestation: &AttestationElectra, @@ -167,8 +73,15 @@ pub mod attesting_indices_electra { attestation: &AttestationElectra, ) -> Result, BlockOperationError> { let committees = beacon_state.get_beacon_committees_at_slot(attestation.data.slot)?; + get_indexed_attestation_from_committees(&committees, attestation) + } + + pub fn get_indexed_attestation_from_committees( + committees: &[BeaconCommittee], + attestation: &AttestationElectra, + ) -> Result, BlockOperationError> { let attesting_indices = get_attesting_indices::( - &committees, + committees, &attestation.aggregation_bits, &attestation.committee_bits, )?;