refactor(stark): unify & clean up the commitment layer#691
refactor(stark): unify & clean up the commitment layer#691diegokingston wants to merge 14 commits into
Conversation
Unifies the two near-identical bit-reversed leaf hashers (per-row + per-row-pair) into one keccak_leaves_bit_reversed_grouped(columns, rows_per_leaf), and the two near-identical commit fns (commit_columns_bit_reversed + commit_composition_polynomial) into commit_bit_reversed(columns, rows_per_leaf), in a new crypto/stark/src/commitment.rs. Removes them from the IsStarkProver trait (they used no self). prover.rs re-exports the named leaf hashers for the math-cuda GPU parity test. Byte-identical (stark 128/128).
Every FRI query opens a value and its symmetric counterpart (LDE positions 2*iota, 2*iota+1). The composition-poly commitment already grouped that pair into one leaf; the trace (main/aux/precomputed) committed one row per leaf and opened TWO leaves (proof + proof_sym) per query. Commit the trace with rows_per_leaf=2 too, so each query opens ONE leaf/path; drop the now-redundant proof_sym from PolynomialOpenings (also shrinks the composition opening, which stored the path twice). verify_opening_pair now reconstructs the paired leaf and verifies once (mirrors verify_composition_poly_opening); the dead verify_opening is removed. Halves trace Merkle authentication-path data per query (smaller proofs + less verifier hashing). stark 128/128 (prove+verify). NOTE: proof FORMAT change (not byte-identical). cuda follow-up: the GPU trace leaf+tree builders (gpu_lde::try_expand_leaf_and_tree_batched_keep/_ext3_keep + math-cuda kernels) still build 1-row leaves and must switch to the row-pair pattern (the GPU composition builder already pairs) or cuda proofs will fail verification.
…ropagation, doc/log fixes
The trace commitment now uses the row-pair leaf layout (ROWS_PER_LEAF=2), same as composition; rows_per_leaf=1 is only kept for the GPU parity test. Update the module/const/wrapper docs that still described the pre-pairing per-row trace.
Extract two helpers on IsStarkProver, collapsing the near-duplicate main-trace and aux-trace commit code: - spill_tree<C>: the identical-except-label disk-spill block, shared by the main / preprocessed-split / aux commit sites (4 call sites). - commit_plain<C>: commit_bit_reversed + spill_tree + TableCommit::plain, shared by the main-trace (non-preprocessed None arm) and aux-trace plain-commit paths. Proof output is byte-identical (stark 128/128, +disk-spill 133/133). The only behavioral delta is in the instruments profiling feature: the aux commit's timing bucket now includes its (tiny) disk spill, matching what the main path already measured. clippy clean on default / disk-spill / instruments; builds on the combined feature set.
diegokingston
changed the title
The trace Merkle commitment moved to a row-pair leaf layout (ROWS_PER_LEAF=2), but the 5 preprocessed-table commitment computers (bitwise/keccak_rc/page/decode/register) still built a 1-row-per-leaf tree manually, so the prover's row-pair precomputed root no longer matched the computed/hardcoded one -> PrecomputedCommitmentMismatch at prove time. - Route all 5 compute_*commitment fns through the shared stark::commitment::commit_bit_reversed(.., ROWS_PER_LEAF), dropping the manual bit-reverse + columns2rows + 1-row BatchedMerkleTree::build. - Regenerate the hardcoded bitwise/keccak_rc/zero_page static commitments for the row-pair layout (via compute_static_commitments). - cargo fmt (prover.rs + touched tables). Verified: static_commitments drift tests 5/5, stark 128/128, clippy + fmt clean, no PrecomputedCommitmentMismatch. Full ELF prove/verify runs in CI (guest artifacts absent locally).
…layout The compile-time decode-commitment const for sub.elf shifted with the row-pair preprocessed commitment; regenerated via commitment_from_elf (the print_decode_commitment_for_sub regen path).
…air value Regenerated from CI's sub.elf (local riscv toolchain unavailable); removed the temporary print instrumentation.
MauroToscano
left a comment
MauroToscano
left a comment
There was a problem hiding this comment.
This cannot be merged, it breaks CUDA
|
/review-ai |
Codex Code ReviewNo issues found in the PR diff. I reviewed the paired-leaf commitment changes across CPU, verifier, proof openings, CUDA leaf/tree paths, and preprocessed table recomputation. The prover/verifier index space and leaf byte ordering appear consistent. Static review only; I did not build or run tests per instructions. |
|
Review: commitment-layer unification Reviewed the diff focusing on correctness of the proof-format change, safety, and the refactor. No bugs found — the change is internally consistent and well covered. Correctness of the trace row-pair change (commit 2) — verified consistent
GPU path Refactor Minor (Low) — stale cross-crate doc comments reference now-renamed/moved CPU functions: |
AI ReviewPR #691 · 19 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-001: Dead/inconsistent public LDE+Keccak wrappers still build per-row leaves
Claim The public entry points coset_lde_batch_base_into_with_leaf_hash (line 604), coset_lde_batch_base_into_with_merkle_tree (line 632), coset_lde_batch_ext3_into_with_leaf_hash (line 862), and coset_lde_batch_ext3_into_with_merkle_tree (line 887) are not called anywhere in the repository and still pass rows_per_leaf=1. The rest of the prover now commits row-pairs (ROWS_PER_LEAF=2), so these wrappers would produce Merkle trees that are incompatible with verify_opening_pair if they were ever used again. They should be removed or updated to row-pair semantics. Evidence grep across the repo shows the only callers of these four symbols are their own definitions and doc-comments in Suggested fix Either delete the four unused non- AI-003: Stale doc comment references removed prover function
Claim The module doc comment says the GPU code matches commit_columns_bit_reversed at crypto/stark/src/prover.rs, but that function was removed and replaced by commit_bit_reversed in crypto/stark/src/commitment.rs. The stale reference will confuse future readers. Evidence Line 6 of crypto/math-cuda/src/merkle.rs contains the old name. prover.rs no longer defines commit_columns_bit_reversed. Suggested fix Update the comment to reference stark::commitment::commit_bit_reversed with rows_per_leaf=2. AI-004: Missing rows_per_leaf vs num_rows guard in public keccak leaf helpers
Claim keccak_leaves_base and keccak_leaves_ext3 accept rows_per_leaf but only assert it is 1 or 2 and that num_rows is a power of two. They do not assert num_rows >= rows_per_leaf. With rows_per_leaf=2 and num_rows=1 (a valid power of two), the output buffer length becomes 0 and keccak_launch_cfg(0) produces a zero-grid launch. This is a malformed input path exposed by the newly added rows_per_leaf parameter. Evidence Functions assert rows_per_leaf == 1 || rows_per_leaf == 2 and num_rows.is_power_of_two(), then allocate (num_rows / rows_per_leaf) * 32 bytes and call keccak_launch_cfg(num_rows / rows_per_leaf). For num_rows=1, rows_per_leaf=2 this creates a zero-length device allocation and zero-block grid. Suggested fix Add assert!(num_rows >= rows_per_leaf, "num_rows must be at least rows_per_leaf") to both keccak_leaves_base and keccak_leaves_ext3. 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 (1) — rejected by the verifier
Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
3 participants
What
Cleans up and unifies the Merkle-tree / bit-reversed commitment layer of the STARK prover, which was duplicated and entangled inside
prover.rs. The commitment how (leaf layout, bit-reverse permutation, Keccak hashing, tree build) now lives in a dedicated module; the prover only orchestrates when to commit.Commits
refactor(stark): extract Merkle commitment into acommitmentmodule — moveskeccak_leaves_*+commit_*out ofprover.rsintocrypto/stark/src/commitment.rs, behind a single grouped core (keccak_leaves_bit_reversed_grouped/commit_bit_reversed). Keeps both serializations (as_bytes/write_bytes_be) andFieldElementPairBackendas-is.perf(stark): row-pair the trace commitment — the trace LDE now uses the same row-pair leaf layout (ROWS_PER_LEAF = 2) the composition poly already used: leafihashes bit-reversed rows2i,2i+1(a FRI query's value + its symmetric counterpart), so one Merkle path authenticates both.verify_opening_pair).refactor(stark): prover cleanup (A+D) —parhelpers at 7 sites,ROWS_PER_LEAFconst, error propagation, doc/log fixes.docs(stark): fix commitment leaf-layout docs after the trace pairing.refactor(prover): dedup commit pipeline (B) — extractcommit_plain<C>+spill_tree<C>onIsStarkProver, collapsing the near-duplicate main-trace / aux-trace plain-commit code.Correctness
cargo test -p stark --lib: 128/128 · with--features disk-spill: 133/133.clippyclean on default /disk-spill/instruments; builds on the combined feature set.instruments, the aux commit's timing bucket now includes its (tiny) disk spill — matching what the main path already measured.Follow-ups (not in this PR)
gpu_lde) still emit 1-row leaves and must migrate to the row-pair layout before cuda builds pass. The default (non-cuda) build is fully verified here.multi_proveinto named phase helpers; move self-less utilities off theIsStarkProvertrait.