Skip to content

feat(tooling): real ethrex block fixture - #880

Closed
nicole-graus wants to merge 8 commits into
mainfrom
feat/ethrex-real-block
Closed

feat(tooling): real ethrex block fixture#880
nicole-graus wants to merge 8 commits into
mainfrom
feat/ethrex-real-block

Conversation

@nicole-graus

@nicole-graus nicole-graus commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Motivation

Every ethrex benchmark uses a synthetic block of N plain ETH transfers. Measured
against a real block, that workload is inverted:

ethrex_bench_20 (synthetic) Hoodi 1265656 (real)
cycles 9,063,727 147,482,439 (16.3×)
keccak calls 411 9,046 (22×)
ECSM calls 80 44 (0.55×)
keccak : ECSM 5.1 : 1 205.6 : 1
gas 420,000 4,402,947

The headline benchmark is ecrecover-dominated; a real block is keccak/trie dominated. The real block makes fewer ECSM calls despite 10× the gas.

Description

  • tooling/ethrex-real-block: reads an ethrex-replay cache JSON with our pinned ethrex types and writes the rkyv ProgramInput the guest consumes. Reading the JSON rather than ethrex-replay's own .bin is deliberate — it tracks ethrex main, where ProgramInput has diverged (extra field, moved module, rkyv 0.8.10 vs our =0.8.16), so its binary would not deserialize in our guest.

  • real_block_executes_under_guest_crypto (tooling/ethrex-real-block): executes the block through LambdaVmEcsmCrypto, the Crypto impl the guest injects. execute_blocks validates the witness header chain, the initial and final state roots, and per block validate_gas_used, validate_receipts_root and validate_requests_hash — so a missing precompile is caught on gas without needing state to diverge.

  • no_kzg_backend_linked (tooling/ethrex-tests): the screen for "does this block need an accelerator we don't have". KZG point evaluation (0x0a) is the only precompile in ethrex-crypto whose availability is feature-gated, and that crate links no KZG backend, so a block calling 0x0a fails test_ethrex_real_block_native there. This canary pins that property rather than leaving it incidental to the dependency graph. Note the converter crate does not screen 0x0a — it links c-kzg transitively via ethrex-config.

  • unmappable_network_is_rejected: ethrex-replay writes network: "LocalDevnet" for any chain outside mainnet/Hoodi/Sepolia, and that resolves to chain_id 9 with every fork active from timestamp 0. Converting under it would replay the block against invented rules and pass every check, since the witness is only validated against whichever config we chose. The converter now refuses.

  • conversion_is_reproducible: asserts the fixture's sha256, not its length — a layout change can preserve the byte count exactly (ChainConfig is fixed-size; rkyv's big_endian feature would byte-swap in place).

  • make ethrex-real-block-fixture. Fixture is ~1 MB and gitignored; the cache URL is pinned to a commit so the input can't change under a fixed name.

How to test

make ethrex-real-block-fixture
cd tooling/ethrex-real-block && cargo test --release

Using the fixture

Execute and count cycles (this is what the numbers above come from):

./target/release/cli execute executor/program_artifacts/rust/ethrex.elf \
    --private-input executor/tests/ethrex_hoodi_1265656.bin --cycles

Cycle breakdown by function:

./target/release/cli execute executor/program_artifacts/rust/ethrex.elf \
    --private-input executor/tests/ethrex_hoodi_1265656.bin \
    --flamegraph /tmp/hoodi.folded

Proving is untested at this size — expect to need continuations for flat peak
memory, and don't assume a plain prove fits in RAM:

./target/release/cli prove executor/program_artifacts/rust/ethrex.elf \
    -o /tmp/hoodi.proof --private-input executor/tests/ethrex_hoodi_1265656.bin \
    --continuations --epoch-size-log2 21 --time

@nicole-graus

Copy link
Copy Markdown
Collaborator Author

/ai-review

@github-actions

Copy link
Copy Markdown

Codex Code Review

  • MediumMakefile: The generated fixture depends only on the cache and main.rs. Changes to Cargo.toml/Cargo.lock, especially the ethrex or rkyv pins, can leave an existing gitignored fixture stale and schema-incompatible while make ethrex-real-block-fixture reports success. Add both Cargo files as prerequisites, or always regenerate the fixture.

