From 7c3ff903ca921d7b61945eb3007e7f347c9f8272 Mon Sep 17 00:00:00 2001 From: ethDreamer Date: Wed, 20 Jul 2022 20:59:38 +0000 Subject: [PATCH] Fix Gossip Penalties During Optimistic Sync Window (#3350) ## Issue Addressed * #3344 ## Proposed Changes There are a number of cases during block processing where we might get an `ExecutionPayloadError` but we shouldn't penalize peers. We were forgetting to enumerate all of the non-penalizing errors in every single match statement where we are making that decision. I created a function to make it explicit when we should and should not penalize peers and I used that function in all places where this logic is needed. This way we won't make the same mistake if we add another variant of `ExecutionPayloadError` in the future. --- .../beacon_chain/src/block_verification.rs | 23 +++++++++++++++---- .../beacon_processor/worker/gossip_methods.rs | 12 +++------- .../network/src/sync/block_lookups/mod.rs | 20 +++++----------- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index a64fb387e3..c8341cd60b 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -335,17 +335,32 @@ pub enum ExecutionPayloadError { terminal_block_hash: ExecutionBlockHash, payload_parent_hash: ExecutionBlockHash, }, - /// The execution node failed to provide a parent block to a known block. This indicates an - /// issue with the execution node. + /// The execution node is syncing but we fail the conditions for optimistic sync /// /// ## Peer scoring /// /// The peer is not necessarily invalid. - PoWParentMissing(ExecutionBlockHash), - /// The execution node is syncing but we fail the conditions for optimistic sync UnverifiedNonOptimisticCandidate, } +impl ExecutionPayloadError { + pub fn penalize_peer(&self) -> bool { + // This match statement should never have a default case so that we are + // always forced to consider here whether or not to penalize a peer when + // we add a new error condition. + match self { + ExecutionPayloadError::NoExecutionConnection => false, + ExecutionPayloadError::RequestFailed(_) => false, + ExecutionPayloadError::RejectedByExecutionEngine { .. } => true, + ExecutionPayloadError::InvalidPayloadTimestamp { .. } => true, + ExecutionPayloadError::InvalidTerminalPoWBlock { .. } => true, + ExecutionPayloadError::InvalidActivationEpoch { .. } => true, + ExecutionPayloadError::InvalidTerminalBlockHash { .. } => true, + ExecutionPayloadError::UnverifiedNonOptimisticCandidate => false, + } + } +} + impl From for ExecutionPayloadError { fn from(e: execution_layer::Error) -> Self { ExecutionPayloadError::RequestFailed(e) 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 2dc02a31b3..b88b58b8bf 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -6,8 +6,7 @@ use beacon_chain::{ observed_operations::ObservationOutcome, sync_committee_verification::{self, Error as SyncCommitteeError}, validator_monitor::get_block_delay_ms, - BeaconChainError, BeaconChainTypes, BlockError, ExecutionPayloadError, ForkChoiceError, - GossipVerifiedBlock, + BeaconChainError, BeaconChainTypes, BlockError, ForkChoiceError, GossipVerifiedBlock, }; use lighthouse_network::{Client, MessageAcceptance, MessageId, PeerAction, PeerId, ReportSource}; use slog::{crit, debug, error, info, trace, warn}; @@ -776,9 +775,7 @@ impl Worker { return None; } // TODO(merge): reconsider peer scoring for this event. - Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_))) - | Err(e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::UnverifiedNonOptimisticCandidate)) - | Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection)) => { + Err(ref e @BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => { debug!(self.log, "Could not verify block for gossip, ignoring the block"; "error" => %e); self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore); @@ -951,10 +948,7 @@ impl Worker { ); self.send_sync_message(SyncMessage::UnknownBlock(peer_id, block)); } - Err(e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_))) - | Err( - e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection), - ) => { + Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => { debug!( self.log, "Failed to verify execution payload"; diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs index 49e1eb290f..2aa4acdb5a 100644 --- a/beacon_node/network/src/sync/block_lookups/mod.rs +++ b/beacon_node/network/src/sync/block_lookups/mod.rs @@ -1,7 +1,7 @@ use std::collections::hash_map::Entry; use std::time::Duration; -use beacon_chain::{BeaconChainTypes, BlockError, ExecutionPayloadError}; +use beacon_chain::{BeaconChainTypes, BlockError}; use fnv::FnvHashMap; use lighthouse_network::{PeerAction, PeerId}; use lru_cache::LRUTimeCache; @@ -435,17 +435,12 @@ impl BlockLookups { BlockError::ParentUnknown(block) => { self.search_parent(block, peer_id, cx); } - e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed( - _, - )) - | e @ BlockError::ExecutionPayloadError( - ExecutionPayloadError::NoExecutionConnection, - ) => { + ref e @ BlockError::ExecutionPayloadError(ref epe) if !epe.penalize_peer() => { // These errors indicate that the execution layer is offline // and failed to validate the execution payload. Do not downscore peer. debug!( self.log, - "Single block lookup failed. Execution layer is offline"; + "Single block lookup failed. Execution layer is offline / unsynced / misconfigured"; "root" => %root, "error" => ?e ); @@ -549,12 +544,9 @@ impl BlockLookups { } } } - BlockProcessResult::Err( - e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_)), - ) - | BlockProcessResult::Err( - e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection), - ) => { + ref e @ BlockProcessResult::Err(BlockError::ExecutionPayloadError(ref epe)) + if !epe.penalize_peer() => + { // These errors indicate that the execution layer is offline // and failed to validate the execution payload. Do not downscore peer. debug!(