Fix gossip verification of duplicate attester slashings (#5385)

* Fix gossip verification of duplicate attester slashings

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -413,14 +413,14 @@ pub struct BeaconChain<T: BeaconChainTypes> {
     /// Maintains a record of slashable message seen over the gossip network or RPC.
     pub observed_slashable: RwLock<ObservedSlashable<T::EthSpec>>,
     /// Maintains a record of which validators have submitted voluntary exits.
-    pub(crate) observed_voluntary_exits: Mutex<ObservedOperations<SignedVoluntaryExit, T::EthSpec>>,
+    pub observed_voluntary_exits: Mutex<ObservedOperations<SignedVoluntaryExit, T::EthSpec>>,
     /// Maintains a record of which validators we've seen proposer slashings for.
-    pub(crate) observed_proposer_slashings: Mutex<ObservedOperations<ProposerSlashing, T::EthSpec>>,
+    pub observed_proposer_slashings: Mutex<ObservedOperations<ProposerSlashing, T::EthSpec>>,
     /// Maintains a record of which validators we've seen attester slashings for.
-    pub(crate) observed_attester_slashings:
+    pub observed_attester_slashings:
         Mutex<ObservedOperations<AttesterSlashing<T::EthSpec>, T::EthSpec>>,
     /// Maintains a record of which validators we've seen BLS to execution changes for.
-    pub(crate) observed_bls_to_execution_changes:
+    pub observed_bls_to_execution_changes:
         Mutex<ObservedOperations<SignedBlsToExecutionChange, T::EthSpec>>,
     /// Provides information from the Ethereum 1 (PoW) chain.
     pub eth1_chain: Option<Eth1Chain<T::Eth1Chain, T::EthSpec>>,

beacon_node/beacon_chain/src/observed_operations.rs

@@ -153,6 +153,11 @@ impl<T: ObservableOperation<E>, E: EthSpec> ObservedOperations<T, E> {
             self.current_fork = head_fork;
         }
     }
+
+    /// Reset the cache. MUST ONLY BE USED IN TESTS.
+    pub fn __reset_for_testing_only(&mut self) {
+        self.observed_validator_indices.clear();
+    }
 }
 
 impl<T: ObservableOperation<E> + VerifyOperationAt<E>, E: EthSpec> ObservedOperations<T, E> {

beacon_node/beacon_chain/tests/op_verification.rs

@@ -2,12 +2,18 @@
 
 #![cfg(not(debug_assertions))]
 
-use beacon_chain::observed_operations::ObservationOutcome;
+use beacon_chain::{
-use beacon_chain::test_utils::{
+    observed_operations::ObservationOutcome,
-    test_spec, AttestationStrategy, BeaconChainHarness, BlockStrategy, DiskHarnessType,
+    test_utils::{
+        test_spec, AttestationStrategy, BeaconChainHarness, BlockStrategy, DiskHarnessType,
+    },
+    BeaconChainError,
 };
 use lazy_static::lazy_static;
 use sloggers::{null::NullLoggerBuilder, Build};
+use state_processing::per_block_processing::errors::{
+    AttesterSlashingInvalid, BlockOperationError, ExitInvalid, ProposerSlashingInvalid,
+};
 use std::sync::Arc;
 use store::{LevelDB, StoreConfig};
 use tempfile::{tempdir, TempDir};
@@ -119,6 +125,75 @@ async fn voluntary_exit() {
     ));
 }
 
