diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 4c7f314d8..97ce142dd 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -5097,7 +5097,7 @@ impl BeaconChain { latest_valid_hash, ref validation_error, } => { - debug!( + warn!( self.log, "Invalid execution payload"; "validation_error" => ?validation_error, @@ -5106,11 +5106,6 @@ impl BeaconChain { "head_block_root" => ?head_block_root, "method" => "fcU", ); - warn!( - self.log, - "Fork choice update invalidated payload"; - "status" => ?status - ); match latest_valid_hash { // The `latest_valid_hash` is set to `None` when the EE @@ -5156,7 +5151,7 @@ impl BeaconChain { PayloadStatus::InvalidBlockHash { ref validation_error, } => { - debug!( + warn!( self.log, "Invalid execution payload block hash"; "validation_error" => ?validation_error, @@ -5164,11 +5159,6 @@ impl BeaconChain { "head_block_root" => ?head_block_root, "method" => "fcU", ); - warn!( - self.log, - "Fork choice update invalidated payload"; - "status" => ?status - ); // The execution engine has stated that the head block is invalid, however it // hasn't returned a latest valid ancestor. // diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 7d5d35010..8c169cfe5 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -280,10 +280,10 @@ pub enum BlockError { /// /// ## Peer scoring /// - /// TODO(merge): reconsider how we score peers for this. - /// - /// The peer sent us an invalid block, but I'm not really sure how to score this in an - /// "optimistic" sync world. + /// The peer sent us an invalid block, we must penalise harshly. + /// If it's actually our fault (e.g. our execution node database is corrupt) we have bigger + /// problems to worry about than losing peers, and we're doing the network a favour by + /// disconnecting. ParentExecutionPayloadInvalid { parent_root: Hash256 }, } diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 5cc8ee2d2..1ac7229cc 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -159,7 +159,7 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>( latest_valid_hash, ref validation_error, } => { - debug!( + warn!( chain.log, "Invalid execution payload"; "validation_error" => ?validation_error, @@ -206,7 +206,7 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>( PayloadStatus::InvalidBlockHash { ref validation_error, } => { - debug!( + warn!( chain.log, "Invalid execution payload block hash"; "validation_error" => ?validation_error, 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 f2b1b3a26..1ec03ae95 100644 --- a/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/gossip_methods.rs @@ -834,7 +834,6 @@ impl Worker { | Err(e @ BlockError::WeakSubjectivityConflict) | Err(e @ BlockError::InconsistentFork(_)) | Err(e @ BlockError::ExecutionPayloadError(_)) - // TODO(merge): reconsider peer scoring for this event. | Err(e @ BlockError::ParentExecutionPayloadInvalid { .. }) | Err(e @ BlockError::GenesisBlock) => { warn!(self.log, "Could not verify block for gossip. Rejecting the block"; diff --git a/beacon_node/network/src/beacon_processor/worker/sync_methods.rs b/beacon_node/network/src/beacon_processor/worker/sync_methods.rs index 6e6e68155..e8182a1d5 100644 --- a/beacon_node/network/src/beacon_processor/worker/sync_methods.rs +++ b/beacon_node/network/src/beacon_processor/worker/sync_methods.rs @@ -513,6 +513,21 @@ impl Worker { }) } } + ref err @ BlockError::ParentExecutionPayloadInvalid { ref parent_root } => { + warn!( + self.log, + "Failed to sync chain built on invalid parent"; + "parent_root" => ?parent_root, + "advice" => "check execution node for corruption then restart it and Lighthouse", + ); + Err(ChainSegmentFailed { + message: format!("Peer sent invalid block. Reason: {err:?}"), + // We need to penalise harshly in case this represents an actual attack. In case + // of a faulty EL it will usually require manual intervention to fix anyway, so + // it's not too bad if we drop most of our peers. + peer_action: Some(PeerAction::LowToleranceError), + }) + } other => { debug!( self.log, "Invalid block received";