From fd45c8726dd46a3b374e14a78e2c14ce6e49fee2 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Wed, 17 May 2023 11:13:08 +0200 Subject: [PATCH] chore: executor error cleanup (#2709) --- crates/blockchain-tree/src/blockchain_tree.rs | 28 ++++++------ crates/blockchain-tree/src/chain.rs | 9 ++-- crates/blockchain-tree/src/shareable.rs | 2 +- crates/consensus/beacon/src/engine/mod.rs | 14 +++--- crates/interfaces/src/blockchain_tree.rs | 6 +-- crates/interfaces/src/error.rs | 2 +- crates/interfaces/src/executor.rs | 23 +++------- crates/revm/src/executor.rs | 45 +++++++++++-------- crates/stages/src/error.rs | 2 +- crates/storage/provider/src/chain.rs | 4 +- .../provider/src/test_utils/executor.rs | 10 ++--- .../storage/provider/src/traits/executor.rs | 6 +-- 12 files changed, 76 insertions(+), 75 deletions(-) diff --git a/crates/blockchain-tree/src/blockchain_tree.rs b/crates/blockchain-tree/src/blockchain_tree.rs index 5bafd2f719..150928acea 100644 --- a/crates/blockchain-tree/src/blockchain_tree.rs +++ b/crates/blockchain-tree/src/blockchain_tree.rs @@ -7,7 +7,7 @@ use reth_db::{cursor::DbCursorRO, database::Database, tables, transaction::DbTx} use reth_interfaces::{ blockchain_tree::BlockStatus, consensus::{Consensus, ConsensusError}, - executor::Error as ExecError, + executor::BlockExecutionError, Error, }; use reth_primitives::{ @@ -158,7 +158,7 @@ impl BlockchainTree return Ok(Some(BlockStatus::Valid)) } - return Err(ExecError::PendingBlockIsFinalized { + return Err(BlockExecutionError::PendingBlockIsFinalized { block_number: block.number, block_hash: block.hash, last_finalized: last_finalized_block, @@ -321,10 +321,10 @@ impl BlockchainTree // Validate that the block is post merge let parent_td = db .header_td(&block.parent_hash)? - .ok_or(ExecError::CanonicalChain { block_hash: block.parent_hash })?; + .ok_or(BlockExecutionError::CanonicalChain { block_hash: block.parent_hash })?; // Pass the parent total difficulty to short-circuit unnecessary calculations. if !self.externals.chain_spec.fork(Hardfork::Paris).active_at_ttd(parent_td, U256::ZERO) { - return Err(ExecError::BlockPreMerge { hash: block.hash }.into()) + return Err(BlockExecutionError::BlockPreMerge { hash: block.hash }.into()) } let canonical_chain = self.canonical_chain(); @@ -336,7 +336,7 @@ impl BlockchainTree let parent_header = db .header(&block.parent_hash)? - .ok_or(ExecError::CanonicalChain { block_hash: block.parent_hash })? + .ok_or(BlockExecutionError::CanonicalChain { block_hash: block.parent_hash })? .seal(block.parent_hash); let chain = AppendableChain::new_canonical_fork( &block, @@ -366,13 +366,13 @@ impl BlockchainTree // get canonical fork. let canonical_fork = self .canonical_fork(chain_id) - .ok_or(ExecError::BlockSideChainIdConsistency { chain_id })?; + .ok_or(BlockExecutionError::BlockSideChainIdConsistency { chain_id })?; // get chain that block needs to join to. let parent_chain = self .chains .get_mut(&chain_id) - .ok_or(ExecError::BlockSideChainIdConsistency { chain_id })?; + .ok_or(BlockExecutionError::BlockSideChainIdConsistency { chain_id })?; let chain_tip = parent_chain.tip().hash(); let canonical_block_hashes = self.block_indices.canonical_chain(); @@ -486,7 +486,7 @@ impl BlockchainTree &mut self, block: SealedBlock, ) -> Result { - let block = block.seal_with_senders().ok_or(ExecError::SenderRecoveryError)?; + let block = block.seal_with_senders().ok_or(BlockExecutionError::SenderRecoveryError)?; self.insert_block(block) } @@ -724,16 +724,16 @@ impl BlockchainTree .externals .shareable_db() .header_td(block_hash)? - .ok_or(ExecError::MissingTotalDifficulty { hash: *block_hash })?; + .ok_or(BlockExecutionError::MissingTotalDifficulty { hash: *block_hash })?; if !self.externals.chain_spec.fork(Hardfork::Paris).active_at_ttd(td, U256::ZERO) { - return Err(ExecError::BlockPreMerge { hash: *block_hash }.into()) + return Err(BlockExecutionError::BlockPreMerge { hash: *block_hash }.into()) } return Ok(()) } let Some(chain_id) = self.block_indices.get_blocks_chain_id(block_hash) else { info!(target: "blockchain_tree", ?block_hash, "Block hash not found in block indices"); - return Err(ExecError::BlockHashNotFoundInChain { block_hash: *block_hash }.into()) + return Err(BlockExecutionError::BlockHashNotFoundInChain { block_hash: *block_hash }.into()) }; let chain = self.chains.remove(&chain_id).expect("To be present"); @@ -842,7 +842,7 @@ impl BlockchainTree let (blocks, state) = chain.into_inner(); tx.append_blocks_with_post_state(blocks.into_blocks().collect(), state) - .map_err(|e| ExecError::CanonicalCommit { inner: e.to_string() })?; + .map_err(|e| BlockExecutionError::CanonicalCommit { inner: e.to_string() })?; tx.commit()?; @@ -882,7 +882,7 @@ impl BlockchainTree // read block and execution result from database. and remove traces of block from tables. let blocks_and_execution = tx .take_block_and_execution_range(self.externals.chain_spec.as_ref(), revert_range) - .map_err(|e| ExecError::CanonicalRevert { inner: e.to_string() })?; + .map_err(|e| BlockExecutionError::CanonicalRevert { inner: e.to_string() })?; tx.commit()?; @@ -1061,7 +1061,7 @@ mod tests { // check if random block is known let old_block = BlockNumHash::new(1, H256([32; 32])); - let err = ExecError::PendingBlockIsFinalized { + let err = BlockExecutionError::PendingBlockIsFinalized { block_number: old_block.number, block_hash: old_block.hash, last_finalized: 10, diff --git a/crates/blockchain-tree/src/chain.rs b/crates/blockchain-tree/src/chain.rs index 93d7f51b9b..a290814bd8 100644 --- a/crates/blockchain-tree/src/chain.rs +++ b/crates/blockchain-tree/src/chain.rs @@ -4,7 +4,7 @@ //! blocks, as well as a list of the blocks the chain is composed of. use crate::{post_state::PostState, PostStateDataRef}; use reth_db::database::Database; -use reth_interfaces::{consensus::Consensus, executor::Error as ExecError, Error}; +use reth_interfaces::{consensus::Consensus, executor::BlockExecutionError, Error}; use reth_primitives::{ BlockHash, BlockNumber, ForkBlock, SealedBlockWithSenders, SealedHeader, U256, }; @@ -97,10 +97,9 @@ impl AppendableChain { EF: ExecutorFactory, { let parent_number = block.number - 1; - let parent = self - .blocks() - .get(&parent_number) - .ok_or(ExecError::BlockNumberNotFoundInChain { block_number: parent_number })?; + let parent = self.blocks().get(&parent_number).ok_or( + BlockExecutionError::BlockNumberNotFoundInChain { block_number: parent_number }, + )?; let mut state = self.state.clone(); diff --git a/crates/blockchain-tree/src/shareable.rs b/crates/blockchain-tree/src/shareable.rs index d950d6c049..76311ce16e 100644 --- a/crates/blockchain-tree/src/shareable.rs +++ b/crates/blockchain-tree/src/shareable.rs @@ -45,7 +45,7 @@ impl BlockchainTreeEngine } let block = block .seal_with_senders() - .ok_or(reth_interfaces::executor::Error::SenderRecoveryError)?; + .ok_or(reth_interfaces::executor::BlockExecutionError::SenderRecoveryError)?; tree.insert_block_inner(block, true) } diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 7aad32a1c8..ecee3fe386 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -7,7 +7,7 @@ use reth_db::{database::Database, tables, transaction::DbTx}; use reth_interfaces::{ blockchain_tree::{BlockStatus, BlockchainTreeEngine}, consensus::ForkchoiceState, - executor::Error as ExecutorError, + executor::BlockExecutionError, p2p::{bodies::client::BodiesClient, headers::client::HeadersClient}, sync::{NetworkSyncUpdater, SyncState}, Error, @@ -272,7 +272,7 @@ where tree_error: Option<&Error>, ) -> Option { // check pre merge block error - if let Some(Error::Execution(ExecutorError::BlockPreMerge { .. })) = tree_error { + if let Some(Error::Execution(BlockExecutionError::BlockPreMerge { .. })) = tree_error { return Some(H256::zero()) } @@ -510,7 +510,7 @@ where #[allow(clippy::single_match)] match &error { - Error::Execution(error @ ExecutorError::BlockPreMerge { .. }) => { + Error::Execution(error @ BlockExecutionError::BlockPreMerge { .. }) => { return PayloadStatus::from_status(PayloadStatusEnum::Invalid { validation_error: error.to_string(), }) @@ -1393,7 +1393,8 @@ mod tests { .await .unwrap(); let expected_result = ForkchoiceUpdated::from_status(PayloadStatusEnum::Invalid { - validation_error: ExecutorError::BlockPreMerge { hash: block1.hash }.to_string(), + validation_error: BlockExecutionError::BlockPreMerge { hash: block1.hash } + .to_string(), }) .with_latest_valid_hash(H256::zero()); @@ -1404,7 +1405,7 @@ mod tests { mod new_payload { use super::*; use reth_interfaces::{ - executor::Error as ExecutorError, test_utils::generators::random_block, + executor::BlockExecutionError, test_utils::generators::random_block, }; use reth_primitives::{Hardfork, U256}; use reth_provider::test_utils::blocks::BlockChainTestData; @@ -1572,7 +1573,8 @@ mod tests { env.send_new_payload_retry_on_syncing(block2.clone().into()).await.unwrap(); let expected_result = PayloadStatus::from_status(PayloadStatusEnum::Invalid { - validation_error: ExecutorError::BlockPreMerge { hash: block2.hash }.to_string(), + validation_error: BlockExecutionError::BlockPreMerge { hash: block2.hash } + .to_string(), }) .with_latest_valid_hash(H256::zero()); assert_eq!(result, expected_result); diff --git a/crates/interfaces/src/blockchain_tree.rs b/crates/interfaces/src/blockchain_tree.rs index 80d308e31f..4c816b4688 100644 --- a/crates/interfaces/src/blockchain_tree.rs +++ b/crates/interfaces/src/blockchain_tree.rs @@ -1,4 +1,4 @@ -use crate::{executor::Error as ExecutionError, Error}; +use crate::{executor::BlockExecutionError, Error}; use reth_primitives::{ BlockHash, BlockNumHash, BlockNumber, SealedBlock, SealedBlockWithSenders, SealedHeader, }; @@ -14,13 +14,13 @@ use std::collections::{BTreeMap, HashSet}; pub trait BlockchainTreeEngine: BlockchainTreeViewer + Send + Sync { /// Recover senders and call [`BlockchainTreeEngine::insert_block`]. fn insert_block_without_senders(&self, block: SealedBlock) -> Result { - let block = block.seal_with_senders().ok_or(ExecutionError::SenderRecoveryError)?; + let block = block.seal_with_senders().ok_or(BlockExecutionError::SenderRecoveryError)?; self.insert_block(block) } /// Recover senders and call [`BlockchainTreeEngine::buffer_block`]. fn buffer_block_without_sender(&self, block: SealedBlock) -> Result<(), Error> { - let block = block.seal_with_senders().ok_or(ExecutionError::SenderRecoveryError)?; + let block = block.seal_with_senders().ok_or(BlockExecutionError::SenderRecoveryError)?; self.buffer_block(block) } diff --git a/crates/interfaces/src/error.rs b/crates/interfaces/src/error.rs index 746dffcb55..4d54bfeb28 100644 --- a/crates/interfaces/src/error.rs +++ b/crates/interfaces/src/error.rs @@ -6,7 +6,7 @@ pub type Result = std::result::Result; #[allow(missing_docs)] pub enum Error { #[error(transparent)] - Execution(#[from] crate::executor::Error), + Execution(#[from] crate::executor::BlockExecutionError), #[error(transparent)] Consensus(#[from] crate::consensus::ConsensusError), diff --git a/crates/interfaces/src/executor.rs b/crates/interfaces/src/executor.rs index 46da8a7047..faa43e6389 100644 --- a/crates/interfaces/src/executor.rs +++ b/crates/interfaces/src/executor.rs @@ -4,23 +4,11 @@ use thiserror::Error; /// BlockExecutor Errors #[allow(missing_docs)] #[derive(Error, Debug, Clone, PartialEq, Eq)] -pub enum Error { +pub enum BlockExecutionError { #[error("EVM reported invalid transaction ({hash:?}): {message}")] EVM { hash: H256, message: String }, - #[error("Verification failed.")] - VerificationFailed, - #[error("Fatal internal error")] - ExecutionFatalError, #[error("Failed to recover sender for transaction")] SenderRecoveryError, - #[error("Receipt cumulative gas used {got:?} is different from expected {expected:?}")] - ReceiptCumulativeGasUsedDiff { got: u64, expected: u64 }, - #[error("Receipt log count {got:?} is different from expected {expected:?}.")] - ReceiptLogCountDiff { got: usize, expected: usize }, - #[error("Receipt log is different.")] - ReceiptLogDiff, - #[error("Receipt log is different.")] - ExecutionSuccessDiff { got: bool, expected: bool }, #[error("Receipt root {got:?} is different than expected {expected:?}.")] ReceiptRootDiff { got: H256, expected: H256 }, #[error("Header bloom filter {got:?} is different than expected {expected:?}.")] @@ -56,15 +44,18 @@ pub enum Error { CanonicalRevert { inner: String }, #[error("Transaction error on commit: {inner:?}")] CanonicalCommit { inner: String }, - #[error("Transaction error on pipeline status update: {inner:?}")] - PipelineStatusUpdate { inner: String }, #[error("Block {hash:?} is pre merge")] BlockPreMerge { hash: H256 }, #[error("Missing total difficulty")] MissingTotalDifficulty { hash: H256 }, + + /// Only used for TestExecutor + #[cfg(feature = "test-utils")] + #[error("Execution unavailable for tests")] + UnavailableForTest, } -impl Error { +impl BlockExecutionError { /// Returns `true` if the error is fatal. pub fn is_fatal(&self) -> bool { matches!(self, Self::CanonicalCommit { .. } | Self::CanonicalRevert { .. }) diff --git a/crates/revm/src/executor.rs b/crates/revm/src/executor.rs index b7a9d64fe6..c9914330d3 100644 --- a/crates/revm/src/executor.rs +++ b/crates/revm/src/executor.rs @@ -7,7 +7,7 @@ use crate::{ to_reth_acc, }; use reth_consensus_common::calc; -use reth_interfaces::executor::Error; +use reth_interfaces::executor::BlockExecutionError; use reth_primitives::{ Account, Address, Block, BlockNumber, Bloom, Bytecode, ChainSpec, Hardfork, Header, Receipt, ReceiptWithBloom, TransactionSigned, Withdrawal, H256, U256, @@ -76,15 +76,17 @@ where &self, body: &[TransactionSigned], senders: Option>, - ) -> Result, Error> { + ) -> Result, BlockExecutionError> { if let Some(senders) = senders { if body.len() == senders.len() { Ok(senders) } else { - Err(Error::SenderRecoveryError) + Err(BlockExecutionError::SenderRecoveryError) } } else { - body.iter().map(|tx| tx.recover_signer().ok_or(Error::SenderRecoveryError)).collect() + body.iter() + .map(|tx| tx.recover_signer().ok_or(BlockExecutionError::SenderRecoveryError)) + .collect() } } @@ -134,14 +136,15 @@ where &mut self, block_number: BlockNumber, post_state: &mut PostState, - ) -> Result<(), Error> { + ) -> Result<(), BlockExecutionError> { let db = self.db(); let mut drained_balance = U256::ZERO; // drain all accounts ether for address in DAO_HARDKFORK_ACCOUNTS { - let db_account = db.load_account(address).map_err(|_| Error::ProviderError)?; + let db_account = + db.load_account(address).map_err(|_| BlockExecutionError::ProviderError)?; let old = to_reth_acc(&db_account.info); // drain balance drained_balance += core::mem::take(&mut db_account.info.balance); @@ -164,9 +167,9 @@ where address: Address, increment: U256, post_state: &mut PostState, - ) -> Result<(), Error> { + ) -> Result<(), BlockExecutionError> { increment_account_balance(self.db(), post_state, block_number, address, increment) - .map_err(|_| Error::ProviderError) + .map_err(|_| BlockExecutionError::ProviderError) } /// Runs a single transaction in the configured environment and proceeds @@ -177,7 +180,7 @@ where &mut self, transaction: &TransactionSigned, sender: Address, - ) -> Result { + ) -> Result { // Fill revm structure. fill_tx_env(&mut self.evm.env.tx, transaction, sender); @@ -195,7 +198,7 @@ where // main execution. self.evm.transact() }; - out.map_err(|e| Error::EVM { hash, message: format!("{e:?}") }) + out.map_err(|e| BlockExecutionError::EVM { hash, message: format!("{e:?}") }) } /// Runs the provided transactions and commits their state to the run-time database. @@ -213,7 +216,7 @@ where block: &Block, total_difficulty: U256, senders: Option>, - ) -> Result<(PostState, u64), Error> { + ) -> Result<(PostState, u64), BlockExecutionError> { // perf: do not execute empty blocks if block.body.is_empty() { return Ok((PostState::default(), 0)) @@ -229,7 +232,7 @@ where // must be no greater than the block’s gasLimit. let block_available_gas = block.header.gas_limit - cumulative_gas_used; if transaction.gas_limit() > block_available_gas { - return Err(Error::TransactionGasLimitMoreThenAvailableBlockGas { + return Err(BlockExecutionError::TransactionGasLimitMoreThenAvailableBlockGas { transaction_gas_limit: transaction.gas_limit(), block_available_gas, }) @@ -276,13 +279,16 @@ where block: &Block, total_difficulty: U256, senders: Option>, - ) -> Result { + ) -> Result { let (mut post_state, cumulative_gas_used) = self.execute_transactions(block, total_difficulty, senders)?; // Check if gas used matches the value set in header. if block.gas_used != cumulative_gas_used { - return Err(Error::BlockGasUsed { got: cumulative_gas_used, expected: block.gas_used }) + return Err(BlockExecutionError::BlockGasUsed { + got: cumulative_gas_used, + expected: block.gas_used, + }) } // Add block rewards @@ -303,7 +309,7 @@ where block: &Block, total_difficulty: U256, senders: Option>, - ) -> Result { + ) -> Result { let post_state = self.execute(block, total_difficulty, senders)?; // TODO Before Byzantium, receipts contained state root that would mean that expensive @@ -496,18 +502,21 @@ pub fn verify_receipt<'a>( expected_receipts_root: H256, expected_logs_bloom: Bloom, receipts: impl Iterator + Clone, -) -> Result<(), Error> { +) -> Result<(), BlockExecutionError> { // Check receipts root. let receipts_with_bloom = receipts.map(|r| r.clone().into()).collect::>(); let receipts_root = reth_primitives::proofs::calculate_receipt_root(&receipts_with_bloom); if receipts_root != expected_receipts_root { - return Err(Error::ReceiptRootDiff { got: receipts_root, expected: expected_receipts_root }) + return Err(BlockExecutionError::ReceiptRootDiff { + got: receipts_root, + expected: expected_receipts_root, + }) } // Create header log bloom. let logs_bloom = receipts_with_bloom.iter().fold(Bloom::zero(), |bloom, r| bloom | r.bloom); if logs_bloom != expected_logs_bloom { - return Err(Error::BloomLogDiff { + return Err(BlockExecutionError::BloomLogDiff { expected: Box::new(expected_logs_bloom), got: Box::new(logs_bloom), }) diff --git a/crates/stages/src/error.rs b/crates/stages/src/error.rs index f12fbf6e95..2531ad1146 100644 --- a/crates/stages/src/error.rs +++ b/crates/stages/src/error.rs @@ -30,7 +30,7 @@ pub enum StageError { block: BlockNumber, /// The underlying execution error. #[source] - error: executor::Error, + error: executor::BlockExecutionError, }, /// Invalid checkpoint passed to the stage #[error("Invalid stage progress: {0}")] diff --git a/crates/storage/provider/src/chain.rs b/crates/storage/provider/src/chain.rs index ae088273e4..4a18bd7438 100644 --- a/crates/storage/provider/src/chain.rs +++ b/crates/storage/provider/src/chain.rs @@ -1,7 +1,7 @@ //! Contains [Chain], a chain of blocks and their final state. use crate::PostState; -use reth_interfaces::{executor::Error as ExecError, Error}; +use reth_interfaces::{executor::BlockExecutionError, Error}; use reth_primitives::{ BlockHash, BlockNumHash, BlockNumber, ForkBlock, Receipt, SealedBlock, SealedBlockWithSenders, TransactionSigned, TxHash, @@ -151,7 +151,7 @@ impl Chain { pub fn append_chain(&mut self, chain: Chain) -> Result<(), Error> { let chain_tip = self.tip(); if chain_tip.hash != chain.fork_block_hash() { - return Err(ExecError::AppendChainDoesntConnect { + return Err(BlockExecutionError::AppendChainDoesntConnect { chain_tip: chain_tip.num_hash(), other_chain_fork: chain.fork_block(), } diff --git a/crates/storage/provider/src/test_utils/executor.rs b/crates/storage/provider/src/test_utils/executor.rs index 81c6ddbff2..50d732e06e 100644 --- a/crates/storage/provider/src/test_utils/executor.rs +++ b/crates/storage/provider/src/test_utils/executor.rs @@ -1,6 +1,6 @@ use crate::{post_state::PostState, BlockExecutor, ExecutorFactory, StateProvider}; use parking_lot::Mutex; -use reth_interfaces::executor::Error as ExecutionError; +use reth_interfaces::executor::BlockExecutionError; use reth_primitives::{Address, Block, ChainSpec, U256}; use std::sync::Arc; /// Test executor with mocked result. @@ -12,8 +12,8 @@ impl BlockExecutor for TestExecutor { _block: &Block, _total_difficulty: U256, _senders: Option>, - ) -> Result { - self.0.clone().ok_or(ExecutionError::VerificationFailed) + ) -> Result { + self.0.clone().ok_or(BlockExecutionError::UnavailableForTest) } fn execute_and_verify_receipt( @@ -21,8 +21,8 @@ impl BlockExecutor for TestExecutor { _block: &Block, _total_difficulty: U256, _senders: Option>, - ) -> Result { - self.0.clone().ok_or(ExecutionError::VerificationFailed) + ) -> Result { + self.0.clone().ok_or(BlockExecutionError::UnavailableForTest) } } diff --git a/crates/storage/provider/src/traits/executor.rs b/crates/storage/provider/src/traits/executor.rs index e25ab2c564..2ff648d43e 100644 --- a/crates/storage/provider/src/traits/executor.rs +++ b/crates/storage/provider/src/traits/executor.rs @@ -1,7 +1,7 @@ //! Executor Factory use crate::{post_state::PostState, StateProvider}; -use reth_interfaces::executor::Error; +use reth_interfaces::executor::BlockExecutionError; use reth_primitives::{Address, Block, ChainSpec, U256}; /// Executor factory that would create the EVM with particular state provider. @@ -33,7 +33,7 @@ pub trait BlockExecutor { block: &Block, total_difficulty: U256, senders: Option>, - ) -> Result; + ) -> Result; /// Executes the block and checks receipts fn execute_and_verify_receipt( @@ -41,5 +41,5 @@ pub trait BlockExecutor { block: &Block, total_difficulty: U256, senders: Option>, - ) -> Result; + ) -> Result; }