+#[tokio::test]
+async fn voluntary_exit_duplicate_in_state() {
+    let db_path = tempdir().unwrap();
+    let store = get_store(&db_path);
+    let harness = get_harness(store.clone(), VALIDATOR_COUNT);
+    let spec = &harness.chain.spec;
+
+    harness
+        .extend_chain(
+            (E::slots_per_epoch() * (spec.shard_committee_period + 1)) as usize,
+            BlockStrategy::OnCanonicalHead,
+            AttestationStrategy::AllValidators,
+        )
+        .await;
+    harness.advance_slot();
+
+    // Exit a validator.
+    let exited_validator = 0;
+    let exit =
+        harness.make_voluntary_exit(exited_validator, Epoch::new(spec.shard_committee_period));
+    let ObservationOutcome::New(verified_exit) = harness
+        .chain
+        .verify_voluntary_exit_for_gossip(exit.clone())
+        .unwrap()
+    else {
+        panic!("exit should verify");
+    };
+    harness.chain.import_voluntary_exit(verified_exit);
+
+    // Make a new block to include the exit.
+    harness
+        .extend_chain(
+            1,
+            BlockStrategy::OnCanonicalHead,
+            AttestationStrategy::AllValidators,
+        )
+        .await;
+
+    // Verify validator is actually exited.
+    assert_ne!(
+        harness
+            .get_current_state()
+            .validators()
+            .get(exited_validator as usize)
+            .unwrap()
+            .exit_epoch,
+        spec.far_future_epoch
+    );
+
+    // Clear the in-memory gossip cache & try to verify the same exit on gossip.
+    // It should still fail because gossip verification should check the validator's `exit_epoch`
+    // field in the head state.
+    harness
+        .chain
+        .observed_voluntary_exits
+        .lock()
+        .__reset_for_testing_only();
+
+    assert!(matches!(
+        harness
+            .chain
+            .verify_voluntary_exit_for_gossip(exit)
+            .unwrap_err(),
+        BeaconChainError::ExitValidationError(BlockOperationError::Invalid(
+            ExitInvalid::AlreadyExited(index)
+        )) if index == exited_validator
+    ));
+}
+
 #[test]
 fn proposer_slashing() {
     let db_path = tempdir().unwrap();
@@ -171,6 +246,63 @@ fn proposer_slashing() {
     ));
 }
 
+#[tokio::test]
+async fn proposer_slashing_duplicate_in_state() {
+    let db_path = tempdir().unwrap();
+    let store = get_store(&db_path);
+    let harness = get_harness(store.clone(), VALIDATOR_COUNT);
+
+    // Slash a validator.
+    let slashed_validator = 0;
+    let slashing = harness.make_proposer_slashing(slashed_validator);
+    let ObservationOutcome::New(verified_slashing) = harness
+        .chain
+        .verify_proposer_slashing_for_gossip(slashing.clone())
+        .unwrap()
+    else {
+        panic!("slashing should verify");
+    };
+    harness.chain.import_proposer_slashing(verified_slashing);
+
+    // Make a new block to include the slashing.
+    harness
+        .extend_chain(
+            1,
+            BlockStrategy::OnCanonicalHead,
+            AttestationStrategy::AllValidators,
+        )
+        .await;
+
+    // Verify validator is actually slashed.
+    assert!(
+        harness
+            .get_current_state()
+            .validators()
+            .get(slashed_validator as usize)
+            .unwrap()
+            .slashed
+    );
+
+    // Clear the in-memory gossip cache & try to verify the same slashing on gossip.
+    // It should still fail because gossip verification should check the validator's `slashed` field
+    // in the head state.
+    harness
+        .chain
+        .observed_proposer_slashings
+        .lock()
+        .__reset_for_testing_only();
+
+    assert!(matches!(
+        harness
+            .chain
+            .verify_proposer_slashing_for_gossip(slashing)
+            .unwrap_err(),
+        BeaconChainError::ProposerSlashingValidationError(BlockOperationError::Invalid(
+            ProposerSlashingInvalid::ProposerNotSlashable(index)
+        )) if index == slashed_validator
+    ));
+}
+
 #[test]
 fn attester_slashing() {
     let db_path = tempdir().unwrap();
@@ -241,3 +373,60 @@ fn attester_slashing() {
         ObservationOutcome::AlreadyKnown
     ));
 }