Comment thread tooling/ethrex-real-block/Cargo.toml Outdated
Comment thread Makefile
Comment thread Makefile
Comment thread tooling/ethrex-real-block/src/main.rs Outdated
Comment thread tooling/ethrex-real-block/src/main.rs
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review — real ethrex block fixture

Read the whole diff statically (converter, Makefile wiring, CI, both new tests). No safety/memory-safety issues: no unsafe, no VM/GPU/executor code touched, and the converter is a host CLI whose failure modes are all ?-propagated into a nonzero exit. The design call I'd most want to defend — read the version-tolerant cache JSON with our pinned ethrex types instead of ethrex-replay's .bin — is the right one and well documented. Findings below, none Critical/High.

Medium

  • real_block_executes_under_guest_crypto is the load-bearing claim in this PR, and its parity with the guest is weaker than stated: this crate adds ethrex-common + ethrex-config with default features on top of the guest's dep set, so an ethrex-crypto feature the guest doesn't enable (c-kzg, secp256k1) can unify in and widen the precompile surface. Inline on Cargo.toml:12 with the cargo tree -e features check.

Low

  • Cache download isn't atomic and the Makefile has no .DELETE_ON_ERROR: — a dropped connection wedges the target permanently. Inline suggestion on the rule.
  • make test-ethrex keeps --include-ignored, so it runs test_ethrex_real_block_vm, the test whose stated intent is "build server only". Inline on Makefile:303.
  • conversion_is_reproducible pins byte length, not bytes; the README's sha256 is checked by nothing. Inline on the test.
  • ETHREX_REAL_BLOCK ?= 1265656 invites an override the rest of the wiring can't honor: the cache URL hardcodes hoodi, and both converter tests hardcode the caches/cache_hoodi_1265656.json path plus block-number/tx/gas asserts. Overriding it produces a confusing test failure rather than a different fixture. Either drop the ?= (make it a deliberate edit, as the README's "add a rule alongside it" suggests) or let the tests take the cache path from an env var.
  • CI cost, worth a conscious decision rather than a fix: the two new steps land in the required Executor tests job and add (a) a raw.githubusercontent.com fetch to every PR run — a transient 5xx there is now a red required check — and (b) a full uncached compile of the ethrex host tree in a third detached workspace. Swatinem/rust-cache in that job has no workspaces: list, so tooling/ethrex-real-block/target (like tooling/ethrex-tests/target today) is never cached. Adding workspaces: | with ., tooling/ethrex-real-block, tooling/ethrex-tests, and an actions/cache on tooling/ethrex-real-block/caches keyed on ETHREX_REPLAY_REV, would pay for itself.
  • Cache silently ignores the cache's chain_config and derives it from network.get_genesis(). Correct for the pinned Hoodi block; for a devnet cache it would either error on a missing genesis file or quietly use a config that isn't the one the block was produced under. Fine to leave, but the doc comment currently frames the omission as harmless.

Nits

  • tooling/ethrex-tests/tests/ethrex.rs:43-44test_ethrex's doc still says it "Replaces the old ethrex_hoodi.bin real-block fixture, which … no longer deserializes." A real-block fixture exists again as of this PR; the sentence now reads as if real blocks are unavailable.
  • The headers reference in the into_execution_witness comment points at a field Cache doesn't have (inline).
  • The two new tests in ethrex.rs are near-identical bodies, but that matches the three tests already there, so leaving them alone is the consistent choice.

The "verified, not assumed" framing on the precompile gate is the right standard, and the caveat about it never having rejected anything is the kind of thing reviewers usually have to dig for — thanks for stating it up front.

@github-actions

Copy link
Copy Markdown

AI Review

PR #880 · 10 changed files

Findings

Status Sev Location Finding Found by
confirmed medium Makefile:303 make test-ethrex now requires network access on first run nemotron
openrouter/nvidia/nemotron-3-ultra-550b-a55b
kimi
openrouter/moonshotai/kimi-k2.7-code
confirmed medium Makefile:304 make test-ethrex runs the unmeasured heavy VM test that CI deliberately skips glm
openrouter/z-ai/glm-5.2
confirmed low .github/workflows/pr_main.yaml:107 CI does not cache the downloaded ethrex-replay cache (~1.5 MB) nemotron
openrouter/nvidia/nemotron-3-ultra-550b-a55b
confirmed low .github/workflows/pr_main.yaml:107 Converter tool built and run twice in CI nemotron
openrouter/nvidia/nemotron-3-ultra-550b-a55b

Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro).

