Feat/skip empty tables - #977
Conversation
|
/ai-review |
Codex Code ReviewNo actionable issues found in the PR diff. Reviewed statically; no builds or tests were run. |
|
Review: skip empty tables Read the full diff plus the surrounding prover/verifier/bus machinery. The core design holds up. I traced the soundness argument end to end and it checks out:
Findings (all in tests / comments — no product-code defects) High — the test the soundness argument cites does not cover the tables it claims. Medium — negative tests can pass for the wrong reason. The new Medium — Low — the helper is a near-verbatim copy of Low — comment split. The new continuation test lands between the two halves of Low / follow-up — KECCAK_RC is still in the fixed set. When Not findings, noted for the record
|
AI ReviewPR #977 · 10 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-001: Disk-spill RAM estimator missing optional accelerator/auxiliary tables
Claim The disk-spill peak-RAM estimator in auto_storage.rs is out of sync with the new optional table model. table_specs only accounts for CPU/MEMW/MEMW_A/MEMW_R/LOAD/LT/SHIFT/MUL/DVRM/BRANCH/COMMIT, BITWISE/DECODE/HALT/REGISTER and PAGE; it omits EQ, BYTEWISE, STORE, CPU32, KECCAK, KECCAK_RND, ECSM, ECDAS and HINT. count_table_lengths likewise does not track row counts for those tables. Because the PR moves several accelerator tables from fixed to optional, programs that actually use them will have their memory footprint underestimated, which can cause auto_storage::decide to select StorageMode::Ram when StorageMode::Disk is required and lead to OOM during proving. Evidence In prover/src/auto_storage.rs, table_specs builds specs from TableLengths fields (cpu_padded_rows, memw_padded_rows, etc.) and never references eq, bytewise, store, cpu32, keccak, keccak_rnd, ecsm, ecdas or hint. In prover/src/tables/trace_builder.rs, count_table_lengths returns TableLengths which lacks those fields and only counts commit_padded_rows among the newly-optional accelerator/auxiliary tables. The comment in table_specs says 'Per-table specs in the same order as air_trace_pairs in prove', but air_trace_pairs (lib.rs) now emits commits, keccaks, keccak_rnds, ecsms, ecdases, hints, eqs, bytewises, stores and cpu32s, none of which are reflected in the RAM estimate. Suggested fix Extend TableLengths in trace_builder.rs/count_table_lengths to count rows for eq, bytewise, store, cpu32, keccak, keccak_rnd, ecsm, ecdas and hint (mirroring how commit_padded_rows is already tracked), then extend auto_storage.rs/table_specs to include corresponding TableSpec entries so peak_bytes accounts for every table that air_trace_pairs can produce. AI-002: Validation relaxation shifts trust from structural checks to bus balance
Claim TableCounts::validate() now only requires cpu>0 and memw_register>0, allowing all other 18 tables to have zero counts. Security for omitted tables relies entirely on LogUp bus balance checks. Evidence The validate() function at lines 171-181 only checks cpu and memw_register. The comment acknowledges: 'What keeps a zero count honest is the LogUp bus, not this check.' The new tests every_table_participates_in_the_bus and no_present_table_contributes_zero_to_the_bus (in skip_empty_tables_tests.rs) verify this property for current tables, but this is a significant trust model change. Suggested fix Consider adding a debug-only assertion or a #[cfg(debug_assertions)] check that validates all tables present in the trace have non-zero counts, to catch accidental omissions during development. The current design is intentional and tested, but the trust shift should be explicitly documented in the security model. AI-005: Test helper panics on prover failure rather than returning false
Claim prove_and_verify_vm_minimal and weigh_the_bus now panic if the prover fails, which changes the contract of several existing negative tests. While the new helpers document that false is reserved for verifier rejection, callers that previously expected a proving failure to return false will now panic. None of the current callers appear to rely on proving failure, but the helper is pub(crate) and a future test that passes a malformed trace expecting a prover-side rejection could abort the test suite instead of failing gracefully. Evidence In prover/src/tests/prove_elfs_tests.rs, weigh_the_bus matches multi_prove_ram with Ok(proof) => proof and Err(e) => panic!(...). prove_and_verify_vm_minimal wraps weigh_the_bus with recheck_with_moved_target=false. All current negative tests use weigh_the_bus(..., true) after a successful honest proof and then mutate the trace, so they only exercise verifier rejection. The risk is future misuse of the pub(crate) helper. Suggested fix Keep the current panic behavior for weigh_the_bus (it is test-only and the distinction is valuable), but add a doc comment on the helper reminding callers that proving failures panic and that negative tests must mutate an already-proven trace. 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 (3) — rejected by the verifier
Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
|
/bench-verify |
|
⏳ Benchmark started on the bench server. Two verifier arms (monolithic + continuations over an ethrex 20-tx block), then the recursion-guest cycle comparison, which adds guest builds on top — longer on a cold runner. The bench server is occupied until it finishes. |
Verifier benchmark —
|
| Metric | main | PR | Δ |
|---|---|---|---|
| Verify time (ABBA, 20 pairs, per-side) | 2.508s | 2.525s | +0.70% 🔴 |
| Proof size (exact, 1 reading) | 102.33 MiB | 102.33 MiB | +0.00% ⚪ |
Per-side (PR can't deserialize the baseline's proof — proof-format change): A/B/B/A cancels machine drift but not proof-specific variance — read the Verify-time Δ as approximate.
pairs: 20 mean A (PR): 2.525s mean B (main): 2.508s
[parametric] paired-t mean +0.70% sd 0.78% se 0.17%
95% CI: [+0.34%, +1.06%] (t df=19 = 2.093)
[robust] median +0.80% Wilcoxon W+=185 W-=25 p(exact)=0.0017 (z=+2.97)
run-to-run jitter: A CV 0.60% B CV 0.45% (lower = steadier)
within-session drift: +0.38% over the run, 1st->2nd half +0.20%
🔴 REAL REGRESSION — PR verifies ~0.70% slower (paired-t and Wilcoxon agree).
ethrex 20-tx block · continuations, epoch 2^20 (3 epochs) · blowup=2, 219 queries
| Metric | main | PR | Δ |
|---|---|---|---|
| Verify time (ABBA, 8 pairs, per-side) | 3.152s | 3.173s | +0.65% 🔴 |
| Proof size (exact, 1 reading) | 177.07 MiB | 166.28 MiB | -6.10% 🟢 |
Per-side (PR can't deserialize the baseline's proof — proof-format change): A/B/B/A cancels machine drift but not proof-specific variance — read the Verify-time Δ as approximate.
pairs: 8 mean A (PR): 3.173s mean B (main): 3.152s
[parametric] paired-t mean +0.65% sd 0.54% se 0.19%
95% CI: [+0.20%, +1.11%] (t df=7 = 2.365)
[robust] median +0.53% Wilcoxon W+=36 W-=0 p(exact)=0.0078 (z=+2.45)
run-to-run jitter: A CV 0.54% B CV 0.58% (lower = steadier)
within-session drift: -0.46% over the run, 1st->2nd half -0.37%
🔴 REAL REGRESSION — PR verifies ~0.65% slower (paired-t and Wilcoxon agree).
Verify-time rows only: drift-free interleaved A/B/B/A, with paired-t and exact Wilcoxon — trust the verdict when the two agree. Proof sizes are single exact readings (no averaging). - = PR faster.
Recursion guest cycles — verifier running INSIDE the VM (main vs PR)
empty program · monolithic · blowup=2, 1 query (diagnostic — NOT a real verifier cost)
Single exact reading per ref — no ABBA: guest cycles are deterministic for a fixed
(guest ELF, input blob), so there is no machine drift to cancel.
| Metric | main | PR | Δ |
|---|---|---|---|
| Guest cycles | 32.1M | 3.7M | -28.5M (-88.63%) |
| Keccak calls | 3029 | 1109 | -1920 |
baseline origin/main 99d7567afe guest=recursion-min.elf
PR 3ae8bf2018e37b483349dfe5a89935a5dae698fc 3ae8bf2018 guest=recursion-min.elf
note: cycles reproduce to ~±100k (build codegen + proof nondeterminism);
treat sub-100k deltas as noise, not signal.
raw (exact integer counts)
ref_b_sha=99d7567afec95c672e78065bc2bacb4416fb7577 ref_b_elf=recursion-min.elf ref_b_cycles=32137866 ref_b_keccak=3029 ref_b_execute_wall_s=1
ref_a_sha=3ae8bf2018e37b483349dfe5a89935a5dae698fc ref_a_elf=recursion-min.elf ref_a_cycles=3652699 ref_a_keccak=1109 ref_a_execute_wall_s=0
delta_cycles=-28485167 delta_keccak=-1920
ethrex 20-tx block · continuations, epoch 2^21 (2 epochs) · blowup=2, 219 queries (128-bit)
Single exact reading per ref — no ABBA: guest cycles are deterministic for a fixed
(guest ELF, input blob), so there is no machine drift to cancel.
| Metric | main | PR | Δ |
|---|---|---|---|
| Guest cycles | 1910.0M | 1690.7M | -219.2M (-11.48%) |
| Keccak calls | 3296811 | 3213617 | -83194 |
baseline origin/main 99d7567afe guest=recursion-cont-blowup2.elf
PR 3ae8bf2018e37b483349dfe5a89935a5dae698fc 3ae8bf2018 guest=recursion-cont-blowup2.elf
note: cycles reproduce to ~±100k (build codegen + proof nondeterminism);
treat sub-100k deltas as noise, not signal.
raw (exact integer counts)
ref_b_sha=99d7567afec95c672e78065bc2bacb4416fb7577 ref_b_elf=recursion-cont-blowup2.elf ref_b_cycles=1909967824 ref_b_keccak=3296811 ref_b_execute_wall_s=31
ref_a_sha=3ae8bf2018e37b483349dfe5a89935a5dae698fc ref_a_elf=recursion-cont-blowup2.elf ref_a_cycles=1690738371 ref_a_keccak=3213617 ref_a_execute_wall_s=26
delta_cycles=-219229453 delta_keccak=-83194
|
/bench |
Benchmark — real block (
|
| Metric | main | PR | Δ |
|---|---|---|---|
| Peak heap | 46886 MB | 47467 MB | +581 MB (+1.2%) ⚪ |
| Prove time | 110.675s | 110.190s | -0.485s (-0.4%) ⚪ |
✅ No significant change.
Prove-time spread 0.7% (110.012s / 110.792s / 110.190s)
Commit: 3ae8bf2 · Baseline: cached · Runner: self-hosted bench
|
/ai-review |
Codex Code ReviewNo actionable issues found in the PR diff. Reviewed statically; no builds or tests were run. |
|
/bench |
|
/bench-verify |
|
⏳ Benchmark started on the bench server. Two verifier arms (monolithic + continuations over an ethrex 20-tx block), then the recursion-guest cycle comparison, which adds guest builds on top — longer on a cold runner. The bench server is occupied until it finishes. |
Every chip used to cost a padded four-row sub-proof — a full commitment, FRI chain and
OOD opening set — even when the program never executed one of its operations. A table a
run never reaches is now left out, and each epoch decides that on its own: a table absent
from one epoch still appears in another that uses it. Computing the set over the whole run
instead would drag every table used anywhere into every epoch, which is most of what there
is to win.
The twelve chunked chips get
chunk_and_generate_optional; CPU and MEMW_R stay mandatory.The six accelerators (COMMIT, KECCAK, KECCAK_RND, ECSM, ECDAS, HINT) get counts of their
own so they can be absent too, which drops
FIXED_TABLE_COUNTfrom 11 to 5 and takes thestatement encoding to V4. BITWISE, DECODE, KECCAK_RC, REGISTER, HALT, the PAGEs and L2G
stay always-present — they have no count for a prover to zero.
Soundness comes from the bus, not from the count check.
TableCounts::validateno longerrequires a positive count on the optional tables; what rejects a proof that declares away a
table whose operations ran is the LogUp balance, which sums the contribution of every AIR
with a trace interaction and stops matching once the CPU's sends have no receiver.
validatestill refuses a proof with no CPU or no register file, and the declared countsare still cross-checked against the sub-proof count — with a checked sum, because they are
prover-supplied and a wrapped total would let one field stay large enough to size a
Vecfrom.
Tested in both directions:
be dropped: MUL, HINT, KECCAK_RND (dispatched by another chip rather than by the CPU),
and COMMIT — the one table whose bus target is not zero — both on its own and together
with the output that sets that target.
is re-verified with the target moved onto the forged sum, and has to pass.
zero, which is the property that would let one be dropped unnoticed however many rows it
carries.
scheme produces. An epoch that declares away a table it carries is rejected on both
branches of the cross-check, and a bundle whose table set varies by epoch survives the
recursion path. Every field of the V4 encoding is shown to move the transcript.