+
+#[tokio::test]
+async fn attester_slashing_duplicate_in_state() {
+    let db_path = tempdir().unwrap();
+    let store = get_store(&db_path);
+    let harness = get_harness(store.clone(), VALIDATOR_COUNT);
+
+    // Slash a validator.
+    let slashed_validator = 0;
+    let slashing = harness.make_attester_slashing(vec![slashed_validator]);
+    let ObservationOutcome::New(verified_slashing) = harness
+        .chain
+        .verify_attester_slashing_for_gossip(slashing.clone())
+        .unwrap()
+    else {
+        panic!("slashing should verify");
+    };
+    harness.chain.import_attester_slashing(verified_slashing);
+
+    // Make a new block to include the slashing.
+    harness
+        .extend_chain(
+            1,
+            BlockStrategy::OnCanonicalHead,
+            AttestationStrategy::AllValidators,
+        )
+        .await;
+
+    // Verify validator is actually slashed.
+    assert!(
+        harness
+            .get_current_state()
+            .validators()
+            .get(slashed_validator as usize)
+            .unwrap()
+            .slashed
+    );
+
+    // Clear the in-memory gossip cache & try to verify the same slashing on gossip.
+    // It should still fail because gossip verification should check the validator's `slashed` field
+    // in the head state.
+    harness
+        .chain
+        .observed_attester_slashings
+        .lock()
+        .__reset_for_testing_only();
+
+    assert!(matches!(
+        harness
+            .chain
+            .verify_attester_slashing_for_gossip(slashing)
+            .unwrap_err(),
+        BeaconChainError::AttesterSlashingValidationError(BlockOperationError::Invalid(
+            AttesterSlashingInvalid::NoSlashableIndices
+        ))
+    ));
+}

consensus/state_processing/src/per_block_processing/process_operations.rs

@@ -231,11 +231,9 @@ pub fn process_attester_slashings<T: EthSpec>(
     spec: &ChainSpec,
 ) -> Result<(), BlockProcessingError> {
     for (i, attester_slashing) in attester_slashings.iter().enumerate() {
-        verify_attester_slashing(state, attester_slashing, verify_signatures, spec)
-            .map_err(|e| e.into_with_index(i))?;
-
         let slashable_indices =
-            get_slashable_indices(state, attester_slashing).map_err(|e| e.into_with_index(i))?;
+            verify_attester_slashing(state, attester_slashing, verify_signatures, spec)
+                .map_err(|e| e.into_with_index(i))?;
 
         for i in slashable_indices {
             slash_validator(state, i as usize, None, ctxt, spec)?;

consensus/state_processing/src/per_block_processing/verify_attester_slashing.rs

@@ -13,16 +13,15 @@ fn error(reason: Invalid) -> BlockOperationError<Invalid> {
 /// Indicates if an `AttesterSlashing` is valid to be included in a block in the current epoch of
 /// the given state.
 ///
-/// Returns `Ok(())` if the `AttesterSlashing` is valid, otherwise indicates the reason for
+/// Returns `Ok(indices)` with `indices` being a non-empty vec of validator indices in ascending
+/// order if the `AttesterSlashing` is valid. Otherwise returns `Err(e)` with the reason for
 /// invalidity.
-///
-/// Spec v0.12.1
 pub fn verify_attester_slashing<T: EthSpec>(
     state: &BeaconState<T>,
     attester_slashing: &AttesterSlashing<T>,
     verify_signatures: VerifySignatures,
     spec: &ChainSpec,
-) -> Result<()> {
+) -> Result<Vec<u64>> {
     let attestation_1 = &attester_slashing.attestation_1;
     let attestation_2 = &attester_slashing.attestation_2;
 
@@ -38,14 +37,12 @@ pub fn verify_attester_slashing<T: EthSpec>(
     is_valid_indexed_attestation(state, attestation_2, verify_signatures, spec)
         .map_err(|e| error(Invalid::IndexedAttestation2Invalid(e)))?;
 
-    Ok(())
+    get_slashable_indices(state, attester_slashing)
 }
 
 /// For a given attester slashing, return the indices able to be slashed in ascending order.
 ///
-/// Returns Ok(indices) if `indices.len() > 0`.
+/// Returns Ok(indices) if `indices.len() > 0`
-///
-/// Spec v0.12.1
 pub fn get_slashable_indices<T: EthSpec>(
     state: &BeaconState<T>,
     attester_slashing: &AttesterSlashing<T>,