fix(evm): propagate state provider errors in the block assembler

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

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
 

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<T> = Result<T, ProviderError>;
 
     #[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<Option<StorageValue>>,
+    }
+
+    impl StubStateProvider {
+        fn returning(storage: ProviderResult<Option<StorageValue>>) -> Self {
+            Self { storage }
+        }
+    }
+
+    impl StateProvider for StubStateProvider {
+        fn storage(
+            &self,
+            _account: Address,
+            _storage_key: StorageKey,
+        ) -> ProviderResult<Option<StorageValue>> {
+            self.storage.clone()
+        }
+    }
+
+    impl BytecodeReader for StubStateProvider {
+        fn bytecode_by_hash(&self, _code_hash: &B256) -> ProviderResult<Option<Bytecode>> {
+            unimplemented!("not reached by assemble_block")
+        }
+    }
+
+    impl BlockHashReader for StubStateProvider {
+        fn block_hash(&self, _number: BlockNumber) -> ProviderResult<Option<B256>> {
+            unimplemented!("not reached by assemble_block")
+        }
+
+        fn canonical_hashes_range(
+            &self,
+            _start: BlockNumber,
+            _end: BlockNumber,
+        ) -> ProviderResult<Vec<B256>> {
+            unimplemented!("not reached by assemble_block")
+        }
+    }
+
+    impl AccountReader for StubStateProvider {
+        fn basic_account(&self, _address: &Address) -> ProviderResult<Option<Account>> {
+            unimplemented!("not reached by assemble_block")
+        }
+    }
+
+    impl StateRootProvider for StubStateProvider {
+        fn state_root(&self, _hashed_state: HashedPostState) -> ProviderResult<B256> {
+            unimplemented!("not reached by assemble_block")
+        }
+
+        fn state_root_from_nodes(&self, _input: TrieInput) -> ProviderResult<B256> {
+            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<B256> {
+            unimplemented!("not reached by assemble_block")
+        }
+
+        fn storage_proof(
+            &self,
+            _address: Address,
+            _slot: B256,
+            _hashed_storage: HashedStorage,
+        ) -> ProviderResult<StorageProof> {
+            unimplemented!("not reached by assemble_block")
+        }
+
+        fn storage_multiproof(
+            &self,
+            _address: Address,
+            _slots: &[B256],
+            _hashed_storage: HashedStorage,
+        ) -> ProviderResult<StorageMultiProof> {
+            unimplemented!("not reached by assemble_block")
+        }
+    }
+
+    impl StateProofProvider for StubStateProvider {
+        fn proof(
+            &self,
+            _input: TrieInput,
+            _address: Address,
+            _slots: &[B256],
+        ) -> ProviderResult<AccountProof> {
+            unimplemented!("not reached by assemble_block")
+        }
+
+        fn multiproof(
+            &self,
+            _input: TrieInput,
+            _targets: MultiProofTargets,
+        ) -> ProviderResult<MultiProof> {
+            unimplemented!("not reached by assemble_block")
+        }
+
+        fn witness(
+            &self,
+            _input: TrieInput,
+            _target: HashedPostState,
+            _mode: ExecutionWitnessMode,
+        ) -> ProviderResult<Vec<Bytes>> {
+            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<Receipt> {
+        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<Receipt>,
+    ) -> 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::<ArcChainSpec>::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::<ProviderError>(),
+            "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::<ArcChainSpec>::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"
+        );
+    }
 }