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.
This commit is contained in:
parent
6d8dfc9eee
commit
7c3ff903ca
@ -335,17 +335,32 @@ pub enum ExecutionPayloadError {
|
|||||||
terminal_block_hash: ExecutionBlockHash,
|
terminal_block_hash: ExecutionBlockHash,
|
||||||
payload_parent_hash: ExecutionBlockHash,
|
payload_parent_hash: ExecutionBlockHash,
|
||||||
},
|
},
|
||||||
/// The execution node failed to provide a parent block to a known block. This indicates an
|
/// The execution node is syncing but we fail the conditions for optimistic sync
|
||||||
/// issue with the execution node.
|
|
||||||
///
|
///
|
||||||
/// ## Peer scoring
|
/// ## Peer scoring
|
||||||
///
|
///
|
||||||
/// The peer is not necessarily invalid.
|
/// The peer is not necessarily invalid.
|
||||||
PoWParentMissing(ExecutionBlockHash),
|
|
||||||
/// The execution node is syncing but we fail the conditions for optimistic sync
|
|
||||||
UnverifiedNonOptimisticCandidate,
|
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<execution_layer::Error> for ExecutionPayloadError {
|
impl From<execution_layer::Error> for ExecutionPayloadError {
|
||||||
fn from(e: execution_layer::Error) -> Self {
|
fn from(e: execution_layer::Error) -> Self {
|
||||||
ExecutionPayloadError::RequestFailed(e)
|
ExecutionPayloadError::RequestFailed(e)
|
||||||
|
@ -6,8 +6,7 @@ use beacon_chain::{
|
|||||||
observed_operations::ObservationOutcome,
|
observed_operations::ObservationOutcome,
|
||||||
sync_committee_verification::{self, Error as SyncCommitteeError},
|
sync_committee_verification::{self, Error as SyncCommitteeError},
|
||||||
validator_monitor::get_block_delay_ms,
|
validator_monitor::get_block_delay_ms,
|
||||||
BeaconChainError, BeaconChainTypes, BlockError, ExecutionPayloadError, ForkChoiceError,
|
BeaconChainError, BeaconChainTypes, BlockError, ForkChoiceError, GossipVerifiedBlock,
|
||||||
GossipVerifiedBlock,
|
|
||||||
};
|
};
|
||||||
use lighthouse_network::{Client, MessageAcceptance, MessageId, PeerAction, PeerId, ReportSource};
|
use lighthouse_network::{Client, MessageAcceptance, MessageId, PeerAction, PeerId, ReportSource};
|
||||||
use slog::{crit, debug, error, info, trace, warn};
|
use slog::{crit, debug, error, info, trace, warn};
|
||||||
@ -776,9 +775,7 @@ impl<T: BeaconChainTypes> Worker<T> {
|
|||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
// TODO(merge): reconsider peer scoring for this event.
|
// TODO(merge): reconsider peer scoring for this event.
|
||||||
Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_)))
|
Err(ref e @BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => {
|
||||||
| Err(e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::UnverifiedNonOptimisticCandidate))
|
|
||||||
| Err(e @BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection)) => {
|
|
||||||
debug!(self.log, "Could not verify block for gossip, ignoring the block";
|
debug!(self.log, "Could not verify block for gossip, ignoring the block";
|
||||||
"error" => %e);
|
"error" => %e);
|
||||||
self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
|
self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
|
||||||
@ -951,10 +948,7 @@ impl<T: BeaconChainTypes> Worker<T> {
|
|||||||
);
|
);
|
||||||
self.send_sync_message(SyncMessage::UnknownBlock(peer_id, block));
|
self.send_sync_message(SyncMessage::UnknownBlock(peer_id, block));
|
||||||
}
|
}
|
||||||
Err(e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_)))
|
Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => {
|
||||||
| Err(
|
|
||||||
e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection),
|
|
||||||
) => {
|
|
||||||
debug!(
|
debug!(
|
||||||
self.log,
|
self.log,
|
||||||
"Failed to verify execution payload";
|
"Failed to verify execution payload";
|
||||||
|
@ -1,7 +1,7 @@
|
|||||||
use std::collections::hash_map::Entry;
|
use std::collections::hash_map::Entry;
|
||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
|
|
||||||
use beacon_chain::{BeaconChainTypes, BlockError, ExecutionPayloadError};
|
use beacon_chain::{BeaconChainTypes, BlockError};
|
||||||
use fnv::FnvHashMap;
|
use fnv::FnvHashMap;
|
||||||
use lighthouse_network::{PeerAction, PeerId};
|
use lighthouse_network::{PeerAction, PeerId};
|
||||||
use lru_cache::LRUTimeCache;
|
use lru_cache::LRUTimeCache;
|
||||||
@ -435,17 +435,12 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
|
|||||||
BlockError::ParentUnknown(block) => {
|
BlockError::ParentUnknown(block) => {
|
||||||
self.search_parent(block, peer_id, cx);
|
self.search_parent(block, peer_id, cx);
|
||||||
}
|
}
|
||||||
e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(
|
ref e @ BlockError::ExecutionPayloadError(ref epe) if !epe.penalize_peer() => {
|
||||||
_,
|
|
||||||
))
|
|
||||||
| e @ BlockError::ExecutionPayloadError(
|
|
||||||
ExecutionPayloadError::NoExecutionConnection,
|
|
||||||
) => {
|
|
||||||
// These errors indicate that the execution layer is offline
|
// These errors indicate that the execution layer is offline
|
||||||
// and failed to validate the execution payload. Do not downscore peer.
|
// and failed to validate the execution payload. Do not downscore peer.
|
||||||
debug!(
|
debug!(
|
||||||
self.log,
|
self.log,
|
||||||
"Single block lookup failed. Execution layer is offline";
|
"Single block lookup failed. Execution layer is offline / unsynced / misconfigured";
|
||||||
"root" => %root,
|
"root" => %root,
|
||||||
"error" => ?e
|
"error" => ?e
|
||||||
);
|
);
|
||||||
@ -549,12 +544,9 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
BlockProcessResult::Err(
|
ref e @ BlockProcessResult::Err(BlockError::ExecutionPayloadError(ref epe))
|
||||||
e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::RequestFailed(_)),
|
if !epe.penalize_peer() =>
|
||||||
)
|
{
|
||||||
| BlockProcessResult::Err(
|
|
||||||
e @ BlockError::ExecutionPayloadError(ExecutionPayloadError::NoExecutionConnection),
|
|
||||||
) => {
|
|
||||||
// These errors indicate that the execution layer is offline
|
// These errors indicate that the execution layer is offline
|
||||||
// and failed to validate the execution payload. Do not downscore peer.
|
// and failed to validate the execution payload. Do not downscore peer.
|
||||||
debug!(
|
debug!(
|
||||||
|
Loading…
Reference in New Issue
Block a user