Conversation
…efund morph-geth runs the ERC20 fee `transfer()` for slotless fee tokens through `evm.Call` inside `buyAltTokenGas()`, before `StateDB.Prepare`. `Prepare` resets the access list and transient storage but not the refund counter, so any SSTORE refund the token contract earns during that protocol call (e.g. clearing the payer's balance slot) is settled against the user's own gas in `refundGas()`. morph-reth ran the same call with a frame-local `Gas` and dropped its refund, so the two clients disagreed on `gasUsed` whenever the fee transfer touched a refundable slot: - a 21000-gas value transfer whose fee clears the payer's balance settles at 16800 on geth and 21000 on reth; - if the main call then hands tokens back to the payer, geth nets +4800 against -4800 and refunds nothing, while reth's main frame ended at -4800 and revm's final-refund cast turned that into the maximum `gas_used / 5` refund (30974 vs 24780). Both paths are reachable on mainnet: token ids 2 and 6 in the L2 token registry have no `balanceSlot` and take the EVM-call path. Record the net refund returned by the fee transfer frame on `MorphEvm` and fold it into the transaction's refund counter before the EIP-3529 cap, matching go-ethereum's accounting.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for this — it is a well-diagnosed bug with a clean, focused fix, and pinning the geth numbers with a standalone It reaches the state root. Disabling the refund carry-over in morph-reth and replaying PR #200's geth-derived fixture ( The trigger is live on mainnet. Mainnet One overlap to settle: PR #200 ( |
|
Thanks for reproducing it and for taking it through the state-root fixture — I had only pinned the On the overlap: my suggestion would be to land #207 first as the standalone consensus fix and rebase #200 on top of it, dropping the That said, if you'd rather keep everything in #200 since it's already carrying the geth-derived fixture, I'm fine closing this one — just say the word. |
|
Thanks — this was a correct diagnosis, and it is why the call path ended up fixed properly rather than patched. Your fix is fully absorbed by #210 ( I measured your fix on its own, against the geth golden fixtures, since that is the clearest way to state what it does. Applied to One caveat so you know exactly what became of your tests: your two named unit tests were not transplanted verbatim — there is no 30_974 assertion in #210, and scenario 2 is pinned by the fixture instead. Say the word and we will port both forms in. Why it landed as #210 rather than #207. The remaining four templates are three further divergences on the same path, none of which your report could have surfaced:
All four were state-root, receipts or admission divergences on the same path. #210 is Why it is timely. The plan is to move every mainnet fee token to the call path, which makes this path universal rather than two tokens (ids 2 and 6, both USDC, today). Unmodified v1.3.0 rejects blocks containing such transactions, so the fix has to reach nodes before the registry entries are flipped. So we would like to close this as superseded — with thanks. Your report is what surfaced the rest of it. |
|
Closing as superseded by #210, which absorbs this fix along with the rest of the call-path divergences found while investigating it. Thanks again for the report — it is what surfaced them. |
* fix(revm): align call-mode fee-token execution with go-ethereum A fee token registered without a `balanceSlot` takes the EVM-call path: the protocol resolves the payer's balance with `balanceOf` and moves the fee with an ERC20 `transfer`. morph-reth diverged from morph-geth on that path in ways that change `gasUsed`, receipts and state roots, so a follower rejects blocks a geth block producer accepts. Measured against the golden fixtures added in the next commit, main fails 8 of 12 case templates on both Emerald and Jade: - The fee `transfer()` frame's SSTORE refund was discarded, so a fee that clears the payer's balance slot earned the user no refund (the receipt `gasUsed` mismatch reported in #207). go-ethereum runs that call through `evm.Call` inside `buyAltTokenGas()` before `StateDB.Prepare`, which does not reset the refund counter, so the refund reaches `refundGas()`. The net counter is now carried on `MorphEvm` and recorded before the EIP-3529 cap, so it can also be cancelled by a negative refund from main execution. - Internal calls ran with a default transaction environment and their own storage, so `balanceOf`/`transfer` saw ORIGIN = 0x0 and an effective gas price of 0. A guard on either read the wrong value and the payer's balance resolved to zero, rejecting an affordable transaction. Internal frames now keep the outer transaction's ORIGIN and GASPRICE, run in the executing block's environment, and `balanceOf` executes as a genuine static frame under the same 200k gas allowance geth uses. - A successful transfer whose return value or balance delta failed a later business check was rolled back with its logs. go-ethereum keeps the state and logs and only reports the failure. The frame now owns its checkpoint: a VM revert still rolls back, a business failure does not, and a database failure stays fatal rather than becoming a verdict about the token. - Zero fees skipped neither `transfer(0)` nor the initial `balanceOf`; geth skips both transfer modes but still performs the balance query. Nonce and cache updates, and call-mode access-list/transient cleanup, still happen. `TokenFeeInfo::effective_fee_limit` replaces the two hand-rolled fee-limit clamps the execution and pool paths each carried, so they cannot drift. `MorphEvm::from_env` gives execution and pool queries one constructor. The receipt builder now reads registry metadata with `load_storage_only`, which never builds a temporary EVM to resolve a balance it does not use. The pool's single call site is adapted to the new `load_for_caller` signature while keeping its previous behaviour: admission still evaluates a call-mode `balanceOf` under the hardfork's defaults. Threading the real head environment through admission is txpool work and is deliberately not part of this change. * test(statetest): add geth-derived golden fixtures for fee-token calls Twelve cases across Emerald and Jade, with state roots, logs roots and transaction gas generated by morph-geth 5744b8f66. Every case registers its token with `balanceSlot = 0`, so all twelve exercise the EVM-call path: deduction clearing the payer's ERC20 balance slot, the one-unit balance control, main-frame revert and OOG, a main call that restores the cleared slot, negative-refund cancellation, ORIGIN and GASPRICE guards, static-call violations, a successful transfer whose refund returns false, a refund that reverts, and zero-fee storage warmth. Note that "balance slot" in these case names is the ERC20's own storage slot that the fee `transfer()` clears, not the registry's optional `balanceSlot` field; the storage-slot fee path is not covered by these fixtures. Replaying them against main (v1.3.0) fails 8 of the 12 templates on both forks. With the preceding commit all 24 outcomes pass. * docs(revm): document the transaction-id invariant behind the mid-tx finalize The call-mode fee path commits its deduction by calling `evm.finalize()` mid-transaction, then re-marks every account and slot cold to reproduce the warmth go-ethereum's `StateDB.Prepare` would leave behind. Nothing explained why the whole journal is discarded there, or which part of that reset the correctness depends on. The load-bearing property is that the reset must not advance the transaction id. Warming a slot runs through `EvmStorageSlot::mark_warm_with_transaction_id`, which re-baselines the EIP-2200 `original_value` to the present value whenever the slot's id differs from the journal's. Had that fired on the slot the deduction just cleared, the main frame's SSTORE would be a create rather than a recreate and the `SubRefund` cancelling the deduction's `+4800` would be lost — measured on `main_restores_cleared_slot`, 23_291 gas becomes 38_391. It cannot fire here because ids stay equal across execution. revm advances the id only when a transaction finishes — `commit_tx()` from `execution_result`, or `discard_tx()` on the error path — both after the main frame; `ExecuteEvm::finalize` then resets it to ZERO before the next transaction. `finalize()` at this point is therefore idempotent for the id, while `commit_tx()` would leave the deduction-warmed slots holding 0 against a journal holding 1. Verified by substitution: swapping `commit_tx()` in fails `main_restores_cleared_slot` with a state root mismatch. * fix(revm): return the fee frames' shared memory to the transaction The fee-token frames are top-level frames that run in the middle of a transaction, so neither of revm's truncation points covers them: `free_child_context` only releases a child frame's region, and `LocalContext::clear` only runs once the whole transaction is done. The frames therefore left their bytes on the context's shared buffer and the main transaction frame started on top of them. Measured on a call-mode MorphTx: the main frame entered with `MSIZE == 32`, and `MLOAD(0)` returned 9_000_000, the payer's post-fee token balance left behind by the internal `balanceOf`. go-ethereum allocates a fresh `Memory` for every interpreter run (core/vm/interpreter.go), so both read zero there, and both read zero here on the ETH-fee control. Any contract that reads memory it never wrote, or branches on MSIZE, produced a different result on morph-reth than on morph-geth. Carve each fee frame's memory out above whatever the buffer already holds, and release it on the way out, including on the error path. The frames keep writing into the context's buffer rather than one of their own: a nested call hands its callee a `CallInput::SharedBuffer` range, and while a contract callee resolves that range against its own frame memory, a precompile callee resolves it against the context's buffer (`CallInput::as_bytes`). A private buffer would hand every precompile called from a fee frame empty calldata — `fee_token_frames_reach_a_precompile_through_memory` fails with `InsufficientTokenBalance { available: 0 }` under that variant. Gas is unaffected, since memory expansion is charged from the per-frame `Gas` counter, and the leak did not cross transactions, since `local_mut().clear()` runs at the end of each one. The 24 geth-derived golden fixtures cannot see any of this: ten of their twelve templates call a codeless EOA and the other two call bytecode that writes before it reads. * docs(revm): correct what the pool's balanceOf query aligns with The comment justified setting the query's ORIGIN to the queried account by claiming go-ethereum's pool does the same. It does not: `getBalanceFunc` builds its EVM on an empty `vm.TxContext{}` (core/tx_pool.go:341), so its ORIGIN is the zero address — and go-ethereum's own execution layer resolves the same `balanceOf` with ORIGIN set to the sender, so its pool disagrees with its own execution. Setting ORIGIN to the account is still the right call, for the opposite reason to the one recorded: admission exists to predict what the builder will be able to include, so it follows this client's execution layer rather than the other client's pool. Say that, and record the one input the query still cannot match — GASPRICE, which stays at the `TxEnv` default of zero because the effective price depends on the next block's base fee. * test(node): run the e2e fee-token path in EVM-call mode Every registered fee token on mainnet now has its `balanceSlot` cleared, so the fee is moved by real `balanceOf` and `transfer` calls into the token contract. The e2e genesis registered `balanceSlot = 2` instead, which put all 128 integration tests on the direct-storage path: the mode that is scheduled to be disabled by a hardfork, and the one production does not use. The node-level behaviour of the mode production does run — receipts, log ordering, pool admission, the replay RPCs — had no integration coverage at all, while the 24 statetest golden fixtures cover only its state effects. Give the test token the ERC20 runtime the gas-regression test already carried inline, and clear the registry's `balanceSlot`. The runtime keeps `balanceOf` at slot 1, so `test_token_balance_slot` still derives the same slot independently and remains a test oracle rather than a second copy of the code under test. Both gas regressions hold unchanged at 48_128 and 50_428: the fee frames run on their own 200k budget, and the deduction books no SSTORE refund while the payer keeps a balance. What does change is the receipt, which now carries the fee deduction and the fee reimbursement around the transaction's own transfer. Assert that ordering — deduction, main, refund — since it is what go-ethereum produces and what indexers read. * fix(revm): make the fee path's load-bearing invariants local and true Six items a review of this branch turned up, each verified against revm 42 and go-ethereum before being acted on. `reimburse_caller_token_fee`'s slot branch reaches `sload`/`sstore` on the token directly, which panic rather than error when the account is absent from `journal.state` (`sload_assume_account_present` -> `ColdLoadSkipped` -> `unwrap_db_error`). It relied on the deduction having loaded it, but the deduction skips both transfer modes for a zero fee. The two cannot disagree today — `eth_to_token_amount` rounds up, so a zero token fee means a zero ETH fee, which returns before the transfer — but that proof lives in another function. Load it where it is needed instead; today the load is a no-op. `evm_call` has the same shape of hidden dependency: its `CallValue::Transfer` frame runs `Journal::transfer_loaded`, whose zero-value path is `self.state.get_mut(&to).unwrap()`. An ordinary CALL is safe because the opcode's `load_acc_and_calc_gas` loaded the account; an internal call has no opcode, so `internal_call_code` is the only load. Its doc comment described itself purely as a warmth-preserving code read. Say what it is also for. `MorphBlockExecutor::hardfork` became write-only when `get_morph_tx_fields` stopped taking a hardfork, leaving a doc comment claiming it is "reused in `commit_transaction`". Removing it leaves `spec` dead as well — it existed only to compute it. Drop both, and the constructor argument with them. Two comments claimed things that are not true of the pinned revm or of the code they describe. `load_token_fee_info` blamed a "30M gas limit" on the previous path, which went through `system_call_one` and so already capped at go-ethereum's 200k; the divergence was the environment and the sender. And `ExecutionResult::Revert` does carry a `logs` field in revm 42 — the fee logs need their side channel because the mid-transaction `finalize()` clears the journal's logs, not because the variant cannot hold them. The pool restated the fee-limit clamp by hand under a "Match REVM semantics" comment, although `TokenFeeInfo::effective_fee_limit` was added to be the one copy. Use it. Finally, `transfer_erc20_with_evm`'s affordability check built its error message with `ok_or`, rendering two U256s and allocating a String on every successful call-mode fee transfer; `ok_or_else` defers it. * test(statetest): cover uninitialized memory and the direct-slot path The twelve golden cases all target either a codeless EOA or bytecode that writes before it reads, and all twelve register the token in EVM-call mode. Two consensus-relevant behaviours were therefore invisible to them. `main_reads_uninitialized_memory` commits MSIZE and MLOAD(0) to storage from the transaction's own frame, before writing either. go-ethereum allocates a fresh `Memory` for every interpreter run, so both read zero and neither SSTORE changes state; a client whose fee frames leave their bytes on the transaction's shared memory writes two non-zero slots and misses the root. Verified to have teeth: reverting the fee frames to a checkpoint-zero `SharedMemory` fails it with a state root mismatch. `slot_deduct_keep` and `slot_deduct_clear` are the first coverage of the registry's direct-slot path, which has to keep working for replaying blocks produced before every mainnet token moved to the call path. `slot_deduct_clear` is byte-for-byte the transaction `deduct_clear` runs and costs 21_000 against its 16_800: clearing the payer's balance through a real `transfer` books a `+4800` SSTORE refund that reaches the transaction, while `SetState` books nothing. That 4_200 is the only way the two modes bill differently, and it is now pinned from both sides. Roots and logs hashes come from morph-geth 4012f174b, which reproduces all twelve existing cases unchanged. * test(node): assert the fee logs survive a reverting main frame `receipt.rs` caches the fee `Transfer` events outside the journal because go-ethereum's `StateDB.logs` is not part of the state snapshot/revert mechanism: when the main frame reverts, the deduction's log must still be in the receipt. Nothing asserted that. Every reverting golden case used a token that emits no logs, so its expected `logs` hash is the empty hash and a client that dropped `pre_fee_logs` on the floor would produce the same value. The property decides the receipt's logs and therefore the block's receipts root. `morph_tx_v0_token_fee_still_charged_on_revert` already reverts the main frame against the real ERC20 test token and already runs through `MorphBlockExecutor` and the production receipt builder. Assert the two fee transfers it must carry, in go-ethereum's order, and that the deduction moved a non-zero fee. Verified to have teeth: not extending `pre_fee_logs` in `build_receipt` fails it. One comment described the code wrongly and is corrected: - The call-mode deduction comment said re-marking accounts and slots cold "reproduces the warmth `Prepare` would have left behind". It does not, and must not: the coinbase and access list are re-warmed later by upstream `pre_execution::load_accounts`, which runs after this deduction because the deduction happens in `validate()`. Say so, and say what breaks if a future change reorders those phases. `load_token_fee_info`'s claim that the old path "capped at `SYSTEM_CALL_GAS_LIMIT`, which is go-ethereum's 200k" reads wrong, because revm's `SYSTEM_CALL_GAS_LIMIT` is 30_000_000. It is right, though: this crate defines its own 200_000 in `exec.rs` and sets it in the `SystemCallEvm` impl, shadowing revm's. Name that shadowing, since the bare constant reads as a mistake and invites exactly the wrong "fix". `expectException` stays presence-only, which reads like an oversight. It is deliberate: go-ethereum's own statetest harness returns early on `len(ExpectException) > 0` under a standing "TODO check error string", so matching the text here would make this runner stricter than the client the fixtures come from. A comment now records that. * refactor(revm): keep the fee-token helpers' invariants inside them Four cleanups from a review of this branch. None of them changes execution. `transfer_erc20_with_slot` needs the token account in `journal.state`, because the journal's `sload`/`sstore` panic rather than error when it is absent, and both callers loaded it themselves with a comment apiece saying why. The helper now loads and touches the token ahead of its checkpoint. That is exactly what the deduction did before. The refund's extra touch is a no-op: a refund only runs after a non-zero deduction, which already touched the token in the same transaction. The slot-path golden fixtures pass unchanged, and the unit test that exercises the helper no longer pre-loads the token. `reimburse_caller_token_fee` built its missing-cache error with `ok_or`, allocating the message on every token refund. It now uses `ok_or_else`. `TokenRegistryEntry`, its `load` and its `load_for_caller` had become `pub` and re-exported with no user outside this crate, and `load_for_caller` hands back a `TokenFeeInfo` without going through `ensure_usable`. They are `pub(crate)` again. The pool keeps using `TokenFeeInfo::load_for_caller`. The handler and token-fee tests each carried an identical database that fails storage reads of one token. A single copy now lives in the token-fee test module, which is `pub(crate)` so the handler tests can use it. * docs(evm): give the real reason fee logs are kept out of the result The receipt builder said fee logs are cached apart from the journal because revm's `ExecutionResult::Revert` carries no logs. In revm 42 it does. The real reason is that the call-mode deduction runs a mid-transaction `finalize()` that clears the journal's logs, so the handler moves them out first, and it drains the refund's logs the same way. `result` then holds only the main frame's logs, which a revert has already discarded. The same wrong claim was corrected in `handler.rs` earlier on this branch; this is the copy that was left behind. * test(statetest): replay a mainnet slot-mode fee-token transaction Slot mode is being retired on mainnet, but blocks that already ran it must keep replaying identically, and only two synthetic golden cases exercised the registry's direct-slot path. The new case replays transaction 0x9ebfdac9040d7c2a8739ffdaae8baf5e7aa22fdb48585f80592de4b4cf39ed44 from block 26836567: a V0 MorphTx paying its fee in token 1 through the direct slot, sent by an EIP-7702-delegated account holding no ETH, whose call transfers that same token. One transaction covers the deduction, the main frame writing the payer's already-debited balance slot, and the slot-mode refund. go-ethereum's state-test runner only signs with `secretKey` and fixes the chain id to 1, so the sender moves to the harness account, carrying its nonce, delegation code and re-keyed token balance, and the fee vault's balance is re-keyed to the harness vault. The prestate tracer reports zero for the balance slots the fee logic reads straight from state, so those, the registry entry and the L1 gas price oracle slots are taken from the parent block. That is exact here because the transaction is alone in its block. Roots come from morph-geth 4012f174b, which passes the fixture, as does this runner. The gas used equals the on-chain receipt's 51_257, and the logs root equals the on-chain logs with the sender topic substituted. Verified to have teeth: swapping the slot-mode refund's direction fails it with a state root mismatch.
2 participants
Summary
For fee tokens registered without a
balanceSlot(EVM-call path), morph-geth and morph-reth disagree ongasUsedwhenever the protocol-level feetransfer()earns an SSTORE refund. This PR makes morph-reth match morph-geth.What morph-geth does
buyAltTokenGas()runs the ERC20transfer()throughevm.CallinsidepreCheck(), i.e. beforeStateDB.Prepare.Prepareresets the access list and transient storage but not the refund counter, so whatever the token contract adds toStateDB.refundduring that call (for example the 4800-gas clearing refund when the payer's balance slot goes to zero) is still there whenrefundGas()computes the user's refund fromst.state.GetRefund().What morph-reth did
transfer_erc20_with_evmexecutes the call with a frame-localGasand discardsframe_result.gas().refunded(), so the main transaction's refund counter starts at zero.Observable divergence
gasUsedgasUsed(before)21000 - min(4800, 21000/5))+4800 - 4800 = 0)-4800; revm'sset_final_refundcasts the negative refund tou64and caps it atgas_used / 5)Both are header
gasUsed/ receipts-root mismatches, so a morph-reth follower rejects the block. It also reaches the state root: the refund changesgas_used, which changes the fee charged and therefore the payer's/coinbase's balances. Replaying PR #200's geth-derived fixture (fee_token_internal_calls.json, golden roots from morph-geth5744b8f66) with the carry-over disabled fails 4 cases on both Emerald and Jade withstate root mismatch(deduct_clear= scenario 1,main_restores_cleared_slot= scenario 2; reproduced by @panos-xyz, see below).The path is live on mainnet: L2TokenRegistry (
0x5300…0021) token ids 2 and 6 havebalanceSlot = 0and take the EVM-call path (both are USDC, FiatTokenV2, active), Jade (jade_fork_time = 1775628000, 2026-04-08) strictly enforces state-root validation, and the trigger (the fee transfer consuming exactly the remaining token balance, controllable viagas_limit/gas_price) is something a user can construct.The morph-geth numbers were confirmed with a standalone
coretest that builds the same slotless token and runsApplyMessage(not included here; happy to share).Fix
Return the net refund recorded by the fee-transfer frame from
transfer_erc20_with_evm, keep it onMorphEvm::pre_fee_gas_refund, andrecord_refundit in therefund()hook beforepost_execution::refundapplies the EIP-3529 cap. The reimbursement-path call is unaffected (geth reads the counter before that transfer runs).Tests
Two unit tests in
crates/revm/src/handler.rsreproduce both scenarios with a hand-assembled slotless ERC20 and assert the morph-geth values (16800 and 30974). Both fail without the fix with exactly the numbers above.cargo fmt --all -- --checkcargo clippy -p morph-revm --all-targets -- -D warningscargo test -p morph-revm -p morph-evmcargo test -p morph-node --features test-utils --test it morph_tx