AI-003: make test-ethrex now requires network access on first run
  • Status: confirmed
  • Severity: medium
  • Location: Makefile:303
  • Found by: nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b, kimi:openrouter/moonshotai/kimi-k2.7-code
  • Verified by: deepseek-verifier:openrouter/deepseek/deepseek-v4-pro
  • Rejected by: -

Claim

The test-ethrex target now depends on $(ETHREX_REAL_BLOCK_FIXTURE), which transitively downloads the ethrex-replay cache from GitHub. This breaks offline builds and surprises developers who previously could run ethrex tests without network.

Evidence

Line 303: test-ethrex: compile-programs-rust $(ETHREX_REAL_BLOCK_FIXTURE). The fixture rule (lines 297-299) depends on $(ETHREX_REAL_BLOCK_CACHE) which downloads via curl (lines 292-295). First run of make test-ethrex will hit the network.

Suggested fix

Document the network requirement in the target's comment, or add a separate test-ethrex-offline target that skips the real-block test. Alternatively, make the real-block fixture an optional dependency.

AI-004: `make test-ethrex` runs the unmeasured heavy VM test that CI deliberately skips
  • Status: confirmed
  • Severity: medium
  • Location: Makefile:304
  • Found by: glm:openrouter/z-ai/glm-5.2
  • Verified by: deepseek-verifier:openrouter/deepseek/deepseek-v4-pro
  • Rejected by: -

Claim

make test-ethrex runs cargo test --release -- --include-ignored without --skip test_ethrex_real_block_vm, so the new heavy/ignored test_ethrex_real_block_vm test runs locally even though the PR deliberately excluded it from CI. The test's own doc comment says it should be opt-in ("Deliberately excluded from the PR CI step ... opt-in until we know what it does to job time"), and CI adds --skip test_ethrex_real_block_vm precisely to keep it out — but the Makefile target that is the local equivalent of that CI step was not given the same skip.

Evidence

Makefile line 303-304: test-ethrex: compile-programs-rust $(ETHREX_REAL_BLOCK_FIXTURE) then cd tooling/ethrex-tests && cargo test --release -- --include-ignored (no --skip test_ethrex_real_block_vm). Compare pr_main.yaml which runs cargo test --release -- --include-ignored --skip test_ethrex_real_block_vm. The new test_ethrex_real_block_vm in ethrex.rs (line 127-138) is #[ignore]d, so --include-ignored picks it up via make test-ethrex.

Suggested fix

Mirror CI in the Makefile: cd tooling/ethrex-tests && cargo test --release -- --include-ignored --skip test_ethrex_real_block_vm, or split into a test-ethrex-all target that includes it and keep test-ethrex skipping it.

AI-006: CI does not cache the downloaded ethrex-replay cache (~1.5 MB)
  • Status: confirmed
  • Severity: low
  • Location: .github/workflows/pr_main.yaml:107
  • Found by: nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b
  • Verified by: deepseek-verifier:openrouter/deepseek/deepseek-v4-pro
  • Rejected by: -

Claim

The CI step 'Generate ethrex real-block fixture' runs make ethrex-real-block-fixture which downloads the cache every workflow run. No actions/cache step stores the downloaded JSON, wasting bandwidth and time.

Evidence

Line 107-108: the step runs make ethrex-real-block-fixture which invokes the curl download. The workflow caches cargo artifacts and ELF artifacts but not the ethrex-replay cache JSON.

Suggested fix

Add a cache step for tooling/ethrex-real-block/caches/ with key based on ETHREX_REPLAY_REV and ETHREX_REAL_BLOCK, similar to how ELF artifacts are cached.

AI-007: Converter tool built and run twice in CI
  • Status: confirmed
  • Severity: low
  • Location: .github/workflows/pr_main.yaml:107
  • Found by: nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b
  • Verified by: deepseek-verifier:openrouter/deepseek/deepseek-v4-pro
  • Rejected by: -

Claim

