Electra attestation changes rm decode impl (sigp#5856)

* Remove Crappy Decode impl for Attestation * Remove Inefficient Attestation Decode impl * Implement Schema Upgrade / Downgrade * Update beacon_node/beacon_chain/src/schema_change/migration_schema_v20.rs Co-authored-by: Michael Sproul <[email protected]> --------- Co-authored-by: Michael Sproul <[email protected]>
realbigsean · Jun 25, 2024 · 9ac59ac · 9ac59ac
1 parent d321a3d
commit 9ac59ac
Show file tree

Hide file tree

Showing 5 changed files with 147 additions and 59 deletions.
diff --git a/beacon_node/lighthouse_network/src/types/pubsub.rs b/beacon_node/lighthouse_network/src/types/pubsub.rs
@@ -7,13 +7,14 @@ use ssz::{Decode, Encode};
 use std::io::{Error, ErrorKind};
 use std::sync::Arc;
 use types::{
-    Attestation, AttesterSlashing, AttesterSlashingBase, AttesterSlashingElectra, BlobSidecar,
-    EthSpec, ForkContext, ForkName, LightClientFinalityUpdate, LightClientOptimisticUpdate,
-    ProposerSlashing, SignedAggregateAndProof, SignedAggregateAndProofBase,
-    SignedAggregateAndProofElectra, SignedBeaconBlock, SignedBeaconBlockAltair,
-    SignedBeaconBlockBase, SignedBeaconBlockBellatrix, SignedBeaconBlockCapella,
-    SignedBeaconBlockDeneb, SignedBeaconBlockElectra, SignedBlsToExecutionChange,
-    SignedContributionAndProof, SignedVoluntaryExit, SubnetId, SyncCommitteeMessage, SyncSubnetId,
+    Attestation, AttestationBase, AttestationElectra, AttesterSlashing, AttesterSlashingBase,
+    AttesterSlashingElectra, BlobSidecar, EthSpec, ForkContext, ForkName,
+    LightClientFinalityUpdate, LightClientOptimisticUpdate, ProposerSlashing,
+    SignedAggregateAndProof, SignedAggregateAndProofBase, SignedAggregateAndProofElectra,
+    SignedBeaconBlock, SignedBeaconBlockAltair, SignedBeaconBlockBase, SignedBeaconBlockBellatrix,
+    SignedBeaconBlockCapella, SignedBeaconBlockDeneb, SignedBeaconBlockElectra,
+    SignedBlsToExecutionChange, SignedContributionAndProof, SignedVoluntaryExit, SubnetId,
+    SyncCommitteeMessage, SyncSubnetId,
 };
 
 #[derive(Debug, Clone, PartialEq)]
@@ -183,19 +184,18 @@ impl<E: EthSpec> PubsubMessage<E> {
                     GossipKind::Attestation(subnet_id) => {
                         let attestation =
                             match fork_context.from_context_bytes(gossip_topic.fork_digest) {
-                                Some(&fork_name) => {
-                                    if fork_name.electra_enabled() {
-                                        Attestation::Electra(
-                                            AttestationElectra::from_ssz_bytes(data)
-                                                .map_err(|e| format!("{:?}", e))?,
-                                        )
-                                    } else {
-                                        Attestation::Base(
-                                            AttestationBase::from_ssz_bytes(data)
-                                                .map_err(|e| format!("{:?}", e))?,
-                                        )
-                                    }
-                                }
+                                Some(ForkName::Base)
+                                | Some(ForkName::Altair)
+                                | Some(ForkName::Bellatrix)
+                                | Some(ForkName::Capella)
+                                | Some(ForkName::Deneb) => Attestation::Base(
+                                    AttestationBase::from_ssz_bytes(data)
+                                        .map_err(|e| format!("{:?}", e))?,
+                                ),
+                                Some(ForkName::Electra) => Attestation::Electra(
+                                    AttestationElectra::from_ssz_bytes(data)
+                                        .map_err(|e| format!("{:?}", e))?,
+                                ),
                                 None => {
                                     return Err(format!(
                                         "Unknown gossipsub fork digest: {:?}",

diff --git a/beacon_node/operation_pool/src/persistence.rs b/beacon_node/operation_pool/src/persistence.rs
@@ -38,7 +38,8 @@ pub struct PersistedOperationPool<E: EthSpec> {
     pub attestations: Vec<(AttestationOnDisk<E>, Vec<u64>)>,
     /// Mapping from sync contribution ID to sync contributions and aggregate.
     pub sync_contributions: PersistedSyncContributions<E>,
-    /// TODO(electra): we've made a DB change here!!!
+    #[superstruct(only(V15))]
+    pub attester_slashings_v15: Vec<SigVerifiedOp<AttesterSlashingBase<E>, E>>,
     /// Attester slashings.
     #[superstruct(only(V20))]
     pub attester_slashings: Vec<SigVerifiedOp<AttesterSlashing<E>, E>>,
@@ -62,7 +63,7 @@ impl<E: EthSpec> PersistedOperationPool<E> {
             .iter()
             .map(|att| {
                 (
-                    att.clone_as_attestation(),
+                    AttestationOnDisk::from(att.clone_as_attestation()),
                     att.indexed.attesting_indices().clone(),
                 )
             })

diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs
@@ -12,7 +12,9 @@ use smallvec::{smallvec, SmallVec};
 use ssz::{Decode, Encode};
 use ssz_derive::{Decode, Encode};
 use std::marker::PhantomData;
-use types::{AttesterSlashing, AttesterSlashingOnDisk, AttesterSlashingRefOnDisk};
+use types::{
+    AttesterSlashing, AttesterSlashingBase, AttesterSlashingOnDisk, AttesterSlashingRefOnDisk,
+};
 use types::{
     BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, ProposerSlashing,
     SignedBlsToExecutionChange, SignedVoluntaryExit,
@@ -366,6 +368,49 @@ impl<E: EthSpec> TransformPersist for AttesterSlashing<E> {
     }
 }
 
+// TODO: Remove this once we no longer support DB schema version 17
+impl<E: EthSpec> TransformPersist for types::AttesterSlashingBase<E> {
+    type Persistable = Self;
+    type PersistableRef<'a> = &'a Self;
+
+    fn as_persistable_ref(&self) -> Self::PersistableRef<'_> {
+        self
+    }
+
+    fn from_persistable(persistable: Self::Persistable) -> Self {
+        persistable
+    }
+}
+// TODO: Remove this once we no longer support DB schema version 17
+impl<E: EthSpec> From<SigVerifiedOp<AttesterSlashingBase<E>, E>>
+    for SigVerifiedOp<AttesterSlashing<E>, E>
+{
+    fn from(base: SigVerifiedOp<AttesterSlashingBase<E>, E>) -> Self {
+        SigVerifiedOp {
+            op: AttesterSlashing::Base(base.op),
+            verified_against: base.verified_against,
+            _phantom: PhantomData,
+        }
+    }
+}
+// TODO: Remove this once we no longer support DB schema version 17
+impl<E: EthSpec> TryFrom<SigVerifiedOp<AttesterSlashing<E>, E>>
+    for SigVerifiedOp<AttesterSlashingBase<E>, E>
+{
+    type Error = String;
+
+    fn try_from(slashing: SigVerifiedOp<AttesterSlashing<E>, E>) -> Result<Self, Self::Error> {
+        match slashing.op {
+            AttesterSlashing::Base(base) => Ok(SigVerifiedOp {
+                op: base,
+                verified_against: slashing.verified_against,
+                _phantom: PhantomData,
+            }),
+            AttesterSlashing::Electra(_) => Err("non-base attester slashing".to_string()),
+        }
+    }
+}
+
 impl TransformPersist for ProposerSlashing {
     type Persistable = Self;
     type PersistableRef<'a> = &'a Self;

diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs
@@ -4,7 +4,6 @@ use derivative::Derivative;
 use rand::RngCore;
 use safe_arith::ArithError;
 use serde::{Deserialize, Serialize};
-use ssz::Decode;
 use ssz_derive::{Decode, Encode};
 use ssz_types::BitVector;
 use std::hash::{Hash, Hasher};
@@ -75,26 +74,7 @@ pub struct Attestation<E: EthSpec> {
     pub committee_bits: BitVector<E::MaxCommitteesPerSlot>,
 }
 
-impl<E: EthSpec> Decode for Attestation<E> {
-    fn is_ssz_fixed_len() -> bool {
-        false
-    }
-
-    fn from_ssz_bytes(bytes: &[u8]) -> Result<Self, ssz::DecodeError> {
-        if let Ok(result) = AttestationBase::from_ssz_bytes(bytes) {
-            return Ok(Attestation::Base(result));
-        }
-
-        if let Ok(result) = AttestationElectra::from_ssz_bytes(bytes) {
-            return Ok(Attestation::Electra(result));
-        }
-
-        Err(ssz::DecodeError::BytesInvalid(String::from(
-            "bytes not valid for any fork variant",
-        )))
-    }
-}
-
+// TODO(electra): think about how to handle fork variants here
 impl<E: EthSpec> TestRandom for Attestation<E> {
     fn random_for_test(rng: &mut impl RngCore) -> Self {
         let aggregation_bits: BitList<E::MaxValidatorsPerCommittee> = BitList::random_for_test(rng);
@@ -501,6 +481,65 @@ impl<'a, E: EthSpec> SlotData for AttestationRef<'a, E> {
     }
 }
 
+#[derive(Debug, Clone, Encode, Decode, PartialEq)]
+#[ssz(enum_behaviour = "union")]
+pub enum AttestationOnDisk<E: EthSpec> {
+    Base(AttestationBase<E>),
+    Electra(AttestationElectra<E>),
+}
+
+impl<E: EthSpec> AttestationOnDisk<E> {
+    pub fn to_ref(&self) -> AttestationRefOnDisk<E> {
+        match self {
+            AttestationOnDisk::Base(att) => AttestationRefOnDisk::Base(att),
+            AttestationOnDisk::Electra(att) => AttestationRefOnDisk::Electra(att),
+        }
+    }
+}
+
+#[derive(Debug, Clone, Encode)]
+#[ssz(enum_behaviour = "union")]
+pub enum AttestationRefOnDisk<'a, E: EthSpec> {
+    Base(&'a AttestationBase<E>),
+    Electra(&'a AttestationElectra<E>),
+}
+
+impl<E: EthSpec> From<Attestation<E>> for AttestationOnDisk<E> {
+    fn from(attestation: Attestation<E>) -> Self {
+        match attestation {
+            Attestation::Base(attestation) => Self::Base(attestation),
+            Attestation::Electra(attestation) => Self::Electra(attestation),
+        }
+    }
+}
+
+impl<E: EthSpec> From<AttestationOnDisk<E>> for Attestation<E> {
+    fn from(attestation: AttestationOnDisk<E>) -> Self {
+        match attestation {
+            AttestationOnDisk::Base(attestation) => Self::Base(attestation),
+            AttestationOnDisk::Electra(attestation) => Self::Electra(attestation),
+        }
+    }
+}
+
+impl<'a, E: EthSpec> From<AttestationRef<'a, E>> for AttestationRefOnDisk<'a, E> {
+    fn from(attestation: AttestationRef<'a, E>) -> Self {
+        match attestation {
+            AttestationRef::Base(attestation) => Self::Base(attestation),
+            AttestationRef::Electra(attestation) => Self::Electra(attestation),
+        }
+    }
+}
+
+impl<'a, E: EthSpec> From<AttestationRefOnDisk<'a, E>> for AttestationRef<'a, E> {
+    fn from(attestation: AttestationRefOnDisk<'a, E>) -> Self {
+        match attestation {
+            AttestationRefOnDisk::Base(attestation) => Self::Base(attestation),
+            AttestationRefOnDisk::Electra(attestation) => Self::Electra(attestation),
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs
@@ -180,21 +180,24 @@ impl<E: EthSpec> LoadCase for ForkChoiceTest<E> {
                         valid,
                     })
                 }
-                Step::Attestation { attestation } => {
-                    if fork_name.electra_enabled() {
-                        ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map(
-                            |attestation| Step::Attestation {
-                                attestation: Attestation::Electra(attestation),
-                            },
-                        )
-                    } else {
-                        ssz_decode_file(&path.join(format!("{}.ssz_snappy", attestation))).map(
-                            |attestation| Step::Attestation {
-                                attestation: Attestation::Base(attestation),
-                            },
-                        )
-                    }
-                }
+                Step::Attestation { attestation } => match fork_name {
+                    ForkName::Base
+                    | ForkName::Altair
+                    | ForkName::Bellatrix
+                    | ForkName::Capella
+                    | ForkName::Deneb => ssz_decode_file(
+                        &path.join(format!("{}.ssz_snappy", attestation)),
+                    )
+                    .map(|attestation| Step::Attestation {
+                        attestation: Attestation::Base(attestation),
+                    }),
+                    ForkName::Electra => ssz_decode_file(
+                        &path.join(format!("{}.ssz_snappy", attestation)),
+                    )
+                    .map(|attestation| Step::Attestation {
+                        attestation: Attestation::Electra(attestation),
+                    }),
+                },
                 Step::AttesterSlashing { attester_slashing } => match fork_name {
                     ForkName::Base
                     | ForkName::Altair