diff --git a/Cargo.lock b/Cargo.lock index 560acdde..5746d04d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1241,6 +1241,7 @@ dependencies = [ "reth-evm", "reth-node-api", "reth-primitives-traits", + "reth-trie-common", "revm", "revm-context-interface", "revm-handler", diff --git a/Cargo.toml b/Cargo.toml index 6d50898f..10f2294a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -219,6 +219,7 @@ reth-storage-api = { git = "https://github.com/paradigmxyz/reth", tag = "v2.2.0" reth-tasks = { git = "https://github.com/paradigmxyz/reth", tag = "v2.2.0" } reth-tracing = { git = "https://github.com/paradigmxyz/reth", tag = "v2.2.0" } reth-transaction-pool = { git = "https://github.com/paradigmxyz/reth", tag = "v2.2.0" } +reth-trie-common = { git = "https://github.com/paradigmxyz/reth", tag = "v2.2.0", default-features = false } reth-trie-parallel = { git = "https://github.com/paradigmxyz/reth", tag = "v2.2.0" } # revm diff --git a/crates/evm/Cargo.toml b/crates/evm/Cargo.toml index f45124fc..7ad534c9 100644 --- a/crates/evm/Cargo.toml +++ b/crates/evm/Cargo.toml @@ -57,6 +57,7 @@ alloy-genesis.workspace = true alloy-rpc-types-trace.workspace = true arc-execution-config = { workspace = true, features = ["test-utils"] } arc-precompiles.workspace = true +reth-trie-common.workspace = true revm-inspectors.workspace = true rstest.workspace = true diff --git a/crates/evm/src/assembler.rs b/crates/evm/src/assembler.rs index 702f2ac8..28d37be7 100644 --- a/crates/evm/src/assembler.rs +++ b/crates/evm/src/assembler.rs @@ -83,10 +83,14 @@ where // Read from state provider if the state is not changed. if value.is_none() { + // A provider failure must not be confused with an unset slot: returning + // `None` here leaves `extra_data` empty, which validate_extra_data_base_fee + // rejects, so a transient read error would produce an invalid block instead + // of failing the build. value = input .state_provider .storage(SYSTEM_ACCOUNTING_ADDRESS, slot) - .unwrap_or(None) + .map_err(BlockExecutionError::other)? } if let Some(value) = value { @@ -106,9 +110,31 @@ where #[cfg(test)] mod tests { + extern crate alloc; + use super::*; - use alloc::sync::Arc; - use arc_execution_config::chainspec::LOCAL_DEV; + use crate::evm::ArcEvmConfig; + use alloc::{sync::Arc, vec, vec::Vec}; + use arc_execution_config::chainspec::{ArcChainSpec, LOCAL_DEV}; + + use alloy_consensus::Header; + use alloy_evm::{block::BlockExecutionResult, EvmEnv}; + use alloy_primitives::{Address, BlockNumber, Bytes, StorageKey, StorageValue, U256}; + use reth_ethereum::storage::{ + AccountReader, BlockHashReader, BytecodeReader, HashedPostStateProvider, + StateProofProvider, StateProvider, StateRootProvider, StorageRootProvider, + }; + use reth_evm::execute::ProviderError; + use reth_primitives_traits::{Account, Bytecode, SealedHeader}; + use reth_trie_common::{ + updates::TrieUpdates, AccountProof, ExecutionWitnessMode, HashedPostState, HashedStorage, + MultiProof, MultiProofTargets, StorageMultiProof, StorageProof, TrieInput, + }; + use revm::context::{BlockEnv, CfgEnv}; + use revm::database::BundleState; + use revm_primitives::hardfork::SpecId; + + type ProviderResult = Result; #[test] fn block_assembler_creation() { @@ -118,4 +144,240 @@ mod tests { // Verify the inner assembler's chain_spec points to the same assert!(Arc::ptr_eq(&assembler.chain_spec, &chain_spec)); } + + /// A `StateProvider` whose `storage` result is fixed by the test. + /// + /// `assemble_block` only reaches the provider through + /// `StateProvider::storage`, so the remaining methods are unreachable + /// stubs that panic rather than fake answers. + struct StubStateProvider { + storage: ProviderResult>, + } + + impl StubStateProvider { + fn returning(storage: ProviderResult>) -> Self { + Self { storage } + } + } + + impl StateProvider for StubStateProvider { + fn storage( + &self, + _account: Address, + _storage_key: StorageKey, + ) -> ProviderResult> { + self.storage.clone() + } + } + + impl BytecodeReader for StubStateProvider { + fn bytecode_by_hash(&self, _code_hash: &B256) -> ProviderResult> { + unimplemented!("not reached by assemble_block") + } + } + + impl BlockHashReader for StubStateProvider { + fn block_hash(&self, _number: BlockNumber) -> ProviderResult> { + unimplemented!("not reached by assemble_block") + } + + fn canonical_hashes_range( + &self, + _start: BlockNumber, + _end: BlockNumber, + ) -> ProviderResult> { + unimplemented!("not reached by assemble_block") + } + } + + impl AccountReader for StubStateProvider { + fn basic_account(&self, _address: &Address) -> ProviderResult> { + unimplemented!("not reached by assemble_block") + } + } + + impl StateRootProvider for StubStateProvider { + fn state_root(&self, _hashed_state: HashedPostState) -> ProviderResult { + unimplemented!("not reached by assemble_block") + } + + fn state_root_from_nodes(&self, _input: TrieInput) -> ProviderResult { + unimplemented!("not reached by assemble_block") + } + + fn state_root_with_updates( + &self, + _hashed_state: HashedPostState, + ) -> ProviderResult<(B256, TrieUpdates)> { + unimplemented!("not reached by assemble_block") + } + + fn state_root_from_nodes_with_updates( + &self, + _input: TrieInput, + ) -> ProviderResult<(B256, TrieUpdates)> { + unimplemented!("not reached by assemble_block") + } + } + + impl HashedPostStateProvider for StubStateProvider { + fn hashed_post_state(&self, _bundle_state: &BundleState) -> HashedPostState { + unimplemented!("not reached by assemble_block") + } + } + + impl StorageRootProvider for StubStateProvider { + fn storage_root( + &self, + _address: Address, + _hashed_storage: HashedStorage, + ) -> ProviderResult { + unimplemented!("not reached by assemble_block") + } + + fn storage_proof( + &self, + _address: Address, + _slot: B256, + _hashed_storage: HashedStorage, + ) -> ProviderResult { + unimplemented!("not reached by assemble_block") + } + + fn storage_multiproof( + &self, + _address: Address, + _slots: &[B256], + _hashed_storage: HashedStorage, + ) -> ProviderResult { + unimplemented!("not reached by assemble_block") + } + } + + impl StateProofProvider for StubStateProvider { + fn proof( + &self, + _input: TrieInput, + _address: Address, + _slots: &[B256], + ) -> ProviderResult { + unimplemented!("not reached by assemble_block") + } + + fn multiproof( + &self, + _input: TrieInput, + _targets: MultiProofTargets, + ) -> ProviderResult { + unimplemented!("not reached by assemble_block") + } + + fn witness( + &self, + _input: TrieInput, + _target: HashedPostState, + _mode: ExecutionWitnessMode, + ) -> ProviderResult> { + unimplemented!("not reached by assemble_block") + } + } + + /// The block number the assembler will derive its storage slot from. + const BLOCK_NUMBER: u64 = 7; + + fn block_env() -> BlockEnv { + BlockEnv { + number: U256::from(BLOCK_NUMBER), + ..Default::default() + } + } + + fn execution_ctx<'a>() -> EthBlockExecutionCtx<'a> { + EthBlockExecutionCtx { + parent_hash: B256::ZERO, + parent_beacon_block_root: None, + ommers: &[], + withdrawals: None, + extra_data: Default::default(), + tx_count_hint: None, + slot_number: None, + } + } + + fn bundle_state() -> BundleState { + BundleState::default() + } + + fn execution_result() -> BlockExecutionResult { + BlockExecutionResult::default() + } + + /// Builds the real `BlockAssemblerInput` that `assemble_block` consumes. + fn assembler_input<'a, 'b>( + state_provider: &'b StubStateProvider, + parent: &'a SealedHeader, + bundle_state: &'a BundleState, + output: &'b BlockExecutionResult, + ) -> BlockAssemblerInput<'a, 'b, ArcEvmConfig, Header> { + BlockAssemblerInput::new( + EvmEnv::new(CfgEnv::new_with_spec(SpecId::default()), block_env()), + execution_ctx(), + parent, + vec![], + output, + bundle_state, + state_provider, + B256::ZERO, + ) + } + + /// A provider read failure must fail the build, not produce a block with + /// empty `extra_data`. + /// + /// Before the fix, `unwrap_or(None)` turned the error into "slot unset", + /// so `assemble_block` returned `Ok` with an empty `extra_data`, which + /// `validate_extra_data_base_fee` then rejects on every validator. + #[test] + fn assemble_block_fails_when_state_provider_errors() { + let assembler = ArcBlockAssembler::::new(LOCAL_DEV.clone()); + let parent = SealedHeader::new(Header::default(), B256::ZERO); + let bundle_state = bundle_state(); + let output = execution_result(); + let provider = + StubStateProvider::returning(Err(ProviderError::BlockHashNotFound(B256::ZERO))); + + let err = assembler + .assemble_block(assembler_input(&provider, &parent, &bundle_state, &output)) + .expect_err("a provider read error must not yield a block"); + + // The provider error is propagated verbatim, so the caller sees the + // real cause rather than a generic "assemble failed". + let internal = err + .as_internal() + .expect("provider errors are internal, not validation errors"); + assert!( + internal.is_other::(), + "expected the ProviderError itself to be propagated, got: {err}" + ); + } + + /// The genuine "slot is not set" case must keep its existing behaviour: + /// log a warning and assemble a block, leaving `extra_data` empty. + #[test] + fn assemble_block_warns_and_continues_when_slot_unset() { + let assembler = ArcBlockAssembler::::new(LOCAL_DEV.clone()); + let parent = SealedHeader::new(Header::default(), B256::ZERO); + let bundle_state = bundle_state(); + let output = execution_result(); + let provider = StubStateProvider::returning(Ok(None)); + + let block = assembler + .assemble_block(assembler_input(&provider, &parent, &bundle_state, &output)) + .expect("an unset slot is not a build failure"); + + assert!( + block.header.extra_data.is_empty(), + "no base fee is known, so extra_data must stay empty" + ); + } }