Skip to content

fix: use live block gas budget for applyPendingChanges() call#282

Merged
RealiCZ merged 7 commits into
mainfrom
cz/fix/system-call-gas-limit-rex5
May 7, 2026
Merged

fix: use live block gas budget for applyPendingChanges() call#282
RealiCZ merged 7 commits into
mainfrom
cz/fix/system-call-gas-limit-rex5

Conversation

@RealiCZ
Copy link
Copy Markdown
Collaborator

@RealiCZ RealiCZ commented Apr 28, 2026

Summary

Fix REX5 pre-block SequencerRegistry.applyPendingChanges() running out of gas on role-activation blocks.

  • The pre-block system call inherited revm's upstream-fixed 30M gas_limit, but applyPendingChanges() rotates roles via SSTOREs whose actual cost scales with REX dynamic storage gas (SALT bucket capacity) — 30M is no longer guaranteed to be enough.
  • New MegaEvm::transact_system_call_with_gas_limit(...) method lets pre-block helpers pick the gas budget explicitly. The default transact_system_call entry point keeps the upstream-fixed 30M, so EIP-2935 / EIP-4788 and every other system call retain byte-level identical behavior across all stable specs.
  • Only transact_apply_pending_changes opts into the new method, calling with max(block.gas_limit, SYSTEM_CALL_GAS_LIMIT_FLOOR). The 30M floor matches revm's hardcoded SystemCallTx::new_system_tx_with_caller literal (revm doesn't export it as a pub const, so we mirror it).

Note: Related fixes in mega-reth (oracle update tx gas limit chosen too early in close_window()) and test-client (block-driver progress, oracle tx shape assertions) are tracked outside this repo.

Test plan

  • cargo fmt --all --check passed
  • cargo clippy --workspace --lib --tests --benches --all-features --locked passed
  • cargo test -p mega-evm — all passed
  • npx prettier --check 'docs/**/*.md' passed

@RealiCZ RealiCZ added spec:unstable Changes to the unstable spec (currently REX5) comp:core Changes to the `mega-evm` core crate comp:doc Changes in the documentation labels Apr 28, 2026
@RealiCZ RealiCZ added api:compatible Only new interface or API is introduced. Existing software is compatible. rust Pull requests that update rust code labels Apr 28, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 28, 2026

LGTM. The implementation correctly mirrors the existing system-call pattern (run_system_call + finalize()), the max(block.gas_limit, SYSTEM_CALL_GAS_LIMIT_FLOOR) logic is sound, and test coverage is thorough (floor, above-floor, and isolation of the default 30M path). The pub(crate) signature tightening to concrete MegaEvm is fine since the only caller in executor.rs already holds a MegaEvm.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.5%. Comparing base (915f71e) to head (8a2a649).

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 28, 2026

The fix is correct and well-scoped. The approach — narrowing the generic in the internal transact_apply_pending_changes, adding a concrete inherent method with an explicit gas limit, and keeping the 30M-default path untouched — correctly preserves backward compatibility for all stable specs while solving the OOG risk for REX5 role-activation blocks.

One doc wording issue flagged inline (the SYSTEM_CALL_GAS_LIMIT_FLOOR comment advises syncing with upstream if they change their default, which would be a backward-compat violation). Everything else looks good — test coverage for both the new method and the floor/ceiling cases is solid.

Comment thread crates/mega-evm/src/constants.rs
Comment thread crates/mega-evm/src/evm/mod.rs
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 28, 2026

LGTM. Previous feedback on the SYSTEM_CALL_GAS_LIMIT_FLOOR doc comment has been addressed — the constant now explicitly states it must not be raised to follow upstream changes, with the full backward-compatibility rationale. The implementation, tests, and docs all look correct.

Copy link
Copy Markdown
Collaborator

