From e7ba48ece70f42e08e0af16f0e321e555f571770 Mon Sep 17 00:00:00 2001 From: panos Date: Mon, 7 Sep 2026 13:20:22 +0800 Subject: [PATCH 1/2] fix: close reth 2.5.2 upgrade gaps in engine validator and intrinsic gas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups to the v2.4.0 -> v2.5.2 upgrade, found while auditing that diff against the upstream sources. Forward `EngineValidator::on_canonical_head_changed`. reth 2.5.0 added this method with an empty default body, so `MorphTreeEngineValidator` — a wrapper that must delegate every method to `inner` — silently replaced `BasicEngineValidator`'s implementation (txpool prewarming, opt-in via `--engine.txpool-prewarming`) with the no-op default instead of failing to compile. Document that forwarding contract on the type so the next upgrade re-checks it: a method with a default body is exactly the case the compiler cannot catch. Derive the EIP-2780 intrinsic-gas info in `validate_initial_tx_gas` from `Cfg::is_amsterdam_eip2780_enabled()` the way revm's own handler does, instead of hardcoding the new `None` argument. `CfgEnv::with_spec_and_mainnet_gas_params` (used by `MorphEvmConfig`) derives that flag from the spec, so a hardcoded `None` would silently diverge from upstream intrinsic gas if a Morph hardfork ever mapped to AMSTERDAM. Behaviour is unchanged today: every Morph hardfork maps below AMSTERDAM. Extend `test_morph_hardforks_do_not_enable_amsterdam_state_gas` to assert the EIP-2780 flag stays off for every Morph hardfork, with a positive control at AMSTERDAM so the guard cannot become vacuous if the flag stops being spec-derived. Restore the intent of `test_eip7702_refund_stays_regular_for_morph_specs`. revm 42 removed `tx_eip7702_state_refund` / `tx_eip7702_auth_refund`, so the test had been reduced to a duplicate of the state-gas assertion above it while keeping a message about refunds, plus a hardcoded 12500. It now asserts the surviving equivalent — the full per-auth refund equals `PER_EMPTY_ACCOUNT_COST - PER_AUTH_BASE_COST` — against revm's constants. Drop `reth-trie-db`: it lost its last use when the `ChangesetCache` parameter was replaced by `OverlayManager`. Also record why `evm_call` hands a fresh journal checkpoint to `Handler::execution` and why its `None` arm is unreachable for Morph. --- Cargo.lock | 1 - Cargo.toml | 1 - crates/chainspec/src/hardfork.rs | 38 +++++++++++++++++++++++++------- crates/node/Cargo.toml | 1 - crates/node/src/validator.rs | 9 ++++++++ crates/revm/src/handler.rs | 23 ++++++++++++++++--- 6 files changed, 59 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d022227..431ef2f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5158,7 +5158,6 @@ dependencies = [ "reth-tracing", "reth-transaction-pool", "reth-trie", - "reth-trie-db", "serde", "serde_json", "tokio", diff --git a/Cargo.toml b/Cargo.toml index da8f2df..deaac7f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -160,7 +160,6 @@ reth-trie = { git = "https://github.com/paradigmxyz/reth", tag = "v2.5.2" } reth-transaction-pool = { git = "https://github.com/paradigmxyz/reth", tag = "v2.5.2" } reth-zstd-compressors = { version = "0.6.0", default-features = false } reth-execution-cache = { git = "https://github.com/paradigmxyz/reth", tag = "v2.5.2" } -reth-trie-db = { git = "https://github.com/paradigmxyz/reth", tag = "v2.5.2" } reth-trie-parallel = { git = "https://github.com/paradigmxyz/reth", tag = "v2.5.2" } reth-trie-sparse = { git = "https://github.com/paradigmxyz/reth", tag = "v2.5.2", default-features = false } diff --git a/crates/chainspec/src/hardfork.rs b/crates/chainspec/src/hardfork.rs index f914e8a..e13a4b9 100644 --- a/crates/chainspec/src/hardfork.rs +++ b/crates/chainspec/src/hardfork.rs @@ -202,7 +202,11 @@ impl From for MorphHardfork { #[cfg(test)] mod tests { use super::*; - use alloy_evm::revm::context_interface::cfg::gas_params::GasParams; + use alloy_evm::revm::{ + context::{Cfg, CfgEnv}, + context_interface::cfg::gas_params::GasParams, + primitives::eip7702, + }; #[test] fn test_morph_hardfork_to_specid_mapping() { @@ -238,27 +242,45 @@ mod tests { 0, "MorphHardfork {fork:?} must not enable EIP-8037 state gas" ); + + // `MorphEvmHandler::validate_initial_tx_gas` derives its EIP-2780 intrinsic-gas + // info from this flag. It must stay disabled for every Morph hardfork. + let cfg = CfgEnv::::default().with_spec_and_mainnet_gas_params(fork); + assert!( + !cfg.is_amsterdam_eip2780_enabled(), + "MorphHardfork {fork:?} must not enable EIP-2780 intrinsic gas" + ); } + + // Positive control: the EIP-2780 flag really is derived from the spec, so the + // per-fork assertions above are not vacuously true. + assert!( + CfgEnv::::default() + .with_spec_and_mainnet_gas_params(SpecId::AMSTERDAM) + .is_amsterdam_eip2780_enabled(), + "EIP-2780 flag is no longer spec-derived; the guard above would be vacuous" + ); } + /// The whole EIP-7702 per-authorization refund must be the *regular* refund, i.e. + /// `PER_EMPTY_ACCOUNT_COST - PER_AUTH_BASE_COST` with nothing diverted elsewhere. + /// + /// revm 42 removed `tx_eip7702_state_refund` / `tx_eip7702_auth_refund`, so the + /// invariant can no longer be written as "regular == total"; asserting the full + /// per-auth delta against revm's own constants is the equivalent that survives. #[test] fn test_eip7702_refund_stays_regular_for_morph_specs() { for spec in [SpecId::CANCUN, SpecId::PRAGUE, SpecId::OSAKA] { let params = GasParams::new_spec(spec); - assert_eq!( - params.tx_eip7702_state_gas_bytecode(), - 0, - "spec={spec:?}: Morph must not route EIP-7702 refunds to state gas" - ); let expected_auth_refund = if spec.is_enabled_in(SpecId::PRAGUE) { - 12500 + eip7702::PER_EMPTY_ACCOUNT_COST - eip7702::PER_AUTH_BASE_COST } else { 0 }; assert_eq!( params.tx_eip7702_auth_refund_regular(), expected_auth_refund, - "spec={spec:?}: Morph EIP-7702 refunds must match expected regular refund" + "spec={spec:?}: the full EIP-7702 per-auth refund must stay in the regular refund" ); } } diff --git a/crates/node/Cargo.toml b/crates/node/Cargo.toml index 820763f..7f707e7 100644 --- a/crates/node/Cargo.toml +++ b/crates/node/Cargo.toml @@ -46,7 +46,6 @@ reth-rpc-eth-api.workspace = true reth-transaction-pool.workspace = true reth-tracing.workspace = true reth-trie.workspace = true -reth-trie-db.workspace = true # Alloy alloy-consensus.workspace = true diff --git a/crates/node/src/validator.rs b/crates/node/src/validator.rs index b33864a..a3b4b50 100644 --- a/crates/node/src/validator.rs +++ b/crates/node/src/validator.rs @@ -164,6 +164,11 @@ fn strict_morph_tree_config(tree_config: reth_node_api::TreeConfig) -> reth_node /// State execution, caching and trie maintenance remain entirely upstream. This /// wrapper preserves the parent-aware L1 queue invariant and the optional /// consensus-layer withdraw-trie-root cross-check. +/// +/// Every [`EngineValidator`] method must be forwarded to `inner`, including the +/// ones that carry a default body in the trait: a missing forward silently +/// replaces upstream behaviour with the empty default instead of failing to +/// compile. Re-check this impl against the trait on every reth upgrade. pub struct MorphTreeEngineValidator where Evm: ConfigureEvm, @@ -348,6 +353,10 @@ where self.inner.on_inserted_executed_block(block) } + fn on_canonical_head_changed(&self, hash: B256, state: &EngineApiTreeState) { + self.inner.on_canonical_head_changed(hash, state); + } + fn payload_builder_resources( &self, parent_hash: B256, diff --git a/crates/revm/src/handler.rs b/crates/revm/src/handler.rs index ce04f66..6aac1ac 100644 --- a/crates/revm/src/handler.rs +++ b/crates/revm/src/handler.rs @@ -7,7 +7,10 @@ use revm::{ Cfg, ContextTr, JournalTr, Transaction, result::{EVMError, ExecutionResult, InvalidTransaction}, }, - context_interface::{Block, journaled_state::account::JournaledAccountTr, result::ResultGas}, + context_interface::{ + Block, cfg::gas_params::Eip2780TxInfo, journaled_state::account::JournaledAccountTr, + result::ResultGas, + }, handler::{EvmTr, FrameTr, Handler, MainnetHandler, post_execution, pre_execution, validation}, inspector::{Inspector, InspectorHandler}, interpreter::{Gas, GasTracker, InitialAndFloorGas, interpreter::EthInterpreter}, @@ -262,6 +265,16 @@ where let disable_eip7623 = cfg.is_eip7623_disabled(); let is_amsterdam_eip8037 = cfg.is_amsterdam_eip8037_enabled(); let tx_gas_limit_cap = cfg.tx_gas_limit_cap(); + // Derive the EIP-2780 intrinsic-gas info the same way revm's own handler does + // rather than hardcoding `None`. Every Morph hardfork maps below AMSTERDAM today + // (asserted by `test_morph_hardforks_do_not_enable_amsterdam_state_gas`), so this + // is `None` in practice — but a hardcoded `None` would silently diverge from + // upstream intrinsic gas the moment that mapping changes. + let eip2780 = cfg.is_amsterdam_eip2780_enabled().then(|| Eip2780TxInfo { + value: tx.value(), + // Self-transfer: a `Call` whose recipient is the sender itself. + is_self_transfer: tx.kind().to() == Some(&tx.caller()), + }); // For L1 message transactions, handle intrinsic gas specially if tx.is_l1_msg() { @@ -274,7 +287,7 @@ where disable_eip7623, is_amsterdam_eip8037, tx_gas_limit_cap, - None, + eip2780, ) .unwrap_or_else(|_| InitialAndFloorGas::new(tx.gas_limit(), 0)); @@ -288,7 +301,7 @@ where disable_eip7623, is_amsterdam_eip8037, tx_gas_limit_cap, - None, + eip2780, ) .map_err(MorphInvalidTransaction::EthInvalidTransaction)?; @@ -814,6 +827,10 @@ where let mut h = MorphEvmHandler::::new(); let init_and_floor_gas = InitialAndFloorGas::new(0, 0); let mut gas = h.tx_gas(evm, &init_and_floor_gas); + // `execution` owns this checkpoint: it commits once the runtime gas phase is done, or + // unwinds to it when that phase runs out of gas. The `None` arm is only reachable + // under EIP-2780 (AMSTERDAM), which Morph never enables, so it is unreachable today; + // it is kept faithful to upstream so a future hardfork mapping cannot silently skip it. let checkpoint = evm.ctx().journal_mut().checkpoint(); match h.execution(evm, checkpoint, &mut gas)? { Some(res) => Ok(res), From 322ad1a18308cf8a8c2637f0abde8819a5b1bcbe Mon Sep 17 00:00:00 2001 From: panos Date: Mon, 7 Sep 2026 15:32:58 +0800 Subject: [PATCH 2/2] refactor(revm): drop custom SLOAD/SSTORE overrides in favour of native instructions (#194) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sload_morph` and `sstore_morph` existed to undo an `original_value` clobber in revm, not to express any Morph rule. `sstore_morph` in particular re-implemented the whole upstream SSTORE lifecycle — static-call check, reentrancy sentry, Berlin cold loading, dynamic gas and refund accounting — to insert one repair in the middle of it. Why they were needed: token fee deduction leaves the caller's fee-token slot with the DB-committed `original_value`, the fee-deducted `present_value` and `is_cold = true`. That is the same triple morph-geth has, where the EIP-2200 committed value (`GetCommittedState`) and the EIP-2929 access list are independent. revm <= 41 conflated them in `EvmStorageSlot::mark_warm_with_transaction_id`, re-baselining `original_value = present_value` whenever a slot was cold — including a slot marked cold within the same transaction. EIP-2200 then saw a clean slot and charged `SSTORE_RESET` (2900) instead of the dirty-slot 100, diverging from morph-geth by 2800 gas per write. Why they are no longer needed: bluealloy/revm#3746 (released in revm-state 42.0.0, still present in 43) keys that re-baseline on the transaction boundary instead of on the cold flag, so the committed value survives an explicit `mark_cold()`. The native instructions now produce geth-equivalent gas on their own. Verified as a controlled comparison against the golden e2e test: revm 41 + overrides 48_128 (previous state) revm 41 - overrides 50_928 (+2800: the divergence these overrides hid) revm 42 - overrides 48_128 (this commit) The middle row also confirms `morph_tx_v0_token_fee_transfer_to_fee_token_contract_gas_regression` is a real tripwire rather than a permanently green assertion, so it is left untouched as the gate for this change. Removing the copies also retires three drifts that had already accumulated in them: - the EIP-8037 branch of upstream `sstore_default_gas_accounting` (`state_gas!` plus `refill_reservoir`) was never mirrored; dormant only because every Morph hardfork maps below AMSTERDAM. - the SSTORE cold-load skip threshold stayed at revm 41's `cold_storage_additional_cost()` (2000) after upstream 42 moved it to `cold_storage_cost()` (2100); unreachable because the Istanbul reentrancy sentry already requires `remaining > call_stipend` (2300). - the `original_value` repair ran for every cold slot, including slots of an account created within the same transaction, where revm reports `ZERO` while the DB may still hold residual storage. `blockhash_morph` (0x40) and the disabled SELFDESTRUCT / BLOBHASH / BLOBBASEFEE opcodes stay: those are genuine Morph semantics, unrelated to revm's version. Closes #187 --- crates/revm/src/evm.rs | 187 ++++------------------------------------- 1 file changed, 16 insertions(+), 171 deletions(-) diff --git a/crates/revm/src/evm.rs b/crates/revm/src/evm.rs index 10d7fe5..a096969 100644 --- a/crates/revm/src/evm.rs +++ b/crates/revm/src/evm.rs @@ -8,21 +8,15 @@ use morph_chainspec::hardfork::MorphHardfork; use revm::{ Context, Inspector, context::{CfgEnv, ContextError, Evm, FrameStack, Journal}, - context_interface::host::LoadError, handler::{ EthFrame, EvmTr, FrameInitOrResult, FrameTr, ItemOrResult, instructions::EthInstructions, }, inspector::InspectorEvmTr, interpreter::{ Host, Instruction, InstructionContext, InstructionExecResult, InstructionResult, - gas::{BLOCKHASH, WARM_STORAGE_READ_COST}, - interpreter::EthInterpreter, - interpreter_types::{RuntimeFlag, StackTr}, - }, - primitives::{ - BLOCK_HASH_HISTORY, - hardfork::SpecId::{BERLIN, ISTANBUL}, + gas::BLOCKHASH, interpreter::EthInterpreter, interpreter_types::StackTr, }, + primitives::BLOCK_HASH_HISTORY, }; /// The Morph EVM context type. @@ -83,159 +77,6 @@ fn blockhash_morph( Ok(()) } -/// Morph custom SLOAD opcode. -/// -/// Fixes `original_value` corruption caused by revm's `mark_warm_with_transaction_id()`. -/// -/// When token fee deduction marks storage slots cold, the main tx's first SLOAD -/// triggers `mark_warm_with_transaction_id()` which resets `original_value = present_value`, -/// losing the true DB-committed original. This makes SSTORE see "clean" slots (2900 gas) -/// instead of "dirty" (100 gas), causing a 2800 gas mismatch vs go-eth. -/// -/// The DB read hits the State cache (O(1)) and only triggers on cold SLOADs. -fn sload_morph( - context: InstructionContext<'_, MorphContext, EthInterpreter>, -) -> InstructionExecResult { - let Some(([], index)) = StackTr::popn_top::<0>(&mut context.interpreter.stack) else { - return Err(InstructionResult::StackUnderflow); - }; - - let target = context.interpreter.input.target_address; - let key = *index; - - let additional_cold_cost = context.host.gas_params().cold_storage_additional_cost(); - let skip_cold = context.interpreter.gas.remaining() < additional_cold_cost; - let res = context.host.sload_skip_cold_load(target, key, skip_cold); - - match res { - Ok(storage) => { - if storage.is_cold { - // Read the true committed value from DB (hits State cache, O(1)). - // This matches go-eth's GetCommittedState() returning the un-modified DB value. - let db_original = context.host.journaled_state.database.storage(target, key); - if let Ok(db_original) = db_original - && let Some(acc) = context.host.journaled_state.inner.state.get_mut(&target) - && let Some(slot) = acc.storage.get_mut(&key) - && slot.original_value != db_original - { - slot.original_value = db_original; - } - - if !context - .interpreter - .gas - .record_regular_cost(additional_cold_cost) - { - return Err(InstructionResult::OutOfGas); - } - } - - *index = storage.data; - } - Err(LoadError::ColdLoadSkipped) => return Err(InstructionResult::OutOfGas), - Err(LoadError::DBError) => return Err(InstructionResult::FatalExternalError), - } - Ok(()) -} - -/// Morph custom SSTORE opcode. -/// -/// Twin of [`sload_morph`]: revm's standard SSTORE warms a cold slot through -/// the same `mark_warm_with_transaction_id()` path as SLOAD, so forced-cold -/// token-fee slots need the same `original_value` restoration before -/// `sstore_dynamic_gas()` reads it for EIP-2200 accounting. -/// -/// Without this, a main tx that writes a fee-deducted slot WITHOUT first -/// SLOADing it sees a "clean" slot (2900 gas SSTORE_RESET, no refund) -/// instead of a "dirty" slot (100 gas SLOAD_GAS plus refund), causing the -/// same 2800-gas-per-write divergence vs go-eth that `sload_morph` fixes. -/// -/// Uses DB-direct lookup (no per-tx runtime map needed). -fn sstore_morph( - context: InstructionContext<'_, MorphContext, EthInterpreter>, -) -> InstructionExecResult { - if context.interpreter.runtime_flag.is_static() { - return Err(InstructionResult::StateChangeDuringStaticCall); - } - - let Some([index, value]) = StackTr::popn::<2>(&mut context.interpreter.stack) else { - return Err(InstructionResult::StackUnderflow); - }; - - let target = context.interpreter.input.target_address; - let spec_id = context.interpreter.runtime_flag.spec_id(); - - if spec_id.is_enabled_in(ISTANBUL) - && context.interpreter.gas.remaining() <= context.host.gas_params().call_stipend() - { - return Err(InstructionResult::ReentrancySentryOOG); - } - - if !context - .interpreter - .gas - .record_regular_cost(context.host.gas_params().sstore_static_gas()) - { - return Err(InstructionResult::OutOfGas); - } - - let mut state_load = if spec_id.is_enabled_in(BERLIN) { - let additional_cold_cost = context.host.gas_params().cold_storage_additional_cost(); - let skip_cold = context.interpreter.gas.remaining() < additional_cold_cost; - match context - .host - .sstore_skip_cold_load(target, index, value, skip_cold) - { - Ok(load) => load, - Err(LoadError::ColdLoadSkipped) => { - return Err(InstructionResult::OutOfGas); - } - Err(LoadError::DBError) => { - return Err(InstructionResult::FatalExternalError); - } - } - } else { - let Some(load) = context.host.sstore(target, index, value) else { - return Err(InstructionResult::FatalExternalError); - }; - load - }; - - // Morph fix: on cold access, restore original_value from the DB-committed value. - // Mirrors sload_morph. Only fires on cold path; zero overhead on warm SSTOREs. - if state_load.is_cold { - let db_original = context.host.journaled_state.database.storage(target, index); - if let Ok(db_original) = db_original - && state_load.data.original_value != db_original - { - state_load.data.original_value = db_original; - if let Some(acc) = context.host.journaled_state.inner.state.get_mut(&target) - && let Some(slot) = acc.storage.get_mut(&index) - { - slot.original_value = db_original; - } - } - } - - let is_istanbul = spec_id.is_enabled_in(ISTANBUL); - let dynamic_gas = context.host.gas_params().sstore_dynamic_gas( - is_istanbul, - &state_load.data, - state_load.is_cold, - ); - if !context.interpreter.gas.record_regular_cost(dynamic_gas) { - return Err(InstructionResult::OutOfGas); - } - - context.interpreter.gas.record_refund( - context - .host - .gas_params() - .sstore_refund(is_istanbul, &state_load.data), - ); - Ok(()) -} - /// MorphEvm extends the Evm with Morph specific types and logic. #[derive(Debug, derive_more::Deref, derive_more::DerefMut)] #[expect(clippy::type_complexity)] @@ -282,22 +123,26 @@ impl MorphEvm { let precompiles = MorphPrecompiles::new_with_spec(spec); let mut instructions = EthInstructions::new_mainnet_with_spec(spec.into()); + // SLOAD (0x54) and SSTORE (0x55) are deliberately NOT overridden. + // + // Token fee deduction leaves the caller's fee-token slot with the DB-committed + // `original_value`, the deducted `present_value` and `is_cold = true` — the same + // triple morph-geth has, where EIP-2200 `GetCommittedState` and the EIP-2929 + // access list are independent. revm <= 41 conflated the two and re-baselined + // `original_value` whenever a slot was cold, which forced Morph to hand-write both + // opcodes just to restore it. bluealloy/revm#3746 (revm-state 42) keys that + // re-baseline on the transaction boundary instead, so the native instructions now + // produce geth-equivalent gas on their own. + // + // `morph_tx_v0_token_fee_transfer_to_fee_token_contract_gas_regression` is the + // tripwire: it fails by exactly 2800 gas if that stops holding. + // Morph custom BLOCKHASH implementation (matches Morph geth). instructions.insert_instruction( 0x40, Instruction::new(blockhash_morph::), BLOCKHASH as u16, ); - // Morph custom SLOAD: fixes original_value corruption from token fee deduction. - instructions.insert_instruction( - 0x54, - Instruction::new(sload_morph::), - WARM_STORAGE_READ_COST as u16, - ); - // Morph custom SSTORE: same original_value fix on the SSTORE cold path. - // Static gas = 0 because sstore_morph manages all gas accounting itself - // (static + dynamic + refund). - instructions.insert_instruction(0x55, Instruction::new(sstore_morph::), 0); // SELFDESTRUCT is disabled in Morph instructions.insert_instruction(0xff, Instruction::unknown(), 0); // BLOBHASH is disabled in Morph