From 043fa2153ed0e33c96b2b48606cfd65a92919cc8 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 19 Aug 2022 04:27:22 +0000 Subject: [PATCH] Revise EE peer penalites (#3485) ## Issue Addressed NA ## Proposed Changes Don't penalize peers for errors that might be caused by an honest optimistic node. ## Additional Info NA --- .../beacon_chain/src/block_verification.rs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 4d84fe35e0..95d5f818f0 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -349,13 +349,27 @@ impl ExecutionPayloadError { // always forced to consider here whether or not to penalize a peer when // we add a new error condition. match self { + // The peer has nothing to do with this error, do not penalize them. ExecutionPayloadError::NoExecutionConnection => false, + // The peer has nothing to do with this error, do not penalize them. ExecutionPayloadError::RequestFailed(_) => false, - ExecutionPayloadError::RejectedByExecutionEngine { .. } => true, + // An honest optimistic node may propagate blocks which are rejected by an EE, do not + // penalize them. + ExecutionPayloadError::RejectedByExecutionEngine { .. } => false, + // This is a trivial gossip validation condition, there is no reason for an honest peer + // to propagate a block with an invalid payload time stamp. ExecutionPayloadError::InvalidPayloadTimestamp { .. } => true, - ExecutionPayloadError::InvalidTerminalPoWBlock { .. } => true, - ExecutionPayloadError::InvalidActivationEpoch { .. } => true, - ExecutionPayloadError::InvalidTerminalBlockHash { .. } => true, + // An honest optimistic node may propagate blocks with an invalid terminal PoW block, we + // should not penalized them. + ExecutionPayloadError::InvalidTerminalPoWBlock { .. } => false, + // This condition is checked *after* gossip propagation, therefore penalizing gossip + // peers for this block would be unfair. There may be an argument to penalize RPC + // blocks, since even an optimistic node shouldn't verify this block. We will remove the + // penalties for all block imports to keep things simple. + ExecutionPayloadError::InvalidActivationEpoch { .. } => false, + // As per `Self::InvalidActivationEpoch`. + ExecutionPayloadError::InvalidTerminalBlockHash { .. } => false, + // Do not penalize the peer since it's not their fault that *we're* optimistic. ExecutionPayloadError::UnverifiedNonOptimisticCandidate => false, } }