fix: improve error message and option to skip unsupported transaction…

… type in debug trace (#606)

* fix: improve error message and option to skip unsupported tx type in debug trace

* Address code review feedback

* Handle unsupported transaction type requested

* Fix test setup

.changeset/empty-cars-join.md

@@ -0,0 +1,5 @@
+---
+"@nomicfoundation/edr": patch
+---
+
+Improved error message and added option to skip unsupported transaction type in `debug_traceTransaction`. Set `__EDR_UNSAFE_SKIP_UNSUPPORTED_TRANSACTION_TYPES=true` as an environment variable to enable this.

crates/edr_provider/src/data.rs

@@ -16,23 +16,23 @@ use alloy_dyn_abi::eip712::TypedData;
 use edr_eth::{
     block::{
         calculate_next_base_fee_per_blob_gas, calculate_next_base_fee_per_gas, miner_reward,
-        BlobGas, BlockOptions,
+        BlobGas, BlockOptions, Header,
     },
     fee_history::FeeHistoryResult,
     filter::{FilteredEvents, LogOutput, SubscriptionType},
     log::FilterLog,
     receipt::BlockReceipt,
     reward_percentile::RewardPercentile,
     signature::{self, RecoveryMessage},
-    transaction::{request::TransactionRequestAndSender, Transaction, TransactionType},
+    transaction::{request::TransactionRequestAndSender, Signed, Transaction, TransactionType},
     Address, BlockSpec, BlockTag, Bytes, Eip1898BlockSpec, SpecId, B256, U256,
 };
 use edr_evm::{
     blockchain::{
-        Blockchain, BlockchainError, ForkedBlockchain, ForkedCreationError, GenesisBlockOptions,
-        LocalBlockchain, LocalCreationError, SyncBlockchain,
+        Blockchain, BlockchainError, ForkedBlockchain, ForkedBlockchainError, ForkedCreationError,
+        GenesisBlockOptions, LocalBlockchain, LocalCreationError, SyncBlockchain,
     },
-    chain_spec::L1ChainSpec,
+    chain_spec::{ChainSpec, L1ChainSpec},
     db::StateRef,
     debug_trace_transaction, execution_result_to_debug_result, mempool, mine_block,
     mine_block_with_single_transaction, register_eip_3155_and_raw_tracers_handles,
@@ -45,12 +45,13 @@ use edr_evm::{
     Account, AccountInfo, BlobExcessGasAndPrice, Block as _, BlockAndTotalDifficulty, BlockEnv,
     Bytecode, CfgEnv, CfgEnvWithHandlerCfg, DebugContext, DebugTraceConfig,
     DebugTraceResultWithTraces, Eip3155AndRawTracers, EvmStorageSlot, ExecutionResult, HashMap,
-    HashSet, MemPool, MineBlockResultAndState, OrderedTransaction, Precompile, RandomHashGenerator,
-    SyncBlock, TxEnv, KECCAK_EMPTY,
+    HashSet, IntoRemoteBlock, MemPool, MineBlockResultAndState, OrderedTransaction, Precompile,
+    RandomHashGenerator, RemoteBlockCreationError, SyncBlock, TxEnv, KECCAK_EMPTY,
 };
 use edr_rpc_eth::{
     client::{EthRpcClient, HeaderMap, RpcClientError},
     error::HttpError,
+    RpcTransactionType,
 };
 use gas::gas_used_ratio;
 use indexmap::IndexMap;
@@ -83,6 +84,9 @@ use crate::{
 const DEFAULT_INITIAL_BASE_FEE_PER_GAS: u64 = 1_000_000_000;
 const EDR_MAX_CACHED_STATES_ENV_VAR: &str = "__EDR_MAX_CACHED_STATES";
 const DEFAULT_MAX_CACHED_STATES: usize = 100_000;
+const EDR_UNSAFE_SKIP_UNSUPPORTED_TRANSACTION_TYPES: &str =
+    "__EDR_UNSAFE_SKIP_UNSUPPORTED_TRANSACTION_TYPES";
+const DEFAULT_SKIP_UNSUPPORTED_TRANSACTION_TYPES: bool = false;
 
 /// The result of executing an `eth_call`.
 #[derive(Clone, Debug)]
@@ -188,6 +192,8 @@ pub struct ProviderData<LoggerErrorT: Debug, TimerT: Clone + TimeSinceEpoch = Cu
     allow_blocks_with_same_timestamp: bool,
     allow_unlimited_contract_size: bool,
     verbose_tracing: bool,
+    // Skip unsupported transaction types in `debugTraceTransaction` instead of throwing an error
+    skip_unsupported_transaction_types: bool,
     // IndexMap to preserve account order for logging.
     local_accounts: IndexMap<Address, k256::SecretKey>,
     filters: HashMap<U256, Filter>,
@@ -229,18 +235,7 @@ impl<LoggerErrorT: Debug, TimerT: Clone + TimeSinceEpoch> ProviderData<LoggerErr
             next_block_base_fee_per_gas,
         } = create_blockchain_and_state(runtime_handle.clone(), &config, &timer, genesis_accounts)?;
 
-        let max_cached_states = std::env::var(EDR_MAX_CACHED_STATES_ENV_VAR).map_or_else(
-            |err| match err {
-                std::env::VarError::NotPresent => {
-                    Ok(NonZeroUsize::new(DEFAULT_MAX_CACHED_STATES).expect("constant is non-zero"))
-                }
-                std::env::VarError::NotUnicode(s) => Err(CreationError::InvalidMaxCachedStates(s)),
-            },
-            |s| {
-                s.parse()
-                    .map_err(|_err| CreationError::InvalidMaxCachedStates(s.into()))
-            },
-        )?;
+        let max_cached_states = get_max_cached_states_from_env()?;
         let mut block_state_cache = LruCache::new(max_cached_states);
         let mut block_number_to_state_id = HashTrieMapSync::default();
 
@@ -255,6 +250,8 @@ impl<LoggerErrorT: Debug, TimerT: Clone + TimeSinceEpoch> ProviderData<LoggerErr
         let is_auto_mining = config.mining.auto_mine;
         let min_gas_price = config.min_gas_price;
 
+        let skip_unsupported_transaction_types = get_skip_unsupported_transaction_types_from_env();
+
         let dao_activation_block = config
             .chains
             .get(&config.chain_id)
@@ -304,6 +301,7 @@ impl<LoggerErrorT: Debug, TimerT: Clone + TimeSinceEpoch> ProviderData<LoggerErr
             allow_blocks_with_same_timestamp,
             allow_unlimited_contract_size,
             verbose_tracing: false,
+            skip_unsupported_transaction_types,
             local_accounts,
             filters: HashMap::default(),
             last_filter_id: U256::ZERO,
@@ -615,18 +613,12 @@ impl<LoggerErrorT: Debug, TimerT: Clone + TimeSinceEpoch> ProviderData<LoggerErr
         transaction_hash: &B256,
         trace_config: DebugTraceConfig,
     ) -> Result<DebugTraceResultWithTraces, ProviderError<LoggerErrorT>> {
-        let block = self
-            .blockchain
-            .block_by_transaction_hash(transaction_hash)?
-            .ok_or_else(|| ProviderError::InvalidTransactionHash(*transaction_hash))?;
-
-        let header = block.header();
+        let (header, transactions) =
+            self.block_data_for_debug_trace_transaction(transaction_hash)?;
 
         let cfg_env = self.create_evm_config_at_block_spec(&BlockSpec::Number(header.number))?;
 
-        let transactions = block.transactions().to_vec();
-
-        let prev_block_number = block.header().number - 1;
+        let prev_block_number = header.number - 1;
         let prev_block_spec = Some(BlockSpec::Number(prev_block_number));
         let verbose_tracing = self.verbose_tracing;
 
@@ -1922,6 +1914,146 @@ impl<LoggerErrorT: Debug, TimerT: Clone + TimeSinceEpoch> ProviderData<LoggerErr
         Ok(transaction_hash)
     }
 
+    /// Returns the block header and the transactions from the block that
+    /// contains the transaction for debug trace transactions.
+    fn block_data_for_debug_trace_transaction(
+        &self,
+        transaction_hash: &B256,
+    ) -> Result<(Header, Vec<Signed>), ProviderError<LoggerErrorT>> {
+        // HACK: This is a hack to make `debug_traceTransaction` return a helpful error
+        // message in fork mode if there is a transaction in the block whose
+        // type is not supported or skip that transaction if an environment
+        // variable is set. This hack is only necessary until proper multichain
+        // support. https://github.com/NomicFoundation/edr/issues/570
+        self.rpc_client
+            .as_ref()
+            .and_then(|rpc_client| {
+                // Use `Result<Option>` in `block_in_place` to be able to short-circuit with
+                // `?`, but we want `Option<Result>` in the end as we're flat
+                // mapping an `Option`.
+                tokio::task::block_in_place::<_, Result<Option<_>, _>>(|| {
+                    // If the transaction is not found in the remote by the provided hash, there are
+                    // two possibilities:
+                    // 1. The transaction is from a local block. In this case, it must have valid
+                    //    transaction types, so we have nothing to do.
+                    // 2. There is no transaction with the provided hash. In this case the abstract
+                    //    interface will return an error.
+                    self.runtime_handle
+                        .block_on(rpc_client.get_transaction_by_hash(*transaction_hash))?
+                        .and_then::<Result<_, _>, _>(|transaction| {
+                            transaction
+                                .block_hash
+                                .ok_or_else(|| {
+                                    // If the transaction doesn't have a block hash, we treat the
+                                    // transaction hash as invalid.
+                                    ProviderError::<LoggerErrorT>::InvalidTransactionHash(
+                                        *transaction_hash,
+                                    )
+                                })
+                                .and_then(|block_hash| {
+                                    self.block_data_for_debug_trance_transaction_from_block_hash(
+                                        &block_hash,
+                                        transaction_hash,
+                                        rpc_client,
+                                    )
+                                })
+                                // Go from `Result<Option>` to `Option<Result>` as we're in a flat
+                                // map for an `Option`
+                                .transpose()
+                        })
+                        // Go back from `Option<Result>` to `Result<Option>` which is the expected
+                        // return type in the `block_in_place` to be able to short-circuit.
+                        .transpose()
+                })
+                // Go from `Result<Option>` to `Option<Result>` as we're in a flat
+                // map for an Option
+                .transpose()
+            })
+            // We have an `Option<Result>`, default with another `Option<Result>` from the generic
+            // blockchain interface if it's `None`.
+            .or_else(|| {
+                self.blockchain
+                    // Returns `Result<Option>`
+                    .block_by_transaction_hash(transaction_hash)
+                    .map_err(ProviderError::<LoggerErrorT>::Blockchain)
+                    // We need to return an `Option<Result>`
+                    .transpose()
+                    .map(|block| {
+                        // Map the value in the result then pass the result back out so the error
+                        // can be propagated if any.
+                        block.map(|block| {
+                            let transactions = block.transactions().to_vec();
+                            let header = block.header().clone();
+                            (header, transactions)
+                        })
+                    })
+            })
+            // Go from `Option<Result>` to `Result<Option>` to short-circuit the error.
+            .transpose()?
+            // If we couldn't find the transaction in the remote or local blockchains through the
+            // generic blockchain interface, then it's definitely invalid.
+            .ok_or_else(|| ProviderError::<LoggerErrorT>::InvalidTransactionHash(*transaction_hash))
+    }
+
+    fn block_data_for_debug_trance_transaction_from_block_hash(
+        &self,
+        block_hash: &B256,
+        transaction_hash: &B256,
+        rpc_client: &Arc<EthRpcClient<L1ChainSpec>>,
+    ) -> Result<Option<(Header, Vec<Signed>)>, ProviderError<LoggerErrorT>> {
+        let mut rpc_block = self
+            .runtime_handle
+            .block_on(rpc_client.get_block_by_hash_with_transaction_data(*block_hash))?
+            .ok_or_else(|| {
+                // If the remote returned a transaction for the transaction hash, but the block
+                // is not found, we treat the transaction hash as invalid.
+                ProviderError::<LoggerErrorT>::InvalidTransactionHash(*transaction_hash)
+            })?;
+
+        // SAFETY: We only need the header from the block later, so it's safe to take
+        // the transactions.
+        let transactions = std::mem::take(&mut rpc_block.transactions)
+            .into_iter()
+            .filter_map(|transaction| {
+                if let RpcTransactionType::Unknown(transaction_type) =
+                    transaction.transaction_type()
+                {
+                    if transaction_hash == &transaction.hash {
+                        Some(Err(ProviderError::<LoggerErrorT>::UnsupportedTransactionTypeForDebugTrace {
+                            transaction_hash: *transaction_hash,
+                            unsupported_transaction_type: transaction_type,
+                        }))
+                    } else if self.skip_unsupported_transaction_types  {
+                        None
+                    } else {
+                        Some(Err(
+                            ProviderError::<LoggerErrorT>::UnsupportedTransactionTypeInDebugTrace {
+                                requested_transaction_hash: *transaction_hash,
+                                unsupported_transaction_hash: transaction.hash,
+                                unsupported_transaction_type: transaction_type,
+                            },
+                        ))
+                    }
+                } else {
+                    Some(
+                        <L1ChainSpec as ChainSpec>::SignedTransaction::try_from(transaction)
+                            .map_err(RemoteBlockCreationError::from)
+                            .map_err(ForkedBlockchainError::from)
+                            .map_err(BlockchainError::from)
+                            .map_err(ProviderError::<LoggerErrorT>::from),
+                    )
+                }
+            })
+            .collect::<Result<Vec<_>, ProviderError<LoggerErrorT>>>()?;
+
+        let block = rpc_block
+            .into_remote_block(rpc_client.clone(), self.runtime_handle.clone())
+            .map_err(ForkedBlockchainError::from)
+            .map_err(BlockchainError::from)?;
+
+        Ok(Some((block.header().clone(), transactions)))
+    }
+
     /// Wrapper over `Blockchain::chain_id_at_block_number` that handles error
     /// conversion.
     fn chain_id_at_block_number(
@@ -2812,6 +2944,26 @@ pub(crate) mod test_utils {
     }
 }
 
+fn get_skip_unsupported_transaction_types_from_env() -> bool {
+    std::env::var(EDR_UNSAFE_SKIP_UNSUPPORTED_TRANSACTION_TYPES)
+        .map_or(DEFAULT_SKIP_UNSUPPORTED_TRANSACTION_TYPES, |s| s == "true")
+}
+
+fn get_max_cached_states_from_env() -> Result<NonZeroUsize, CreationError> {
+    std::env::var(EDR_MAX_CACHED_STATES_ENV_VAR).map_or_else(
+        |err| match err {
+            std::env::VarError::NotPresent => {
+                Ok(NonZeroUsize::new(DEFAULT_MAX_CACHED_STATES).expect("constant is non-zero"))
+            }
+            std::env::VarError::NotUnicode(s) => Err(CreationError::InvalidMaxCachedStates(s)),
+        },
+        |s| {
+            s.parse()
+                .map_err(|_err| CreationError::InvalidMaxCachedStates(s.into()))
+        },
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use std::convert::Infallible;

crates/edr_provider/src/error.rs

@@ -202,6 +202,17 @@ pub enum ProviderError<LoggerErrorT> {
         current_hardfork: SpecId,
         minimum_hardfork: SpecId,
     },
+    #[error("Cannot perform debug tracing on transaction '{requested_transaction_hash:?}', because its block includes transaction '{unsupported_transaction_hash:?}' with unsupported type '{unsupported_transaction_type}'")]
+    UnsupportedTransactionTypeInDebugTrace {
+        requested_transaction_hash: B256,
+        unsupported_transaction_hash: B256,
+        unsupported_transaction_type: u64,
+    },
+    #[error("Cannot perform debug tracing on transaction '{transaction_hash:?}', because it has unsupported transaction type '{unsupported_transaction_type}'")]
+    UnsupportedTransactionTypeForDebugTrace {
+        transaction_hash: B256,
+        unsupported_transaction_type: u64,
+    },
     #[error("{method_name} - Method not supported")]
     UnsupportedMethod { method_name: String },
 }
@@ -272,6 +283,8 @@ impl<LoggerErrorT: Debug> From<ProviderError<LoggerErrorT>> for jsonrpc::Error {
             ProviderError::UnsupportedEIP1559Parameters { .. } => INVALID_PARAMS,
             ProviderError::UnsupportedEIP4844Parameters { .. } => INVALID_PARAMS,
             ProviderError::UnsupportedMethod { .. } => -32004,
+            ProviderError::UnsupportedTransactionTypeInDebugTrace { .. } => INVALID_INPUT,
+            ProviderError::UnsupportedTransactionTypeForDebugTrace { .. } => INVALID_INPUT,
         };
 
         let data = match &value {