Fix handling of cross-fork messages in op pool

consensus/state_processing/Cargo.toml

@@ -14,6 +14,7 @@ bls = { path = "../../crypto/bls" }
 integer-sqrt = "0.1.5"
 itertools = "0.10.0"
 eth2_ssz = "0.4.1"
+eth2_ssz_derive = "0.3.0"
 eth2_ssz_types = "0.2.2"
 merkle_proof = { path = "../merkle_proof" }
 safe_arith = { path = "../safe_arith" }
@@ -26,6 +27,7 @@ smallvec = "1.6.1"
 arbitrary = { version = "1.0", features = ["derive"], optional = true }
 lighthouse_metrics = { path = "../../common/lighthouse_metrics", optional = true }
 lazy_static = { version = "1.4.0", optional = true }
+derivative = "2.1.1"
 
 [features]
 default = ["legacy-arith", "metrics"]

consensus/state_processing/src/verify_operation.rs

@@ -5,36 +5,112 @@ use crate::per_block_processing::{
     verify_attester_slashing, verify_exit, verify_proposer_slashing,
 };
 use crate::VerifySignatures;
+use derivative::Derivative;
+use smallvec::{smallvec, SmallVec};
+use ssz::{Decode, Encode};
+use ssz_derive::{Decode, Encode};
+use std::marker::PhantomData;
 use types::{
-    AttesterSlashing, BeaconState, ChainSpec, EthSpec, ProposerSlashing, SignedVoluntaryExit,
+    AttesterSlashing, BeaconState, ChainSpec, Epoch, EthSpec, Fork, ForkVersion, ProposerSlashing,
+    SignedVoluntaryExit,
 };
 
+const MAX_FORKS_VERIFIED_AGAINST: usize = 2;
+
 /// Wrapper around an operation type that acts as proof that its signature has been checked.
 ///
-/// The inner field is private, meaning instances of this type can only be constructed
+/// The inner `op` field is private, meaning instances of this type can only be constructed
 /// by calling `validate`.
