From afbe88f5837acfc8f5bc11deb8a181502691d3b7 Mon Sep 17 00:00:00 2001 From: Thomas Coratger <60488569+tcoratger@users.noreply.github.com> Date: Wed, 20 Sep 2023 09:25:54 +0200 Subject: [PATCH] feat(error): revamp `make_canonical` error (#3899) Co-authored-by: Matthias Seitz --- crates/blockchain-tree/src/blockchain_tree.rs | 14 +++---- crates/consensus/beacon/src/engine/mod.rs | 14 +++---- .../interfaces/src/blockchain_tree/error.rs | 38 +++++++++++++++++++ crates/interfaces/src/error.rs | 3 ++ 4 files changed, 55 insertions(+), 14 deletions(-) diff --git a/crates/blockchain-tree/src/blockchain_tree.rs b/crates/blockchain-tree/src/blockchain_tree.rs index 3c6b04202..f2be9262c 100644 --- a/crates/blockchain-tree/src/blockchain_tree.rs +++ b/crates/blockchain-tree/src/blockchain_tree.rs @@ -9,7 +9,7 @@ use crate::{ use reth_db::{cursor::DbCursorRO, database::Database, tables, transaction::DbTx}; use reth_interfaces::{ blockchain_tree::{ - error::{BlockchainTreeError, InsertBlockError, InsertBlockErrorKind}, + error::{BlockchainTreeError, CanonicalError, InsertBlockError, InsertBlockErrorKind}, BlockStatus, CanonicalOutcome, InsertPayloadOk, }, consensus::{Consensus, ConsensusError}, @@ -937,12 +937,12 @@ impl BlockchainTree if let Some(header) = self.find_canonical_header(block_hash)? { info!(target: "blockchain_tree", ?block_hash, "Block is already canonical, ignoring."); let td = self.externals.database().provider()?.header_td(block_hash)?.ok_or( - BlockExecutionError::from(BlockValidationError::MissingTotalDifficulty { + CanonicalError::from(BlockValidationError::MissingTotalDifficulty { hash: *block_hash, }), )?; if !self.externals.chain_spec.fork(Hardfork::Paris).active_at_ttd(td, U256::ZERO) { - return Err(BlockExecutionError::from(BlockValidationError::BlockPreMerge { + return Err(CanonicalError::from(BlockValidationError::BlockPreMerge { hash: *block_hash, }) .into()) @@ -952,10 +952,10 @@ impl BlockchainTree let Some(chain_id) = self.block_indices.get_blocks_chain_id(block_hash) else { warn!(target: "blockchain_tree", ?block_hash, "Block hash not found in block indices"); - // TODO: better error - return Err( - BlockExecutionError::BlockHashNotFoundInChain { block_hash: *block_hash }.into() - ) + return Err(CanonicalError::from(BlockchainTreeError::BlockHashNotFoundInChain { + block_hash: *block_hash, + }) + .into()) }; let chain = self.chains.remove(&chain_id).expect("To be present"); diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index f47da4827..cd772bf13 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -11,7 +11,7 @@ use futures::{Future, StreamExt}; use reth_db::database::Database; use reth_interfaces::{ blockchain_tree::{ - error::{InsertBlockError, InsertBlockErrorKind}, + error::{BlockchainTreeError, CanonicalError, InsertBlockError, InsertBlockErrorKind}, BlockStatus, BlockchainTreeEngine, CanonicalOutcome, InsertPayloadOk, }, consensus::ForkchoiceState, @@ -690,7 +690,7 @@ where PayloadStatus::new(PayloadStatusEnum::Valid, Some(state.head_block_hash)) } Err(error) => { - if let Error::Execution(ref err) = error { + if let Error::Canonical(ref err) = error { if err.is_fatal() { tracing::error!(target: "consensus::engine", ?err, "Encountered fatal error"); return Err(error) @@ -929,10 +929,8 @@ where #[allow(clippy::single_match)] match &error { - Error::Execution( - error @ BlockExecutionError::Validation(BlockValidationError::BlockPreMerge { - .. - }), + Error::Canonical( + error @ CanonicalError::Validation(BlockValidationError::BlockPreMerge { .. }), ) => { warn!(target: "consensus::engine", ?error, ?state, "Failed to canonicalize the head hash"); return PayloadStatus::from_status(PayloadStatusEnum::Invalid { @@ -1497,7 +1495,9 @@ where // it's part of the canonical chain: if it's the safe or the finalized block if matches!( err, - Error::Execution(BlockExecutionError::BlockHashNotFoundInChain { .. }) + Error::Canonical(CanonicalError::BlockchainTree( + BlockchainTreeError::BlockHashNotFoundInChain { .. } + )) ) { // if the inserted block is the currently targeted `finalized` or `safe` // block, we will attempt to make them canonical, diff --git a/crates/interfaces/src/blockchain_tree/error.rs b/crates/interfaces/src/blockchain_tree/error.rs index 83bdf0023..f6a467222 100644 --- a/crates/interfaces/src/blockchain_tree/error.rs +++ b/crates/interfaces/src/blockchain_tree/error.rs @@ -33,6 +33,34 @@ pub enum BlockchainTreeError { BlockBufferingFailed { block_hash: BlockHash }, } +/// Result alias for `CanonicalError` +pub type CanonicalResult = std::result::Result; + +/// Canonical Errors +#[allow(missing_docs)] +#[derive(thiserror::Error, Debug, Clone, PartialEq, Eq)] +pub enum CanonicalError { + /// Error originating from validation operations. + #[error(transparent)] + Validation(#[from] BlockValidationError), + /// Error originating from blockchain tree operations. + #[error(transparent)] + BlockchainTree(#[from] BlockchainTreeError), + /// Error indicating a transaction reverted during execution. + #[error("Transaction error on revert: {inner:?}")] + CanonicalRevert { inner: String }, + /// Error indicating a transaction failed to commit during execution. + #[error("Transaction error on commit: {inner:?}")] + CanonicalCommit { inner: String }, +} + +impl CanonicalError { + /// Returns `true` if the error is fatal. + pub fn is_fatal(&self) -> bool { + matches!(self, Self::CanonicalCommit { .. } | Self::CanonicalRevert { .. }) + } +} + /// Error thrown when inserting a block failed because the block is considered invalid. #[derive(thiserror::Error)] #[error(transparent)] @@ -161,6 +189,9 @@ pub enum InsertBlockErrorKind { /// An internal error occurred, like interacting with the database. #[error("Internal error")] Internal(Box), + /// Canonical error. + #[error(transparent)] + Canonical(CanonicalError), } impl InsertBlockErrorKind { @@ -214,6 +245,12 @@ impl InsertBlockErrorKind { // any other error, such as database errors, are considered internal errors false } + InsertBlockErrorKind::Canonical(err) => match err { + CanonicalError::BlockchainTree(_) | + CanonicalError::CanonicalCommit { .. } | + CanonicalError::CanonicalRevert { .. } => false, + CanonicalError::Validation(_) => true, + }, } } @@ -274,6 +311,7 @@ impl From for InsertBlockErrorKind { Error::Provider(err) => InsertBlockErrorKind::Internal(Box::new(err)), Error::Network(err) => InsertBlockErrorKind::Internal(Box::new(err)), Error::Custom(err) => InsertBlockErrorKind::Internal(err.into()), + Error::Canonical(err) => InsertBlockErrorKind::Canonical(err), } } } diff --git a/crates/interfaces/src/error.rs b/crates/interfaces/src/error.rs index b8b72fc92..284d6c481 100644 --- a/crates/interfaces/src/error.rs +++ b/crates/interfaces/src/error.rs @@ -20,6 +20,9 @@ pub enum Error { #[error(transparent)] Network(#[from] reth_network_api::NetworkError), + #[error(transparent)] + Canonical(#[from] crate::blockchain_tree::error::CanonicalError), + #[error("{0}")] Custom(std::string::String), }