- 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

Dockerfile

@@ -1,7 +1,7 @@
 FROM rust:1.75.0-bullseye AS builder
 RUN apt-get update && apt-get -y upgrade && apt-get install -y cmake libclang-dev
 COPY . lighthouse
-ARG FEATURES
+ARG FEATURES=spec-minimal
 ARG PROFILE=release
 ARG CARGO_USE_GIT_CLI=true
 ENV FEATURES $FEATURES

beacon_node/beacon_chain/src/attestation_verification.rs

@@ -55,10 +55,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};
@@ -248,9 +249,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<BeaconChainError> for Error {
     fn from(e: BeaconChainError) -> Self {
-        Error::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),
+        }
     }
 }
 
@@ -265,10 +287,17 @@ enum CheckAttestationSignature {
 /// `IndexedAttestation` can be derived.
 ///
 /// These attestations have *not* undergone signature verification.
+/// The `observed_attestation_key_root` is the hashed value of an `ObservedAttestationKey`.
 struct IndexedAggregatedAttestation<'a, T: BeaconChainTypes> {
     signed_aggregate: &'a SignedAggregateAndProof<T::EthSpec>,
     indexed_attestation: IndexedAttestation<T::EthSpec>,
-    attestation_data_root: Hash256,
+    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
@@ -466,18 +495,25 @@ 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(),
+            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);
-            return Err(Error::AttestationSupersetKnown(attestation_data_root));
+            return Err(Error::AttestationSupersetKnown(
+                observed_attestation_key_root,
+            ));
         }
 
         let aggregator_index = signed_aggregate.message().aggregator_index();
@@ -523,7 +559,7 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
         if attestation.is_aggregation_bits_zero() {
             Err(Error::EmptyAggregationBitfield)
         } else {
-            Ok(attestation_data_root)
+            Ok(observed_attestation_key_root)
         }
     }
 
@@ -533,10 +569,8 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
         chain: &BeaconChain<T>,
     ) -> Result<Self, AttestationSlashInfo<'a, T, Error>> {
         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(
@@ -545,11 +579,12 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
                 ))
             }
         };
-
         let get_indexed_attestation_with_committee =
             |(committees, _): (Vec<BeaconCommittee>, 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)
@@ -559,13 +594,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
@@ -579,6 +614,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,
@@ -591,13 +627,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,
@@ -611,11 +652,10 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
                 ))
             }
         };
-
         Ok(IndexedAggregatedAttestation {
             signed_aggregate,
             indexed_attestation,
-            attestation_data_root,
+            observed_attestation_key_root,
         })
     }
 }
@@ -624,7 +664,7 @@ impl<'a, T: BeaconChainTypes> VerifiedAggregatedAttestation<'a, T> {
     /// Run the checks that happen after the indexed attestation and signature have been checked.
     fn verify_late_checks(
         signed_aggregate: &SignedAggregateAndProof<T::EthSpec>,
-        attestation_data_root: Hash256,
+        observed_attestation_key_root: Hash256,
         chain: &BeaconChain<T>,
     ) -> Result<(), Error> {
         let attestation = signed_aggregate.message().aggregate();
@@ -637,11 +677,13 @@ 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);
-            return Err(Error::AttestationSupersetKnown(attestation_data_root));
+            return Err(Error::AttestationSupersetKnown(
+                observed_attestation_key_root,
+            ));
         }
 
         // Observe the aggregator so we don't process another aggregate from them.
@@ -701,7 +743,7 @@ impl<'a, T: BeaconChainTypes> VerifiedAggregatedAttestation<'a, T> {
         let IndexedAggregatedAttestation {
             signed_aggregate,
             indexed_attestation,
-            attestation_data_root,
+            observed_attestation_key_root,
         } = signed_aggregate;
 
         match check_signature {
@@ -725,7 +767,9 @@ impl<'a, T: BeaconChainTypes> VerifiedAggregatedAttestation<'a, T> {
             CheckAttestationSignature::No => (),
         };
 
-        if let Err(e) = Self::verify_late_checks(signed_aggregate, attestation_data_root, chain) {
+        if let Err(e) =
+            Self::verify_late_checks(signed_aggregate, observed_attestation_key_root, chain)
+        {
             return Err(SignatureValid(indexed_attestation, e));
         }
 
@@ -1147,7 +1191,6 @@ pub fn verify_attestation_signature<T: BeaconChainTypes>(
         &chain.spec,
     )
     .map_err(BeaconChainError::SignatureSetError)?;
-
     metrics::stop_timer(signature_setup_timer);
 
     let _signature_verification_timer =
@@ -1324,16 +1367,14 @@ pub fn obtain_indexed_attestation_and_committees_per_slot<T: BeaconChainTypes>(
     })
 }
 
-// TODO(electra) update comments below to reflect logic changes
-// i.e. this now runs the map_fn on a list of committees for the slot of the provided attestation
 /// Runs the `map_fn` with the committee and committee count per slot for the given `attestation`.
 ///
-/// This function exists in this odd "map" pattern because efficiently obtaining the committee for
-/// an attestation can be complex. It might involve reading straight from the
+/// This function exists in this odd "map" pattern because efficiently obtaining the committees for
+/// 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 committee for `attestation` isn'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<T, F, R>(
     chain: &BeaconChain<T>,

beacon_node/beacon_chain/src/attestation_verification/batch.rs

@@ -66,7 +66,6 @@ where
             .ok_or(BeaconChainError::ValidatorPubkeyCacheLockTimeout)?;
 
         let mut signature_sets = Vec::with_capacity(num_indexed * 3);
-
         // Iterate, flattening to get only the `Ok` values.
         for indexed in indexing_results.iter().flatten() {
             let signed_aggregate = &indexed.signed_aggregate;

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -85,7 +85,9 @@ use futures::channel::mpsc::Sender;
 use itertools::process_results;
 use itertools::Itertools;
 use kzg::Kzg;
-use operation_pool::{AttestationRef, OperationPool, PersistedOperationPool, ReceivedPreCapella};
+use operation_pool::{
+    CompactAttestationRef, OperationPool, PersistedOperationPool, ReceivedPreCapella,
+};
 use parking_lot::{Mutex, RwLock};
 use proto_array::{DoNotReOrg, ProposerHeadError};
 use safe_arith::SafeArith;
@@ -1609,6 +1611,20 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         Ok((duties, dependent_root, execution_status))
     }
 
+    pub fn get_aggregated_attestation(
+        &self,
+        attestation: &AttestationRef<T::EthSpec>,
+    ) -> Result<Option<Attestation<T::EthSpec>>, 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(),
+            ),
+        }
+    }
+
     /// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`.
     ///
     /// The attestation will be obtained from `self.naive_aggregation_pool`.
@@ -2185,7 +2201,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                 self.log,
                 "Stored unaggregated attestation";
                 "outcome" => ?outcome,
-                "index" => attestation.data().index,
+                "index" => attestation.committee_index(),
                 "slot" => attestation.data().slot.as_u64(),
             ),
             Err(NaiveAggregationError::SlotTooLow {
@@ -2204,7 +2220,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                         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());

beacon_node/beacon_chain/src/early_attester_cache.rs

@@ -123,7 +123,6 @@ impl<E: EthSpec> EarlyAttesterCache<E> {
             item.committee_lengths
                 .get_committee_length::<E>(request_slot, request_index, spec)?;
 
-        // TODO(electra) make fork-agnostic
         let attestation = if spec.fork_name_at_slot::<E>(request_slot) >= ForkName::Electra {
             let mut committee_bits = BitVector::default();
             if committee_len > 0 {

beacon_node/beacon_chain/src/naive_aggregation_pool.rs

@@ -261,9 +261,9 @@ impl<E: EthSpec> AggregateMap for AggregatedAttestationMap<E> {
         };
 
         let attestation_key = AttestationKey::from_attestation_ref(a)?;
-        let attestation_data_root = attestation_key.tree_hash_root();
+        let attestation_key_root = attestation_key.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(aggregation_bit)
                 .map_err(|_| Error::InconsistentBitfieldLengths)?
@@ -285,7 +285,7 @@ impl<E: EthSpec> AggregateMap for AggregatedAttestationMap<E> {
             }
 
             self.map
-                .insert(attestation_data_root, a.clone_as_attestation());
+                .insert(attestation_key_root, a.clone_as_attestation());
             Ok(InsertOutcome::NewItemInserted {
                 committee_index: aggregation_bit,
             })