diff --git a/crates/blockchain-tree-api/src/error.rs b/crates/blockchain-tree-api/src/error.rs index 2619f1b509..5ef146b087 100644 --- a/crates/blockchain-tree-api/src/error.rs +++ b/crates/blockchain-tree-api/src/error.rs @@ -209,6 +209,52 @@ impl InsertBlockErrorData { } } +struct InsertBlockErrorDataTwo { + block: SealedBlock, + kind: InsertBlockErrorKindTwo, +} + +impl std::fmt::Display for InsertBlockErrorDataTwo { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "Failed to insert block (hash={}, number={}, parent_hash={}): {}", + self.block.hash(), + self.block.number, + self.block.parent_hash, + self.kind + ) + } +} + +impl std::fmt::Debug for InsertBlockErrorDataTwo { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("InsertBlockError") + .field("error", &self.kind) + .field("hash", &self.block.hash()) + .field("number", &self.block.number) + .field("parent_hash", &self.block.parent_hash) + .field("num_txs", &self.block.body.len()) + .finish_non_exhaustive() + } +} + +impl std::error::Error for InsertBlockErrorDataTwo { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.kind) + } +} + +impl InsertBlockErrorDataTwo { + const fn new(block: SealedBlock, kind: InsertBlockErrorKindTwo) -> Self { + Self { block, kind } + } + + fn boxed(block: SealedBlock, kind: InsertBlockErrorKindTwo) -> Box { + Box::new(Self::new(block, kind)) + } +} + /// Error thrown when inserting a block failed because the block is considered invalid. #[derive(thiserror::Error)] #[error(transparent)] @@ -271,52 +317,6 @@ impl std::fmt::Debug for InsertBlockErrorTwo { } } -struct InsertBlockErrorDataTwo { - block: SealedBlock, - kind: InsertBlockErrorKindTwo, -} - -impl std::fmt::Display for InsertBlockErrorDataTwo { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!( - f, - "Failed to insert block (hash={}, number={}, parent_hash={}): {}", - self.block.hash(), - self.block.number, - self.block.parent_hash, - self.kind - ) - } -} - -impl std::fmt::Debug for InsertBlockErrorDataTwo { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("InsertBlockError") - .field("error", &self.kind) - .field("hash", &self.block.hash()) - .field("number", &self.block.number) - .field("parent_hash", &self.block.parent_hash) - .field("num_txs", &self.block.body.len()) - .finish_non_exhaustive() - } -} - -impl std::error::Error for InsertBlockErrorDataTwo { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - Some(&self.kind) - } -} - -impl InsertBlockErrorDataTwo { - const fn new(block: SealedBlock, kind: InsertBlockErrorKindTwo) -> Self { - Self { block, kind } - } - - fn boxed(block: SealedBlock, kind: InsertBlockErrorKindTwo) -> Box { - Box::new(Self::new(block, kind)) - } -} - /// All error variants possible when inserting a block #[derive(Debug, thiserror::Error)] pub enum InsertBlockErrorKindTwo { @@ -335,19 +335,27 @@ pub enum InsertBlockErrorKindTwo { } impl InsertBlockErrorKindTwo { - /// Returns true if the error is caused by an invalid block + /// Returns an [`InsertBlockValidationError`] if the error is caused by an invalid block. /// - /// This is intended to be used to determine if the block should be marked as invalid. - #[allow(clippy::match_same_arms)] - pub fn is_invalid_block(self) -> Result<(), InsertBlockFatalError> { + /// Returns an [`InsertBlockFatalError`] if the error is caused by an error that is not + /// validation related or is otherwise fatal. + /// + /// This is intended to be used to determine if we should respond `INVALID` as a response when + /// processing a new block. + pub fn ensure_validation_error( + self, + ) -> Result { match self { - Self::SenderRecovery | Self::Consensus(_) => Ok(()), + Self::SenderRecovery => Ok(InsertBlockValidationError::SenderRecovery), + Self::Consensus(err) => Ok(InsertBlockValidationError::Consensus(err)), // other execution errors that are considered internal errors Self::Execution(err) => { match err { - BlockExecutionError::Validation(_) | BlockExecutionError::Consensus(_) => { - // this is caused by an invalid block - Ok(()) + BlockExecutionError::Validation(err) => { + Ok(InsertBlockValidationError::Validation(err)) + } + BlockExecutionError::Consensus(err) => { + Ok(InsertBlockValidationError::Consensus(err)) } // these are internal errors, not caused by an invalid block BlockExecutionError::Internal(error) => { @@ -371,6 +379,27 @@ pub enum InsertBlockFatalError { BlockExecutionError(#[from] InternalBlockExecutionError), } +/// Error variants that are caused by invalid blocks +#[derive(Debug, thiserror::Error)] +pub enum InsertBlockValidationError { + /// Failed to recover senders for the block + #[error("failed to recover senders for block")] + SenderRecovery, + /// Block violated consensus rules. + #[error(transparent)] + Consensus(#[from] ConsensusError), + /// Validation error, transparently wrapping [`BlockValidationError`] + #[error(transparent)] + Validation(#[from] BlockValidationError), +} + +impl InsertBlockValidationError { + /// Returns true if this is a block pre merge error. + pub const fn is_block_pre_merge(&self) -> bool { + matches!(self, Self::Validation(BlockValidationError::BlockPreMerge { .. })) + } +} + /// All error variants possible when inserting a block #[derive(Debug, thiserror::Error)] pub enum InsertBlockErrorKind { diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs index f367c067bb..aa6e531565 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -9,7 +9,7 @@ use reth_beacon_consensus::{ OnForkChoiceUpdated, MIN_BLOCKS_FOR_PIPELINE_RUN, }; use reth_blockchain_tree::{ - error::{InsertBlockErrorKindTwo, InsertBlockErrorTwo}, + error::{InsertBlockErrorKindTwo, InsertBlockErrorTwo, InsertBlockFatalError}, BlockAttachment, BlockBuffer, BlockStatus, }; use reth_blockchain_tree_api::InsertPayloadOk; @@ -315,7 +315,7 @@ pub trait EngineApiTreeHandler { &mut self, payload: ExecutionPayload, cancun_fields: Option, - ) -> ProviderResult>; + ) -> Result, InsertBlockFatalError>; /// Invoked when we receive a new forkchoice update message. Calls into the blockchain tree /// to resolve chain forks and ensure that the Execution Layer is working with the latest valid @@ -1501,7 +1501,7 @@ where &mut self, payload: ExecutionPayload, cancun_fields: Option, - ) -> ProviderResult> { + ) -> Result, InsertBlockFatalError> { trace!(target: "engine", "invoked new payload"); // Ensures that the given payload does not violate any consensus rules that concern the // block's layout, like: @@ -1571,19 +1571,50 @@ where PayloadStatus::from_status(PayloadStatusEnum::Syncing) } else { let mut latest_valid_hash = None; - let status = match self.insert_block_without_senders(block).unwrap() { - InsertPayloadOk::Inserted(BlockStatus::Valid(_)) | - InsertPayloadOk::AlreadySeen(BlockStatus::Valid(_)) => { - latest_valid_hash = Some(block_hash); - PayloadStatusEnum::Valid + + match self.insert_block_without_senders(block) { + Ok(status) => { + let status = match status { + InsertPayloadOk::Inserted(BlockStatus::Valid(_)) | + InsertPayloadOk::AlreadySeen(BlockStatus::Valid(_)) => { + latest_valid_hash = Some(block_hash); + PayloadStatusEnum::Valid + } + InsertPayloadOk::Inserted(BlockStatus::Disconnected { .. }) | + InsertPayloadOk::AlreadySeen(BlockStatus::Disconnected { .. }) => { + // not known to be invalid, but we don't know anything else + PayloadStatusEnum::Syncing + } + }; + + PayloadStatus::new(status, latest_valid_hash) } - InsertPayloadOk::Inserted(BlockStatus::Disconnected { .. }) | - InsertPayloadOk::AlreadySeen(BlockStatus::Disconnected { .. }) => { - // not known to be invalid, but we don't know anything else - PayloadStatusEnum::Syncing + Err(error) => { + let (block, error) = error.split(); + + // if invalid block, we check the validation error. Otherwise return the fatal + // error. + let validation_err = error.ensure_validation_error()?; + + // If the error was due to an invalid payload, the payload is added to the + // invalid headers cache and `Ok` with [PayloadStatusEnum::Invalid] is + // returned. + warn!(target: "engine::tree", invalid_hash=?block.hash(), invalid_number=?block.number, %validation_err, "Invalid block error on new payload"); + let latest_valid_hash = if validation_err.is_block_pre_merge() { + // zero hash must be returned if block is pre-merge + Some(B256::ZERO) + } else { + self.latest_valid_hash_for_invalid_payload(block.parent_hash)? + }; + + // keep track of the invalid header + self.state.invalid_headers.insert(block.header); + PayloadStatus::new( + PayloadStatusEnum::Invalid { validation_error: validation_err.to_string() }, + latest_valid_hash, + ) } - }; - PayloadStatus::new(status, latest_valid_hash) + } }; let mut outcome = TreeOutcome::new(status);