From a90fc90df8be565173e6951c315c2ae4d3d73c3c Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 30 Jul 2024 18:54:05 -0400 Subject: [PATCH] feat: add better engine insert block error type (#9884) --- crates/blockchain-tree-api/src/error.rs | 166 +++++++++++++++++++++++- crates/engine/tree/src/tree/mod.rs | 30 +++-- 2 files changed, 182 insertions(+), 14 deletions(-) diff --git a/crates/blockchain-tree-api/src/error.rs b/crates/blockchain-tree-api/src/error.rs index 5a6ffd3f0c..2619f1b509 100644 --- a/crates/blockchain-tree-api/src/error.rs +++ b/crates/blockchain-tree-api/src/error.rs @@ -1,7 +1,9 @@ //! Error handling for the blockchain tree use reth_consensus::ConsensusError; -use reth_execution_errors::{BlockExecutionError, BlockValidationError}; +use reth_execution_errors::{ + BlockExecutionError, BlockValidationError, InternalBlockExecutionError, +}; use reth_primitives::{BlockHash, BlockNumber, SealedBlock}; pub use reth_storage_errors::provider::ProviderError; @@ -207,6 +209,168 @@ impl InsertBlockErrorData { } } +/// Error thrown when inserting a block failed because the block is considered invalid. +#[derive(thiserror::Error)] +#[error(transparent)] +pub struct InsertBlockErrorTwo { + inner: Box, +} + +// === impl InsertBlockErrorTwo === + +impl InsertBlockErrorTwo { + /// Create a new `InsertInvalidBlockErrorTwo` + pub fn new(block: SealedBlock, kind: InsertBlockErrorKindTwo) -> Self { + Self { inner: InsertBlockErrorDataTwo::boxed(block, kind) } + } + + /// Create a new `InsertInvalidBlockError` from a consensus error + pub fn consensus_error(error: ConsensusError, block: SealedBlock) -> Self { + Self::new(block, InsertBlockErrorKindTwo::Consensus(error)) + } + + /// Create a new `InsertInvalidBlockError` from a consensus error + pub fn sender_recovery_error(block: SealedBlock) -> Self { + Self::new(block, InsertBlockErrorKindTwo::SenderRecovery) + } + + /// Create a new `InsertInvalidBlockError` from an execution error + pub fn execution_error(error: BlockExecutionError, block: SealedBlock) -> Self { + Self::new(block, InsertBlockErrorKindTwo::Execution(error)) + } + + /// Consumes the error and returns the block that resulted in the error + #[inline] + pub fn into_block(self) -> SealedBlock { + self.inner.block + } + + /// Returns the error kind + #[inline] + pub const fn kind(&self) -> &InsertBlockErrorKindTwo { + &self.inner.kind + } + + /// Returns the block that resulted in the error + #[inline] + pub const fn block(&self) -> &SealedBlock { + &self.inner.block + } + + /// Consumes the type and returns the block and error kind. + #[inline] + pub fn split(self) -> (SealedBlock, InsertBlockErrorKindTwo) { + let inner = *self.inner; + (inner.block, inner.kind) + } +} + +impl std::fmt::Debug for InsertBlockErrorTwo { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Debug::fmt(&self.inner, f) + } +} + +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 { + /// Failed to recover senders for the block + #[error("failed to recover senders for block")] + SenderRecovery, + /// Block violated consensus rules. + #[error(transparent)] + Consensus(#[from] ConsensusError), + /// Block execution failed. + #[error(transparent)] + Execution(#[from] BlockExecutionError), + /// Provider error. + #[error(transparent)] + Provider(#[from] ProviderError), +} + +impl InsertBlockErrorKindTwo { + /// Returns true 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> { + match self { + Self::SenderRecovery | Self::Consensus(_) => Ok(()), + // 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(()) + } + // these are internal errors, not caused by an invalid block + BlockExecutionError::Internal(error) => { + Err(InsertBlockFatalError::BlockExecutionError(error)) + } + } + } + Self::Provider(err) => Err(InsertBlockFatalError::Provider(err)), + } + } +} + +/// Error variants that are not caused by invalid blocks +#[derive(Debug, thiserror::Error)] +pub enum InsertBlockFatalError { + /// A provider error + #[error(transparent)] + Provider(#[from] ProviderError), + /// An internal / fatal block execution error + #[error(transparent)] + BlockExecutionError(#[from] InternalBlockExecutionError), +} + /// 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 3bddad5b5d..f367c067bb 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -9,9 +9,10 @@ use reth_beacon_consensus::{ OnForkChoiceUpdated, MIN_BLOCKS_FOR_PIPELINE_RUN, }; use reth_blockchain_tree::{ - error::InsertBlockErrorKind, BlockAttachment, BlockBuffer, BlockStatus, + error::{InsertBlockErrorKindTwo, InsertBlockErrorTwo}, + BlockAttachment, BlockBuffer, BlockStatus, }; -use reth_blockchain_tree_api::{error::InsertBlockError, InsertPayloadOk}; +use reth_blockchain_tree_api::InsertPayloadOk; use reth_chain_state::{ CanonicalInMemoryState, ExecutedBlock, MemoryOverlayStateProvider, NewCanonicalChain, }; @@ -1016,16 +1017,19 @@ where debug!(target: "engine", elapsed = ?now.elapsed(), %block_count ,"connected buffered blocks"); } - fn buffer_block_without_senders(&mut self, block: SealedBlock) -> Result<(), InsertBlockError> { + fn buffer_block_without_senders( + &mut self, + block: SealedBlock, + ) -> Result<(), InsertBlockErrorTwo> { match block.try_seal_with_senders() { Ok(block) => self.buffer_block(block), - Err(block) => Err(InsertBlockError::sender_recovery_error(block)), + Err(block) => Err(InsertBlockErrorTwo::sender_recovery_error(block)), } } - fn buffer_block(&mut self, block: SealedBlockWithSenders) -> Result<(), InsertBlockError> { + fn buffer_block(&mut self, block: SealedBlockWithSenders) -> Result<(), InsertBlockErrorTwo> { if let Err(err) = self.validate_block(&block) { - return Err(InsertBlockError::consensus_error(err, block.block)) + return Err(InsertBlockErrorTwo::consensus_error(err, block.block)) } self.state.buffer.insert_block(block); Ok(()) @@ -1233,25 +1237,25 @@ where fn insert_block_without_senders( &mut self, block: SealedBlock, - ) -> Result { + ) -> Result { match block.try_seal_with_senders() { Ok(block) => self.insert_block(block), - Err(block) => Err(InsertBlockError::sender_recovery_error(block)), + Err(block) => Err(InsertBlockErrorTwo::sender_recovery_error(block)), } } fn insert_block( &mut self, block: SealedBlockWithSenders, - ) -> Result { + ) -> Result { self.insert_block_inner(block.clone()) - .map_err(|kind| InsertBlockError::new(block.block, kind)) + .map_err(|kind| InsertBlockErrorTwo::new(block.block, kind)) } fn insert_block_inner( &mut self, block: SealedBlockWithSenders, - ) -> Result { + ) -> Result { if self.block_by_hash(block.hash())?.is_some() { let attachment = BlockAttachment::Canonical; // TODO: remove or revise attachment return Ok(InsertPayloadOk::AlreadySeen(BlockStatus::Valid(attachment))) @@ -1260,13 +1264,13 @@ where // validate block consensus rules self.validate_block(&block)?; - let state_provider = self.state_provider(block.parent_hash).unwrap(); + let state_provider = self.state_provider(block.parent_hash)?; let executor = self.executor_provider.executor(StateProviderDatabase::new(&state_provider)); let block_number = block.number; let block_hash = block.hash(); let block = block.unseal(); - let output = executor.execute((&block, U256::MAX).into()).unwrap(); + let output = executor.execute((&block, U256::MAX).into())?; self.consensus.validate_block_post_execution( &block, PostExecutionInput::new(&output.receipts, &output.requests),