Electra attestations #5712 review

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};
@@ -519,7 +520,7 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
  .tree_hash_root();
 
  // [New in Electra:EIP7549]
- verify_committee_index(attestation, &chain.spec)?;
+ verify_committee_index(attestation)?;
 
  if chain
  .observed_attestations
@@ -596,59 +597,62 @@ 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<BeaconCommittee>, CommitteesPerSlot)| {
- 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):
+ 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.
- 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 });
- }
+ 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;
 
- // 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 });
- }
+ let committee = committees
+ .get(index as usize)
+ .ok_or(Error::NoCommitteeForSlotAndIndex { slot, 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,
- })
- }
+ 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) => {
+ 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(
  &committees,
- signed_aggregate,
- &chain.spec,
+ &signed_aggregate.message.aggregate,
  )
  .map_err(|e| BeaconChainError::from(e).into())
  }
@@ -837,7 +841,7 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> {
  }
 
  // [New in Electra:EIP7549]
- verify_committee_index(attestation, &chain.spec)?;
+ verify_committee_index(attestation)?;
 
  // Attestations must be for a known block. If the block is unknown, we simply drop the
  // attestation and do not delay consideration for later.
@@ -1337,17 +1341,10 @@ pub fn verify_signed_aggregate_signatures<T: BeaconChainTypes>(
 
 /// 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<E: EthSpec>(
- attestation: AttestationRef<E>,
- spec: &ChainSpec,
-) -> Result<(), Error> {
- if spec.fork_name_at_slot::<E>(attestation.data().slot) >= ForkName::Electra {
+pub fn verify_committee_index<E: EthSpec>(attestation: AttestationRef<E>) -> Result<(), Error> {
+ if let Ok(committee_bits) = attestation.committee_bits() {
  // Check to ensure that the attestation is for a single committee.
- let num_committee_bits = get_committee_indices::<E>(
- attestation
- .committee_bits()
- .map_err(|e| Error::BeaconChainError(e.into()))?,
- );
+ let num_committee_bits = get_committee_indices::<E>(committee_bits);
  if num_committee_bits.len() != 1 {
  return Err(Error::NotExactlyOneCommitteeBitSet(
  num_committee_bits.len(),
@@ -1424,6 +1421,8 @@ pub fn obtain_indexed_attestation_and_committees_per_slot<T: BeaconChainTypes>(
 ///
 /// 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`.
+///
+/// Committees are sorted by ascending index order 0..committees_per_slot
 fn map_attestation_committees<T, F, R>(
  chain: &BeaconChain<T>,
  attestation: AttestationRef<T::EthSpec>,

beacon_node/beacon_chain/src/attester_cache.rs

@@ -15,6 +15,7 @@ use state_processing::state_advance::{partial_state_advance, Error as StateAdvan
 use std::collections::HashMap;
 use std::ops::Range;
 use types::{
+ attestation::Error as AttestationError,
  beacon_state::{
  compute_committee_index_in_epoch, compute_committee_range_in_epoch, epoch_committee_count,
  },
@@ -59,6 +60,7 @@ pub enum Error {
  InverseRange {
  range: Range<usize>,
  },
+ AttestationError(AttestationError),
 }
 
 impl From<BeaconStateError> for Error {

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -122,7 +122,6 @@ use store::{
 use task_executor::{ShutdownReason, TaskExecutor};
 use tokio_stream::Stream;
 use tree_hash::TreeHash;
-use types::attestation::AttestationBase;
 use types::blob_sidecar::FixedBlobSidecarList;
 use types::payload::BlockProductionVersion;
 use types::*;
@@ -1994,36 +1993,15 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
  };
  drop(cache_timer);
 
- if self.spec.fork_name_at_slot::<T::EthSpec>(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(),
- }))
- }
+ Ok(Attestation::<T::EthSpec>::empty_for_signing(
+ request_index,
+ committee_len,
+ request_slot,
+ beacon_block_root,
+ justified_checkpoint,
+ target,
+ &self.spec,
+ )?)
  }
 
  /// Performs the same validation as `Self::verify_unaggregated_attestation_for_gossip`, but for

beacon_node/beacon_chain/src/early_attester_cache.rs

@@ -6,7 +6,6 @@ use crate::{
 use parking_lot::RwLock;
 use proto_array::Block as ProtoBlock;
 use std::sync::Arc;
-use types::attestation::AttestationBase;
 use types::*;
 
 pub struct CacheItem<E: EthSpec> {
@@ -123,40 +122,16 @@ impl<E: EthSpec> EarlyAttesterCache<E> {
  item.committee_lengths
  .get_committee_length::<E>(request_slot, request_index, spec)?;
 
- let attestation = if spec.fork_name_at_slot::<E>(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(),
- })
- };
+ let attestation = Attestation::empty_for_signing(
+ request_index,
+ committee_len,
+ request_slot,
+ item.beacon_block_root,
+ item.source,
+ item.target,
+ spec,
+ )
+ .map_err(Error::AttestationError)?;
 
  metrics::inc_counter(&metrics::BEACON_EARLY_ATTESTER_CACHE_HITS);
 

beacon_node/beacon_chain/src/observed_aggregates.rs

@@ -473,12 +473,13 @@ where
 #[cfg(not(debug_assertions))]
 mod tests {
  use super::*;
- use types::{test_utils::test_random_instance, Hash256};
+ use types::{test_utils::test_random_instance, AttestationBase, Hash256};
 
  type E = types::MainnetEthSpec;
 
  fn get_attestation(slot: Slot, beacon_block_root: u64) -> Attestation<E> {
- let mut a: Attestation<E> = test_random_instance();
+ let a: AttestationBase<E> = test_random_instance();
+ let mut a = Attestation::Base(a);
  a.data_mut().slot = slot;
  a.data_mut().beacon_block_root = Hash256::from_low_u64_be(beacon_block_root);
  a