-#[derive(Debug, PartialEq, Eq, Clone)]
-pub struct SigVerifiedOp<T>(T);
+#[derive(Derivative, Debug, Clone, Encode, Decode)]
+#[derivative(
+    PartialEq,
+    Eq,
+    Hash(bound = "T: Encode + Decode + std::hash::Hash, E: EthSpec")
+)]
+pub struct SigVerifiedOp<T: Encode + Decode, E: EthSpec> {
+    op: T,
+    verified_against: VerifiedAgainst,
+    #[ssz(skip_serializing, skip_deserializing)]
+    _phantom: PhantomData<E>,
+}
+
+/// Information about the fork versions that this message was verified against.
+///
+/// In general it is not safe to assume that a `SigVerifiedOp` constructed at some point in the past
+/// will continue to be valid in the presence of a changing `state.fork()`. The reason for this
+/// is that the fork versions that the message's epochs map to might change.
+///
+/// For example a proposer slashing at a phase0 slot verified against an Altair state will use
+/// the phase0 fork version, but will become invalid once the Bellatrix fork occurs because that
+/// slot will start to map to the Altair fork version. This is because `Fork::get_fork_version` only
+/// remembers the most recent two forks.
+///
+/// In the other direction, a proposer slashing at a Bellatrix slot verified against an Altair state
+/// will use the Altair fork version, but will become invalid once the Bellatrix fork occurs because
+/// that slot will start to map to the Bellatrix fork version.
+///
+/// We need to store multiple `ForkVersion`s because attester slashings contain two indexed
+/// attestations which may be signed using different versions.
+#[derive(Debug, PartialEq, Eq, Clone, Hash, Encode, Decode)]
+pub struct VerifiedAgainst {
+    fork_versions: SmallVec<[ForkVersion; MAX_FORKS_VERIFIED_AGAINST]>,
+}
+
+impl<T, E> SigVerifiedOp<T, E>
+where
+    T: VerifyOperation<E>,
+    E: EthSpec,
+{
+    /// This function must be private because it assumes that `op` has already been verified.
+    fn new(op: T, state: &BeaconState<E>) -> Self {
+        let verified_against = VerifiedAgainst {
+            fork_versions: op
+                .verification_epochs()
+                .into_iter()
+                .map(|epoch| state.fork().get_fork_version(epoch))
+                .collect(),
+        };
+
+        SigVerifiedOp {
+            op,
+            verified_against,
+            _phantom: PhantomData,
+        }
+    }
 
-impl<T> SigVerifiedOp<T> {
     pub fn into_inner(self) -> T {
-        self.0
+        self.op
     }
 
     pub fn as_inner(&self) -> &T {
-        &self.0
+        &self.op
+    }
+
+    pub fn signature_is_still_valid(&self, current_fork: &Fork) -> bool {
+        self.as_inner()
+            .verification_epochs()
+            .into_iter()
+            .zip(self.verified_against.fork_versions.iter())
+            .all(|(epoch, verified_fork_version)| {
+                current_fork.get_fork_version(epoch) == *verified_fork_version
+            })
     }
 }
 
 /// Trait for operations that can be verified and transformed into a `SigVerifiedOp`.
-pub trait VerifyOperation<E: EthSpec>: Sized {
+pub trait VerifyOperation<E: EthSpec>: Encode + Decode + Sized {
     type Error;
 
     fn validate(
         self,
         state: &BeaconState<E>,
         spec: &ChainSpec,
-    ) -> Result<SigVerifiedOp<Self>, Self::Error>;
+    ) -> Result<SigVerifiedOp<Self, E>, Self::Error>;
+
+    /// Return the epochs at which parts of this message were verified.
+    ///
+    /// These need to map 1-to-1 to the `SigVerifiedOp::verified_against` for this type.
+    fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]>;
 }
 
 impl<E: EthSpec> VerifyOperation<E> for SignedVoluntaryExit {
@@ -44,9 +120,13 @@ impl<E: EthSpec> VerifyOperation<E> for SignedVoluntaryExit {
         self,
         state: &BeaconState<E>,
         spec: &ChainSpec,
-    ) -> Result<SigVerifiedOp<Self>, Self::Error> {
+    ) -> Result<SigVerifiedOp<Self, E>, Self::Error> {
         verify_exit(state, &self, VerifySignatures::True, spec)?;
-        Ok(SigVerifiedOp(self))
+        Ok(SigVerifiedOp::new(self, state))
+    }
+
+    fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> {
+        smallvec![self.message.epoch]
     }
 }
 
@@ -57,9 +137,16 @@ impl<E: EthSpec> VerifyOperation<E> for AttesterSlashing<E> {
         self,
         state: &BeaconState<E>,
         spec: &ChainSpec,
-    ) -> Result<SigVerifiedOp<Self>, Self::Error> {
+    ) -> Result<SigVerifiedOp<Self, E>, Self::Error> {
         verify_attester_slashing(state, &self, VerifySignatures::True, spec)?;
-        Ok(SigVerifiedOp(self))
+        Ok(SigVerifiedOp::new(self, state))
+    }
+
+    fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> {
+        smallvec![
+            self.attestation_1.data.target.epoch,
+            self.attestation_2.data.target.epoch
+        ]
     }
 }
 
@@ -70,8 +157,17 @@ impl<E: EthSpec> VerifyOperation<E> for ProposerSlashing {
         self,
         state: &BeaconState<E>,
         spec: &ChainSpec,
-    ) -> Result<SigVerifiedOp<Self>, Self::Error> {
+    ) -> Result<SigVerifiedOp<Self, E>, Self::Error> {
         verify_proposer_slashing(&self, state, VerifySignatures::True, spec)?;
-        Ok(SigVerifiedOp(self))
+        Ok(SigVerifiedOp::new(self, state))
+    }
+
+    fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> {
+        // Only need a single epoch because the slots of the two headers must be equal.
+        smallvec![self
+            .signed_header_1
+            .message
+            .slot
+            .epoch(E::slots_per_epoch())]
     }
 }