Electra attestation changes sean review (sigp#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(

consensus/state_processing/src/verify_operation.rs

@@ -48,7 +48,6 @@ pub trait TransformPersist {
 pub struct SigVerifiedOp<T: TransformPersist, E: EthSpec> {
     op: T,
     verified_against: VerifiedAgainst,
-    //#[ssz(skip_serializing, skip_deserializing)]
     _phantom: PhantomData<E>,
 }
 

consensus/types/src/aggregate_and_proof.rs

@@ -33,7 +33,8 @@ use tree_hash_derive::TreeHash;
         derive(Debug, PartialEq, TreeHash, Serialize,),
         serde(untagged, bound = "E: EthSpec"),
         tree_hash(enum_behaviour = "transparent")
-    )
+    ),
+    map_ref_into(AttestationRef)
 )]
 #[derive(
     arbitrary::Arbitrary, Debug, Clone, PartialEq, Serialize, Deserialize, Encode, TreeHash,
@@ -59,19 +60,17 @@ pub struct AggregateAndProof<E: EthSpec> {
 impl<'a, E: EthSpec> AggregateAndProofRef<'a, E> {
     /// Returns `true` if `validator_pubkey` signed over `self.aggregate.data.slot`.
     pub fn aggregate(self) -> AttestationRef<'a, E> {
-        match self {
-            AggregateAndProofRef::Base(a) => AttestationRef::Base(&a.aggregate),
-            AggregateAndProofRef::Electra(a) => AttestationRef::Electra(&a.aggregate),
-        }
+        map_aggregate_and_proof_ref_into_attestation_ref!(&'a _, self, |inner, cons| {
+            cons(&inner.aggregate)
+        })
     }
 }
 impl<E: EthSpec> AggregateAndProof<E> {
     /// Returns `true` if `validator_pubkey` signed over `self.aggregate.data.slot`.
-    pub fn aggregate(&self) -> AttestationRef<E> {
-        match self {
-            AggregateAndProof::Base(a) => AttestationRef::Base(&a.aggregate),
-            AggregateAndProof::Electra(a) => AttestationRef::Electra(&a.aggregate),
-        }
+    pub fn aggregate<'a>(&'a self) -> AttestationRef<'a, E> {
+        map_aggregate_and_proof_ref_into_attestation_ref!(&'a _, self.to_ref(), |inner, cons| {
+            cons(&inner.aggregate)
+        })
     }
 }
 

consensus/types/src/attestation.rs

@@ -1,6 +1,6 @@
 use crate::slot_data::SlotData;
+use crate::Checkpoint;
 use crate::{test_utils::TestRandom, Hash256, Slot};
-use crate::{Checkpoint, ForkName};
 use derivative::Derivative;
 use safe_arith::ArithError;
 use serde::{Deserialize, Serialize};
@@ -111,7 +111,7 @@ impl<E: EthSpec> Attestation<E> {
         target: Checkpoint,
         spec: &ChainSpec,
     ) -> Result<Self, Error> {
-        if spec.fork_name_at_slot::<E>(slot) >= ForkName::Electra {
+        if spec.fork_name_at_slot::<E>(slot).electra_enabled() {
             let mut committee_bits: BitVector<E::MaxCommitteesPerSlot> = BitVector::default();
             committee_bits
                 .set(committee_index as usize, true)
@@ -289,16 +289,6 @@ impl<'a, E: EthSpec> AttestationRef<'a, E> {
 }
 
 impl<E: EthSpec> AttestationElectra<E> {
-    /// Are the aggregation bitfields of these attestations disjoint?
-    // TODO(electra): check whether the definition from CompactIndexedAttestation::should_aggregate
-    // is useful where this is used, i.e. only consider attestations disjoint when their committees
-    // match AND their aggregation bits do not intersect.
-    pub fn signers_disjoint_from(&self, other: &Self) -> bool {
-        self.aggregation_bits
-            .intersection(&other.aggregation_bits)
-            .is_zero()
-    }
-
     pub fn committee_index(&self) -> Option<u64> {
         self.get_committee_indices().first().cloned()
     }
@@ -316,7 +306,6 @@ impl<E: EthSpec> AttestationElectra<E> {
     /// The aggregation bitfields must be disjoint, and the data must be the same.
     pub fn aggregate(&mut self, other: &Self) {
         debug_assert_eq!(self.data, other.data);
-        debug_assert!(self.signers_disjoint_from(other));
         self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits);
         self.signature.add_assign_aggregate(&other.signature);
     }
@@ -370,19 +359,11 @@ impl<E: EthSpec> AttestationElectra<E> {
 }
 
 impl<E: EthSpec> AttestationBase<E> {
-    /// Are the aggregation bitfields of these attestations disjoint?
-    pub fn signers_disjoint_from(&self, other: &Self) -> bool {
-        self.aggregation_bits
-            .intersection(&other.aggregation_bits)
-            .is_zero()
-    }
-
     /// Aggregate another Attestation into this one.
     ///
     /// The aggregation bitfields must be disjoint, and the data must be the same.
     pub fn aggregate(&mut self, other: &Self) {
         debug_assert_eq!(self.data, other.data);
-        debug_assert!(self.signers_disjoint_from(other));
         self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits);
         self.signature.add_assign_aggregate(&other.signature);
     }

consensus/types/src/fork_name.rs

@@ -120,6 +120,10 @@ impl ForkName {
         }
     }
 
+    pub fn deneb_enabled(self) -> bool {
+        self >= ForkName::Deneb
+    }
+
     pub fn electra_enabled(self) -> bool {
         self >= ForkName::Electra
     }

consensus/types/src/signed_aggregate_and_proof.rs

@@ -34,7 +34,9 @@ use tree_hash_derive::TreeHash;
         ),
         serde(bound = "E: EthSpec"),
         arbitrary(bound = "E: EthSpec"),
-    )
+    ),
+    map_into(Attestation),
+    map_ref_into(AggregateAndProofRef)
 )]
 #[derive(
     arbitrary::Arbitrary, Debug, Clone, PartialEq, Serialize, Deserialize, Encode, TreeHash,
@@ -102,19 +104,17 @@ impl<E: EthSpec> SignedAggregateAndProof<E> {
         }
     }
 
-    pub fn message(&self) -> AggregateAndProofRef<E> {
-        match self {
-            SignedAggregateAndProof::Base(message) => AggregateAndProofRef::Base(&message.message),
-            SignedAggregateAndProof::Electra(message) => {
-                AggregateAndProofRef::Electra(&message.message)
-            }
-        }
+    pub fn message<'a>(&'a self) -> AggregateAndProofRef<'a, E> {
+        map_signed_aggregate_and_proof_ref_into_aggregate_and_proof_ref!(
+            &'a _,
+            self.to_ref(),
+            |inner, cons| { cons(&inner.message) }
+        )
     }
 
     pub fn into_attestation(self) -> Attestation<E> {
-        match self {
-            Self::Base(att) => Attestation::Base(att.message.aggregate),
-            Self::Electra(att) => Attestation::Electra(att.message.aggregate),
-        }
+        map_signed_aggregate_and_proof_into_attestation!(self, |inner, cons| {
+            cons(inner.message.aggregate)
+        })
     }
 }