refactor: apply simplify-review cleanups across core, common, and the binaries - #170
refactor: apply simplify-review cleanups across core, common, and the binaries#170flyq wants to merge 3 commits into
Conversation
… binaries 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❌ Patch coverage is ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef0bd2ef4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| tokio::task::spawn_blocking(move || { | ||
| verify_block_integrity(&block)?; | ||
| Ok(block) | ||
| }) | ||
| .await |
There was a problem hiding this comment.
Keep block verification out of the RPC attempt timeout
When skip_block_verification=false and a large full block takes longer than per_attempt_timeout to verify, this spawn_blocking join is now inside round_robin_with_backoff's per-provider timeout; the retry loop can classify a healthy provider as stalled, retry the same block, and leave the blocking verification task running in the background. This affects the validator's default get_block(..., full_txs=true) path even without an overall deadline, because per-attempt timeouts are always applied. Consider fetching inside the timed RPC attempt but running the blocking integrity check after a successful fetch, or otherwise excluding the join from the provider-stall timeout.
Useful? React with 👍 / 👎.
/simplify pass over the PR: the `metas` recycling was a vestigial half-collapse — a loop-level buffer threaded through mem::take, a tuple return, and a destructuring assignment purely to reuse one small Vec allocation per batch, next to multi-ms blocking disk work. Build it locally inside the spawn_blocking closure and return only the batch; the advanced count reads batch.len(), which it always equalled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Taken individually I agree with almost every change in here. My ask is about the packaging: this is labelled a cleanup, and it carries at least six behavior changes plus five public-API removals across 30 files. That makes each one un-reviewable in isolation, un-bisectable, and un-revertable independently.
Item 2 below is the concrete cost. Moving verify_block_integrity into spawn_blocking is a good change on its own, but it also moved verification inside round_robin_with_backoff's per-attempt timeout, so a healthy provider serving a large block can be classified as stalled, retried, and leave the blocking task running behind it. As a standalone three-line PR that consequence is visible on sight; at position 2 of 6 in a 30-file cleanup it is not.
Suggest splitting into a genuinely behavior-preserving cleanup plus a handful of small, individually-motivated changes. Concretely, here is what I count as behavior change:
1. Behavior changes shipped under a refactor label
witness_apismay now be empty, and a witness call with zero providersassert!s — a new panic path in a long-running binary. I checked the reachability:ValidatorFetcher::fetchtakes the R2 branch whenr2_witness.is_some(), and there is no secondrpc_client.get_witness*call site inbin/stateless-validator, so it is currently unreachable. It is also strictly better than the old "hand it the data endpoints as a placeholder" behavior, which silently pointed witness calls at the wrong endpoints. Still worth being deliberate aboutassert!vs a typed error in a validator that is expected to stay up.verify_block_integritymoved tospawn_blocking— the per-attempt-timeout interaction described above.verify_block_integrityrewritten — clones removed,trie_hash()replaced by explicitencode_2718+keccak256. These are equivalent (Encodable2718::trie_hash()is defined askeccak256(encoded_2718())) and it removes N clones plus a second encode pass, so it is a good optimization — of a security-critical verification function, which deserves its own PR and its own line in a changelog.gas_usednow comes fromexecution_result.gas_usedinstead ofreceipts.last().cumulative_gas_used(). This is the one I would most like to see argued explicitly. It feeds the header check, and MegaETH carries system transactions inextra_data, so whether the two agree depends on whether the executor folds system-transaction gas into its total. If they can disagree, this is a consensus divergence. Please state why they are equivalent and add an assertion test pinning it.chain_advancertakesArc<S>/Arc<H>and commits viaspawn_blocking— a concurrency model change. The panic handling (try_into_panic→resume_unwind, preserving inline semantics) is careful and correct.evm_database:plain_value()→find(), dropping a heap allocation per state read. A hot-path optimization.
2. Public API removals, consumed cross-repo
mega-reth consumes this crate as stateless-core = { git = ..., tag = "v2.0.14" }. Removed or changed here:
pub fn mega_mainnet_hardforks()pub const BLOB_GASPRICE_UPDATE_FRACTION(replaced by revm'sBLOB_BASE_FEE_UPDATE_FRACTION_CANCUN— same value, 3338477, I checked; worth one confirming line in the body since it is a consensus parameter)pub fn LightWitnessExecutor::kvs()validate_block/validate_block_deriving_updates/replay_blocklose thewriterparameter, which removes EIP-3155 trace output entirely — a feature deletion, not a cleanupChainStore: ContractStoresupertrait dropped
Nothing breaks at the pinned tag, but the next bump has to be coordinated. The supertrait change is a genuine improvement and has a concrete downstream payoff worth naming: mega-reth's MegaValidatorChainStore carries a ContractStore impl its own comment calls "intentionally inert", existing only to satisfy that bound — it can be deleted on the bump. Please say in the body that the next tag is breaking and what the downstream change is.
3. An invariant that lost its only enforcement
Removing mega_mainnet_hardforks() and the explicit reordering loop makes MegaethGenesisHardforks::into_vec's literal the single source of activation order. That is the right direction — two sources of the same ordering was the drift risk.
But nothing tests the order. test_merge_mega_hardforks_in_op_hardforks asserts spec.hardforks.fork(MegaHardfork::MiniRex) == Timestamp(3) and similar — map lookups, order-independent. Meanwhile chain_spec.hardfork(timestamp) — which #171 uses to select BlockLimits — is order-sensitive. A wrong literal order yields wrong block limits, i.e. consensus divergence, with the entire suite green.
Please add a test asserting the relative order of the MegaETH forks in forks_iter(), plus a comment on the literal saying new hardforks must be inserted at their activation position and the test bumped alongside.
Worth keeping, whatever the split
ChainStore/ContractStore decoupling with its stated rationale; BackoffSchedule collecting jitter/cap/1ms-floor into one type (previously duplicated between the R2 and RPC retry loops, comments and all); dropping in_flight_blocks as a second source of truth for task_to_block; the dead-parameter chain cleanup; replacing the test-only set_anchor_block backdoor with the production reset_to_anchor path; BlockMeta::from_header collapsing four identical field copies.
…sed definition Fork selection by timestamp walks forks_iter() in insertion order, so the into_vec literal is the consensus activation order — assert the relative order of all ten MegaETH forks end-to-end through from_genesis. Also pin mega-evm's gas_used definition (last receipt's cumulative gas) with a debug assertion at the header-check derivation site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Closing per review: this will be resplit into a behavior-preserving cleanup plus individually-motivated PRs (plan being drawn up; work starts after the currently open PRs merge). The branch stays as source material. |
…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>
|
Split as agreed, six stacked PRs: #194 (behavior-preserving cleanup + unused-API removals) → #195 (EIP-3155 writer deletion) → #196 (verify_block_integrity / gas_used) → #197 (blocking work off the runtime, incl. the per-attempt-timeout interaction this review flagged) → #198 (hot-path allocations) → #199 (empty witness endpoints + typed range error). Each carries the part of this review it answers. |
Summary
Two
/simplifyreview passes (4 review agents each: reuse, simplification, efficiency, altitude) overstateless-coreandbin/stateless-validator, with the surviving findings applied — net −73 lines. No functional change on canonical-block paths; behavior-adjacent changes are called out below.stateless-core
ChainSpec::from_genesis:MegaethGenesisHardforks::into_vec()already yields canonical activation order (its literal is now documented as the single source of that order), and the "unknown fork" append branch was unreachable; the unusedmega_mainnet_hardforks()template is gone with it.trace_writerparameter threaded throughreplay_block/verify_and_replay/validate_block/validate_block_deriving_updates(no caller here or in mega-reth ever passedSome), which also deletes therun_plainclosure + cfg dance. mega-reth impact: its call sites drop a trailingNoneat the next version bump.WitnessExternalEnv::new/from_light_witnessnow share onefrom_metadata_kvsbody; deadLightWitnessExecutor::kvs()accessor and a doubled doc block oncreate_evm_envremoved.BLOB_BASE_FEE_UPDATE_FRACTION_CANCUNinstead of a hand-copied consensus constant;gas_usedreadsBlockExecutionResult::gas_usedinstead of recomputing from the last receipt.WitnessDatabasereads use salt'sfind()+ in-place decode and stack-allocated key encodings (pub(crate)helpersencode()also delegates to) — two heap allocations per witnessed state read and one perbucket_id_for_*call removed; dropped a per-missSaltValueclone inLightWitness::metadata.chain_advancerrunspre_advance+advance_chainunderspawn_blockingso redb commits (and the trace server's multi-MB block-data writes) no longer pin an async runtime worker; panics still propagate unchanged viaresume_unwind. Also removed thein_flight_blocksmirror set, themetaslockstep vector, and the dead_reasonparameter.ChainStoreno longer carries theContractStoresupertrait (nothing consumed contracts through it); the forced stubs on the pipeline mock and the trace server'sStubBlockStoreare deleted.stateless-validator + shared crates
RpcClientas placeholder witness endpoints: the constructor accepts an empty witness list, and a witness call with zero providers panics with a structural message instead of silently retryingmega_getBlockWitnessagainst data endpoints. The trace server is unaffected (clap already enforces a witness source at startup).BackoffPolicy::schedule()/BackoffSchedulein stateless-common now own the jittered-doubling arithmetic previously duplicated between the RPC round-robin loop and the R2 GET loop; both consume it (fastranddep dropped from the bin).install_prometheus_exporterhoisted intostateless_common::metrics; both binaries call it and themetrics-exporter-prometheusdep moved from the bins into common.BlockMeta::from_headeradded in core, replacing the three defaulting header→meta projections (validator + two in debug-trace-server); the strict anchor-init site inapp.rskeeps its explicit missing-withdrawals-root error by design.verify_block_integritynow runs on the blocking pool and encodes each tx exactly once (keccak of that encoding checks the hash, the same bytes feed the transactions-root trie) — previously 2 clones + 2 encodes per tx inline on runtime threads.on_remote_heightfn-pointer injection removed;run_with_signalstakesreport_validation: boolinstead of anOption<String>it only ever.is_some()'d; phantometh_getTransactionByHashmetric series dropped; test-onlyset_anchor_blockdeleted (the roundtrip test now exercisesreset_to_anchor);workers.rsrenamed torunner.rs(docs updated); integration mock handlers deduped onto shared lookup helpers (also fixing a drifted bareunwrapon malformed params);make_block_metatest fixture unified.Testing
cargo fmt/clippy --workspace --all-targets --all-features(0 warnings) /cargo sortclean; full workspace suite 328 passed / 0 failed; no-std check and no-std test compile (--no-default-features) pass.Notes
RpcClientconstructor check (startup rejection → loud panic at first misuse); the constructor test pins the new contract.replay_blockand the trace server'sTracingEnv::new, resolving theirBlockLimitsdrift.🤖 Generated with Claude Code
Review response (2026-08-04)
Behavior-change inventory (from review, kept in one place deliberately):
witness_apismay be empty; a witness call with zero providers asserts — currently unreachable (the R2 branch is taken first, and there is no secondget_witness*call site in the validator). Assert-vs-typed-error is ledgered as a small follow-up PR.verify_block_integritymoved intospawn_blocking— its interaction withround_robin_with_backoff's per-attempt timeout (a healthy provider serving a large block could be classified as stalled while the blocking task runs on) is acknowledged and ledgered as its own follow-up PR rather than patched inside this one.verify_block_integrityrewritten:trie_hash()→ explicitencode_2718+keccak256(definitionally equal —Encodable2718::trie_hashiskeccak256(encoded_2718())), removing N clones and a second encode pass.gas_usedswitched fromreceipts.last().cumulative_gas_used()toexecution_result.gas_used— definitionally equivalent: mega-evm'sfinish()computesgas_usedas exactlyreceipts.last().cumulative_gas_used(), so both expressions read the same source regardless of how system transactions are accounted. Pinned by adebug_assert_eq!at the derivation site, exercised by the mainnet single-block integration test.chain_advancertakesArc<S>/Arc<H>and commits viaspawn_blocking; panics are preserved viatry_into_panic→resume_unwind.evm_database:plain_value()→find(), dropping a heap allocation per state read.Hardfork ordering now has enforcement:
test_mega_hardforks_iterate_in_activation_orderasserts the relative order of all ten MegaETH forks end-to-end throughfrom_genesis(order is consensus-relevant: timestamp fork selection walksforks_iter()in insertion order), and theinto_vecliteral's comment points at the test.Breaking at the next tag — mega-reth consumes
stateless-corepinned at v2.0.14; nothing breaks until the bump, which needs coordination:mega_mainnet_hardforks()andLightWitnessExecutor::kvs()removed.BLOB_GASPRICE_UPDATE_FRACTIONreplaced by revm'sBLOB_BASE_FEE_UPDATE_FRACTION_CANCUN— same value (3338477), a consensus parameter, verified equal.validate_block/validate_block_deriving_updates/replay_blockdrop thewriterparameter, deliberately removing EIP-3155 trace output (no caller here or in mega-reth ever passedSome); mega-reth call sites drop a trailingNoneon the bump.ChainStore: ContractStoresupertrait dropped — downstream payoff: mega-reth'sMegaValidatorChainStoredeletes its intentionally-inertContractStoreimpl on the bump.