perf: ARO pruned 11-edit terminal set (TERMINAL_CONFIRMED_WITH_TRADE)#347
perf: ARO pruned 11-edit terminal set (TERMINAL_CONFIRMED_WITH_TRADE)#347mega-putin[bot] wants to merge 11 commits into
Conversation
…ompiles Terminal r4: TERMINAL_CONFIRMED_WITH_TRADE 105 improved, 1 regressed (eip7702_authlist/rex5/400 +0.3368%) 55 control rows max|Δ%|=3.35%, all within 5.0% bound 3 files, +265/-72 Baseline: 7d409ae (current main) Toolchain: codspeed=4.18.3;cargo-codspeed=5.0.1;valgrind=3.26.0.codspeed5;rustc=1.96.0 Stamp: /tmp/terminal-r4.json sha256=1ecbcaddec7eb130c8c3eea05cfe88a31e71260b3292c7e46cc6bffbcfd96998
mega-putin
Bot
added
comp:core
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 6.71%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
|
🧬 Mutation testing — ✅ PASSDiff mutation score: 100.0% (9/9 viable mutants killed)
No new test gaps introduced by this change. 🎉 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
RealiCZ
added
spec:stable
🧬 Mutation testing — ✅ PASSDiff mutation score: 100.0% (21/21 viable mutants killed)
No new test gaps introduced by this change. 🎉 |
|
Confirmed. The outcome is intended to stay identical, and I updated the spec text to match the implementation. This change now documents the |
|
🔧 Pushed CI fixes. Agent log |
…-by: engineer-agent
|
🔧 CI fixes are ready. Agent log |
There was a problem hiding this comment.
✅ Clean
- Reviewed the incremental change since prior review (
366ebae): depth-guard now covers CALLCODE/DELEGATECALL in bothframe_initandinspect_frame_init, plus the accompanying spec text update ininterception.md/rex5.mdand new CALLCODE/DELEGATECALL guard tests. Also re-checked the r4 baseline changes:post_execution_without_eip7702_refund_workfast path, REX5 inlinedtransactloop, hand-inlinedadd/pop/push1/log_unchecked, and the precompile OOG-first refactor inprecompiles.rs. - Behavioral equivalence spot-checks:
- The debug-asserted invariants (
eip7702_refund == 0for non-7702 tx;gas_limit >= initial_gasunder REX5) are enforced upstream — revm defaultapply_eip7702_auth_listearly-returns 0, and REX5validate()rejectsinitial_gas > gas_limitbeforepre_executionruns. gen_call_too_deep_resultmatches revmmake_call_framedepth-check output exactly (CallTooDeep,Gas::new(gas_limit), clonedreturn_memory_offset), and for CALLCODE/DELEGATECALL the sub-trackerpush_empty_framevsbefore_frame_init+ immediate pop are net-equivalent (target_address = caller, so no state-growth recorded; REX5pop_frame_unwind_parentunwinds any KV flag on the revert path).log_uncheckedis safe underLog::new_uncheckedbecauseN <= 4is statically enforced by the wired sites (LOG0..LOG4);discard_topset_len(len-1)is safe because U256 has no drop glue and the empty-stack guard runs first.- The precompile refactor preserves the pre-existing gas-recording for both OOG (
gas.limit()/ KZG fixed cost) and ok-or-revert (normalizedspent) branches.
- The debug-asserted invariants (
- Docs and mutation suppressions updated in step with the code; the two new
equivalentsuppressions correctly identify that REX5 -> REX2/REX4 makes thedebug_assert!gate weaker-but-still-always-true. - No new actionable findings.
- Still open from earlier reviews: none.
Generated-by: engineer-agent
|
🔧 Pushed CI fixes. Agent log |
|
No actionable review feedback remains from I verified the current head
Still in progress:
The worktree is clean. No code changes were needed, so there is no new commit and no PR comment to post. |
There was a problem hiding this comment.
✅ Clean
- PR HEAD (
c1075c1) is unchanged since the prior automated review (same commit). No incremental code to re-litigate. - Prior round already verified the substantive changes: REX4/REX5 guard hoisting in
frame_init, extended CALLCODE/DELEGATECALL coverage inframe_init+inspect_frame_init, REX5 inlined transact loop, non-EIP-7702 post-execution fast-path, hand-inlinedadd/pop/push1/log_uncheckedwrappers withStackInspectTr::discard_top, and the precompile OOG-first refactor. - Docs (
interception.md,rex5.md) and mutation suppressions are in step with the code. - No new actionable findings.
- Still open from earlier reviews: none.
|
No actionable review feedback remains from |
There was a problem hiding this comment.
✅ Clean
- PR HEAD (
f0637bf) is unchanged since the prior automated review at the same commit — no incremental code to re-litigate. - Full-diff re-check confirms the substantive changes remain sound: REX4 exceeded-limit gate hoisted before interceptor dispatch, REX5 CALL_STACK_LIMIT guard now covers all
FrameInput::Callschemes in bothframe_initandinspect_frame_init, non-EIP-7702post_executionfast path guarded bydebug_assert_eq!(eip7702_refund, 0), REX5 inlinedtransactloop guarded by the REX5 finalvalidate()initial-gas check, hand-inlinedadd/pop/push1/log_uncheckedwithLog::new_uncheckedstatically bounded to N ≤ 4 viaStackInspectTr::discard_top, and the precompile OOG-first refactor that preserves compute-gas accounting on all paths (KZG fixed cost, non-KZG forwarded limit, success/revert spent). - Docs (
interception.md,rex5.md) and mutation suppressions are consistent with the code. - No new actionable findings.
- Still open from earlier reviews: none.
|
I addressed the actionable CI feedback from the mutation job by adding a direct regression test that proves I committed it as Verification passed locally:
No PR reply is needed for the latest |
Generated-by: engineer-agent
There was a problem hiding this comment.
Approve — clean perf refactor, one minor cleanup nit inline.
Scope check
Perf changes for the ARO terminal set:
- execution.rs: REX5-inlined transact_run path; new post_execution_without_eip7702_refund_work fast-path; reordered frame_init so REX4 exceeded-limit / REX5 depth guards run before pre-Rex3 oracle detection; widened REX5 depth guard to all 4 CallScheme variants.
- instructions.rs: inlined push1/pop/add/log_unchecked, added StackInspectTr::discard_top.
- precompiles.rs: restructured REX5 gas accounting into early-return + ok_or_revert paths (semantically equivalent).
- Test coverage added for opcode wrappers and the widened depth guard.
Verified
- Fast-path matches OpHandler::refund when tx_type != Eip7702 (only skips a no-op record_refund(0)).
- frame_init reordering is safe: REX4 implies REX3, so REX4/REX5 guards and pre-Rex3 oracle detection are mutually exclusive.
- Widened CallScheme guard is safe under Isthmus: for CALLCODE/DELEGATECALL/STATICCALL, is_call=false in load_acc_and_calc_gas, so charged_new_account_state_gas=false; reservoir is 0 with no AMSTERDAM.
- Log::new_unchecked is safe because N is compile-time bounded to 0..=4 (debug_assert).
- Inlined push1/pop/add are byte-for-byte semantically equivalent to revm originals.
Findings
- [Minor] Redundant stack length pre-check in log_unchecked — see inline.
Follow-up (pre-existing, not blocking)
gen_call_too_deep_result builds a fresh CallOutcome without threading inputs.reservoir or inputs.charged_new_account_state_gas. Not introduced by this PR; harmless under current cfg (reservoir is 0 under Isthmus and the depth guard now runs before pre-Rex3 oracle detection where charged_new_account_state_gas matters), but worth a comment explaining the invariant so a future fork that turns reservoir non-zero does not silently drop state gas on the CallTooDeep path.
Generated-by: engineer-agent
|
🔧 Pushed CI fixes. Agent log |
There was a problem hiding this comment.
🧭 Re-review update
- Scope: incremental commits since 481cb55 — extended REX5 depth guard to all
FrameInput::Callschemes (with matching inspector-path change), new mutation-killing tests (execution_mutation.rs, expandedopcode_wrappers.rsandcall_too_deep_guard.rs), collapsed duplicate precompile spec-gate branch, and equivalent-mutant suppressions. - Verified: docs (
docs/spec/upgrades/rex5.md,docs/spec/system-contracts/interception.md) are updated to reflect that the depth gate now covers all call schemes (not justCALL/STATICCALL), keeping spec text in sync with code. - Verified: mutation suppressions in
mutants/suppressions.tomlare justified — theMegaSpecId::REX3→ REX2/REX4 debug-assert mutants are truly equivalent under monotonic spec ordering, and thepost_execution_without_eip7702_refund_workdeposit-branch equivalents hold under the ISTHMUS ⊇ REGOLITH invariant (with the non-equivalent|| → &&mutant pinned by the new deposit-vs-regular gas equality test). - No new actionable findings this round.
Still open from earlier reviews:
- [Minor] Redundant
stack.len() < Npre-check inlog_unchecked— thread. The code atcrates/mega-evm/src/evm/instructions.rs:1686-1689still eagerly halts before the subsequentpopn::<N>(), which already returnsNoneon underflow with an identical halt. Not a correctness bug; deletable at the author's discretion.
Generated-by: engineer-agent
|
@mega-putin Could you please fix the CI? |
|
Fixed locally and committed a follow-up that targets the remaining mutation/CI coverage gaps. This also removes the redundant |
Generated-by: engineer-agent
There was a problem hiding this comment.
Clean re-review
- Scope: incremental commits since prior review (7afd416..cff42be) — two files touched: instructions.rs (removed the redundant stack.len() < N pre-check in log_unchecked) and execution_mutation.rs (test-tightening only).
- log_unchecked cleanup lands the nit from the earlier round: popn::() already halts on underflow with byte-identical semantics, so the pre-check was dead work. Prior nit thread now resolved.
- Refund test now clears REFUND_SLOTS = 4 SSTOREs so the London refund cap gets hit and asserts 0 < normal_refunded < 4 * 4800, keeping set_final_refund observable. New test_eip7702_existing_authority_records_final_refund locks in the post-execution EIP-7702 fast-path (non-zero final refund + authority nonce/code delta).
- No new actionable findings.
- Still open from earlier reviews: none — the one prior Claude nit was addressed in cff42be and its thread has been resolved.
|
No action is needed on the latest It is approval-only, reports no actionable findings, and asks no question, so there is nothing to patch, no PR reply to post, and no new commit to make. The worktree is clean, and the latest local commit remains |
flyq
left a comment
flyq
left a comment
There was a problem hiding this comment.
1. The remaining red check: 2 inconclusive timeout mutants (not real gaps)
cargo-mutants gate reports diff score 100.0% (24/24 viable killed), 0 survivors, 2 suppressed, and 2 timed-out — the exit-1 is the gate's "no inconclusive timeouts" policy, not a coverage gap:
execution.rs:283:24: replace || with && in post_execution_without_eip7702_refund_workexecution.rs:521:37: replace == with != in run_without_catch_error
Both live in the new EIP-7702 refund fast-path.
Notably, the suppression file's own comment claims the ||→&& mutant "remains unsuppressed and is pinned by tests/execution_mutation.rs" — logically true (test_deposit_refund_matches_regular_tx_under_isthmus kills it deterministically: the mutation drops the deposit's final refund and the gas_used equality fails), but CI could not prove it: the per-mutant suite run hit the 123s timeout cap (242s/267s builds on a cold cache suggest a loaded runner, not a hang).
Same story for the ==→!= gate inversion (its non-7702 arm is a no-op by the PR's own equivalence argument; the 7702 arm loses the auth-list refund and existing 7702 tests would fail).
2. Recommended fix — collapse the vacuous branch instead of suppressing around it
The two new suppressions justify themselves by proving is_regolith is always true (every MegaSpecId maps to OpSpecId::ISTHMUS ⊇ REGOLITH), which makes !is_deposit || is_regolith at execution.rs:283 a constant true.
If that is the argument, the branch should not exist: replace the conditional (and the is_deposit/is_regolith locals) with an unconditional set_final_refund(...) plus debug_assert!(spec.is_enabled_in(OpSpecId::REGOLITH)) and a comment pointing at OpHandler::refund (op-revm-8.1.0/src/handler.rs:304–326).
This one change simultaneously: removes both timeout mutants (nothing left to mutate), removes both suppressions (and the governance question in finding 5), and makes the replica honest about the invariant instead of carrying dead deposit logic.
Alternative if the upstream-mirroring shape is preferred: raise the cargo-mutants timeout multiplier in the gate workflow — but that leaves the false "pinned by tests" comment in suppressions.toml, which should be corrected either way.
(Dropping the fast-path entirely remains defensible — see finding 3 — but unlike #346 the win is no longer negligible: the subcall_1000 rows show per-tx fixed costs matter for MegaETH's small-tx-dense workload.)
|
Applied the suggested cleanup. I collapsed the unreachable deposit/refund branch in This matches the actual reachable-spec invariant more honestly and avoids carrying timeout-prone mutants around a vacuous condition. |
Generated-by: engineer-agent
There was a problem hiding this comment.
Verdict: 1 finding.
Re-review after commit 042cf90 collapse of the deposit-refund branch and mutants/suppressions cleanup. Correctness pieces from prior rounds resolved:
- Refund fast path (flyq #2): the invariant every MegaSpecId maps to OpSpecId::ISTHMUS which enables REGOLITH is tested by test_all_specs_map_to_isthmus_and_prague and re-asserted at runtime by the new debug_assert. New test_deposit_refund_matches_regular_tx_under_isthmus pins normal == deposit refund > 0 and < REFUND_SLOTS * 4800 so the collapse stays observably behavior-preserving.
- Mutant-suppression whack-a-mole (flyq #1): is_deposit / is_regolith suppressions removed. Two new REX3 to REX2 / REX4 entries in mutants/suppressions.toml are correctly justified as equivalent under monotonic spec gates.
- EIP-7702 final-refund path: test_eip7702_existing_authority_records_final_refund asserts gas_refunded > 0 on an existing-authority auth list, killing the == to != mutant on the auth-branch predicate.
- Widened depth guard (frame_init line 979 / inspect_frame_init line 1252): covering all FrameInput::Call schemes is behavior-preserving; gen_call_too_deep_result matches revm synthetic result shape. Pinned by the two new inspect_frame_init tests.
- Precompile compute-gas restructure: semantically identical to prior branch layout; new test_kzg_precompile_equivalence_does_not_record_compute_gas and test_rex5_non_kzg_precompile_error_records_forwarded_limit lock in the boundaries.
- Inlined transact_run REX5 fast path: skip of redundant before_execution OOG check is safe because REX5 validate() already rejects those txs. Gated on spec.is_enabled(MegaSpecId::REX5) so pre-REX5 behavior is untouched.
- log_unchecked / discard_top unsafe blocks: Log::new_unchecked safe because N <= 4 debug_asserted; set_len(len-1) safe because U256 has no drop glue and the top-of-stack precondition is preserved by the caller.
One docstring-drift finding from the branch restructure in precompiles.rs — inline comment below.
|
Fixed. I realigned the two large comment blocks in No behavior changed; this is just comment-anchor cleanup to match the restructured control flow. |
Generated-by: engineer-agent
There was a problem hiding this comment.
Clean re-review. Incremental change (042cf90 to 0fd06a2) is the precompiles.rs comment realignment: the three commentary blocks now each sit directly above the matching branch, resolving the earlier read-order concern. No new actionable findings on the surviving 11-edit set. Still open from earlier reviews: none.
|
No action is needed on the latest It is approval-only, reports no actionable findings, and asks no question, so there is nothing to patch, no PR reply to post, and no new commit to make. The latest local commit remains |
|
❌ I tried to fix the failing checks but the agent did not produce any changes (rc=1, duration 1518s). A human should take a look. |
3 participants
Summary
ARO terminal gate r4 on current main (7d409ae): 11 surviving edits across
execution.rs,instructions.rs, andprecompiles.rs. 4 edits dropped by protocol (2 unappliable due to main drift, 1 unappliable, 1 reverify-fail).Delta (Ir-first, terminal r4 stamp)
Top rows by |Δ%|:
Full 32-row bench_ir_rows in stamp.
Terminal verdict: TERMINAL_CONFIRMED_WITH_TRADE (105 improved, 1 regressed, 55 control rows max|Δ%|=3.35%)
Traded regression
Within 1.0% trade cap.
Files changed
crates/mega-evm/src/evm/execution.rs— frame_init REX4/REX5 guard hoisting, post_execution EIP-7702 fast-path, transact REX5 inlined loopcrates/mega-evm/src/evm/instructions.rs— push1/pop/add hand-inlined compute-gas, log0-4 thin wrappers withLog::new_unchecked,StackInspectTr::discard_topcrates/mega-evm/src/evm/precompiles.rs— precompile gas accounting restructured for REX5 non-OOG pathProvenance
Autonomous optimization campaign (ARO,
mega-evm-v2-diverge-r2, terminal r4):/tmp/terminal-r4.jsonsha2561ecbcaddec7eb130c8c3eea05cfe88a31e71260b3292c7e46cc6bffbcfd96998Verification
cargo build --release -p mega-evm: ✅cargo test --release -p mega-evm: ✅ (recheck gate)Supersedes #346.