refactor: single home for block-execution env assembly, fixing the fallback branch's BlockLimits drift - #171
Conversation
… binaries Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…imits drift Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude review status
🛠️ Review did not finish Attempted head This round did not publish: MODEL_ACTION_FAILED in phase review_retry. Anything listed below is from the last round that did. Re-run the workflow or push a new commit to try again. |
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
/simplify pass over the PR: - create_block_execution_env returns a named BlockExecutionEnv<ENV> instead of an anonymous 3-tuple: the assembled shape (factory generics included) is now spelled once in core, and TracingEnv collapses its three mirrored fields into one, letting the trace server drop its mega_evm and OpAlloyReceiptBuilder imports — closing the residual drift channel the single-home consolidation left open - module doc's Key Functions list now advertises create_block_execution_env as the shared entry point (create_evm_env demoted to its sub-step) - test helper uses LightWitness::default() instead of a field-by-field struct literal Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…method The macro existed to bind an executor whose concrete type the bin could not reasonably name — spelling it there would re-import the mega-evm type vocabulary. With BlockExecutionEnv now the named single home in core, that home can also own the executor prologue: start_executor_with_inspector creates the executor and applies the pre-execution changes, returning the concrete EnvExecutor type (spelled once, next to the env that produces it). The ten dispatch sites become plain method calls with inferred types, and the macro is gone; executor.evm field accesses keep working on the concrete type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nction The replay loop needs only trait-level access to the executor, so a generic fn with the same HRTB shape core's execute_transactions already uses (for<'t> &'t Recovered<OpTxEnvelope>: ExecutableTx<E>) expresses it without naming any concrete executor type. The bin gains a direct op-alloy-consensus dependency for the envelope type in the bound. The two remaining macros stay by necessity: log_at! (tracing levels must be compile-time constants; the runtime-level dispatch IS the macro) and the validator's metric! (const &'static str concatenation requires concat!). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vincent-k2026
left a comment
There was a problem hiding this comment.
The consolidation is right and the test pins it. One question about how the title frames the fix.
What's good
- The drift is real and was silent: the trace server used
BlockLimits::no_limits()while the validator usedno_limits().with_block_gas_limit(header.gas_limit). Same block, and tracing could succeed where validation would hit the gas ceiling — the worst kind of divergence, because tracing is the tool you reach for when validation disagrees with you. Withcreate_block_execution_envas the single home, the two cannot fork again, andexecution_env_no_hardfork_fallback_caps_block_gasfails if someone reintroduces the fully-unlimited fallback. setup_executor!/replay_preceding_txs!become a method and a generic function. The macros existed to dodge spelling the executor type;BlockExecutionEnv::start_executor_with_inspectorspells it once, next to the env that produces it, and every call site binds by inference. Nine call sites got simpler.
Non-blocking
Please make the motivating case explicit. The divergence only exists on the "no MegaETH hardfork active" fallback branch. Mainnet and testnet genesis both schedule hardforks, so chain_spec.hardfork(ts) is always Some and the two paths already agree in production. "fixing BlockLimits drift" in the title reads like a live bug.
If it is unreachable in production, saying so — "preventive consolidation, plus fixing the fallback branch's divergence" — is more accurate and costs nothing. If it is reachable (some devnet genesis without hardforks, a test harness), please name the trigger, because that changes the merge urgency.
|
@vincent-k2026 Agreed — retitled to name the fallback branch, and the description now has a Reachability section stating it explicitly: mainnet and testnet genesis both schedule hardforks, so the diverging fallback is unreachable in production; this is preventive consolidation plus fixing the fallback branch's divergence, with no production merge urgency. |
…idation #170 closed (being resplit per review); this commit drops its content from the branch and re-fits the block-execution-env extraction onto main's executor shape (writer param, gas_used derivation, and the BLOB constant stay as on main). Tree verified: core 102 / dts 106 / validator 35+13 tests green, fmt/clippy clean, zero #170 leakage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tests start_executor_with_inspector was only reachable through the trace-server binary, leaving the shared wiring invisible to core's coverage; construct it over an empty state and run the pre-execution changes here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vincent-k2026
left a comment
There was a problem hiding this comment.
Re-reviewed at d536e8a. Everything I asked for landed, and the rebase changed this PR's character enough that I re-read the whole diff rather than just my old points — approving.
The rebase is the big one. With #170 closed and its content dropped, the net diff against main is four files (executor.rs +174/-27, tracing_executor.rs +48/-81, plus two dependency lines). That is a single-purpose PR you can hold in your head, which is what the split was for.
Re-read of the current diff:
create_block_execution_envis the one home for theEvmEnv+ factory +MegaBlockExecutionCtxassembly, and the fallback branch is nowno_limits().with_block_gas_limit(header.gas_limit)on both sides. Both branches are pinned by tests, andBlockLimits: PartialEqmeans drift in any field fails them — not just the gas limit.- Replacing
setup_executor!/replay_preceding_txs!withstart_executor_with_inspectorand a genericreplay_preceding_txsis a clear improvement over the macros: the executor's concrete type is spelled once next to the env that produces it, and thefor<'t> &'t Recovered<OpTxEnvelope>: ExecutableTx<E>bound states what the loop actually needs instead of hiding it behind textual substitution. execution_env_starts_executor_with_inspector(68b4ee3) is a good addition — the prologue was previously only reachable through the trace-server binary, so core's own suite never touched it.
Framing fixed. The retitle names the fallback branch, and the Reachability section states plainly that mainnet/testnet genesis both schedule hardforks so production already agreed — no false urgency. The "Root cause (history)" archaeology (#53 introduced the mapping, #69 copied it seven weeks later minus with_block_gas_limit, git log -S showing nobody touched it since) is more than I asked for and is the right way to argue an omission was accidental rather than policy.
Non-blocking leftovers:
- Summary still opens with "Stacked on #170 (retarget to
mainafter it merges)". #170 is closed and the base is alreadymain— drop that sentence so the merge commit doesn't reference a PR that never landed. BlockExecutionEnvstill has noDebug;lintis green so no repo convention requires it. Purely optional.
lint is green now. pr-review is red on every open PR in the repo (org claude-pr-review action fails with ENOENT ... symlink before reviewing) — infra, not this PR.
…sons Brings in #168/#169/#171/#172. Conflict resolutions combine both sides: lookup_block_by_number keeps #168's control flow (resolve_block_number returns (number, tag_hash) and the tag-bound hash skips canonical-hash resolution) with this branch's data_provider_failure error funnel on all three fallible steps, and resolve_block_number keeps #168's tuple signature and unified upstream-tag arm with this branch's typed UnsupportedBlockTag rejection for `pending` (-32602 via the funnel). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Extracts
create_block_execution_envinstateless-core::executor— the(chain_spec, header, ext_env) → (EvmEnv, MegaBlockExecutorFactory, MegaBlockExecutionCtx)assembly incl. the hardfork →BlockLimitsmapping — and has bothreplay_blockand the trace server'sTracingEnv::newconsume it. This deletes the trace server's hand-copied assembly (tracing_executor.rs) and with it the one line where the copies had drifted.Root cause (history)
BlockLimits::no_limits().with_block_gas_limit(header.gas_limit)entered core's replay fallback in chore: Upgrade salt, mega-evm version & update test_data #53 (2025-12-13), when the mega-evm upgrade replacedBlockLimits::from_evm_env(&evm_env)with the hardfork-aware mapping — core's replay has enforced the block's own gas ceiling in every era.BlockLimits::no_limits(), from its very first commit; no commit since ever touched it (git log -S'with_block_gas_limit'on the file: zero hits) and feat: add standalone debug-trace-server for debug_*/trace_* RPC methods #69's description never mentions limits.Behavior
On MegaETH mainnet the fallback branch is unreachable (MiniRex activates at genesis, so
hardfork()is alwaysSome), and for canonical blocks the two fallbacks execute identically anyway (gas_used ≤ gas_limitby consensus). The only observable change: on a chain with no hardfork schedule, tracing a block that exceeds its own declared gas limit now fails the same way validation does, instead of silently tracing past the ceiling.Testing
Two new regression tests pin the mapping through the shared helper: the no-hardfork fallback must equal
no_limits().with_block_gas_limit(header.gas_limit)exactly, and the active-hardfork path must equalBlockLimits::from_hardfork_and_block_gas_limit(..)(BlockLimitsderivesPartialEq, so drift in any limit field fails the test). Full workspace suite 330 passed / 0 failed; fmt / clippy (0 warnings) /cargo sort/ no-std check all clean.Notes
Future mega-evm upgrades (new hardfork variants, new
BlockLimitsfields, ctx changes) now get wired once instead of mirrored by hand across the two binaries — this was the churn point flagged in the upgrade checklist.🤖 Generated with Claude Code
Reachability (review follow-up)
The divergence lives only on the "no MegaETH hardfork active" fallback branch. Mainnet and testnet genesis both schedule hardforks, so
chain_spec.hardfork(ts)isSomefrom genesis onward and the two paths already agreed in production. This PR is therefore preventive consolidation, plus fixing the fallback branch's divergence — the fallback is reachable only for a genesis with no MegaETH forks scheduled (bare devnets, test harnesses), which is also why the merge carries no production urgency.