diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 5102381a1a1..f8c221f9e37 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -169,6 +169,7 @@ pub enum BlockError { WouldRevertFinalizedSlot { block_slot: Slot, finalized_slot: Slot, + block_root: Hash256, }, /// The block conflicts with finalization, no need to propagate. /// @@ -412,6 +413,19 @@ impl std::fmt::Display for BlockError { } } +impl BlockError { + /// Return the block root of the block that failed processing if it was already computed. + /// + /// This is used to avoid some double block root computations, which represent a small DoS + /// vector. + pub fn failed_block_root(&self) -> Option { + match self { + Self::WouldRevertFinalizedSlot { block_root, .. } => Some(*block_root), + _ => None, + } + } +} + impl From for BlockError { fn from(e: BlockSignatureVerifierError) -> Self { match e { @@ -708,9 +722,6 @@ impl GossipVerifiedBlock { let block_root = get_block_root(&block); - // Disallow blocks that conflict with the anchor (weak subjectivity checkpoint), if any. - check_block_against_anchor_slot(block.message(), chain)?; - // Do not gossip a block from a finalized slot. check_block_against_finalized_slot(block.message(), block_root, chain)?; @@ -937,8 +948,8 @@ impl SignatureVerifiedBlock { .fork_name(&chain.spec) .map_err(BlockError::InconsistentFork)?; - // Check the anchor slot before loading the parent, to avoid spurious lookups. - check_block_against_anchor_slot(block.message(), chain)?; + // Check the block against the finalized slot to prevent spurious lookups. + check_block_against_finalized_slot(block.message(), block_root, chain)?; let (mut parent, block) = load_parent(block_root, block, chain)?; @@ -1515,19 +1526,6 @@ fn check_block_skip_slots( 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>, - chain: &BeaconChain, -) -> Result<(), BlockError> { - if let Some(anchor_slot) = chain.store.get_anchor_slot() { - if block.slot() <= anchor_slot { - return Err(BlockError::WeakSubjectivityConflict); - } - } - Ok(()) -} - /// Returns `Ok(())` if the block is later than the finalized slot on `chain`. /// /// Returns an error if the block is earlier or equal to the finalized slot, or there was an error @@ -1553,6 +1551,7 @@ fn check_block_against_finalized_slot( Err(BlockError::WouldRevertFinalizedSlot { block_slot: block.slot(), finalized_slot, + block_root, }) } else { Ok(()) 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 1ec03ae954f..cd6e2e2c57c 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -729,10 +729,11 @@ impl Worker { .verify_block_for_gossip(block.clone()) .await; - let block_root = if let Ok(verified_block) = &verification_result { - verified_block.block_root - } else { - block.canonical_root() + let block_root = match &verification_result { + Ok(verified_block) => verified_block.block_root, + Err(e) => e + .failed_block_root() + .unwrap_or_else(|| block.canonical_root()), }; // Write the time the block was observed into delay cache. @@ -799,10 +800,8 @@ impl Worker { return None; } Err(e @ BlockError::FutureSlot { .. }) - | Err(e @ BlockError::WouldRevertFinalizedSlot { .. }) | Err(e @ BlockError::BlockIsAlreadyKnown) - | Err(e @ BlockError::RepeatProposal { .. }) - | Err(e @ BlockError::NotFinalizedDescendant { .. }) => { + | Err(e @ BlockError::RepeatProposal { .. }) => { debug!(self.log, "Could not verify block for gossip. Ignoring the block"; "error" => %e); // Prevent recurring behaviour by penalizing the peer slightly. @@ -820,6 +819,23 @@ impl Worker { self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); return None; } + Err(e @ BlockError::WouldRevertFinalizedSlot { .. }) => { + // Ignore the block (per the spec), but apply a low-tolerance penalty. Peers that + // send us blocks prior to finalization on gossip are either severely faulty or + // malicious. + debug!( + self.log, + "Could not verify block for gossip. Ignoring the block"; + "error" => %e + ); + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); + self.gossip_penalize_peer( + peer_id, + PeerAction::LowToleranceError, + "gossip_block_low", + ); + return None; + } Err(e @ BlockError::StateRootMismatch { .. }) | Err(e @ BlockError::IncorrectBlockProposer { .. }) | Err(e @ BlockError::BlockSlotLimitReached) @@ -835,7 +851,8 @@ impl Worker { | Err(e @ BlockError::InconsistentFork(_)) | Err(e @ BlockError::ExecutionPayloadError(_)) | Err(e @ BlockError::ParentExecutionPayloadInvalid { .. }) - | Err(e @ BlockError::GenesisBlock) => { + | Err(e @ BlockError::GenesisBlock) + | Err(e @ BlockError::NotFinalizedDescendant { .. }) => { warn!(self.log, "Could not verify block for gossip. Rejecting the block"; "error" => %e); self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject);