Pass failed gossip blocks to the slasher (#2047)

## Issue Addressed

Closes #2042

## Proposed Changes

Pass blocks that fail gossip verification to the slasher. Blocks that are successfully verified are not passed immediately, but will be passed as part of full block verification.

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -235,7 +235,7 @@ pub struct BeaconChain<T: BeaconChainTypes> {
     /// Arbitrary bytes included in the blocks.
     pub(crate) graffiti: Graffiti,
     /// Optional slasher.
-    pub(crate) slasher: Option<Arc<Slasher<T::EthSpec>>>,
+    pub slasher: Option<Arc<Slasher<T::EthSpec>>>,
 }
 
 type BeaconBlockAndState<T> = (BeaconBlock<T>, BeaconState<T>);

beacon_node/beacon_chain/src/block_verification.rs

@@ -289,6 +289,37 @@ impl<E: EthSpec> BlockSlashInfo<BlockError<E>> {
     }
 }
 
+/// Process invalid blocks to see if they are suitable for the slasher.
+///
+/// If no slasher is configured, this is a no-op.
+fn process_block_slash_info<T: BeaconChainTypes>(
+    chain: &BeaconChain<T>,
+    slash_info: BlockSlashInfo<BlockError<T::EthSpec>>,
+) -> BlockError<T::EthSpec> {
+    if let Some(slasher) = chain.slasher.as_ref() {
+        let (verified_header, error) = match slash_info {
+            BlockSlashInfo::SignatureNotChecked(header, e) => {
+                if verify_header_signature(chain, &header).is_ok() {
+                    (header, e)
+                } else {
+                    return e;
+                }
+            }
+            BlockSlashInfo::SignatureInvalid(e) => return e,
+            BlockSlashInfo::SignatureValid(header, e) => (header, e),
+        };
+
+        slasher.accept_block_header(verified_header);
+        error
+    } else {
+        match slash_info {
+            BlockSlashInfo::SignatureNotChecked(_, e)
+            | BlockSlashInfo::SignatureInvalid(e)
+            | BlockSlashInfo::SignatureValid(_, e) => e,
+        }
+    }
+}
+
 /// Verify all signatures (except deposit signatures) on all blocks in the `chain_segment`. If all
 /// signatures are valid, the `chain_segment` is mapped to a `Vec<SignatureVerifiedBlock>` that can
 /// later be transformed into a `FullyVerifiedBlock` without re-checking the signatures. If any
@@ -403,31 +434,7 @@ pub trait IntoFullyVerifiedBlock<T: BeaconChainTypes>: Sized {
                 }
                 fully_verified
             })
-            .map_err(|slash_info| {
-                // Process invalid blocks to see if they are suitable for the slasher.
-                if let Some(slasher) = chain.slasher.as_ref() {
-                    let (verified_header, error) = match slash_info {
-                        BlockSlashInfo::SignatureNotChecked(header, e) => {
-                            if verify_header_signature(chain, &header).is_ok() {
-                                (header, e)
-                            } else {
-                                return e;
-                            }
-                        }
-                        BlockSlashInfo::SignatureInvalid(e) => return e,
-                        BlockSlashInfo::SignatureValid(header, e) => (header, e),
-                    };
-
-                    slasher.accept_block_header(verified_header);
-                    error
-                } else {
-                    match slash_info {
-                        BlockSlashInfo::SignatureNotChecked(_, e)
-                        | BlockSlashInfo::SignatureInvalid(e)
-                        | BlockSlashInfo::SignatureValid(_, e) => e,
-                    }
-                }
-            })
+            .map_err(|slash_info| process_block_slash_info(chain, slash_info))
     }
 
     /// Convert the block to fully-verified form while producing data to aid checking slashability.
@@ -447,6 +454,21 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
     pub fn new(
         block: SignedBeaconBlock<T::EthSpec>,
         chain: &BeaconChain<T>,
+    ) -> Result<Self, BlockError<T::EthSpec>> {
+        // If the block is valid for gossip we don't supply it to the slasher here because
+        // we assume it will be transformed into a fully verified block. We *do* need to supply
+        // it to the slasher if an error occurs, because that's the end of this block's journey,
+        // and it could be a repeat proposal (a likely cause for slashing!).
+        let header = block.signed_block_header();
+        Self::new_without_slasher_checks(block, chain).map_err(|e| {
+            process_block_slash_info(chain, BlockSlashInfo::from_early_error(header, e))
+        })
+    }
+
+    /// As for new, but doesn't pass the block to the slasher.
+    fn new_without_slasher_checks(
+        block: SignedBeaconBlock<T::EthSpec>,
+        chain: &BeaconChain<T>,
     ) -> Result<Self, BlockError<T::EthSpec>> {
         // Do not gossip or process blocks from future slots.
         let present_slot_with_tolerance = chain

beacon_node/beacon_chain/tests/block_verification.rs

@@ -7,7 +7,10 @@ use beacon_chain::{
     test_utils::{AttestationStrategy, BeaconChainHarness, BlockStrategy, EphemeralHarnessType},
     BeaconSnapshot, BlockError,
 };
+use slasher::{Config as SlasherConfig, Slasher};
+use std::sync::Arc;
 use store::config::StoreConfig;
+use tempfile::tempdir;
 use types::{
     test_utils::generate_deterministic_keypair, AggregateSignature, AttestationData,
     AttesterSlashing, Checkpoint, Deposit, DepositData, Epoch, EthSpec, Hash256,
@@ -794,3 +797,31 @@ fn block_gossip_verification() {
         "the second proposal by this validator should be rejected"
     );
 }
+
+#[test]
+fn verify_block_for_gossip_slashing_detection() {
+    let mut harness = get_harness(VALIDATOR_COUNT);
+
+    let slasher_dir = tempdir().unwrap();
+    let slasher = Arc::new(
+        Slasher::open(
+            SlasherConfig::new(slasher_dir.path().into()),
+            harness.logger().clone(),
+        )
+        .unwrap(),
+    );
+    harness.chain.slasher = Some(slasher.clone());
+
+    let state = harness.get_current_state();
+    let (block1, _) = harness.make_block(state.clone(), Slot::new(1));
+    let (block2, _) = harness.make_block(state, Slot::new(1));
+
+    let verified_block = harness.chain.verify_block_for_gossip(block1).unwrap();
+    harness.chain.process_block(verified_block).unwrap();
+    unwrap_err(harness.chain.verify_block_for_gossip(block2));
+
+    // Slasher should have been handed the two conflicting blocks and crafted a slashing.
+    slasher.process_queued(Epoch::new(0)).unwrap();
+    let proposer_slashings = slasher.get_proposer_slashings();
+    assert_eq!(proposer_slashings.len(), 1);
+}

beacon_node/network/src/service/tests.rs

@@ -77,7 +77,7 @@ mod tests {
         });
 
         let raw_runtime = Arc::try_unwrap(runtime).unwrap();
-        raw_runtime.shutdown_timeout(tokio::time::Duration::from_secs(10));
+        raw_runtime.shutdown_timeout(tokio::time::Duration::from_secs(300));
 
         // Load the persisted dht from the store
         let persisted_enrs = load_dht(store);