bench: default CI benches to ethrex 100tx continuations; widen rkyv pointers for >2 GiB proofs - #867
bench: default CI benches to ethrex 100tx continuations; widen rkyv pointers for >2 GiB proofs#867ColoCarletti wants to merge 6 commits into
Conversation
|
/ai-review |
Codex Code Review
|
Review — bench workload defaults + rkyv
|
AI ReviewPR #867 · 8 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-001: Inconsistent pair-count rounding between CPU and GPU ABBA workflows
Claim bench-abba.yml clamps pair count to [2,40] but does not round odd values to even, while benchmark-gpu.yml does. Odd pair counts unbalance the AB/BA drift-cancellation pattern. Evidence In bench-abba.yml, after the diff: Suggested fix Add the same odd-pair rounding block from benchmark-gpu.yml (lines 144-147) to bench-abba.yml after the PAIRS clamp, so odd counts are silently rounded up to the next even number for both workflows. AI-004: bench-abba.yml timeout ceiling leaves minimal headroom at max pairs
Claim 600-minute timeout for 40 pairs of 100tx continuations (~7 min/prove = 560 min) leaves only ~40 min for dual builds and overhead. A slower runner or transient delay could hit the ceiling and kill the run. Evidence Workflow comment states 100tx-continuations prove is ~7 min on 16-core; 40 pairs = 80 proves = 560 min. Builds add ~10-20 min. Timeout is 600 min (line 35). Suggested fix Increase timeout to 660-720 minutes, or reduce max pairs clamp from 40 to 36 to restore headroom. AI-006: Stale '>=96GB RAM' in offer-failure error message inconsistent with the 48 GB filter
Claim The offer-search error message still reports the old '>=96GB RAM' floor while the actual query filters cpu_ram>=48 and this PR's updated comment now documents 48 GB, so a failed offer search misleads the operator about the real constraint. Evidence The PR rewrites the comment above the QUERY to explain the 48 GB floor (continuations ~10 GB, legacy 5tx monolithic fits, widens dedicated pool ~15 vs ~11), but the echo '::error::No RTX 5090 offer matched ... >=96GB RAM ...' line below was left unchanged, so the user-visible failure text contradicts both the query (cpu_ram>=48) and the new comment. Suggested fix Update the error message to match the live filter and comment: "(... >=16 cores, >=48GB RAM, >=64GB disk, driver>=${MIN_DRIVER}, <= $${PRICE_CAP}/hr)". Reviewer Lanes
Verification Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Discarded candidates (4) — rejected by the verifier
Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
There was a problem hiding this comment.
⚠️ AI-generated review. Produced by an automated agent and posted from this account — not written by hand. Findings below are leads to verify, not confirmed conclusions.
This PR turns on rkyv's pointer_width_64 across the proof-format crates, bumps RECURSION_INPUT_VERSION, and adds a CI pre-flight that blocks bench runs on branches predating the change. Both halves are internally consistent: the feature is on all six crates that declare rkyv, host and guest compile RECURSION_INPUT_VERSION from the same constant, every proof read stays on validated rkyv::access/from_bytes so old-format bytes yield an Err rather than a panic or unchecked access, the now-64-bit archived table dimensions remain gated by checked_mul plus the exact AIR-derived equality checks, and token parsing validates digits-only before every [ -lt ]. The findings below are workflow-side, plus one missing guard on the invariant the PR turns on.
.github/workflows/bench-abba.yml:102-112 (duplicated at benchmark-gpu.yml:135-146) — the pre-flight does not fail open on a failed fetch, contrary to its comment ("Skip if the fetch fails"). gh api prints HTTP error bodies to stdout, so on any 404/403/5xx SIDE holds a JSON error blob rather than the empty string: [ -n "$SIDE" ] passes, grep -q pointer_width_64 misses, and the job hard-exits with PR branch predates the rkyv pointer_width_64 fix without having read anything. Reproducible against this repo — gh api "repos/$GITHUB_REPOSITORY/contents/prover/Cargo.toml?ref=<bad-sha>" -H "Accept: application/vnd.github.raw" 2>/dev/null || true returns a {"message":"No commit found for the ref ...","status":"404"} body on stdout. A rate-limited token or a transient contents-API failure therefore turns the default cont100 bench into a hard block that blames the author for a stale branch. Fix: branch on gh's exit status (if SIDE=$(gh api ...); then ... fi) and/or require the payload to look like the file (e.g. it also contains rkyv) before treating a missing string as evidence the branch is stale. Secondary: the check is purely textual on one file's contents, so it also matches the feature name appearing only in the explanatory comment added above the dep; the whole check is deletable once every open branch postdates this merge.
.github/workflows/bench-abba.yml:38 — the Acknowledge step still runs before the cfg step (52-124), the reverse of benchmark-gpu.yml, which this PR restructured so config (71) precedes ack (163) and the notice interpolates steps.config.outputs.pairs/workload. Two effects. The notice is a hardcoded literal, so /bench-abba 4 cont10 still gets "~4-5 hr at the default 20 pairs of ethrex 100tx continuations; pass a smaller pair count or cont10 for a quicker run" — hedged by "at the default" so not false, but uninformative; pre-existing, widened by the new workload axis. New here: the fail-fast at line 110 fires after the "bench server is occupied until it finishes" comment, which — unlike the GPU workflow's marker-based update-in-place — is a plain createComment that is never retracted, and it is the default path (cont100 ⇒ TX_COUNT >= 40) for any un-rebased branch. The always() Post result step then posts a second comment headed `<sha>` vs `main` ( pairs, ethrex) — pairs/workload are written at 118-123, after the exit 1, while head_sha lands at 63 before it — whose "Last log lines" block tails /tmp/abba_out.txt from a previous run, since the bench runner is persistent self-hosted and nothing cleans /tmp. Fix: move Acknowledge after cfg and interpolate steps.cfg.outputs.pairs/workload; the cfg step costs seconds, and the fail-fast then exits before any occupancy claim is posted. Optionally write the outputs block before the fail-fast (or rm -f /tmp/abba_out.txt at job start) so the failure comment is not half-blank with a stale log.
prover/Cargo.toml:27-29 — the pointer_width_64 invariant is held only by the "Keep in sync across all proof-format crates" comment plus five copies of it. Nothing in code or CI pins it: no const assert, no test, and RECURSION_INPUT_VERSION cannot help, since host and guest compile it from the same source (prover/src/lib.rs:214, checked at :265) — a rebuilt-but-misconfigured binary still advertises v2. rkyv compile_error!s on conflicting pointer-width features, but with none enabled it silently defaults to u32. Feature unification makes dropping it from a single main-workspace crate harmless; the two cases that bite are losing it from a whole graph (hoisting rkyv into [workspace.dependencies], or a version bump that drops the feature list) and the separate non-member bench_vs/lambda/recursion guest workspace drifting from the host. Both stay green in CI, since every existing test round-trips archives that fit 32-bit offsets, and only resurface as a serialization failure at the end of a >2 GiB prove. Mirror the const _: () = { assert!(...) } pattern already at prover/src/lib.rs:216-229: const _: () = assert!(size_of::<rkyv::primitive::ArchivedUsize>() == 8, "proof format requires rkyv pointer_width_64");. That compiles into both the host and the riscv64 guest graph (the guest path-depends on lambda-vm-prover, whose rkyv dep is non-optional), so it covers the drift the CI grep cannot see; placing it in crypto/math or crypto/stark under their rkyv feature would additionally cover standalone builds of the crates that define the archived types.
Nits:
- prover/src/lib.rs:257 — no test covers the prefix rejection path (
Noneon a short blob / wrong magic / wrong version, surfaced asError::Execution("recursion blob: bad magic or version")at :286, continuation twin at recursion.rs:312); the three tests in prover/src/tests/recursion_smoke_test.rs (600, 649, 711) all leave the 12-byte prefix intact. Pre-existing (only the constant's value changes) and bounded to a less legible bytecheckErr, but worth closing since the breaking-change note leans on clean rejection and blobs are persisted (test_dump_recursion_inputwrites/tmp/recursion_input.bin, prebuiltrecursion-*.elfguests are cached);recursion_archive_bytesis pure, so a test needs no proving — fabricate a 12-byte prefix plus dummy bytes and assertNonefor (a) version patched to1u32LE at bytes 4..8, (b) a flipped magic byte, (c) a blob shorter thanRECURSION_INPUT_PREFIX_LEN. - prover/src/lib.rs:256 — the doc comment claiming an
access_uncheckedfollows is stale; there is none in tree.
What
/bench-gpuand/bench-abbanow default to ethrex 100tx--continuations(the production proving mode). New syntax:/bench-gpu [N] [cont[TX]|mono[TX]]—mono5keeps the legacy monolithic workload.pointer_width_64: the default 32-bit offsets cap an archive at ~2 GiB, and the 100tx continuation proof is 3.13 GiB — proving worked, writing the file panicked. Proof size +2%, proving/verify time unchanged.Breaking
Proof files serialized before this change can't be read by new binaries (and viceversa).
RECURSION_INPUT_VERSIONbumped 1→2 so stale recursion blobs are rejected cleanly. No proofs are persisted in-repo or in CI.Validation (16-core + RTX 5090)
Notes
contbench ≥40tx; the config step now fails fast with a clear message instead of burning the build.