feat: handle tree execution errors gracefully (paradigmxyz#9920)

megaeth-labs · Jul 31, 2024 · 7864ae0 · 7864ae0
1 parent 893bb2d
commit 7864ae0
Show file tree

Hide file tree

Showing 2 changed files with 128 additions and 68 deletions.
diff --git 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<Self> {
+        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<Self> {
-        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<InsertBlockValidationError, InsertBlockFatalError> {
         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
@@ -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<CancunPayloadFields>,
-    ) -> ProviderResult<TreeOutcome<PayloadStatus>>;
+    ) -> Result<TreeOutcome<PayloadStatus>, 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<CancunPayloadFields>,
-    ) -> ProviderResult<TreeOutcome<PayloadStatus>> {
+    ) -> Result<TreeOutcome<PayloadStatus>, 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);