The CI runs make ethrex-real-block-fixture (which builds and runs the converter via cargo run), then immediately runs cd tooling/ethrex-real-block && cargo test --release (which rebuilds and runs the converter's tests). This duplicates work.

Evidence

Line 108: make ethrex-real-block-fixture expands to cd tooling/ethrex-real-block && cargo run --release -- .... Line 115: cd tooling/ethrex-real-block && cargo test --release. The converter binary is rebuilt unless cargo cache hits, but the test step doesn't benefit from the fixture step's build.

Suggested fix

Minor optimization: in the fixture step, use cargo build --release then run the binary directly, or restructure so the test step reuses the built binary. Not critical given small binary size.

Reviewer Lanes

Lane Model Prompt Status Findings
glm openrouter/z-ai/glm-5.2 general success 2
kimi openrouter/moonshotai/kimi-k2.7-code general success 4
minimax minimax/MiniMax-M3 general error: opencode failed (provider/auth/runtime error) and no findings were submitted 0
moonmath zro/minimax-m3 general success 1
nemotron openrouter/nvidia/nemotron-3-ultra-550b-a55b general success 4

Verification Lanes

Lane Model Status Confirmed Rejected Uncertain
deepseek-verifier openrouter/deepseek/deepseek-v4-pro success 4 3 0

Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report.

Discarded candidates (3) — rejected by the verifier
  • Network-downloaded cache is not integrity-checked (Makefile:292, found by kimi:openrouter/moonshotai/kimi-k2.7-code, nemotron:openrouter/nvidia/nemotron-3-ultra-550b-a55b, glm:openrouter/z-ai/glm-5.2, moonmath:zro/minimax-m3) — The cache is downloaded from a GitHub raw URL pinned to a specific commit hash (2693e0182a8734117151d8ea2891eda5afc60383). Git commit hashes are cryptographically immutable — you cannot force-push to change what content a given commit hash resolves to on GitHub. The URL is also HTTPS, providing integrity in transit. The README (lines 68-70) explicitly documents this design choice: "the pinned commit is what makes the input immutable, and conversion_is_reproducible pins the block's stats and serialized length." The test at main.rs:134 asserts the exact byte length (1,021,207), so any corruption would fail the test. The comparison to prepare-sysroot is imperfect — sysroot downloads from a URL that could be re-tagged, whereas a commit hash is content-addressed.
  • Serialized length assertion will fail on different platforms (tooling/ethrex-real-block/src/main.rs:134, found by kimi:openrouter/moonshotai/kimi-k2.7-code) — The rkyv dependency in Cargo.toml line 24 explicitly uses features = ["std", "unaligned"]. The unaligned feature is specifically designed to produce bit-for-bit identical output across platforms by eliminating alignment-dependent padding from the serialized format. The finding's claim that "rkyv's serialized size is not guaranteed to be bit-for-bit identical across different platforms" is incorrect when the unaligned feature is enabled — that is its entire purpose. The README correctly states "Output is deterministic for a given cache file." The length assertion at line 134 would only change if the rkyv layout of ethrex types changed (e.g., an ethrex rev bump), which is exactly what the test is meant to catch.
  • CLI hard-codes default cache filename in test but not in cache rule (tooling/ethrex-real-block/src/main.rs:101, found by kimi:openrouter/moonshotai/kimi-k2.7-code) — The test at main.rs line 101 hardcodes const CACHE: &str = "caches/cache_hoodi_1265656.json" which matches the default Makefile value (ETHREX_REAL_BLOCK defaults to 1265656). The conversion_is_reproducible test (line 131) explicitly asserts assert_eq!(summary.first_block_number, 1_265_656) — it is a regression test for one specific known-good block, not a generic test. If someone overrides ETHREX_REAL_BLOCK to a different block, they would naturally update the test constants alongside. This is how hardcoded test fixture paths work; it is not a bug. The test cannot generically validate any arbitrary block because block stats (gas, transaction count) differ per block.

Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts.

… sha256 not length, skip heavy VM test in local target, and add offline variant
@nicole-graus
nicole-graus marked this pull request as ready for review July 30, 2026 17:39
@MauroToscano

Copy link
Copy Markdown
Contributor

Superseded by #884, which includes every commit from this branch (the branch was forked from it and later merged with current main) plus the benchmark wiring, fixture hosting, and measured calibration that grew out of the review. Closing in favor of that PR — the validation work here carries over unchanged.

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.

5 participants