Conversation
Upgrade the workspace from revm 27 to revm 40.0.3, with op-revm and alloy-op-evm pinned to the Optimism monorepo revision wired to reth v2.3.0. revm 40 reworked several interpreter and handler internals that MegaETH's frozen specs depend on. This change adapts the integration so that the observable semantics of all existing specs stay identical: - Static-gas pre-charge: step() now deducts each opcode's static gas from a per-spec gas table before the handler runs. Volatile-guarded opcodes get zeroed table entries and charge inside the wrapper at the revm-27 equivalent position; all other opcodes add the pre-charge back into the compute-gas measuring window (STATIC_GAS_TABLE). - Halt handling: limit-triggered halts go through set_halt_action!, which preserves remaining gas for sender refund instead of revm 40's spend_all-on-OOG, and explicitly replaces pending frame actions. - Host entry points: the oracle SLOAD customizations (env override, forced-cold pricing, access marking) moved into sload_skip_cold_load; resident journal entries keep their own cold pricing; access-list warming only applies to entries with storage keys; CALL-family account loads distinguish the raw operand from its EIP-7702 delegate. - System calls: pre-REX5 system calls keep the 30M gas limit via PRE_REX5_SYSTEM_CALL_GAS_LIMIT (the upstream constant drifted to 30M + EIP-8037 reservoir) and continue to skip post_execution. - Precompiles: error paths normalize spent gas back to zero; the KZG fixed-rate compute-gas branch keys on bytecode_address. - CfgEnv: spec changes go through set_spec_and_mainnet_gas_params so GasParams follow the spec; tx_chain_id_check keeps its revm-27 default for untouched configurations (with_cfg_unpinned opts out); EIP-8037 is pinned off in every conversion. - Block executor: commit_transaction failures are latched and surfaced in finish(); state hooks moved onto revm's State; results carry tx_type instead of a cloned envelope. Verified with the full workspace test suite, the EEST state tests, the replay corpus, and a mainnet replay of 1,222 transactions matching on-chain receipts.
…-warm addresses The pre-revm-40 CALL-family host entry resolved the raw operand's EIP-7702 delegate before loading it, and that resolution materialized the operand's journal entry (cold, code hydrated) as a side effect. The subsequent inherited load then hit the resident branch and kept the entry's own coldness, so a Rex6 first touch of an address whose warmth exists only as a preload (a precompile, the coinbase, or an address-only access-list entry) was priced cold. revm 40 resolves the delegate inside the CALL instruction, so the migration replaced the ahead-of-load resolution with phase-based marking of revm's own loads — losing the materialization side effect: the first load saw a fresh entry, honored the pre-warmed sets, and priced warm, 2,500 gas below the frozen Rex6 behavior in both EVM gas and compute gas. Reproduce the materialization in load_account_info_skip_cold_load for a Rex6 CALL-family raw-operand load, before the resident-coldness read. Caught by the cross-spec compute-gas suite's CALLCODE spec-arc test.
RealiCZ
added
spec:stable
Claude review status
✅ Review clean Last reviewed: New this round: 0 finding(s), 0 question(s) · Resolved this round: 0 · Open questions: 0 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
RealiCZ
added
comp:misc
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 2.1%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | rex4/50_txs |
2.8 ms | 1.7 ms | +60.47% |
| ⚡ | rex5/50_txs |
2.8 ms | 1.8 ms | +59.4% |
| ⚡ | rex4/10_txs |
689.4 µs | 493.5 µs | +39.7% |
| ⚡ | op_revm_pinned |
2.9 ms | 2.1 ms | +39.19% |
| ⚡ | rex5/10_txs |
719.9 µs | 523.1 µs | +37.61% |
| ⚡ | op_revm_pinned |
1.5 ms | 1.1 ms | +36.62% |
| ⚡ | revm_pinned |
2.8 ms | 2.1 ms | +36.09% |
| ⚡ | op_revm_pinned |
5.5 ms | 4.1 ms | +33.39% |
| ⚡ | revm_pinned |
1.5 ms | 1.1 ms | +33.32% |
| ⚡ | revm_pinned |
1,162.4 µs | 890.9 µs | +30.47% |
| ⚡ | op_revm_pinned |
1,170.6 µs | 900.1 µs | +30.05% |
| ⚡ | mini_rex/5_mixed_txs |
497.6 µs | 394.5 µs | +26.13% |
| ⚡ | equivalence |
3.3 ms | 2.6 ms | +25.83% |
| ⚡ | rex4/10_txs |
1,220.7 µs | 978.3 µs | +24.78% |
| ⚡ | rex5/10_txs |
1,247.1 µs | 999.8 µs | +24.74% |
| ⚡ | equivalence |
6.2 ms | 5 ms | +24.43% |
| ⚡ | equivalence |
1.7 ms | 1.4 ms | +23.84% |
| ⚡ | revm_pinned |
3.4 ms | 2.8 ms | +22.58% |
| ⚡ | mini_rex/sstore_sload_100 |
384.3 µs | 314.6 µs | +22.15% |
| ⚡ | equivalence |
1.4 ms | 1.1 ms | +21.44% |
| ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing cz/chore/upgrade-revm-40 (0072855) with main (a39bd1e)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88c6b0439d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🧬 Mutation testing — ✅ PASSDiff mutation score: 100.0% (19/19 viable mutants killed)
No new test gaps introduced by this change. 🎉 |
🧬 Mutation testing — ✅ PASSDiff mutation score: 100.0% (100/100 viable mutants killed)
No new test gaps introduced by this change. 🎉 |
flyq
left a comment
flyq
left a comment
There was a problem hiding this comment.
Review summary
Verdict: not mergeable yet — one frozen-spec blocker (modexp) and two silently-accepted frozen-spec drifts that need explicit sign-off — but the migration's foundation is excellent and the hard parts are done right. Everything below was verified against both upstream trees (revm-interpreter 24↔37, revm-handler 8↔20, revm-context 8↔18, revm-precompile 25↔36, revm-state/database 7↔12/15, op-revm 8.1.0 ↔ the pinned git 20.0.0), not just by reading this repo.
Mechanisms verified as faithful re-pins: the zeroed static-gas entries equal the volatile-guarded set exactly per spec, and the add-back arithmetic reproduces revm-27 compute-gas totals with no double-count on any wrapper; set_halt_action! preserves remaining gas (vs revm 40's spend-all-on-OOG) through to try_rescue_gas on all six limit-halt sites, and both interpreter loops skip the fallback halt when an action is set; the three gas-leakage paths and the resource-limit latch protocol survive the handler rework; PRE_REX5_SYSTEM_CALL_GAS_LIMIT correctly freezes 30M against upstream's drift to 31,566,720, and run_system_call still skips validate/pre/post_execution; precompile error paths restore revm-27's no-spend halt shape and bytecode_address keying reproduces revm-27 dispatch (CALLCODE/DELEGATECALL-to-KZG identical); EIP-8037 is pinned off at every conversion, normalized before the untouched-default classification; the block executor's receipt assembly, state-hook-before-apply ordering, pre-block sequence, and interception contract are byte-equal or provably equivalent; and the resident-entry cold-pricing compensation (resident_entry_prices_cold + access_list_preloads_account) faithfully restores revm-27 journal pricing across the full warm/cold matrix — upstream's occupied path now consults the warm sets, and the new rex/precompile_access_coldness.rs + rex/access_list_resident_warming.rs files are what hold that compensation in place. The Rex6 fix commit (88c6b04) is correct on scope (CALL-family RawOperand bracket only, REX6+ only — pre-REX6 never depended on the side effect, and CALLCODE inspecting the caller instead of the target is exactly why the drift surfaced on the CALLCODE spec-arc), idempotency, delegate pricing, and EVM-gas/compute-gas symmetry.
The test migration deserves its own call-out: zero pre-existing expectations changed. A token-level sweep of the full ~8k-line tests diff found no lost constant and no deleted test (958→1020, removals = ∅), every assertion edit is a value-identical API reshape (each verified against the upstream definitions, e.g. gas_used()→tx_gas_used()), and the #358 compute-gas suite + snapshot are byte-identical to main — the source was fixed to match unchanged pins, which is the right direction.
Blocking / decision items are inline comments: the unwrapped modexp OSAKA reorder (blocker, cheap wrapper fix), the CALL-family under-funded-frame detention-mark loss (accepted in a code comment + doc but undisclosed in the PR body — needs sign-off and a replay-corpus scan), the disabled-SELFDESTRUCT halt-reason drift (cheap table fix), and the tx_chain_id_check production-path default (overlaps the open codex P1).
CI notes: the spec-gate survivor is an equivalent mutant — see the inline comment at host.rs:425; a justified mutants/suppressions.toml entry is the honest fix. CodSpeed: the −20% on revm_pinned/op_revm_pinned measures the upstream 27→40 jump itself — those "control lanes" changed meaning in this one PR, while .github/scripts/bench_compare.py:278-281 still documents them as immutable and still lists the deleted revm_latest/op_revm_latest rows (:410); the number to actually track across this PR is the mega-vs-pinned overhead ratio. Non-anchorable minor: dual alloy-eip7928 (0.3.7 via alloy-eips, 0.4.5 via revm-state) in the lockfile — dormant pre-Amsterdam, but it is the one gap in the manifest comment's "single source identity" promise.
Since the PR is [WIP] pending your own semantics review: the per-mechanism list above is exactly what that review would re-derive — suggest spending the remaining effort on the corners still unaudited here: a line-diff of the remaining eth precompiles across revm-precompile 25→36 (the modexp reorder proves upstream slips semantic changes into these files), and FromRecoveredTx/FromTxWithEncoded field-by-field parity.
Merge-order: land #362 first — this PR does not touch spec.rs, and a dry-run cross-merge conflicts only on executor.rs + instructions.rs, so the cheaper rebase is on this side.
🤖 Generated with Claude Code
| false | ||
| } | ||
|
|
||
| fn as_invalid_tx_err(&self) -> Option<&InvalidTransaction> { |
There was a problem hiding this comment.
[minor — needs a mega-reth-side test] Returning None here (and in the second impl below) is defensible — there is genuinely no InvalidTransaction counterpart for a Mega limit rejection — but these impls exist purely for reth's error classification and have zero in-repo callers (they are also this file's codecov-0% lines). If any reth path treats None as "internal error → abort payload building" rather than "drop this tx", a Mega limit rejection gets misclassified, and nothing in this repo would catch it. Worth one integration test on the mega-reth side pinning the intended classification.
The KZG fixed-cost compute-gas arm keys on bytecode_address — the address revm dispatches precompiles by — which diverges from target_address under DELEGATECALL and CALLCODE. The existing suite only exercised CALL, where the two coincide, leaving the dispatch-address choice untested. Add exact-value differential tests for both schemes. The CALLCODE variant access-lists the precompile with a storage key so it loads warm on both specs: REX4's CALLCODE wrapper pre-inspects the call target (cold materialization, +2,500) while REX5's inspects the executing account, and that unrelated arc would otherwise pollute the REX4/REX5 differential. Also refresh a stale comment on the 30M system-call gas-limit test: since the default entry overrides upstream's drifted constant, the literal assertion pins MegaETH's own value, not the upstream default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7ced8add7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sit, delegated load)
…opt EIP-7883 in Rex7 MINI_REX adopted the Osaka / EIP-7883 ModExp schedule through the revm implementation current at the time, which short-circuited zero-base/zero-modulus inputs to the 500-gas minimum before computing the formula cost. revm 40's implementation computes and charges the full formula cost for those inputs, as the EIP text specifies (its multiplication complexity floors at 16), and additionally halts with PrecompileOOG when the forwarded gas covers the minimum but not the formula cost — a receipt-level divergence on every frozen spec that installs the Osaka schedule. Pin the frozen specs (MINI_REX through REX6) to the historical short-circuit with a wrapper that reproduces the old check order — minimum gas, header parse, EIP-7823 size limits, then the short-circuit — and defers every other input to the upstream implementation. REX7 installs upstream's implementation unwrapped, deliberately adopting the EIP-faithful schedule as its first recorded behavior change, documented on the Rex7 upgrade page. EQUIVALENCE (inherited Berlin schedule) is structurally unaffected: the Berlin formula collapses to its flat minimum for these inputs under either ordering. Reported in review by flyq.
… snapshot and publish blocker Review follow-ups without behavior change: - gas-detention.md overpromised that a halting frame's volatile access applies detention immediately. The frozen behavior (identical before and after the migration: the wrapper's cap application is skipped when the opcode body halts) is that the registration survives and the reduced cap binds at the transaction's next volatile-guarded opcode. State the enforcement-point semantics, switch "performs the read" to "registers the access" (the oracle path can bail before the actual load), and make the CALL-family exclusion explicit instead of implied by the opcode list. - MegaEvm::mega_cfg is a construction-time snapshot; document that mutating the context configuration through the mutable deref desyncs it and that reconfiguration means rebuilding the EVM. - The OP-family git pins reject `cargo publish`, blocking the release publish workflow until upstream ships revm-40-compatible registry releases; note it where the pins are declared.
…ity entry points revm 40 flipped the CfgEnv::tx_chain_id_check default from false to true. The migration first answered with a default-shape inference: with_cfg pinned the revm-27 false back only for a configuration that was field-for-field CfgEnv::new_with_spec, and took every configured one at face value. Both review rounds flagged the same cliff: every production embedder configures at least a chain id, so the face-value branch is the one production hits, and the same embedder source that ran without the gate on revm 27 silently inherits it after the upgrade. Make the contract explicit instead of inferred: with_cfg and the deprecated new_with_context pin the gate off unconditionally — they are the compatibility entry points, and every frozen MegaETH spec ran without the gate — while with_cfg_unpinned takes the field as provided for embedders that enable it on purpose. The default-shape inference and its is_untouched_revm_default_cfg helper are gone. Also pin EIP-8037 off in new_with_shared_ext_envs, aligning the one construction path that relied on the upstream default staying pre-Amsterdam. The REX5 system-tx guard tests opt into the gate deliberately; they now re-apply it on the live context after the factory pin. A new factory test pins the production path: a configured EvmEnv comes out of create_evm with the gate off.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2076c8e310
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7027c5c2cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Each oracle `cargo test` call now runs under a wall-clock ceiling. Without one, a mutant that makes the tested path or a test loop forever hangs the whole campaign: no result, no state progress, no journal recovery, and a manual kill can land while the product file is still mutated. - `--test-timeout SECONDS` overrides the ceiling; the default is derived per layer from the clean-tree baseline as max(300s, ceil(5 x layer baseline)), falling back to an absolute 1800s when no baseline wall time exists. - On expiry the child's whole process group is killed (SIGTERM, 10s grace, SIGKILL) so cargo's rustc and test binaries cannot orphan onto the `target/` lock. - The timeout path goes through the normal journal compare-and-restore and the post-restore cleanliness assert. - The result is recorded as killed with kill_kind=timeout / timed_out / timeout_s (additive state fields); the report counts timeouts separately from assertion kills so an over-tight threshold stays visible.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2cffd313b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The gate's 20 timed-out mutants were not non-terminating: every one is detected, but only by tests at the end of nextest's global queue, and the per-mutant test ceiling (3x a baseline measured with the suite running alone) is smaller than the same suite needs when `--jobs $(nproc)` siblings compete for the cores. The runs were cut off mid-queue and reported as inconclusive. - Pin the legacy Osaka ModExp check order and boundaries with unit tests in the lib target, which runs before every integration binary: minimum-gas boundary, each EIP-7823 length limit at and above its edge, and the zero-base/zero-modulus flat charge. These kill 15 of the 16 ModExp mutants within the run's first seconds, eight of which no test covered at all. - Suppress the one equivalent mutant (precompiles.rs:135:49): weakening the first `||` only defers to upstream `osaka_run`, whose EIP-7823 check returns the identical halt. - Size the timeout ceiling for the contended run so a late-queue kill is reported as caught rather than as a timeout, and document the failure mode.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fe0ec4dea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Specification section stated the KZG and ModExp overrides without a lower-bound declaration, silently claiming Equivalence; the code installs them only from MiniRex.
Nothing produces the field: the local corpus never used it, current EEST does not emit it, and it never landed in geth (EIP-7742 was withdrawn); upstream revm's statetest Env has since removed its parent-target field as well. Its raw gas-unit interpretation also contradicted the count the name promises. Excess blob gas derivation now always uses the Cancun target, and deny_unknown_fields rejects any fixture still carrying the field instead of silently misreading it.
RealiCZ
changed the title
revm 40 moved the log list onto every ExecutionResult variant and fills it from the journal, so "a failed transaction's receipt carries no logs" is no longer guaranteed by the type. MegaETH rewrites an already-committed frame result into a failure at two sites — most visibly the pre-REX5 CREATE code-deposit charge in after_frame_run, which runs once the constructor's checkpoint is committed — and those committed logs then reached the receipt, changing the receipts root against on-chain history. Clear them at execution_result, the seam every entry point converges on, so all consumers inherit it. status, gasUsed and the post-state are unchanged. Tests cover the REX post-commit halt, the REX4 frame-local revert rewrite, and the REX5 pre-charge that keeps the exceed ahead of the commit.
Several comments explained an empty log list by saying revm had already rolled the failed frame's logs back. That is false wherever a successful frame's result is rewritten into a failure after its checkpoint was committed: revm committed the frame and never sees the failure, so the logs survive until the transaction result is finalized.
The log strip at the transaction result seam is unconditional only because EIP-7708 is off: once it is active, logs are emitted outside every frame checkpoint and a failed transaction carries them legitimately. Nothing said so, which is the same defect this change set went and corrected elsewhere. Name the dependency at the strip, and list at the spec-mapping test what advancing the mapping would break — EIP-7708 here, and the EIP-7825 gas-limit cap that would reject every system call.
The note added with the strip explained it by EIP-7708, which is not the reason. Upstream added the log fields deliberately, to hand back logs emitted before a failure that had been discarded until then, and marked the change breaking. Say what actually holds instead, and say it as a measurement rather than a derivation: under Prague an ordinary revert and an ordinary halt both arrive with an empty list, because the frame's checkpoint revert emptied the journal long before the result was assembled; the one upstream path that survives a revert is a failing precompile's logs, and no MegaETH precompile emits any. So the only thing left to drop is what a post-commit rewrite stranded. Record both ways that measurement can go stale — a spec mapped past Prague, or a MegaETH precompile that emits logs — so the next reader re-measures instead of trusting the conclusion.
|
Reviewed as a targeted pass over the highest-risk seams ( Genuinely good:
Blocking1. The body says "Marked [WIP]: a manual semantics review of the migration is still in progress", but the title carries no WIP, the PR is not a draft, and it is Should fix2.
Documenting it is good; leaving the latch enforceable only by that comment is not. Anywhere in mega-reth that reads 3. Ordering note for #347. Once static gas moves into |
Brings in #362 (single-source hardfork spec resolution). Conflicts were resolved by keeping the revm 40 API shapes and adding the alias spec rungs on top: - instruction and static gas table construction groups MINI_REX_1 and MINI_REX_2 with their behavior targets under the three-argument EthInstructions::new, and the alias table test also pins the gas table - the block executor keeps the commit-error latch and drops the SystemCaller field this branch had already removed - transact_deploy_sequencer_registry_for takes the plain database like the rest of the file - the partial-ladder tests install the state hook on the State database, where it lives on this branch - the alias table test compares Instruction entries through Debug, as revm 40 wraps the fn pointer in an opaque struct - the modexp suppression is re-anchored to its shifted line
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7be3ef268a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SPEC_ORDER stays the behavior-introducing ladder; the alias rungs are parsed from MegaSpecId::behavior and removed before the source comparison, so the harness enumerates again now that MINI_REX_1 and MINI_REX_2 exist.
| self.spec = cfg.spec; | ||
| self.inner = self.inner.with_cfg(cfg.into_op_cfg()); | ||
| if intent == CfgIntent::Pinned { | ||
| self.inner.cfg.tx_chain_id_check = false; |
There was a problem hiding this comment.
Non-blocking for this PR — but I'd like point 2 tracked and picked up in a follow-up rather than dropped.
with_cfg pins tx_chain_id_check off unconditionally, and create_evm routes through it, so this is the production path too. No live impact that I can find — mega-reth never sets the flag, and its only chain-id enforcement is the pool (transaction-pool/src/validate/eth.rs:480).
Two things:
-
An embedder that explicitly sets
truenow loses it silently.with_cfg_unpinnedisn't reachable fromEvmFactory, though a post-construction write toevm.ctx.cfgdoes stick — which is whatsystem_tx_replay.rsrelies on. Pre-PR the field passed through. Is silently overriding an explicit setting the intent here, as opposed to honoring it or rejecting it loudly? -
Since the pool is the only place this is currently enforced, anything reaching execution without traversing it is unchecked at every layer. Could we enforce it in the EVM from Rex7? It's still the unstable spec, so it can absorb the behavior change that flipping the gate on implies, where the frozen specs can't. I'd leave the shape to you — deposits and the REX5+ system-tx path are the two areas I'd want to see covered.
| self.spec = cfg.spec; | ||
| self.inner = self.inner.with_cfg(cfg.into_op_cfg()); | ||
| if intent == CfgIntent::Pinned { | ||
| self.inner.cfg.tx_chain_id_check = false; |
There was a problem hiding this comment.
Following up on point 2 with something concrete.
If the gate is ever enabled, the REX5+ system-tx guard at execution.rs:113 rejects chain_id: None unconditionally — but revm's canonical check, which that guard's comment says it mirrors, exempts legacy and custom types. MegaETH system txs are TxLegacy, and mega-reth builds them by inheriting tx.chain_id() from the triggering user tx (payload_executor.rs:1141), which is None for a pre-155 legacy tx the pool accepts. So the guard reads as stricter than intended.
Rex7 looks like the right place to fix both together — the guard and the gate. And if it's spec-gated (tx_chain_id_check = spec.is_enabled(REX7)), CfgIntent and with_cfg_unpinned fall out entirely: the spec answers the "did the caller mean this or is it just the upstream default" question that the bool can't. Worth considering where that gate lives — apply_cfg covers all six with_cfg call sites in the repo, on_new_tx covers everything and matches how force_amsterdam_eip8037_off already handles the same problem.
| /// | ||
| /// DB-dependent pre-frame usage may still be recorded later during pre-execution. | ||
| pub(crate) fn on_new_tx(&mut self) { | ||
| force_amsterdam_eip8037_off(&mut self.inner.cfg); |
There was a problem hiding this comment.
Non-blocking — question about the pinning strategy rather than the pin itself.
The doc on force_amsterdam_eip8037_off justifies settling this per-transaction rather than at entry points on two grounds I had trouble reproducing:
- The re-enable path (
set_spec_and_mainnet_gas_paramsORs the flag on for Amsterdam+) can't fire while everyMegaSpecIdmaps to Prague — andtest_all_specs_map_to_isthmus_and_praguealready forces the conversation if that ever changes. If Amsterdam does get mapped, whether EIP-8037 should be on seems like a decision to make deliberately at that point rather than one pre-answered here. - "a per-entry-point pin has to be re-derived every time one of them is rewired or a new path appears" — but those entry points are four bounded, in-crate sites, and entry-point pinning is exactly what
tx_chain_id_checkdoes three lines up.
Meanwhile every production path I can find — factory, the keyless-deploy sandbox (sandbox/execution.rs:622, the one construction outside the factory), mega-evme, t8n, state-test — funnels through MegaEvm::new, which already pins. Is the on_new_tx write covering something that one doesn't, beyond an embedder mutating evm.ctx.cfg on a live context? If that's a requirement it's worth stating outright; if not, a single pin would drop a per-tx config write and the second place this value is decided.
Related to the other thread: this flag and tx_chain_id_check currently use opposite strategies (choke point vs. entry points). However the Rex7 question lands, one agreed strategy for both would be easier to follow.
| /// the journal still takes its resident-entry branch and still records the `account_warmed` | ||
| /// entry that re-cools the account if the frame reverts, so nothing else about the load moves. | ||
| #[inline] | ||
| fn resident_entry_prices_cold(&self, address: &Address) -> bool { |
There was a problem hiding this comment.
Non-blocking for this PR — a Rex7 follow-up.
The correction itself is right: I checked both versions, and revm 27's resident (Occupied) branch prices from the entry alone and never consults warm_preloaded_addresses, while revm 40 added that consultation. So this faithfully restores frozen behavior and nothing should change here.
But the behavior it preserves looks like a bug worth fixing later. Precompiles, the coinbase, and address-only access-list entries are never inserted into journal state during pre-execution — they live only in the pre-warm set, and that set is consulted exactly once, on the Vacant branch when the entry is first created. MegaETH's own bookkeeping consumes that one chance: inspect_account inserts with mark_cold(), and it runs before the opcode's own load in the SELFDESTRUCT wrapper (instructions.rs:2590), the CALL-family storage wrapper (:2003), and CREATE (:2107). Net effect: a SELFDESTRUCT to — or CALL into — a precompile or the coinbase pays the ~2,600 cold surcharge purely as an artifact of our pre-inspection, not from any intended rule. revm has a TODO next to reset_preloaded_addresses pointing at the same weakness.
The expected rule seems to be that pre-warm membership is final: warm regardless of what the journal holds. That's a real gas change, so Rex7 rather than here — could it be picked up alongside the other Rex7 items? Worth deciding whether the fix is general (drop the resident-entry correction from Rex7 so the pre-warm set wins everywhere) or narrow to the SELFDESTRUCT beneficiary.
| }; | ||
| Self { spec, inner: instruction_table } | ||
| } | ||
| } | ||
|
|
||
| /// Returns the static gas table the interpreter pre-charges from for `spec`. | ||
| /// |
There was a problem hiding this comment.
Rex7 proposal: can the volatile-access guard concede the underfunded-frame case?
This whole mechanism — the per-spec zeroing here, charge_static_gas!, the _self_charged handler variants, and the hand-maintained volatile_guarded_opcodes() list — exists to keep the disableVolatileDataAccess guard reachable by a frame that can't afford revm 40's static pre-charge. Correct for the frozen specs, and the reasoning in the doc comments is good.
But it leaves a correspondence that has to be maintained by hand in two places, and a mismatch is silent and consensus-visible in both directions: an entry zeroed without a guard makes the opcode free, a guard whose entry stayed makes it charge twice. The test pins it today, but the list is manual.
Worth asking whether Rex7 should just let the pre-charge OOG a frame that is too poor to afford the opcode, and only run the guard when gas suffices. The conceded case is a frame holding less than 2–100 gas that was going to die anyway; in exchange the guarded opcodes go back to revm's vanilla table and the mechanism disappears for new specs.
The per-spec modules already give us the seam to do that without disturbing the frozen specs: mod rex7 could stop delegating to rex6::instruction_table() and wire its own table with gas_table(table) = table. The existing machinery stays where it is and becomes append-never, and the duplication is scoped to the handful of opcodes that actually differ.
Not for this PR — just flagging while Rex7 is still open, since it's unfixable once frozen. There's a broader version of this too: Gas is a concrete field on Interpreter, not an associated type on InterpreterTypes, which is why every opcode has to be wrapped to measure compute gas at all. Might be worth an upstream ask to revm.
| fn debug_check_frozen_detention_window<H: HostExt + ?Sized>( | ||
| host: &mut H, | ||
| opcode: u8, | ||
| raw_target: Option<Address>, |
There was a problem hiding this comment.
Blocking: this debug-only check mutates the journal, making gas depend on the build profile.
best_effort_resolve_eip7702_delegate_address takes &mut self and reaches inspect_account, whose Vacant arm does entry.insert(account) after mark_cold(). It pushes no journal entry, so a frame revert can't undo it — JournalEntry::AccountWarmed's revert only re-cools, it never removes a state entry. The account stays resident-and-cold for the rest of the transaction.
resident_entry_prices_cold then prices exactly that state as cold, exempting only access-list entries that carry storage keys. So:
- debug: check runs → target materialized resident-cold → a later touch of it costs 2,600
- release: the whole check is
#[cfg(debug_assertions)]and absent → fresh load consultswarm_precompiles/warm_coinbase_account→ the same touch costs 100
REX6+ only (the || short-circuits below that), and only for an address that would otherwise be warm — a precompile, the coinbase, or an address-only access-list entry. But a REX6 CALL to a precompile that runs out of gas on the static or value-transfer charge is an ordinary path, and it leaves the two builds 2,500 gas apart in both EVM gas and compute gas.
Two consequences:
cargo testis a debug build, so the whole suite exercises an EVM that charges differently from the shipped one.- It defeats the check's stated purpose. The doc comment says this is "the tripwire such a replay must run with" — but
debug_assert!only fires in a debug build, and a debug build is the one whose gas is wrong. The full-history replay meant to prove no transaction sits in the window would itself diverge from history, on unrelated transactions.
The check is already documented as deliberately over-approximate, so it doesn't need to resolve anything it can't see: peeking journal.state and skipping when the target isn't resident would keep the detection and drop the side effect. Worth a test pinning the same gas value under both profiles either way.
Separately, three lines down: the assert message points at REVM_40_REVIEW_GUIDE.md (wontfix #20), which isn't in the repo or anywhere in its git history. That message is the only instruction anyone gets at the moment this fires.
5 participants
Summary
op-revm/alloy-op-evmpinned to the Optimism monorepo revision wired to reth v2.3.0, andalloy-evm0.36.0.Frozen-semantics re-pinning
step()deducts static gas from a per-spec gas table before the handler runs. Volatile-guarded opcodes get zeroed table entries and charge inside the wrapper at the revm-27-equivalent position; all other opcodes add the pre-charge back into the compute-gas measuring window (STATIC_GAS_TABLE).set_halt_action!, which preserves remaining gas for sender refund instead of revm 40'sspend_all-on-OOG and explicitly replaces pending frame actions.sload_skip_cold_load; resident journal entries keep their own cold pricing; access-list warming only applies to entries with storage keys; CALL-family account loads distinguish the raw operand from its EIP-7702 delegate.PRE_REX5_SYSTEM_CALL_GAS_LIMIT(the upstream constant drifted to 30M + EIP-8037 reservoir) and continue to skippost_execution.bytecode_address.set_spec_and_mainnet_gas_paramssoGasParamsfollow the spec;tx_chain_id_checkkeeps its revm-27 default for untouched configurations (with_cfg_unpinnedopts out); EIP-8037 is pinned off in every conversion.commit_transactionfailures are latched and surfaced infinish(); state hooks moved onto revm'sState; results carrytx_typeinstead of a cloned envelope.Rex6 fix (second commit)
Before revm 40, the CALL-family host entry resolved the raw operand's EIP-7702 delegate before loading it, and that resolution materialized the operand's journal entry (cold, code hydrated) as a side effect, so a Rex6 first touch of a preload-warm address (precompile, coinbase, address-only access-list entry) was priced cold.
revm 40 resolves the delegate inside the CALL instruction; the migration's phase-based marking lost the materialization side effect, pricing such first touches warm — 2,500 gas below frozen Rex6 behavior in both EVM gas and compute gas.
The fix reproduces the materialization in
load_account_info_skip_cold_loadfor Rex6 CALL-family raw-operand loads.Caught by
test_callcode_cold_first_touch_follows_the_spec_arcfrom #358.Accepted deviation: the frozen detention window (debug-tripwired)
debug_check_frozen_detention_window) on every plain out-of-gas exit of the guarded wrappers: it fires exactly when a halt leaves the beneficiary — or, from REX6, its one-hop delegate — unmarked where revm 27 marked it. Release builds compile it out (verified absent from the release rlib; CodSpeed should report zero delta).step()'s table pre-charge before any wrapper runs, so that era is covered by the replay gate alone.gas-detention.mdlisted EXTCODECOPY among the opcodes that register volatile access before charging; its copy cost is charged before the load, so it belongs with the CALL family on the excluded side.debug_assertions+overflow_checksarmed throughout. The revm-40 charge order is confirmed as the accepted behavior; the archived wrapper backfill remains unused.Output format changes (no execution change)
"refund": 0and memory words gain a0xprefix. Gas and gasCost values in the trace fixtures are byte-identical to main — execution did not move, serialization did. Anyone diffingmega-evme run --traceoutput against archived traces (or geth) will see the field-level drift.Verification
cargo test --workspace: 1,468 passed / 0 failed (includes the EEST state-test suite, the replay corpus, and the cross-spec compute-gas snapshot suite from docs: specify compute gas accounting and pin it with a cross-spec test harness #358).36081e7) and the halt-receipt log leak this gate caught (fixed in this PR,7156d05) — and a 92/92-chunk re-verification with both fixes confirmed zero remaining divergence.cargo clippywith-D warnings,cargo fmt --check,cargo sort --check, and theriscv64imac-unknown-none-elfno_std check all pass.Status
Marked [WIP]: a manual semantics review of the migration is still in progress; no further code changes are expected unless the review finds issues.