Electra attestation changes sean review (#5972)

* instantiate empty bitlist in unreachable code

* clean up error conversion

* fork enabled bool cleanup

* remove a couple todos

* return bools instead of options in `aggregate` and use the result

* delete commented out code

* use map macros in simple transformations

* remove signers_disjoint_from

* get ef tests compiling

* get ef tests compiling

* update intentionally excluded files

beacon_node/beacon_chain/src/attestation_verification.rs

@@ -61,10 +61,9 @@ use std::borrow::Cow;
 use strum::AsRefStr;
 use tree_hash::TreeHash;
 use types::{
-    Attestation, AttestationRef, BeaconCommittee,
-    BeaconStateError::{self, NoCommitteeFound},
-    ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation,
-    SelectionProof, SignedAggregateAndProof, Slot, SubnetId,
+    Attestation, AttestationRef, BeaconCommittee, BeaconStateError::NoCommitteeFound, ChainSpec,
+    CommitteeIndex, Epoch, EthSpec, Hash256, IndexedAttestation, SelectionProof,
+    SignedAggregateAndProof, Slot, SubnetId,
 };
 
 pub use batch::{batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations};
@@ -266,30 +265,9 @@ 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 {
-        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),
-        }
+        Self::BeaconChainError(e)
     }
 }
 
@@ -1169,7 +1147,7 @@ pub fn verify_propagation_slot_range<S: SlotClock, E: EthSpec>(
 
     let current_fork =
         spec.fork_name_at_slot::<E>(slot_clock.now().ok_or(BeaconChainError::UnableToReadSlot)?);
-    let earliest_permissible_slot = if current_fork < ForkName::Deneb {
+    let earliest_permissible_slot = if !current_fork.deneb_enabled() {
         one_epoch_prior
     // EIP-7045
     } else {

beacon_node/operation_pool/src/attestation.rs

@@ -184,8 +184,8 @@ pub fn earliest_attestation_validators<E: EthSpec>(
     // Bitfield of validators whose attestations are new/fresh.
     let mut new_validators = match attestation.indexed {
         CompactIndexedAttestation::Base(indexed_att) => indexed_att.aggregation_bits.clone(),
-        // TODO(electra) per the comments above, this code path is obsolete post altair fork, so maybe we should just return an empty bitlist here?
-        CompactIndexedAttestation::Electra(_) => todo!(),
+        // This code path is obsolete post altair fork, so we just return an empty bitlist here.
+        CompactIndexedAttestation::Electra(_) => return BitList::with_capacity(0).unwrap(),
     };
 
     let state_attestations = if attestation.checkpoint.target_epoch == state.current_epoch() {

beacon_node/operation_pool/src/attestation_storage.rs

@@ -165,22 +165,22 @@ impl<E: EthSpec> CompactIndexedAttestation<E> {
                 CompactIndexedAttestation::Electra(this),
                 CompactIndexedAttestation::Electra(other),
             ) => this.should_aggregate(other),
-            // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with?
             _ => false,
         }
     }
 
-    pub fn aggregate(&mut self, other: &Self) -> Option<()> {
+    /// Returns `true` if aggregated, otherwise `false`.
+    pub fn aggregate(&mut self, other: &Self) -> bool {
         match (self, other) {
             (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => {
-                this.aggregate(other)
+                this.aggregate(other);
+                true
             }
             (
                 CompactIndexedAttestation::Electra(this),
                 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,
+            _ => false,
         }
     }
 }
@@ -192,7 +192,7 @@ impl<E: EthSpec> CompactIndexedAttestationBase<E> {
             .is_zero()
     }
 
-    pub fn aggregate(&mut self, other: &Self) -> Option<()> {
+    pub fn aggregate(&mut self, other: &Self) {
         self.attesting_indices = self
             .attesting_indices
             .drain(..)
@@ -201,8 +201,6 @@ impl<E: EthSpec> CompactIndexedAttestationBase<E> {
             .collect();
         self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits);
         self.signature.add_assign_aggregate(&other.signature);
-
-        Some(())
     }
 }
 
@@ -216,9 +214,10 @@ impl<E: EthSpec> CompactIndexedAttestationElectra<E> {
                 .is_zero()
     }
 
-    pub fn aggregate_same_committee(&mut self, other: &Self) -> Option<()> {
+    /// Returns `true` if aggregated, otherwise `false`.
+    pub fn aggregate_same_committee(&mut self, other: &Self) -> bool {
         if self.committee_bits != other.committee_bits {
-            return None;
+            return false;
         }
         self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits);
         self.attesting_indices = self
@@ -228,7 +227,7 @@ impl<E: EthSpec> CompactIndexedAttestationElectra<E> {
             .dedup()
             .collect();
         self.signature.add_assign_aggregate(&other.signature);
-        Some(())
+        true
     }
 
     pub fn aggregate_with_disjoint_committees(&mut self, other: &Self) -> Option<()> {
@@ -318,8 +317,7 @@ impl<E: EthSpec> AttestationMap<E> {
 
         for existing_attestation in attestations.iter_mut() {
             if existing_attestation.should_aggregate(&indexed) {
-                existing_attestation.aggregate(&indexed);
-                aggregated = true;
+                aggregated = existing_attestation.aggregate(&indexed);
             } else if *existing_attestation == indexed {
                 aggregated = true;
             }

beacon_node/operation_pool/src/lib.rs

@@ -39,7 +39,7 @@ use std::ptr;
 use types::{
     sync_aggregate::Error as SyncAggregateError, typenum::Unsigned, AbstractExecPayload,
     Attestation, AttestationData, AttesterSlashing, BeaconState, BeaconStateError, ChainSpec,
-    Epoch, EthSpec, ForkName, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange,
+    Epoch, EthSpec, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange,
     SignedVoluntaryExit, Slot, SyncAggregate, SyncCommitteeContribution, Validator,
 };
 
@@ -316,10 +316,10 @@ impl<E: EthSpec> OperationPool<E> {
             )
             .inspect(|_| num_curr_valid += 1);
 
-        let curr_epoch_limit = if fork_name < ForkName::Electra {
-            E::MaxAttestations::to_usize()
-        } else {
+        let curr_epoch_limit = if fork_name.electra_enabled() {
             E::MaxAttestationsElectra::to_usize()
+        } else {
+            E::MaxAttestations::to_usize()
         };
         let prev_epoch_limit = if let BeaconState::Base(base_state) = state {
             std::cmp::min(