Create a custom penalize_sync_peer method for clarity

pawanjay176 · pawanjay176 · commit aa6a1bc85088 · 2025-09-24T12:16:05.000-07:00
diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs
@@ -418,7 +418,7 @@ pub enum ExecutionPayloadError {
 }
 
 impl ExecutionPayloadError {
-    pub fn penalize_peer(&self) -> bool {
+    pub fn penalize_gossip_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.
@@ -447,6 +447,39 @@ impl ExecutionPayloadError {
             ExecutionPayloadError::UnverifiedNonOptimisticCandidate => false,
         }
     }
+
+    pub fn penalize_sync_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 {
+            // 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,
+            // For the sync case, we do not want a peer to keep sending us blocks that our
+            // execution engine considers invalid.
+            //
+            // Also, we ask peers for blocks over sync/rpc only when they indicate
+            // that they have fully validated a given block (using their status message).
+            //
+            // Hence, we should penalize for this error in the sync case.
+            ExecutionPayloadError::RejectedByExecutionEngine { .. } => true,
+            // There is no reason for an honest peer to propagate a block with an invalid
+            // payload time stamp.
+            ExecutionPayloadError::InvalidPayloadTimestamp { .. } => true,
+            // We do not want to receive these blocks over rpc even though the gossip
+            // case is still allowed.
+            ExecutionPayloadError::InvalidTerminalPoWBlock { .. } => true,
+            // We should penalize RPC blocks, since even an optimistic node shouldn't
+            // verify this block.
+            ExecutionPayloadError::InvalidActivationEpoch { .. } => true,
+            // As per `Self::InvalidActivationEpoch`.
+            ExecutionPayloadError::InvalidTerminalBlockHash { .. } => true,
+            // Do not penalize the peer since it's not their fault that *we're* optimistic.
+            ExecutionPayloadError::UnverifiedNonOptimisticCandidate => false,
+        }
+    }
 }
 
 impl From<execution_layer::Error> for ExecutionPayloadError {
diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs
@@ -1330,7 +1330,9 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
                 return None;
             }
-            Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => {
+            Err(ref e @ BlockError::ExecutionPayloadError(ref epe))
+                if !epe.penalize_gossip_peer() =>
+            {
                 debug!(error = %e, "Could not verify block for gossip. Ignoring the block");
                 self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
                 return None;
@@ -1562,7 +1564,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                     "Block with unknown parent attempted to be processed"
                 );
             }
-            Err(e @ BlockError::ExecutionPayloadError(epe)) if !epe.penalize_peer() => {
+            Err(e @ BlockError::ExecutionPayloadError(epe)) if !epe.penalize_gossip_peer() => {
                 debug!(
                     error = %e,
                     "Failed to verify execution payload"
diff --git a/beacon_node/network/src/network_beacon_processor/sync_methods.rs b/beacon_node/network/src/network_beacon_processor/sync_methods.rs
@@ -11,8 +11,7 @@ use beacon_chain::data_availability_checker::AvailabilityCheckError;
 use beacon_chain::data_availability_checker::MaybeAvailableBlock;
 use beacon_chain::{
     AvailabilityProcessingStatus, BeaconChainTypes, BlockError, ChainSegmentResult,
-    ExecutionPayloadError, HistoricalBlockError, NotifyExecutionLayer,
-    validator_monitor::get_slot_delay_ms,
+    HistoricalBlockError, NotifyExecutionLayer, validator_monitor::get_slot_delay_ms,
 };
 use beacon_processor::{
     AsyncFn, BlockingFn, DuplicateCache,
@@ -773,7 +772,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 Err(ChainSegmentFailed {
                     message: format!("Block has an unknown parent: {}", parent_root),
                     // Peers are faulty if they send non-sequential blocks.
-                    peer_action: Some(PeerAction::LowToleranceError), // todo(pawan): revise this
+                    peer_action: Some(PeerAction::LowToleranceError),
                     faulty_component: Some(FaultyComponent::Blocks),
                 })
             }
@@ -852,20 +851,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 })
             }
             ref err @ BlockError::ExecutionPayloadError(ref epe) => {
-                if matches!(epe, ExecutionPayloadError::RejectedByExecutionEngine { .. }) {
-                    debug!(
-                        error = ?err,
-                        "Invalid execution payload rejected by EE"
-                    );
-                    Err(ChainSegmentFailed {
-                        message: format!(
-                            "Peer sent a block containing invalid execution payload. Reason: {:?}",
-                            err
-                        ),
-                        peer_action: Some(PeerAction::LowToleranceError),
-                        faulty_component: Some(FaultyComponent::Blocks), // todo(pawan): recheck this
-                    })
-                } else if !epe.penalize_peer() {
+                if !epe.penalize_sync_peer() {
                     // These errors indicate an issue with the EL and not the `ChainSegment`.
                     // Pause the syncing while the EL recovers
                     debug!(
diff --git a/beacon_node/network/src/sync/block_lookups/mod.rs b/beacon_node/network/src/sync/block_lookups/mod.rs
@@ -617,7 +617,9 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
                         request_state.revert_to_awaiting_processing()?;
                         Action::ParentUnknown { parent_root }
                     }
-                    ref e @ BlockError::ExecutionPayloadError(ref epe) if !epe.penalize_peer() => {
+                    ref e @ BlockError::ExecutionPayloadError(ref epe)
+                        if !epe.penalize_sync_peer() =>
+                    {
                         // These errors indicate that the execution layer is offline
                         // and failed to validate the execution payload. Do not downscore peer.
                         debug!(
