Skip to content

refactor: apply simplify-review cleanups across core, common, and the binaries - #170

Open
flyq wants to merge 2 commits into
mainfrom
liquan/refactor/simplify-cleanups
Open

refactor: apply simplify-review cleanups across core, common, and the binaries#170
flyq wants to merge 2 commits into
mainfrom
liquan/refactor/simplify-cleanups

Conversation

@flyq

@flyq flyq commented Aug 1, 2026

Copy link
Copy Markdown
Member

Summary

Two /simplify review passes (4 review agents each: reuse, simplification, efficiency, altitude) over stateless-core and bin/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

  • Deleted the no-op hardfork reorder loop in 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 unused mega_mainnet_hardforks() template is gone with it.
  • Removed the never-used EIP-3155 trace_writer parameter threaded through replay_block / verify_and_replay / validate_block / validate_block_deriving_updates (no caller here or in mega-reth ever passed Some), which also deletes the run_plain closure + cfg dance. mega-reth impact: its call sites drop a trailing None at the next version bump.
  • WitnessExternalEnv::new / from_light_witness now share one from_metadata_kvs body; dead LightWitnessExecutor::kvs() accessor and a doubled doc block on create_evm_env removed.
  • Reuse: the blob base-fee fraction comes from revm's BLOB_BASE_FEE_UPDATE_FRACTION_CANCUN instead of a hand-copied consensus constant; gas_used reads BlockExecutionResult::gas_used instead of recomputing from the last receipt.
  • Hot path: WitnessDatabase reads use salt's find() + in-place decode and stack-allocated key encodings (pub(crate) helpers encode() also delegates to) — two heap allocations per witnessed state read and one per bucket_id_for_* call removed; dropped a per-miss SaltValue clone in LightWitness::metadata.
  • Pipeline: chain_advancer runs pre_advance + advance_chain under spawn_blocking so redb commits (and the trace server's multi-MB block-data writes) no longer pin an async runtime worker; panics still propagate unchanged via resume_unwind. Also removed the in_flight_blocks mirror set, the metas lockstep vector, and the dead _reason parameter.
  • ChainStore no longer carries the ContractStore supertrait (nothing consumed contracts through it); the forced stubs on the pipeline mock and the trace server's StubBlockStore are deleted.

stateless-validator + shared crates

  • R2 mode no longer hands the data endpoints to RpcClient as 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 retrying mega_getBlockWitness against data endpoints. The trace server is unaffected (clap already enforces a witness source at startup).
  • BackoffPolicy::schedule() / BackoffSchedule in stateless-common now own the jittered-doubling arithmetic previously duplicated between the RPC round-robin loop and the R2 GET loop; both consume it (fastrand dep dropped from the bin).
  • install_prometheus_exporter hoisted into stateless_common::metrics; both binaries call it and the metrics-exporter-prometheus dep moved from the bins into common.
  • BlockMeta::from_header added in core, replacing the three defaulting header→meta projections (validator + two in debug-trace-server); the strict anchor-init site in app.rs keeps its explicit missing-withdrawals-root error by design.
  • verify_block_integrity now 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.
  • Validator cleanups: on_remote_height fn-pointer injection removed; run_with_signals takes report_validation: bool instead of an Option<String> it only ever .is_some()'d; phantom eth_getTransactionByHash metric series dropped; test-only set_anchor_block deleted (the roundtrip test now exercises reset_to_anchor); workers.rs renamed to runner.rs (docs updated); integration mock handlers deduped onto shared lookup helpers (also fixing a drifted bare unwrap on malformed params); make_block_meta test fixture unified.

Testing

cargo fmt / clippy --workspace --all-targets --all-features (0 warnings) / cargo sort clean; full workspace suite 328 passed / 0 failed; no-std check and no-std test compile (--no-default-features) pass.

Notes

  • The empty-witness-list change relaxes a public RpcClient constructor check (startup rejection → loud panic at first misuse); the constructor test pins the new contract.
  • Known follow-up (next PR): hoist the block-execution env assembly shared by replay_block and the trace server's TracingEnv::new, resolving their BlockLimits drift.

🤖 Generated with Claude Code

… binaries

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mega-maxwell

mega-maxwell Bot commented Aug 1, 2026

Copy link
Copy Markdown

Claude review status

Living comment — rewritten in place. The review workflow keeps this single comment up to date instead of posting a new one each round, so it always describes the latest reviewed commit and the earlier text is intentionally gone. No reply is needed here; reply to a finding in its own review thread, and answer an open question in a reply on this PR. The next review round reconciles your answer.

🛠️ Review did not finish

Attempted head 9549ffff · updated 2026-08-02T07:46:23+00:00

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

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.13208% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.9%. Comparing base (682ec1a) to head (9549fff).

Files with missing lines Patch % Lines
crates/stateless-core/src/db.rs 0.0% 8 Missing ⚠️
crates/stateless-core/src/evm_database.rs 94.7% 1 Missing ⚠️
crates/stateless-core/src/light_witness.rs 0.0% 1 Missing ⚠️

☔ View full report in Codecov by Harness.
📢 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment on lines +510 to +514
tokio::task::spawn_blocking(move || {
verify_block_integrity(&block)?;
Ok(block)
})
.await

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant