Skip to content

Commit

Permalink
Add more logging for invalid payloads (#3515)
Browse files Browse the repository at this point in the history
## Issue Addressed

NA

## Proposed Changes

Adds more `debug` logging to help troubleshoot invalid execution payload blocks. I was doing some of this recently and found it to be challenging.

With this PR we should be able to grep `Invalid execution payload` and get one-liners that will show the block, slot and details about the proposer.

I also changed the log in `process_invalid_execution_payload` since it was a little misleading; the `block_root` wasn't necessary the block which had an invalid payload.

## Additional Info

NA
  • Loading branch information
paulhauner committed Aug 29, 2022
1 parent 8609cce commit 1a833ec
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 8 deletions.
29 changes: 24 additions & 5 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3678,9 +3678,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
) -> Result<(), Error> {
debug!(
self.log,
"Invalid execution payload in block";
"latest_valid_ancestor" => ?op.latest_valid_ancestor(),
"block_root" => ?op.block_root(),
"Processing payload invalidation";
"op" => ?op,
);

// Update the execution status in fork choice.
Expand Down Expand Up @@ -4160,8 +4159,18 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
Ok(())
}
PayloadStatus::Invalid {
latest_valid_hash, ..
latest_valid_hash,
ref validation_error,
} => {
debug!(
self.log,
"Invalid execution payload";
"validation_error" => ?validation_error,
"latest_valid_hash" => ?latest_valid_hash,
"head_hash" => ?head_hash,
"head_block_root" => ?head_block_root,
"method" => "fcU",
);
warn!(
self.log,
"Fork choice update invalidated payload";
Expand Down Expand Up @@ -4192,7 +4201,17 @@ impl<T: BeaconChainTypes> BeaconChain<T> {

Err(BeaconChainError::ExecutionForkChoiceUpdateInvalid { status })
}
PayloadStatus::InvalidBlockHash { .. } => {
PayloadStatus::InvalidBlockHash {
ref validation_error,
} => {
debug!(
self.log,
"Invalid execution payload block hash";
"validation_error" => ?validation_error,
"head_hash" => ?head_hash,
"head_block_root" => ?head_block_root,
"method" => "fcU",
);
warn!(
self.log,
"Fork choice update invalidated payload";
Expand Down
33 changes: 31 additions & 2 deletions beacon_node/beacon_chain/src/execution_payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use state_processing::per_block_processing::{
};
use std::sync::Arc;
use tokio::task::JoinHandle;
use tree_hash::TreeHash;
use types::*;

pub type PreparePayloadResult<Payload> = Result<Payload, BlockProductionError>;
Expand Down Expand Up @@ -112,8 +113,22 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>(
Ok(PayloadVerificationStatus::Optimistic)
}
PayloadStatus::Invalid {
latest_valid_hash, ..
latest_valid_hash,
ref validation_error,
} => {
debug!(
chain.log,
"Invalid execution payload";
"validation_error" => ?validation_error,
"latest_valid_hash" => ?latest_valid_hash,
"execution_block_hash" => ?execution_payload.execution_payload.block_hash,
"root" => ?block.tree_hash_root(),
"graffiti" => block.body().graffiti().as_utf8_lossy(),
"proposer_index" => block.proposer_index(),
"slot" => block.slot(),
"method" => "new_payload",
);

// latest_valid_hash == 0 implies that this was the terminal block
// Hence, we don't need to run `BeaconChain::process_invalid_execution_payload`.
if latest_valid_hash == ExecutionBlockHash::zero() {
Expand All @@ -132,7 +147,21 @@ async fn notify_new_payload<'a, T: BeaconChainTypes>(

Err(ExecutionPayloadError::RejectedByExecutionEngine { status }.into())
}
PayloadStatus::InvalidBlockHash { .. } => {
PayloadStatus::InvalidBlockHash {
ref validation_error,
} => {
debug!(
chain.log,
"Invalid execution payload block hash";
"validation_error" => ?validation_error,
"execution_block_hash" => ?execution_payload.execution_payload.block_hash,
"root" => ?block.tree_hash_root(),
"graffiti" => block.body().graffiti().as_utf8_lossy(),
"proposer_index" => block.proposer_index(),
"slot" => block.slot(),
"method" => "new_payload",
);

// Returning an error here should be sufficient to invalidate the block. We have no
// information to indicate its parent is invalid, so no need to run
// `BeaconChain::process_invalid_execution_payload`.
Expand Down
2 changes: 1 addition & 1 deletion consensus/proto_array/src/proto_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ four_byte_option_impl!(four_byte_option_usize, usize);
four_byte_option_impl!(four_byte_option_checkpoint, Checkpoint);

/// Defines an operation which may invalidate the `execution_status` of some nodes.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub enum InvalidationOperation {
/// Invalidate only `block_root` and it's descendants. Don't invalidate any ancestors.
InvalidateOne { block_root: Hash256 },
Expand Down

0 comments on commit 1a833ec

Please sign in to comment.