From 21ac05bfbc3eb6e9412e79f5495fa08c3e553982 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 30 May 2024 17:51:34 +0200 Subject: [PATCH] Fix failing attestation tests and misc electra attestation cleanup (#5810) * - get attestation related beacon chain tests to pass - observed attestations are now keyed off of data + committee index - rename op pool attestationref to compactattestationref - remove unwraps in agg pool and use options instead - cherry pick some changes from ef-tests-electra * cargo fmt * fix failing test * Revert dockerfile changes * make committee_index return option * function args shouldnt be a ref to attestation ref * fmt * fix dup imports --------- Co-authored-by: realbigsean --- .../src/attestation_verification.rs | 124 +++++--- beacon_node/beacon_chain/src/beacon_chain.rs | 22 +- .../beacon_chain/src/early_attester_cache.rs | 1 - .../src/naive_aggregation_pool.rs | 20 +- beacon_node/beacon_chain/src/test_utils.rs | 282 ++++++++++++------ .../tests/attestation_verification.rs | 44 ++- .../beacon_chain/tests/block_verification.rs | 56 ++-- .../tests/payload_invalidation.rs | 7 +- beacon_node/beacon_chain/tests/store_tests.rs | 32 +- beacon_node/http_api/src/lib.rs | 4 +- .../http_api/src/publish_attestations.rs | 2 +- .../lighthouse_network/src/types/pubsub.rs | 8 +- beacon_node/operation_pool/src/attestation.rs | 2 +- .../operation_pool/src/attestation_storage.rs | 102 ++++--- beacon_node/operation_pool/src/lib.rs | 4 +- consensus/fork_choice/tests/tests.rs | 7 +- .../src/common/get_attesting_indices.rs | 97 +++++- .../per_block_processing/signature_sets.rs | 2 +- consensus/types/src/attestation.rs | 45 ++- consensus/types/src/beacon_state.rs | 9 +- consensus/types/src/indexed_attestation.rs | 3 + .../types/src/signed_aggregate_and_proof.rs | 1 - consensus/types/src/subnet_id.rs | 4 +- validator_client/src/attestation_service.rs | 10 +- 24 files changed, 612 insertions(+), 276 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 3b3c6ed2302..412615c5250 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -47,7 +47,10 @@ use proto_array::Block as ProtoBlock; use slog::debug; use slot_clock::SlotClock; use state_processing::{ - common::{attesting_indices_base, attesting_indices_electra}, + common::{ + attesting_indices_base, + attesting_indices_electra::{self, get_committee_indices}, + }, per_block_processing::errors::{AttestationValidationError, BlockOperationError}, signature_sets::{ indexed_attestation_signature_set_from_pubkeys, @@ -57,10 +60,11 @@ use state_processing::{ use std::borrow::Cow; use strum::AsRefStr; use tree_hash::TreeHash; +use tree_hash_derive::TreeHash; use types::{ - Attestation, AttestationRef, BeaconCommittee, BeaconStateError::NoCommitteeFound, ChainSpec, - CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, SelectionProof, - SignedAggregateAndProof, Slot, SubnetId, + Attestation, AttestationData, AttestationRef, BeaconCommittee, BeaconStateError, + BeaconStateError::NoCommitteeFound, ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, + Hash256, IndexedAttestation, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, }; pub use batch::{batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations}; @@ -262,9 +266,30 @@ pub enum Error { BeaconChainError(BeaconChainError), } +// TODO(electra) the error conversion changes here are to get a test case to pass +// this could easily be cleaned up impl From for Error { fn from(e: BeaconChainError) -> Self { - Self::BeaconChainError(e) + match &e { + BeaconChainError::BeaconStateError(beacon_state_error) => { + if let BeaconStateError::AggregatorNotInCommittee { aggregator_index } = + beacon_state_error + { + Self::AggregatorNotInCommittee { + aggregator_index: *aggregator_index, + } + } else if let BeaconStateError::InvalidSelectionProof { aggregator_index } = + beacon_state_error + { + Self::InvalidSelectionProof { + aggregator_index: *aggregator_index, + } + } else { + Error::BeaconChainError(e) + } + } + _ => Error::BeaconChainError(e), + } } } @@ -286,6 +311,12 @@ struct IndexedAggregatedAttestation<'a, T: BeaconChainTypes> { observed_attestation_key_root: Hash256, } +#[derive(TreeHash)] +pub struct ObservedAttestationKey { + pub committee_index: u64, + pub attestation_data: AttestationData, +} + /// Wraps a `Attestation` that has been verified up until the point that an `IndexedAttestation` can /// be derived. /// @@ -486,14 +517,21 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }); } - // Ensure the valid aggregated attestation has not already been seen locally. - let attestation_data = attestation.data(); - let attestation_data_root = attestation_data.tree_hash_root(); + let observed_attestation_key_root = ObservedAttestationKey { + committee_index: attestation + .committee_index() + .ok_or(Error::NotExactlyOneCommitteeBitSet(0))?, + attestation_data: attestation.data().clone(), + } + .tree_hash_root(); + + // [New in Electra:EIP7549] + verify_committee_index(attestation, &chain.spec)?; if chain .observed_attestations .write() - .is_known_subset(attestation, attestation_data_root) + .is_known_subset(attestation, observed_attestation_key_root) .map_err(|e| Error::BeaconChainError(e.into()))? { metrics::inc_counter(&metrics::AGGREGATED_ATTESTATION_SUBSETS); @@ -555,10 +593,8 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { chain: &BeaconChain, ) -> Result> { use AttestationSlashInfo::*; - - let attestation = signed_aggregate.message().aggregate(); - let aggregator_index = signed_aggregate.message().aggregator_index(); - let attestation_data_root = match Self::verify_early_checks(signed_aggregate, chain) { + let observed_attestation_key_root = match Self::verify_early_checks(signed_aggregate, chain) + { Ok(root) => root, Err(e) => { return Err(SignatureNotChecked( @@ -567,12 +603,12 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { )) } }; - - // Committees must be sorted by ascending index order 0..committees_per_slot let get_indexed_attestation_with_committee = |(committees, _): (Vec, CommitteesPerSlot)| { - match attestation { - AttestationRef::Base(att) => { + 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) @@ -582,13 +618,13 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { 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 { - // 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(), + signed_aggregate.message.selection_proof.clone(), ); if !selection_proof @@ -602,6 +638,7 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { if !committee.committee.contains(&(aggregator_index as usize)) { return Err(Error::AggregatorNotInCommittee { aggregator_index }); } + attesting_indices_base::get_indexed_attestation( committee.committee, att, @@ -614,13 +651,18 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }) } } - AttestationRef::Electra(att) => { - attesting_indices_electra::get_indexed_attestation(&committees, att) - .map_err(|e| BeaconChainError::from(e).into()) + SignedAggregateAndProof::Electra(signed_aggregate) => { + attesting_indices_electra::get_indexed_attestation_from_signed_aggregate( + &committees, + signed_aggregate, + &chain.spec, + ) + .map_err(|e| BeaconChainError::from(e).into()) } } }; + let attestation = signed_aggregate.message().aggregate(); let indexed_attestation = match map_attestation_committees( chain, attestation, @@ -659,7 +701,7 @@ impl<'a, T: BeaconChainTypes> VerifiedAggregatedAttestation<'a, T> { if let ObserveOutcome::Subset = chain .observed_attestations .write() - .observe_item(attestation, Some(attestation_data_root)) + .observe_item(attestation, Some(observed_attestation_key_root)) .map_err(|e| Error::BeaconChainError(e.into()))? { metrics::inc_counter(&metrics::AGGREGATED_ATTESTATION_SUBSETS); @@ -802,7 +844,7 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> { } // [New in Electra:EIP7549] - verify_committee_index(attestation)?; + verify_committee_index(attestation, &chain.spec)?; // Attestations must be for a known block. If the block is unknown, we simply drop the // attestation and do not delay consideration for later. @@ -826,7 +868,7 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> { chain: &BeaconChain, ) -> Result<(u64, SubnetId), Error> { let expected_subnet_id = SubnetId::compute_subnet_for_attestation::( - &attestation, + attestation, committees_per_slot, &chain.spec, ) @@ -1130,7 +1172,7 @@ pub fn verify_propagation_slot_range( let current_fork = spec.fork_name_at_slot::(slot_clock.now().ok_or(BeaconChainError::UnableToReadSlot)?); - let earliest_permissible_slot = if !current_fork.deneb_enabled() { + let earliest_permissible_slot = if current_fork < ForkName::Deneb { one_epoch_prior // EIP-7045 } else { @@ -1302,10 +1344,17 @@ pub fn verify_signed_aggregate_signatures( /// Verify that the `attestation` committee index is properly set for the attestation's fork. /// This function will only apply verification post-Electra. -pub fn verify_committee_index(attestation: AttestationRef) -> Result<(), Error> { - if let Ok(committee_bits) = attestation.committee_bits() { +pub fn verify_committee_index( + attestation: AttestationRef, + spec: &ChainSpec, +) -> Result<(), Error> { + if spec.fork_name_at_slot::(attestation.data().slot) >= ForkName::Electra { // Check to ensure that the attestation is for a single committee. - let num_committee_bits = get_committee_indices::(committee_bits); + let num_committee_bits = get_committee_indices::( + attestation + .committee_bits() + .map_err(|e| Error::BeaconChainError(e.into()))?, + ); if num_committee_bits.len() != 1 { return Err(Error::NotExactlyOneCommitteeBitSet( num_committee_bits.len(), @@ -1358,7 +1407,8 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( attesting_indices_electra::get_indexed_attestation(&committees, att) .map(|attestation| (attestation, committees_per_slot)) .map_err(|e| { - if e == BlockOperationError::BeaconStateError(NoCommitteeFound) { + let index = att.committee_index().unwrap_or(0); + if e == BlockOperationError::BeaconStateError(NoCommitteeFound(index)) { Error::NoCommitteeForSlotAndIndex { slot: att.data.slot, index: att.committee_index(), @@ -1372,16 +1422,14 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( }) } -// 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 committees for -/// an attestation's slot can be complex. It might involve reading straight from the +/// an attestations slot can be complex. It might involve reading straight from the /// `beacon_chain.shuffling_cache` or it might involve reading it from a state from the DB. Due to /// the complexities of `RwLock`s on the shuffling cache, a simple `Cow` isn't suitable here. /// -/// If the committees for an `attestation`'s slot aren't found in the `shuffling_cache`, we will read a state +/// If the committees for an `attestation`'s slot isn't found in the `shuffling_cache`, we will read a state /// from disk and then update the `shuffling_cache`. fn map_attestation_committees( chain: &BeaconChain, @@ -1421,7 +1469,7 @@ where .unwrap_or_else(|_| { Err(Error::NoCommitteeForSlotAndIndex { slot: attestation.data().slot, - index: attestation.committee_index(), + index: attestation.committee_index().unwrap_or(0), }) })) }) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 0d0bcbc935e..a41c021d203 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1661,6 +1661,24 @@ impl BeaconChain { } } + /// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`. + /// + /// The attestation will be obtained from `self.naive_aggregation_pool`. + pub fn get_aggregated_attestation_base( + &self, + attestation: AttestationRef, + ) -> Result>, Error> { + match attestation { + AttestationRef::Base(att) => self.get_aggregated_attestation_base(&att.data), + AttestationRef::Electra(att) => self.get_aggregated_attestation_electra( + att.data.slot, + &att.data.tree_hash_root(), + att.committee_index() + .ok_or(Error::AttestationCommitteeIndexNotSet)?, + ), + } + } + /// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`. /// /// The attestation will be obtained from `self.naive_aggregation_pool`. @@ -2212,7 +2230,7 @@ impl BeaconChain { self.log, "Stored unaggregated attestation"; "outcome" => ?outcome, - "index" => attestation.data().index, + "index" => attestation.committee_index(), "slot" => attestation.data().slot.as_u64(), ), Err(NaiveAggregationError::SlotTooLow { @@ -2231,7 +2249,7 @@ impl BeaconChain { self.log, "Failed to store unaggregated attestation"; "error" => ?e, - "index" => attestation.data().index, + "index" => attestation.committee_index(), "slot" => attestation.data().slot.as_u64(), ); return Err(Error::from(e).into()); diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 8ed4e5db40b..936f4de3ee1 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -123,7 +123,6 @@ impl EarlyAttesterCache { item.committee_lengths .get_committee_length::(request_slot, request_index, spec)?; - // TODO(electra) make fork-agnostic let attestation = if spec.fork_name_at_slot::(request_slot) >= ForkName::Electra { let mut committee_bits = BitVector::default(); if committee_len > 0 { diff --git a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs index 896d5d5baa8..15a4638adb8 100644 --- a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs +++ b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs @@ -151,18 +151,10 @@ impl AggregateMap for AggregatedAttestationMap { .collect::>(), }; - let committee_index = set_bits - .first() - .copied() - .ok_or(Error::NoAggregationBitsSet)?; - - if set_bits.len() > 1 { - return Err(Error::MoreThanOneAggregationBitSet(set_bits.len())); - } + let attestation_key = AttestationKey::from_attestation_ref(a)?; + let attestation_key_root = attestation_key.tree_hash_root(); - let attestation_data_root = a.data().tree_hash_root(); - - if let Some(existing_attestation) = self.map.get_mut(&attestation_data_root) { + if let Some(existing_attestation) = self.map.get_mut(&attestation_key_root) { if existing_attestation .get_aggregation_bit(committee_index) .map_err(|_| Error::InconsistentBitfieldLengths)? @@ -180,8 +172,10 @@ impl AggregateMap for AggregatedAttestationMap { } self.map - .insert(attestation_data_root, a.clone_as_attestation()); - Ok(InsertOutcome::NewItemInserted { committee_index }) + .insert(attestation_key_root, a.clone_as_attestation()); + Ok(InsertOutcome::NewItemInserted { + committee_index: aggregation_bit, + }) } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index db4c65f0caa..34ad6d43244 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1170,7 +1170,7 @@ where }; let subnet_id = SubnetId::compute_subnet_for_attestation::( - &attestation.to_ref(), + attestation.to_ref(), committee_count, &self.chain.spec, ) @@ -1344,7 +1344,10 @@ where // If there are any attestations in this committee, create an aggregate. if let Some((attestation, _)) = committee_attestations.first() { let bc = state - .get_beacon_committee(attestation.data().slot, attestation.data().index) + .get_beacon_committee( + attestation.data().slot, + attestation.committee_index().unwrap(), + ) .unwrap(); // Find an aggregator if one exists. Return `None` if there are no @@ -1373,42 +1376,40 @@ where let fork_name = self.spec.fork_name_at_slot::(slot); - let aggregate = if fork_name.electra_enabled() { - self.chain.get_aggregated_attestation_electra( - slot, - &attestation.data().tree_hash_root(), - bc.index, - ) + let aggregate = if fork_name >= ForkName::Electra { + self.chain + .get_aggregated_attestation_electra( + slot, + &attestation.data().tree_hash_root(), + bc.index, + ) + .unwrap() + .unwrap_or_else(|| { + committee_attestations.iter().skip(1).fold( + attestation.clone(), + |mut agg, (att, _)| { + agg.aggregate(att.to_ref()); + agg + }, + ) + }) } else { self.chain .get_aggregated_attestation_base(attestation.data()) - } - .unwrap() - .unwrap_or_else(|| { - committee_attestations.iter().skip(1).fold( - attestation.clone(), - |mut agg, (att, _)| { - agg.aggregate(att.to_ref()); - agg - }, - ) - }); + .unwrap() + .unwrap_or_else(|| { + committee_attestations.iter().skip(1).fold( + attestation.clone(), + |mut agg, (att, _)| { + agg.aggregate(att.to_ref()); + agg + }, + ) + }) + }; // If the chain is able to produce an aggregate, use that. Otherwise, build an // aggregate locally. - let aggregate = self - .chain - .get_aggregated_attestation(attestation.data()) - .unwrap() - .unwrap_or_else(|| { - committee_attestations.iter().skip(1).fold( - attestation.clone(), - |mut agg, (att, _)| { - agg.aggregate(att.to_ref()); - agg - }, - ) - }); let signed_aggregate = SignedAggregateAndProof::from_aggregate( aggregator_index as u64, @@ -1538,54 +1539,101 @@ where ) -> AttesterSlashing { let fork = self.chain.canonical_head.cached_head().head_fork(); - // TODO(electra): consider making this test fork-agnostic - let mut attestation_1 = IndexedAttestationBase { - attesting_indices: VariableList::new(validator_indices).unwrap(), - data: AttestationData { - slot: Slot::new(0), - index: 0, - beacon_block_root: Hash256::zero(), - target: Checkpoint { - root: Hash256::zero(), - epoch: target1.unwrap_or(fork.epoch), + let fork_name = self.spec.fork_name_at_slot::(Slot::new(0)); + + let mut attestation_1 = if fork_name >= ForkName::Electra { + IndexedAttestation::Electra(IndexedAttestationElectra { + attesting_indices: VariableList::new(validator_indices).unwrap(), + data: AttestationData { + slot: Slot::new(0), + index: 0, + beacon_block_root: Hash256::zero(), + target: Checkpoint { + root: Hash256::zero(), + epoch: target1.unwrap_or(fork.epoch), + }, + source: Checkpoint { + root: Hash256::zero(), + epoch: source1.unwrap_or(Epoch::new(0)), + }, }, - source: Checkpoint { - root: Hash256::zero(), - epoch: source1.unwrap_or(Epoch::new(0)), + signature: AggregateSignature::infinity(), + }) + } else { + IndexedAttestation::Base(IndexedAttestationBase { + attesting_indices: VariableList::new(validator_indices).unwrap(), + data: AttestationData { + slot: Slot::new(0), + index: 0, + beacon_block_root: Hash256::zero(), + target: Checkpoint { + root: Hash256::zero(), + epoch: target1.unwrap_or(fork.epoch), + }, + source: Checkpoint { + root: Hash256::zero(), + epoch: source1.unwrap_or(Epoch::new(0)), + }, }, - }, - signature: AggregateSignature::infinity(), + signature: AggregateSignature::infinity(), + }) }; let mut attestation_2 = attestation_1.clone(); - attestation_2.data.index += 1; - attestation_2.data.source.epoch = source2.unwrap_or(Epoch::new(0)); - attestation_2.data.target.epoch = target2.unwrap_or(fork.epoch); + attestation_2.data_mut().index += 1; + attestation_2.data_mut().source.epoch = source2.unwrap_or(Epoch::new(0)); + attestation_2.data_mut().target.epoch = target2.unwrap_or(fork.epoch); for attestation in &mut [&mut attestation_1, &mut attestation_2] { - // TODO(electra) we could explore iter mut here - for i in attestation.attesting_indices.iter() { - let sk = &self.validator_keypairs[*i as usize].sk; + match attestation { + IndexedAttestation::Base(attestation) => { + for i in attestation.attesting_indices.iter() { + let sk = &self.validator_keypairs[*i as usize].sk; let genesis_validators_root = self.chain.genesis_validators_root; - let domain = self.chain.spec.get_domain( - attestation.data.target.epoch, - Domain::BeaconAttester, - &fork, - genesis_validators_root, - ); - let message = attestation.data.signing_root(domain); + let domain = self.chain.spec.get_domain( + attestation.data.target.epoch, + Domain::BeaconAttester, + &fork, + genesis_validators_root, + ); + let message = attestation.data.signing_root(domain); + + attestation.signature.add_assign(&sk.sign(message)); + } + } + IndexedAttestation::Electra(attestation) => { + for i in attestation.attesting_indices.iter() { + let sk = &self.validator_keypairs[*i as usize].sk; + + let genesis_validators_root = self.chain.genesis_validators_root; + + let domain = self.chain.spec.get_domain( + attestation.data.target.epoch, + Domain::BeaconAttester, + &fork, + genesis_validators_root, + ); + let message = attestation.data.signing_root(domain); - attestation.signature.add_assign(&sk.sign(message)); + attestation.signature.add_assign(&sk.sign(message)); + } + } } } - // TODO(electra): fix this test - AttesterSlashing::Base(AttesterSlashingBase { - attestation_1, - attestation_2, - }) + if fork_name >= ForkName::Electra { + AttesterSlashing::Electra(AttesterSlashingElectra { + attestation_1: attestation_1.as_electra().unwrap().clone(), + attestation_2: attestation_2.as_electra().unwrap().clone(), + }) + } else { + AttesterSlashing::Base(AttesterSlashingBase { + attestation_1: attestation_1.as_base().unwrap().clone(), + attestation_2: attestation_2.as_base().unwrap().clone(), + }) + } } pub fn make_attester_slashing_different_indices( @@ -1609,45 +1657,95 @@ where }, }; - // TODO(electra): make this test fork-agnostic - let mut attestation_1 = IndexedAttestationBase { - attesting_indices: VariableList::new(validator_indices_1).unwrap(), - data: data.clone(), - signature: AggregateSignature::infinity(), - }; + let (mut attestation_1, mut attestation_2) = if fork_name >= ForkName::Electra { + let attestation_1 = IndexedAttestationElectra { + attesting_indices: VariableList::new(validator_indices_1).unwrap(), + data: data.clone(), + signature: AggregateSignature::infinity(), + }; + + let attestation_2 = IndexedAttestationElectra { + attesting_indices: VariableList::new(validator_indices_2).unwrap(), + data, + signature: AggregateSignature::infinity(), + }; + + ( + IndexedAttestation::Electra(attestation_1), + IndexedAttestation::Electra(attestation_2), + ) + } else { + let attestation_1 = IndexedAttestationBase { + attesting_indices: VariableList::new(validator_indices_1).unwrap(), + data: data.clone(), + signature: AggregateSignature::infinity(), + }; + + let attestation_2 = IndexedAttestationBase { + attesting_indices: VariableList::new(validator_indices_2).unwrap(), + data, + signature: AggregateSignature::infinity(), + }; - let mut attestation_2 = IndexedAttestationBase { - attesting_indices: VariableList::new(validator_indices_2).unwrap(), - data, - signature: AggregateSignature::infinity(), + ( + IndexedAttestation::Base(attestation_1), + IndexedAttestation::Base(attestation_2), + ) }; - attestation_2.data.index += 1; + attestation_2.data_mut().index += 1; let fork = self.chain.canonical_head.cached_head().head_fork(); for attestation in &mut [&mut attestation_1, &mut attestation_2] { - for i in attestation.attesting_indices.iter() { - let sk = &self.validator_keypairs[*i as usize].sk; + match attestation { + IndexedAttestation::Base(attestation) => { + for i in attestation.attesting_indices.iter() { + let sk = &self.validator_keypairs[*i as usize].sk; + + let genesis_validators_root = self.chain.genesis_validators_root; + + let domain = self.chain.spec.get_domain( + attestation.data.target.epoch, + Domain::BeaconAttester, + &fork, + genesis_validators_root, + ); + let message = attestation.data.signing_root(domain); + + attestation.signature.add_assign(&sk.sign(message)); + } + } + IndexedAttestation::Electra(attestation) => { + for i in attestation.attesting_indices.iter() { + let sk = &self.validator_keypairs[*i as usize].sk; let genesis_validators_root = self.chain.genesis_validators_root; - let domain = self.chain.spec.get_domain( - attestation.data.target.epoch, - Domain::BeaconAttester, - &fork, - genesis_validators_root, - ); - let message = attestation.data.signing_root(domain); + let domain = self.chain.spec.get_domain( + attestation.data.target.epoch, + Domain::BeaconAttester, + &fork, + genesis_validators_root, + ); + let message = attestation.data.signing_root(domain); - attestation.signature.add_assign(&sk.sign(message)); + attestation.signature.add_assign(&sk.sign(message)); + } + } } } - // TODO(electra): fix this test - AttesterSlashing::Base(AttesterSlashingBase { - attestation_1, - attestation_2, - }) + if fork_name >= ForkName::Electra { + AttesterSlashing::Electra(AttesterSlashingElectra { + attestation_1: attestation_1.as_electra().unwrap().clone(), + attestation_2: attestation_2.as_electra().unwrap().clone(), + }) + } else { + AttesterSlashing::Base(AttesterSlashingBase { + attestation_1: attestation_1.as_base().unwrap().clone(), + attestation_2: attestation_2.as_base().unwrap().clone(), + }) + } } pub fn make_proposer_slashing(&self, validator_index: u64) -> ProposerSlashing { diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 2c5ee0388bd..e5b1f8b264c 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -2,6 +2,7 @@ use beacon_chain::attestation_verification::{ batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations, Error, + ObservedAttestationKey, }; use beacon_chain::observed_aggregates::ObservedAttestationKey; use beacon_chain::test_utils::{MakeAttestationOptions, HARNESS_GENESIS_TIME}; @@ -129,7 +130,12 @@ fn get_valid_unaggregated_attestation( let validator_committee_index = 0; let validator_index = *head .beacon_state - .get_beacon_committee(current_slot, valid_attestation.data().index) + .get_beacon_committee( + current_slot, + valid_attestation + .committee_index() + .expect("should get committee index"), + ) .expect("should get committees") .committee .get(validator_committee_index) @@ -147,8 +153,8 @@ fn get_valid_unaggregated_attestation( ) .expect("should sign attestation"); - let subnet_id = SubnetId::compute_subnet_for_attestation::( - &valid_attestation.to_ref(), + let subnet_id = SubnetId::compute_subnet_for_attestation::( + valid_attestation.to_ref(), head.beacon_state .get_committee_count_at_slot(current_slot) .expect("should get committee count"), @@ -174,7 +180,12 @@ fn get_valid_aggregated_attestation( let current_slot = chain.slot().expect("should get slot"); let committee = state - .get_beacon_committee(current_slot, aggregate.data().index) + .get_beacon_committee( + current_slot, + aggregate + .committee_index() + .expect("should get committee index"), + ) .expect("should get committees"); let committee_len = committee.committee.len(); @@ -217,7 +228,7 @@ fn get_valid_aggregated_attestation( /// attestation. fn get_non_aggregator( chain: &BeaconChain, - aggregate: &AttestationRef, + aggregate: AttestationRef, ) -> (usize, SecretKey) { let head = chain.head_snapshot(); let state = &head.beacon_state; @@ -225,7 +236,12 @@ fn get_non_aggregator( // TODO(electra) make fork-agnostic let committee = state - .get_beacon_committee(current_slot, aggregate.data().index) + .get_beacon_committee( + current_slot, + aggregate + .committee_index() + .expect("should get committee index"), + ) .expect("should get committees"); let committee_len = committee.committee.len(); @@ -372,7 +388,7 @@ impl GossipTester { pub fn non_aggregator(&self) -> (usize, SecretKey) { get_non_aggregator( &self.harness.chain, - &self.valid_aggregate.message().aggregate(), + self.valid_aggregate.message().aggregate(), ) } @@ -664,7 +680,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().committee_index().expect("should get committee index")) .expect("should get committees") .committee .len(); @@ -780,12 +796,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( - aggregator_index - ) + } if aggregator_index == VALIDATOR_COUNT as u64 )) }, @@ -841,7 +852,10 @@ async fn aggregated_gossip_verification() { assert!(matches!( err, AttnError::AttestationSupersetKnown(hash) - if hash == tester.valid_aggregate.message().aggregate().data().tree_hash_root() + if hash == ObservedAttestationKey { + committee_index: tester.valid_aggregate.message().aggregate().expect("should get committee index"), + attestation_data: tester.valid_aggregate.message().aggregate().data().clone(), + }.tree_hash_root() )) }, ) diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 7483f0f13a7..f7c7a0e6e60 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/beacon_node/beacon_chain/tests/block_verification.rs @@ -668,15 +668,23 @@ async fn invalid_signature_attester_slashing() { for &block_index in BLOCK_INDICES { let harness = get_invalid_sigs_harness(&chain_segment).await; let mut snapshots = chain_segment.clone(); - let indexed_attestation = IndexedAttestationBase { - attesting_indices: vec![0].into(), - data: AttestationData { - slot: Slot::new(0), - index: 0, - beacon_block_root: Hash256::zero(), - source: Checkpoint { - epoch: Epoch::new(0), - root: Hash256::zero(), + let fork_name = harness.chain.spec.fork_name_at_slot::(Slot::new(0)); + + let attester_slashing = if fork_name >= ForkName::Electra { + let indexed_attestation = IndexedAttestationElectra { + attesting_indices: vec![0].into(), + data: AttestationData { + slot: Slot::new(0), + index: 0, + beacon_block_root: Hash256::zero(), + source: Checkpoint { + epoch: Epoch::new(0), + root: Hash256::zero(), + }, + target: Checkpoint { + epoch: Epoch::new(0), + root: Hash256::zero(), + }, }, signature: junk_aggregate_signature(), }; @@ -702,12 +710,14 @@ async fn invalid_signature_attester_slashing() { root: Hash256::zero(), }, }, - }, - signature: junk_aggregate_signature(), - }; - let attester_slashing = AttesterSlashingBase { - attestation_1: indexed_attestation.clone(), - attestation_2: indexed_attestation, + signature: junk_aggregate_signature(), + }; + let attester_slashing = AttesterSlashingBase { + attestation_1: indexed_attestation.clone(), + attestation_2: indexed_attestation, + }; + + AttesterSlashing::Base(attester_slashing) }; let (mut block, signature) = snapshots[block_index] @@ -718,31 +728,33 @@ async fn invalid_signature_attester_slashing() { match &mut block.body_mut() { BeaconBlockBodyRefMut::Base(ref mut blk) => { blk.attester_slashings - .push(attester_slashing) + .push(attester_slashing.as_base().unwrap().clone()) .expect("should update attester slashing"); } BeaconBlockBodyRefMut::Altair(ref mut blk) => { blk.attester_slashings - .push(attester_slashing) + .push(attester_slashing.as_base().unwrap().clone()) .expect("should update attester slashing"); } BeaconBlockBodyRefMut::Merge(ref mut blk) => { blk.attester_slashings - .push(attester_slashing) + .push(attester_slashing.as_base().unwrap().clone()) .expect("should update attester slashing"); } BeaconBlockBodyRefMut::Capella(ref mut blk) => { blk.attester_slashings - .push(attester_slashing) + .push(attester_slashing.as_base().unwrap().clone()) .expect("should update attester slashing"); } BeaconBlockBodyRefMut::Deneb(ref mut blk) => { blk.attester_slashings - .push(attester_slashing) + .push(attester_slashing.as_base().unwrap().clone()) .expect("should update attester slashing"); } - BeaconBlockBodyRefMut::Electra(_) => { - panic!("electra test not implemented!"); + BeaconBlockBodyRefMut::Electra(ref mut blk) => { + blk.attester_slashings + .push(attester_slashing.as_electra().unwrap().clone()) + .expect("should update attester slashing"); } } snapshots[block_index].beacon_block = diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index c4c1ee19915..4dc7d20e227 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1223,16 +1223,13 @@ async fn attesting_to_optimistic_head() { let get_aggregated = || { rig.harness .chain - .get_aggregated_attestation(attestation.data()) + .get_aggregated_attestation(attestation.to_ref()) }; let get_aggregated_by_slot_and_root = || { rig.harness .chain - .get_aggregated_attestation_by_slot_and_root( - attestation.data().slot, - &attestation.data().tree_hash_root(), - ) + .get_aggregated_attestation(attestation.to_ref()) }; /* diff --git a/beacon_node/beacon_chain/tests/store_tests.rs b/beacon_node/beacon_chain/tests/store_tests.rs index 9b6268ba789..ef873909309 100644 --- a/beacon_node/beacon_chain/tests/store_tests.rs +++ b/beacon_node/beacon_chain/tests/store_tests.rs @@ -1023,15 +1023,29 @@ async fn multiple_attestations_per_block() { for snapshot in harness.chain.chain_dump().unwrap() { let slot = snapshot.beacon_block.slot(); - assert_eq!( - snapshot - .beacon_block - .as_ref() - .message() - .body() - .attestations_len() as u64, - if slot <= 1 { 0 } else { committees_per_slot } - ); + let fork_name = harness.chain.spec.fork_name_at_slot::(slot); + + if fork_name >= ForkName::Electra { + assert_eq!( + snapshot + .beacon_block + .as_ref() + .message() + .body() + .attestations_len() as u64, + if slot <= 1 { 0 } else { 1 } + ); + } else { + assert_eq!( + snapshot + .beacon_block + .as_ref() + .message() + .body() + .attestations_len() as u64, + if slot <= 1 { 0 } else { committees_per_slot } + ); + } } } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 72047f9ace2..c0d8ee3cf6f 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3369,7 +3369,7 @@ pub fn serve( "error" => format!("{:?}", e), "request_index" => index, "aggregator_index" => aggregate.message().aggregator_index(), - "attestation_index" => aggregate.message().aggregate().data().index, + "attestation_index" => aggregate.message().aggregate().committee_index(), "attestation_slot" => aggregate.message().aggregate().data().slot, ); failures.push(api_types::Failure::new(index, format!("Verification: {:?}", e))); @@ -3390,7 +3390,7 @@ pub fn serve( "error" => format!("{:?}", e), "request_index" => index, "aggregator_index" => verified_aggregate.aggregate().message().aggregator_index(), - "attestation_index" => verified_aggregate.attestation().data().index, + "attestation_index" => verified_aggregate.attestation().committee_index(), "attestation_slot" => verified_aggregate.attestation().data().slot, ); failures.push(api_types::Failure::new(index, format!("Fork choice: {:?}", e))); diff --git a/beacon_node/http_api/src/publish_attestations.rs b/beacon_node/http_api/src/publish_attestations.rs index 541ba8b7871..00654765325 100644 --- a/beacon_node/http_api/src/publish_attestations.rs +++ b/beacon_node/http_api/src/publish_attestations.rs @@ -141,7 +141,7 @@ pub async fn publish_attestations( // move the `attestations` vec into the blocking task, so this small overhead is unavoidable. let attestation_metadata = attestations .iter() - .map(|att| (att.data().slot, att.data().index)) + .map(|att| (att.data().slot, att.committee_index())) .collect::>(); // Gossip validate and publish attestations that can be immediately processed. diff --git a/beacon_node/lighthouse_network/src/types/pubsub.rs b/beacon_node/lighthouse_network/src/types/pubsub.rs index 008cf48f298..b3b97ba7892 100644 --- a/beacon_node/lighthouse_network/src/types/pubsub.rs +++ b/beacon_node/lighthouse_network/src/types/pubsub.rs @@ -401,17 +401,17 @@ impl std::fmt::Display for PubsubMessage { ), PubsubMessage::AggregateAndProofAttestation(att) => write!( f, - "Aggregate and Proof: slot: {}, index: {}, aggregator_index: {}", + "Aggregate and Proof: slot: {}, index: {:?}, aggregator_index: {}", att.message().aggregate().data().slot, - att.message().aggregate().data().index, + att.message().aggregate().committee_index(), att.message().aggregator_index(), ), PubsubMessage::Attestation(data) => write!( f, - "Attestation: subnet_id: {}, attestation_slot: {}, attestation_index: {}", + "Attestation: subnet_id: {}, attestation_slot: {}, attestation_index: {:?}", *data.0, data.1.data().slot, - data.1.data().index, + data.1.committee_index(), ), PubsubMessage::VoluntaryExit(_data) => write!(f, "Voluntary Exit"), PubsubMessage::ProposerSlashing(_data) => write!(f, "Proposer Slashing"), diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 579d37264aa..c6ed6eb7f6e 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -1,4 +1,4 @@ -use crate::attestation_storage::{AttestationRef, CompactIndexedAttestation}; +use crate::attestation_storage::{CompactAttestationRef, CompactIndexedAttestation}; use crate::max_cover::MaxCover; use crate::reward_cache::RewardCache; use state_processing::common::{ diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 0ec1ac2f2e8..43b1c3abbb3 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -41,7 +41,6 @@ pub struct SplitAttestation { pub indexed: CompactIndexedAttestation, } -// TODO(electra): rename this type #[derive(Debug, Clone)] pub struct CompactAttestationRef<'a, E: EthSpec> { pub checkpoint: &'a CheckpointKey, @@ -171,7 +170,7 @@ impl CompactIndexedAttestation { } } - pub fn aggregate(&mut self, other: &Self) { + pub fn aggregate(&mut self, other: &Self) -> Option<()> { match (self, other) { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { this.aggregate(other) @@ -181,7 +180,7 @@ impl CompactIndexedAttestation { CompactIndexedAttestation::Electra(other), ) => this.aggregate_same_committee(other), // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? - _ => (), + _ => None, } } } @@ -193,7 +192,7 @@ impl CompactIndexedAttestationBase { .is_zero() } - pub fn aggregate(&mut self, other: &Self) { + pub fn aggregate(&mut self, other: &Self) -> Option<()> { self.attesting_indices = self .attesting_indices .drain(..) @@ -202,6 +201,8 @@ impl CompactIndexedAttestationBase { .collect(); self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.signature.add_assign_aggregate(&other.signature); + + Some(()) } } @@ -215,9 +216,11 @@ impl CompactIndexedAttestationElectra { .is_zero() } - pub fn aggregate_same_committee(&mut self, other: &Self) { + pub fn aggregate_same_committee(&mut self, other: &Self) -> Option<()> { // TODO(electra): remove assert in favour of Result - assert_eq!(self.committee_bits, other.committee_bits); + if self.committee_bits != other.committee_bits { + return None; + } self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.attesting_indices = self .attesting_indices @@ -226,34 +229,48 @@ impl CompactIndexedAttestationElectra { .dedup() .collect(); self.signature.add_assign_aggregate(&other.signature); + Some(()) } - pub fn aggregate_with_disjoint_committees(&mut self, other: &Self) { - // TODO(electra): remove asserts or use Result - assert!(self + pub fn aggregate_with_disjoint_committees(&mut self, other: &Self) -> Option<()> { + if !self .committee_bits .intersection(&other.committee_bits) - .is_zero(),); + .is_zero() + { + return None; + } // The attestation being aggregated in must only have 1 committee bit set. - assert_eq!(other.committee_bits.num_set_bits(), 1); + if other.committee_bits.num_set_bits() != 1 { + return None; + } + // Check we are aggregating in increasing committee index order (so we can append // aggregation bits). - assert!(self.committee_bits.highest_set_bit() < other.committee_bits.highest_set_bit()); + if self.committee_bits.highest_set_bit() >= other.committee_bits.highest_set_bit() { + return None; + } self.committee_bits = self.committee_bits.union(&other.committee_bits); - self.aggregation_bits = - bitlist_extend(&self.aggregation_bits, &other.aggregation_bits).unwrap(); - self.attesting_indices = self - .attesting_indices - .drain(..) - .merge(other.attesting_indices.iter().copied()) - .dedup() - .collect(); - self.signature.add_assign_aggregate(&other.signature); + if let Some(agg_bits) = bitlist_extend(&self.aggregation_bits, &other.aggregation_bits) { + self.aggregation_bits = agg_bits; + + self.attesting_indices = self + .attesting_indices + .drain(..) + .merge(other.attesting_indices.iter().copied()) + .dedup() + .collect(); + self.signature.add_assign_aggregate(&other.signature); + + return Some(()); + } + + None } - pub fn committee_index(&self) -> u64 { - *self.get_committee_indices().first().unwrap_or(&0u64) + pub fn committee_index(&self) -> Option { + self.get_committee_indices().first().copied() } pub fn get_committee_indices(&self) -> Vec { @@ -350,27 +367,28 @@ impl AttestationMap { continue; } }; - let committee_index = electra_attestation.committee_index(); - if let Some(existing_attestation) = - best_attestations_by_committee.get_mut(&committee_index) - { - // Search for the best (most aggregation bits) attestation for this committee - // index. - if electra_attestation.aggregation_bits.num_set_bits() - > existing_attestation.aggregation_bits.num_set_bits() + if let Some(committee_index) = electra_attestation.committee_index() { + if let Some(existing_attestation) = + best_attestations_by_committee.get_mut(&committee_index) { - // New attestation is better than the previously known one for this - // committee. Replace it. - std::mem::swap(existing_attestation, &mut electra_attestation); + // Search for the best (most aggregation bits) attestation for this committee + // index. + if electra_attestation.aggregation_bits.num_set_bits() + > existing_attestation.aggregation_bits.num_set_bits() + { + // New attestation is better than the previously known one for this + // committee. Replace it. + std::mem::swap(existing_attestation, &mut electra_attestation); + } + // Put the inferior attestation into the list of aggregated attestations + // without performing any cross-committee aggregation. + aggregated_attestations + .push(CompactIndexedAttestation::Electra(electra_attestation)); + } else { + // First attestation seen for this committee. Place it in the map + // provisionally. + best_attestations_by_committee.insert(committee_index, electra_attestation); } - // Put the inferior attestation into the list of aggregated attestations - // without performing any cross-committee aggregation. - aggregated_attestations - .push(CompactIndexedAttestation::Electra(electra_attestation)); - } else { - // First attestation seen for this committee. Place it in the map - // provisionally. - best_attestations_by_committee.insert(committee_index, electra_attestation); } } diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 6acd7b5a9e8..f9021bb258d 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -1281,9 +1281,7 @@ mod release_tests { for att in &best_attestations { match fork_name { ForkName::Electra => { - // TODO(electra) some attestations only have 2 or 3 agg bits set - // others have 5 - assert!(att.num_set_aggregation_bits() >= 2); + assert!(att.num_set_aggregation_bits() >= small_step_size); } _ => { assert!(att.num_set_aggregation_bits() >= big_step_size); diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 3b05cfab1fd..d2935dbca45 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -435,7 +435,12 @@ impl ForkChoiceTest { let validator_committee_index = 0; let validator_index = *head .beacon_state - .get_beacon_committee(current_slot, attestation.data().index) + .get_beacon_committee( + current_slot, + attestation + .committee_index() + .expect("should get committee index"), + ) .expect("should get committees") .committee .get(validator_committee_index) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 9bbc4853c85..3c648b53f48 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -13,7 +13,6 @@ pub mod attesting_indices_base { ) -> 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(), @@ -52,6 +51,100 @@ 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, @@ -148,7 +241,7 @@ pub mod attesting_indices_electra { Ok(indices) } - fn get_committee_indices( + pub fn get_committee_indices( committee_bits: &BitVector, ) -> Vec { committee_bits diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs index b8316063a3d..85dbe4da79b 100644 --- a/consensus/state_processing/src/per_block_processing/signature_sets.rs +++ b/consensus/state_processing/src/per_block_processing/signature_sets.rs @@ -326,6 +326,7 @@ where genesis_validators_root, ); + // TODO(electra), signing root isnt unique in the case of electra let message = indexed_attestation.data().signing_root(domain); Ok(SignatureSet::multiple_pubkeys(signature, pubkeys, message)) @@ -436,7 +437,6 @@ where let message = slot.signing_root(domain); let signature = signed_aggregate_and_proof.message().selection_proof(); let validator_index = signed_aggregate_and_proof.message().aggregator_index(); - Ok(SignatureSet::single_pubkey( signature, get_pubkey(validator_index as usize).ok_or(Error::ValidatorUnknown(validator_index))?, diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 2ebc605196d..dd9827ce65c 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -22,8 +22,6 @@ pub enum Error { AlreadySigned(usize), SubnetCountIsZero(ArithError), IncorrectStateVariant, - InvalidCommitteeLength, - InvalidCommitteeIndex, } #[superstruct( @@ -45,7 +43,9 @@ pub enum Error { serde(bound = "E: EthSpec", deny_unknown_fields), arbitrary(bound = "E: EthSpec"), ), - ref_attributes(derive(TreeHash), tree_hash(enum_behaviour = "transparent")) + ref_attributes(derive(TreeHash), tree_hash(enum_behaviour = "transparent")), + cast_error(ty = "Error", expr = "Error::IncorrectStateVariant"), + partial_getter_error(ty = "Error", expr = "Error::IncorrectStateVariant") )] #[derive( Debug, @@ -77,14 +77,14 @@ pub struct Attestation { // TODO(electra): think about how to handle fork variants here impl TestRandom for Attestation { fn random_for_test(rng: &mut impl RngCore) -> Self { - let aggregation_bits: BitList = BitList::random_for_test(rng); - // let committee_bits: BitList = BitList::random_for_test(rng); + let aggregation_bits = BitList::random_for_test(rng); let data = AttestationData::random_for_test(rng); let signature = AggregateSignature::random_for_test(rng); + let committee_bits = BitVector::random_for_test(rng); - Self::Base(AttestationBase { + Self::Electra(AttestationElectra { aggregation_bits, - // committee_bits, + committee_bits, data, signature, }) @@ -181,9 +181,9 @@ impl Attestation { } } - pub fn committee_index(&self) -> u64 { + pub fn committee_index(&self) -> Option { match self { - Attestation::Base(att) => att.data.index, + Attestation::Base(att) => Some(att.data.index), Attestation::Electra(att) => att.committee_index(), } } @@ -232,12 +232,31 @@ impl<'a, E: EthSpec> AttestationRef<'a, E> { } } - pub fn committee_index(&self) -> u64 { + pub fn committee_index(&self) -> Option { match self { - AttestationRef::Base(att) => att.data.index, + AttestationRef::Base(att) => Some(att.data.index), AttestationRef::Electra(att) => att.committee_index(), } } + + pub fn set_aggregation_bits(&self) -> Vec { + match self { + Self::Base(att) => att + .aggregation_bits + .iter() + .enumerate() + .filter(|(_i, bit)| *bit) + .map(|(i, _bit)| i) + .collect::>(), + Self::Electra(att) => att + .aggregation_bits + .iter() + .enumerate() + .filter(|(_i, bit)| *bit) + .map(|(i, _bit)| i) + .collect::>(), + } + } } impl AttestationElectra { @@ -251,8 +270,8 @@ impl AttestationElectra { .is_zero() } - pub fn committee_index(&self) -> u64 { - *self.get_committee_indices().first().unwrap_or(&0u64) + pub fn committee_index(&self) -> Option { + self.get_committee_indices().first().cloned() } pub fn get_committee_indices(&self) -> Vec { diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 68e4b9f0eea..5c6915b245c 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -159,7 +159,14 @@ pub enum Error { IndexNotSupported(usize), InvalidFlagIndex(usize), MerkleTreeError(merkle_proof::MerkleTreeError), - NoCommitteeFound, + NoCommitteeFound(CommitteeIndex), + InvalidCommitteeIndex(CommitteeIndex), + InvalidSelectionProof { + aggregator_index: u64, + }, + AggregatorNotInCommittee { + aggregator_index: u64, + }, } /// Control whether an epoch-indexed field can be indexed at the next epoch or not. diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index 3ffbd72bb68..bbf6fdc3653 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -206,14 +206,17 @@ impl Decode for IndexedAttestation { } } +// TODO(electra): think about how to handle fork variants here impl TestRandom for IndexedAttestation { fn random_for_test(rng: &mut impl RngCore) -> Self { let attesting_indices = VariableList::random_for_test(rng); + // let committee_bits: BitList = BitList::random_for_test(rng); let data = AttestationData::random_for_test(rng); let signature = AggregateSignature::random_for_test(rng); Self::Base(IndexedAttestationBase { attesting_indices, + // committee_bits, data, signature, }) diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index 9f94849b421..c1603a0af13 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -75,7 +75,6 @@ impl SignedAggregateAndProof { genesis_validators_root, spec, ); - let target_epoch = message.aggregate().data().slot.epoch(E::slots_per_epoch()); let domain = spec.get_domain( target_epoch, diff --git a/consensus/types/src/subnet_id.rs b/consensus/types/src/subnet_id.rs index 18f2b838f3b..ccd313ebac0 100644 --- a/consensus/types/src/subnet_id.rs +++ b/consensus/types/src/subnet_id.rs @@ -40,7 +40,7 @@ impl SubnetId { /// Compute the subnet for an attestation where each slot in the /// attestation epoch contains `committee_count_per_slot` committees. pub fn compute_subnet_for_attestation( - attestation: &AttestationRef, + attestation: AttestationRef, committee_count_per_slot: u64, spec: &ChainSpec, ) -> Result { @@ -48,7 +48,7 @@ impl SubnetId { Self::compute_subnet::( attestation.data().slot, - attestation.committee_index(), + committee_index, committee_count_per_slot, spec, ) diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 4213edc4392..404fe41249b 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -14,11 +14,11 @@ use std::ops::Deref; use std::sync::Arc; use tokio::time::{sleep, sleep_until, Duration, Instant}; use tree_hash::TreeHash; +use types::ForkName; use types::{ - attestation::AttestationBase, AggregateSignature, Attestation, AttestationData, BitList, - ChainSpec, CommitteeIndex, EthSpec, Slot, + attestation::AttestationBase, AggregateSignature, Attestation, AttestationData, + AttestationElectra, BitList, BitVector, ChainSpec, CommitteeIndex, EthSpec, Slot, }; -use types::{AttestationElectra, BitVector, ForkName}; /// Builds an `AttestationService`. pub struct AttestationServiceBuilder { @@ -643,7 +643,7 @@ impl AttestationService { "aggregator" => signed_aggregate_and_proof.message().aggregator_index(), "signatures" => attestation.num_set_aggregation_bits(), "head_block" => format!("{:?}", attestation.data().beacon_block_root), - "committee_index" => attestation.data().index, + "committee_index" => attestation.committee_index(), "slot" => attestation.data().slot.as_u64(), "type" => "aggregated", ); @@ -657,7 +657,7 @@ impl AttestationService { "Failed to publish attestation"; "error" => %e, "aggregator" => signed_aggregate_and_proof.message().aggregator_index(), - "committee_index" => attestation.data().index, + "committee_index" => attestation.committee_index(), "slot" => attestation.data().slot.as_u64(), "type" => "aggregated", );