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