feat(error): revamp make_canonical error (#3899)

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

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<DB: Database, C: Consensus, EF: ExecutorFactory> BlockchainTree<DB, C, EF>
         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<DB: Database, C: Consensus, EF: ExecutorFactory> BlockchainTree<DB, C, EF>
 
         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");
 

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,

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<T> = std::result::Result<T, CanonicalError>;
+
+/// 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<dyn std::error::Error + Send + Sync>),
+    /// 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<crate::Error> 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),
         }
     }
 }

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),
 }