fix(op-revm): reset BVM_ETH to cold after deposit mint to match op-geth - #27
Merged
Merged
Conversation
Deposit minting writes BVM_ETH via the journal (load_account/sload/sstore), which warms the BVM_ETH account + storage slots for the subsequent EVM execution. op-geth mints via StateDB.SetState(), which never touches the EVM access list, so it stays cold. This warm/cold divergence was patched over with a static BVM_ETH_MINT_GAS_COMPENSATION (4500) gated on `!tx.input().is_empty()`. That heuristic mis-fired for non-empty-calldata deposits to EOAs (hoodi-qa2 block 59294: reth 25628 vs geth 21320 -> receiptsRoot/blockHash fork), and over/under-compensated other access paths since the real cold-access cost is dynamic. Root-cause fix: after process_eth_deposit, reset BVM_ETH (account + touched storage slots) back to EIP-2929 cold via a new JournalColdExt, so the EVM observes it exactly like op-geth (cold then falls back to the tx access list / to / precompiles). The static compensation constant and the refund-stage hack are removed entirely. Scope: op-revm only, no core revm changes. The JournalColdExt bound is carried by OpContextTr so no per-call-site threading is needed. Tests: process_eth_deposit_leaves_bvm_eth_cold (mechanism); deposit_to_eoa_with_calldata_no_compensation_matches_geth (block 59294 replay asserts 21320, not 25628); existing mainnet fixture 89718944 (L2StandardBridge) still passes. cargo test -p op-revm: all green.
…ATE, nested Builds on top of the JournalColdExt fix with 13 additional tests that broaden coverage beyond the original 7. Groups added: Cold-state matrix completion (process_eth_deposit scenarios not in the base PR): * process_eth_deposit_transfer_only_self_no_storage_writes — eth_tx_value with from==to; transfer_inner early-returns, no slots written. * process_eth_deposit_transfer_only_distinct_cools_balance_slots — eth_tx_value with from!=to; balance[from] + balance[to] cooled. * process_eth_deposit_mint_only_flag_with_both_values_skips_transfer — mint_only=true with both values; balance[to] not in storage. End-to-end gas regression (paths the prior 4500 heuristic mishandled): * deposit_to_contract_no_bvm_eth_access_no_overcharge — contract target with STOP body; gas_used does not include the stale 4500. * deposit_direct_to_bvm_eth_stop_body_no_overcharge — tx.to = BVM_ETH with STOP stub; verifies cooling doesn't break EIP-2929 tx.to pre-warming. * deposit_direct_to_bvm_eth_sload_pays_cold_cost — tx.to = BVM_ETH with `PUSH1 2; SLOAD; POP; STOP` stub; verifies the SLOAD pays cold cost (~2100), proving the slot was cooled by JournalColdExt. * deposit_nested_call_to_bvm_eth_pays_cold_cost — outer contract CALLs BVM_ETH (cold account, 2600) which then SLOADs (cold storage, 2100). The fixture 89718944 covers this path with real bytecode; this is its synthetic unit-level companion. Revert paths: * deposit_with_target_revert_no_overcharge — target REVERT body; asserts ExecutionResult::Revert and gas_used < 22000 (the prior refund-stage 4500 was applied even on revert paths). * deposit_with_target_revert_pre_mint_persists — same shape, asserts pre-EVM BVM_ETH mint persists in journal per OP deposit spec. * catch_error_full_revert_then_remint_cooling_idempotent — simulates the handler::catch_error sequence (full checkpoint_revert + re-mint via mint_only=true) and asserts cooling is reapplied. * cooling_restored_after_inner_frame_warms_then_reverts_slot — inner EVM frame sloads (warms) a cooled BVM_ETH slot then reverts; JournalEntry::StorageWarmed::revert restores cold. CREATE deposits (TxKind::Create, the create-derived destination branch in transfer_inner): * process_eth_deposit_create_transfer_cools_create_address_slot — eth_tx_value only; balance[caller] + balance[caller.create(nonce)] cooled. * process_eth_deposit_create_mint_and_transfer_cools_all_slots — both values; total_supply + balance[caller] + balance[caller.create(nonce)] cooled. All 13 use the existing PR naming convention. A small inspector helper `assert_bvm_eth_and_slots_cold` reads cold flags directly off the journaled state (no sload side-effects), making multi-slot assertions in one pass. cargo test -p op-revm: 121 passing (108 + 13).
pandainzoo
approved these changes
Jun 3, 2026
byteflyfunny
approved these changes
Jun 3, 2026
…h production The 7 end-to-end deposit gas/behavior tests were pinned to OpSpecId::ISTHMUS (eth_spec PRAGUE), but block 59294 — and all Mantle hoodi-qa2 deposits — execute under OpSpecId::ARSIA (eth_spec OSAKA). ISTHMUS exercises a different EVM gas regime and leaves the is_arsia branches uncovered. Switch deposit_to_eoa_with_calldata_no_compensation_matches_geth (block 59294 replay) plus the 6 other end-to-end gas tests to ARSIA so they validate under the deployed spec. ARSIA superset of OSAKA, so one ARSIA spec covers the Osaka EVM rules plus Mantle ARSIA features. Mechanism/cold-flag tests and the mainnet fixture replays (path_1/2/3, spec-bound to their historical blocks) stay on ISTHMUS. cargo test -p op-revm: 121 passed, 0 failed (gas values unchanged; 21320 floor holds under Osaka).
byteflyfunny
added a commit
that referenced
this pull request
Aug 4, 2026
Resolves the divergence between the v98-based BVM_ETH fixes on `main` (PRs #27, #29, #31) and their v107-based rework on this branch (PRs #28, #32, #33, #36). Both lines implement the same op-geth parity semantics against different revm baselines, so `op-revm/handler.rs` and `transaction/bvm_eth.rs` conflicted broadly. Resolution principle: keep the v107 code shape (this branch), but ensure every semantic fix and every test present on `main` survives the merge. Conflict resolution (38 hunks across 7 files) - context/interface/result.rs: keep `gas: ResultGas` + logs on all three variants (upstream bluealloy#3413/bluealloy#3424). Retain main's `serde(default)` on `Halt.logs` so pre-existing serialized halts still decode, and its rationale doc. `logs()`/`into_logs()` now also surface Revert logs, which v107 carries and main's enum could not. - handler/post_execution.rs: keep the upstream path that forwards `take_logs()` into Halt. This is what lets a failed deposit's pre-frame BVM_ETH mint/transfer logs survive without main's checkpoint_revert-and-rebuild. - op-revm/handler.rs: take this branch. Its `execution_result` relabels the failed deposit in place instead of reverting and re-applying mint/transfer/nonce, avoiding the divergence risk of rebuilding state. main's test set here is a strict subset of this branch's. - op-revm/transaction/bvm_eth.rs: impl region verified byte-identical to this branch's pre-merge version. Ported main's 13 JournalColdExt cooling tests (PR #27), which had no counterpart here — cold/warm state drives gas and therefore the state root. - examples/block_traces: took main's per-receipt reconciliation (status/cumGas/logs/bloom vs on-chain) and range summary, ported to the v107 `tx_gas_used()`/`into_logs()` API, and folded the previous per-tx gas check back in as an extra `gas` field. - examples/custom_precompile_journal, ee-tests: v107 API shape only. The ported cooling tests pass against this branch's v107 implementation with no assertion changes — only `..Default::default()` added for the new `AccountInfo::account_id` field — which independently confirms the two cooling implementations are semantically equivalent. Verification - cargo test -p op-revm: 138 passed, 0 failed (125 + 13 ported) - cargo test --workspace --exclude revm-ee-tests: 411 passed, 0 failed - cargo clippy --workspace --all-targets: no warnings - cargo fmt --all --check: clean
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BVM_ETH deposit minting writes through the journal (
load_account/sload/sstore), which warms the BVM_ETH account + storage slots for the subsequent EVM execution. op-geth instead mints viaStateDB.SetState(), which never touches the EVM access list, so BVM_ETH stays cold. This warm/cold divergence was previously patched over with a staticBVM_ETH_MINT_GAS_COMPENSATION(4500) applied inrefund(), gated on(eth_value || eth_tx_value).is_some() && !tx.input().is_empty().That heuristic is unsound: it mis-fires for non-empty-calldata deposits whose target is an EOA (no EVM code runs, so BVM_ETH is never accessed), and it cannot match the dynamic cold-access cost of contract paths. On hoodi-qa2 block 59294 a deposit with
input=0xdeadbeef01020304to an EOA produced reth 25,628 vs geth 21,320 →receiptsRoot/blockHashfork (reth halted at 59365).Root-cause fix
After
process_eth_deposit, reset BVM_ETH (account + touched storage slots) back to EIP-2929 cold via a newJournalColdExt::mark_address_cold, so subsequent EVM execution observes it exactly like op-geth. The cold check then falls back to the tx access list /to/ precompiles, soto == BVM_ETHand access-list cases stay correct. The static compensation constant and the entire refund-stage hack are removed.mark_address_coldonly resets the EIP-2929 warm flags; the minted balance/totalSupply (journaled viatouch_account+StorageChanged) remain committed.Scope
git diffagainstv2.2.2shows onlycrates/op-revm/**).JournalColdExtbound is carried byOpContextTr, so no per-call-site trait-bound threading is needed.Why not just narrow the condition
History shows narrowing the compensation condition (
tx.to == BvmEth::ADDRESS, commit3bac6c55) broke the L2StandardBridge path and was reverted (8b2eb85f). Any static compensation is fundamentally wrong because the real cost depends on the EVM access path; eliminating the warm/cold divergence at the source is the only correct fix.Tests (all green:
cargo test -p op-revm)process_eth_deposit_leaves_bvm_eth_cold— mechanism: after deposit mint, BVM_ETH account + balance slot reportis_cold.deposit_to_eoa_with_calldata_no_compensation_matches_geth— deterministic replay of the hoodi-qa2 block 59294 fork tx; assertsgas_used == 21_320(op-geth EIP-7623 floor), not25_628.test_bvm_eth_deposit, incl. block 89718944 = L2StandardBridge) still pass → confirms the bridge path is unaffected.Notes
main(== tagv2.2.2, the deployed version where the bug lives), so the diff is exactly the fix. May need a separate port tomantle-elysium(222 commits ahead).docs/deposit-tx-gas-divergence-investigation.mdin the consuming repo.