@Troublor Troublor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is well-scoped and the policy is correctly gated on the explicit opt-in path, so stable specs keep byte-identical behavior. Two comments to address:

  1. The transact_apply_pending_changes unit test only exercises gas-limit propagation (no pending role changes), not the OOG scenario the fix targets. A system-level regression test under crates/mega-evm/tests/rex5/ with a heavy SALT-bucket setup would prevent silent re-regression.
  2. The "default system call keeps 30M" test is a structural assertion on tx.base.gas_limit, not a behavioral check on EIP-2935 / EIP-4788 paths under REX5. The "byte-identical across all stable specs" claim is defended by code review rather than tests.

Comment thread crates/mega-evm/src/system/sequencer_registry.rs
Comment thread crates/mega-evm/src/evm/mod.rs
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 1, 2026

LGTM. Both of @Troublor's concerns from the previous review have been addressed by the new integration test file crates/mega-evm/tests/rex5/apply_pending_changes_gas_budget.rs:

  1. OOG regression coveragetest_rex5_apply_pending_changes_succeeds_under_heavy_storage_gas drives the full block-executor path (apply_pre_execution_changes()) under a heavy SALT setup (2000× multiplier, ≈80M dynamic storage gas for the two role-rotation SSTOREs), and test_rex5_apply_pending_changes_oogs_under_upstream_30m_cap pins the baseline that the upstream 30M cap really is insufficient under the same scenario. A regression that routes the call back through transact_system_call would fail the first test even if the unit tests pass.

  2. Behavioral isolation tests — the EIP-2935 and EIP-4788 isolation assertions are now behavioral (OOG under heavy SALT), not just structural checks on tx.base.gas_limit. They prove the upstream paths still hit the 30M cap rather than the widened block budget.

@RealiCZ RealiCZ requested a review from Troublor May 6, 2026 19:06
Copy link
Copy Markdown

@vincent-k2026 vincent-k2026 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Diagnosis is clear, fix is local, and the test design is excellent — particularly:

  • test_default_system_call_keeps_upstream_30m_gas_limit using the literal 30_000_000 instead of SYSTEM_CALL_GAS_LIMIT_FLOOR to deliberately catch upstream drift rather than auto-aligning to it.
  • test_rex5_apply_pending_changes_oogs_under_upstream_30m_cap as a negative baseline that will start failing if SALT bucket pricing ever drops back under 30M, prompting a re-examination of whether the fix is still load-bearing.
  • The EIP-2935 / EIP-4788 isolation tests pinning that REX5 only widens applyPendingChanges.

Verified separately that SequencerRegistry.sol does not call gasleft(), so the gas_limit change is not bytecode-observable — state/receipts/logs identical for blocks where 30M was already sufficient. REX5 hasn't activated, so no replay impact either.

Two non-blocking nits:

  1. Naming: SYSTEM_CALL_GAS_LIMIT_FLOOR lives under constants::rex5 but the value mirrors revm's universal upstream literal, not a REX5 parameter. The REX5-specific decision is which call sites opt into the widened budget, not the floor value itself. Worth hoisting to top-level constants (or a system_call submodule named after its provenance) so a future REX spec can reuse it without aliasing.

  2. PR body mentions related fixes in mega-reth (oracle close_window gas limit) and test-client. Could those PRs be linked here? Want to make sure the merge order is coordinated and devnet validation covers both halves before the REX5 activation block. This PR alone is forward-compatible.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

LGTM. All prior feedback from Troublor has been addressed by the new apply_pending_changes_gas_budget.rs integration test file, which covers both the OOG-regression scenario (heavy SALT, 2000× multiplier, confirms rotation commits via the block executor) and the behavioral isolation invariant (EIP-2935 / EIP-4788 paths confirmed to OOG under 30M even with the widened block budget). Implementation, unit tests, integration tests, and docs all look correct.

@RealiCZ RealiCZ merged commit 79828cb into main May 7, 2026
25 checks passed
@RealiCZ RealiCZ deleted the cz/fix/system-call-gas-limit-rex5 branch May 7, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api:compatible Only new interface or API is introduced. Existing software is compatible. comp:core Changes to the `mega-evm` core crate comp:doc Changes in the documentation rust Pull requests that update rust code spec:unstable Changes to the unstable spec (currently REX5)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants