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 <[email protected]>

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,21 @@ 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()
+                    .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`.
@@ -2185,7 +2202,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 +2221,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());
@@ -2329,7 +2346,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     pub fn filter_op_pool_attestation(
         &self,
         filter_cache: &mut HashMap<(Hash256, Epoch), bool>,
-        att: &AttestationRef<T::EthSpec>,
+        att: &CompactAttestationRef<T::EthSpec>,
         state: &BeaconState<T::EthSpec>,
     ) -> bool {
         *filter_cache
@@ -4948,11 +4965,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         initialize_epoch_cache(&mut state, &self.spec)?;
 
         let mut prev_filter_cache = HashMap::new();
-        let prev_attestation_filter = |att: &AttestationRef<T::EthSpec>| {
+        let prev_attestation_filter = |att: &CompactAttestationRef<T::EthSpec>| {
             self.filter_op_pool_attestation(&mut prev_filter_cache, att, &state)
         };
         let mut curr_filter_cache = HashMap::new();
-        let curr_attestation_filter = |att: &AttestationRef<T::EthSpec>| {
+        let curr_attestation_filter = |att: &CompactAttestationRef<T::EthSpec>| {
             self.filter_op_pool_attestation(&mut curr_filter_cache, att, &state)
         };
 

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/errors.rs

@@ -226,6 +226,8 @@ pub enum BeaconChainError {
     LightClientError(LightClientError),
     UnsupportedFork,
     MilhouseError(MilhouseError),
+    AttestationError(AttestationError),
+    AttestationCommitteeIndexNotSet,
 }
 
 easy_from_to!(SlotProcessingError, BeaconChainError);
@@ -256,6 +258,7 @@ easy_from_to!(AvailabilityCheckError, BeaconChainError);
 easy_from_to!(EpochCacheError, BeaconChainError);
 easy_from_to!(LightClientError, BeaconChainError);
 easy_from_to!(MilhouseError, BeaconChainError);
+easy_from_to!(AttestationError, BeaconChainError);
 
 #[derive(Debug)]
 pub enum BlockProductionError {

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,
             })