From c76afc6630740216aa8f73eecd2e56f9a833719c Mon Sep 17 00:00:00 2001 From: Mac L Date: Tue, 20 Jun 2023 05:20:36 +0000 Subject: [PATCH] Remove legacy `max-skip-slots` checks (#4403) ## Proposed Changes Remove `max-skip-slots` checks when processing blocks. This was legacy code which was previously used in the Medalla testnet to sync to the correct fork. With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity. ## Additional Notes The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection. --- .../beacon_chain/src/block_verification.rs | 35 ------------------- beacon_node/beacon_chain/src/chain_config.rs | 3 +- .../beacon_processor/worker/gossip_methods.rs | 1 - beacon_node/src/cli.rs | 2 +- 4 files changed, 2 insertions(+), 39 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index dba38af9b..3cb8fbdb5 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -141,8 +141,6 @@ pub enum BlockError { /// It's unclear if this block is valid, but it cannot be processed without already knowing /// its parent. ParentUnknown(Arc>), - /// The block skips too many slots and is a DoS risk. - TooManySkippedSlots { parent_slot: Slot, block_slot: Slot }, /// The block slot is greater than the present slot. /// /// ## Peer scoring @@ -786,9 +784,6 @@ impl GossipVerifiedBlock { parent_block.root }; - // Reject any block that exceeds our limit on skipped slots. - check_block_skip_slots(chain, parent_block.slot, block.message())?; - // We assign to a variable instead of using `if let Some` directly to ensure we drop the // write lock before trying to acquire it again in the `else` clause. let proposer_opt = chain @@ -942,9 +937,6 @@ impl SignatureVerifiedBlock { let (mut parent, block) = load_parent(block_root, block, chain)?; - // Reject any block that exceeds our limit on skipped slots. - check_block_skip_slots(chain, parent.beacon_block.slot(), block.message())?; - let state = cheap_state_advance_to_obtain_committees( &mut parent.pre_state, parent.beacon_state_root, @@ -1135,9 +1127,6 @@ impl ExecutionPendingBlock { return Err(BlockError::ParentUnknown(block)); } - // Reject any block that exceeds our limit on skipped slots. - check_block_skip_slots(chain, parent.beacon_block.slot(), block.message())?; - /* * Perform cursory checks to see if the block is even worth processing. */ @@ -1492,30 +1481,6 @@ impl ExecutionPendingBlock { } } -/// Check that the count of skip slots between the block and its parent does not exceed our maximum -/// value. -/// -/// Whilst this is not part of the specification, we include this to help prevent us from DoS -/// attacks. In times of dire network circumstance, the user can configure the -/// `import_max_skip_slots` value. -fn check_block_skip_slots( - chain: &BeaconChain, - parent_slot: Slot, - block: BeaconBlockRef<'_, T::EthSpec>, -) -> Result<(), BlockError> { - // Reject any block that exceeds our limit on skipped slots. - if let Some(max_skip_slots) = chain.config.import_max_skip_slots { - if block.slot() > parent_slot + max_skip_slots { - return Err(BlockError::TooManySkippedSlots { - parent_slot, - block_slot: block.slot(), - }); - } - } - - Ok(()) -} - /// Returns `Ok(())` if the block's slot is greater than the anchor block's slot (if any). fn check_block_against_anchor_slot( block: BeaconBlockRef<'_, T::EthSpec>, diff --git a/beacon_node/beacon_chain/src/chain_config.rs b/beacon_node/beacon_chain/src/chain_config.rs index a74fdced1..34a5c9a4e 100644 --- a/beacon_node/beacon_chain/src/chain_config.rs +++ b/beacon_node/beacon_chain/src/chain_config.rs @@ -17,8 +17,7 @@ pub const FORK_CHOICE_LOOKAHEAD_FACTOR: u32 = 24; #[derive(Debug, PartialEq, Eq, Clone, Deserialize, Serialize)] pub struct ChainConfig { - /// Maximum number of slots to skip when importing a consensus message (e.g., block, - /// attestation, etc). + /// Maximum number of slots to skip when importing an attestation. /// /// If `None`, there is no limit. pub import_max_skip_slots: Option, diff --git a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs index 121a27fec..e3cff0010 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -835,7 +835,6 @@ impl Worker { | Err(e @ BlockError::NonLinearParentRoots) | Err(e @ BlockError::BlockIsNotLaterThanParent { .. }) | Err(e @ BlockError::InvalidSignature) - | Err(e @ BlockError::TooManySkippedSlots { .. }) | Err(e @ BlockError::WeakSubjectivityConflict) | Err(e @ BlockError::InconsistentFork(_)) | Err(e @ BlockError::ExecutionPayloadError(_)) diff --git a/beacon_node/src/cli.rs b/beacon_node/src/cli.rs index e763d93f8..206cd3c72 100644 --- a/beacon_node/src/cli.rs +++ b/beacon_node/src/cli.rs @@ -685,7 +685,7 @@ pub fn cli_app<'a, 'b>() -> App<'a, 'b> { Arg::with_name("max-skip-slots") .long("max-skip-slots") .help( - "Refuse to skip more than this many slots when processing a block or attestation. \ + "Refuse to skip more than this many slots when processing an attestation. \ This prevents nodes on minority forks from wasting our time and disk space, \ but could also cause unnecessary consensus failures, so is disabled by default." )