From c1ec386d183af33ef3d3c85e6dd4749660195888 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Fri, 4 Dec 2020 05:03:30 +0000 Subject: [PATCH] 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 | 2 +- .../beacon_chain/src/block_verification.rs | 72 ++++++++++++------- .../beacon_chain/tests/block_verification.rs | 31 ++++++++ beacon_node/network/src/service/tests.rs | 2 +- 4 files changed, 80 insertions(+), 27 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 7cd90ad4ee..51f0c2ea47 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -235,7 +235,7 @@ pub struct BeaconChain { /// Arbitrary bytes included in the blocks. pub(crate) graffiti: Graffiti, /// Optional slasher. - pub(crate) slasher: Option>>, + pub slasher: Option>>, } type BeaconBlockAndState = (BeaconBlock, BeaconState); diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 71d34b626c..21d8ddc541 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -289,6 +289,37 @@ impl BlockSlashInfo> { } } +/// 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( + chain: &BeaconChain, + slash_info: BlockSlashInfo>, +) -> BlockError { + 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` that can /// later be transformed into a `FullyVerifiedBlock` without re-checking the signatures. If any @@ -403,31 +434,7 @@ pub trait IntoFullyVerifiedBlock: 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 GossipVerifiedBlock { pub fn new( block: SignedBeaconBlock, chain: &BeaconChain, + ) -> Result> { + // 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, + chain: &BeaconChain, ) -> Result> { // Do not gossip or process blocks from future slots. let present_slot_with_tolerance = chain diff --git a/beacon_node/beacon_chain/tests/block_verification.rs b/beacon_node/beacon_chain/tests/block_verification.rs index 47d7f53cf9..1bd12b6e30 100644 --- a/beacon_node/beacon_chain/tests/block_verification.rs +++ b/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); +} diff --git a/beacon_node/network/src/service/tests.rs b/beacon_node/network/src/service/tests.rs index 43653f5f2d..b9b9e3e3af 100644 --- a/beacon_node/network/src/service/tests.rs +++ b/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);