Cleanup/prover src refactor#596
Conversation
`carry_idx` is set during constraint construction and only ever takes values 0 or 1. The default match arm cannot be reached at runtime, so unreachable!() with a documenting message is the idiomatic spelling.
Nine sites in `trace_builder.rs` duplicated the same shift-by-[0,16,32,48] loop that decomposes a u64 into four IS_HALFWORD bitwise lookups. Replace with a single `push_u64_as_halfwords(ops, op_type, value)` helper at the top of the module; each call site collapses to one self-documenting line.
DECODE, BITWISE, KECCAK_RC, PAGE, and REGISTER all committed their
precomputed columns through the same six-step pipeline (interpolate -> LDE
-> bit-reverse -> columns2rows -> Merkle build), with five copies of the
same three .expect("... <table> ...") strings differing only by the
table label. `bitwise.rs` additionally dual-forked every step under
`#[cfg(feature = "parallel")]`.
Extract `commit_preprocessed_columns(columns, options, label)` into a
new `tables/preprocessed.rs`. The cfg-fork lives there only. Each
table's `compute_preprocessed_commitment` collapses to building its
own column data plus a single helper call.
Side effect: the four tables that were previously sequential (decode,
keccak_rc, page, register) now use the parallel path uniformly.
Net -116 LOC across 7 files; one site to maintain when the pipeline
evolves (eg. Merkle backend swap).
…ecutorLog
A malformed executor log could previously panic the prover via several
.expect() sites:
- `commit_count exceeds u32 range` (collect_commit_memw_ops, collect_ops_from_cpu)
- `commit index exceeds u32 range`
- `keccak state lane address overflows u64` (collect_keccak_memw_ops,
collect_ops_from_cpu, collect_bitwise_from_keccak)
- `keccak state byte address overflows u64`
Each is now returned as `Error::InvalidExecutorLog(&'static str)`, threaded
through the four `collect_*` helpers' new `Result` return types. Callers
were already in `Result`-returning paths, so propagation is a single `?`
per site. Test caller wraps with `.expect("...")`.
Defense in depth: the executor is the first validation layer; the prover
no longer crashes if a regression there allows an out-of-range value
through. Mirrors the verifier panic-removal work on
`cleanup/stark-verifier-refactor`.
Most table `bus_interactions()` functions are dense walls of
`BusInteraction::sender(... vec![BusValue::Packed { ... Packing::Direct }])`
blocks — 5-7 lines of structural noise per logical interaction.
Add `tables/bus_builder.rs` with a small builder whose intent-named methods
describe the constraint rather than the data structure:
- `send_halfword(col, &mu)` — IS_HALFWORD range check on a direct-packed col
- `send_msb16(in_col, out_col, &mu)` — MSB16 lookup pair
- `send_b20(col, &mu)` / `send_b20_linear(&mu, terms)` — B20 checks
- `send(...)` / `recv(...)` — generic forms
- `raw(interaction)` — escape hatch for one-off shapes
No macros: plain Rust methods, navigable with go-to-definition.
`Multiplicity` is taken by reference and cloned inside helpers so a single
`let mu = ...;` can drive many interactions without per-site `.clone()`.
Migrate `tables/lt.rs` as the first table — its 9 interactions go from
~120 to ~40 LOC and each reads as a spec line. Behavior unchanged;
24 lt_tests pass.
The mul/dvrm/shift/keccak_rnd migrations are bigger and ship as
follow-up commits, after this API gets review.
Apply BusInteractionsBuilder to the three remaining medium tables: - `shift.rs::bus_interactions` ~225 -> ~110 LOC - `mul.rs::bus_interactions` ~310 -> ~115 LOC - `dvrm.rs::bus_interactions` ~580 -> ~200 LOC Net -380 LOC. Each interaction reads as a spec line; the IS_HALFWORD, MSB16, and IS_B20 senders use the dedicated `send_halfword`, `send_msb16`, and `send_b20_linear` helpers, while heterogeneous interactions (Mul/Lt/Zero/Hwsl/Shift/Dvrm receivers) use the generic `send`/`recv` methods. Behavior unchanged; per-table tests pass (16 dvrm, 24 lt). Pre-existing UnknownSyscall(5) ELF failures unrelated.
Codex Code ReviewFindings Performance: bitwise.rs now generates the full No security vulnerabilities or correctness bugs found in the reviewed diff. I could not run |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| options: &ProofOptions, | ||
| table_label: &'static str, | ||
| ) -> Commitment { | ||
| let num_rows = columns[0].len(); |
There was a problem hiding this comment.
Low – panic on empty columns
columns[0] panics if called with an empty slice. All current callers are safe, but this is an undocumented precondition that future callers could violate silently.
| let num_rows = columns[0].len(); | |
| debug_assert!(!columns.is_empty(), "commit_preprocessed_columns: columns must be non-empty"); | |
| let num_rows = columns[0].len(); |
|
|
||
| /// Send a B20 range check on a virtual linear-combination value (e.g. a carry). | ||
| /// IS_B20[linear_terms] | ||
| pub fn send_b20_linear(&mut self, mult: &Multiplicity, terms: Vec<LinearTerm>) -> &mut Self { |
There was a problem hiding this comment.
Low – parameter order inconsistency
Every other named helper is (col, mult) or (input_col, output_col, mult) — mult is always last. Here mult comes before terms, which makes the API harder to learn and grep for patterns.
| pub fn send_b20_linear(&mut self, mult: &Multiplicity, terms: Vec<LinearTerm>) -> &mut Self { | |
| pub fn send_b20_linear(&mut self, terms: Vec<LinearTerm>, mult: &Multiplicity) -> &mut Self { |
(and update the one call-site in the codebase accordingly)
| // Read+write x254 (global commit index) at ts | ||
| { | ||
| let (old_index, old_ts) = register_state.read_index(); | ||
| let count_u32 = u32::try_from(count) |
There was a problem hiding this comment.
Low – duplicate validation
collect_ops_from_cpu already validates and converts op.commit_count to u32 (line ~430) before calling this function. Here the same field is read from the struct and converted again independently.
Consider accepting count_u32: u32 as a parameter instead of re-reading and re-validating op.commit_count. This would also eliminate the near-duplicate error message.
Code ReviewGood refactoring PR. The three main wins — factoring out the preprocessed commitment pipeline, converting Four minor findings (all Low severity): 1. Panic on empty 2. Duplicate 3. Inconsistent error type for the same invariant 4. Parameter order inconsistency in |
`keccak_rnd::bus_interactions` was the largest single bus_interactions function in the crate (~460 LOC of repeated XOR_BYTE / AND_BYTE / IS_BYTE / HWSL senders). Migrate to BusInteractionsBuilder. Add two more helpers to bus_builder: - `send_is_byte(col, &mu)` — IS_BYTE on a direct-packed column. - `send_xor_byte(x, y, result, &mu)` — XOR_BYTE on three direct cols. These collapse the four XOR_BYTE/IS_BYTE walls (theta Cxz chain, theta final, chi, iota; plus rho IS_BYTE range checks) into one-line calls. Net -215 LOC on keccak_rnd.rs (~460 -> ~245 in the function); the rest of the file (column layout, constraints) is unchanged.
Codex Code ReviewFound one issue:
Verification note: I attempted |
| options: &ProofOptions, | ||
| table_label: &'static str, | ||
| ) -> Commitment { | ||
| let num_rows = columns[0].len(); |
There was a problem hiding this comment.
Low — implicit panic on empty column slice
columns[0] panics if columns is empty. All current callers are safe, but the invariant is silent. Consider asserting it:
| let num_rows = columns[0].len(); | |
| assert!(!columns.is_empty(), "commit_preprocessed_columns: no columns for {table_label}"); | |
| let num_rows = columns[0].len(); |
| let count = u32::try_from(op.commit_count) | ||
| .map_err(|_| Error::InvalidExecutorLog("commit_count exceeds u32 range"))?; |
There was a problem hiding this comment.
Low — redundant conversion
collect_commit_memw_ops (line 428, called with ?) already validates commit_count fits in u32 and returns Err if it doesn't, so this second try_from can never return Err. The checked_add on current_commit_index is still meaningful (it guards a separate counter), but the conversion itself is dead error-path code.
ReviewClean refactoring PR. No security issues or correctness bugs found. Two low-severity observations left as inline comments:
|
`commit_preprocessed_columns` interpolated each column to coefficient form and then evaluated it back onto the LDE coset — two full FFT pipelines per column (iFFT, then scale+FFT), plus a rebuilt twiddle table on every call. Replace it with `stark::prover::commit_lde_columns`: the same path the prover uses for ordinary trace columns. Each column goes from evaluation form straight to its LDE in a single fused `Polynomial::coset_lde_full` pass; the inverse/forward twiddle tables and coset weights are built once (`LdeTwiddles::from_params`) and shared across every column. The Merkle commit reuses `keccak_leaves_bit_reversed` — the prover's own leaf hashing. `commit_preprocessed_columns` is now a thin adapter; there is no separate interpolate-then-extend round trip and no per-column twiddle rebuild. - `LdeTwiddles::from_params(domain_size, blowup, coset_offset)` exposed so twiddles can be built without a full `Domain` (which also allocates the LDE coset vector). `LdeTwiddles::new(&Domain)` now delegates to it. - `commit_lde_columns` added as a public `stark::prover` entry point. - New test `commit_lde_columns_matches_interpolate_then_evaluate` pins the invariant that the fused path yields the byte-identical Merkle root the old interpolate-then-evaluate pipeline did, for n = 2^2..2^8. 125 stark tests pass; 273 prover lib tests pass (77 pre-existing UnknownSyscall(5) ELF failures unrelated). `make lint` clean.
Self-review of the PR surfaced unused API: - `BusInteractionsBuilder::send_b20` — no caller; every B20 send in the migrated tables is a linear combination (`send_b20_linear`). Removed. - `BusInteractionsBuilder::raw` — the "escape hatch" had no caller; the generic `send` / `recv` cover every heterogeneous interaction. Removed. - `BusInteractionsBuilder::new` + its `Default` impl — every `bus_interactions()` knows its interaction count and uses `with_capacity`; the zero-arg constructor was never called. Removed (which also removes the `Default` that only existed to satisfy `clippy::new_without_default`). - `commit_preprocessed_columns`: dropped the redundant `::<GoldilocksField>` turbofish on `commit_lde_columns` — `F` is inferred from the arguments — and the now-unused `GoldilocksField` import. Added a note on why the prover crate is monomorphic over Goldilocks while `commit_lde_columns` itself stays generic. No behavior change. 125 stark + 273 prover lib tests pass; lint clean.
Self-review of PR #596's commit pipeline: Duplicate LDE-commit path - `compute_precomputed_commitment_for_testing` re-implemented what the new `commit_lde_columns` already does: it routed through `Domain::new` (which eagerly builds the LDE-coset roots-of-unity vector, never used for a commitment), then `LdeTwiddles::new`, `compute_lde_from_columns_cached` and `commit_columns_bit_reversed`. It now delegates to `commit_lde_columns` directly — one fused coset-LDE pass with shared twiddles — so the test helper and the production preprocessed-commit path are the same code. - That made `compute_lde_from_columns_cached` dead (its only caller was the helper; its doc still claimed phase-A/C/Round-2-4 use that no longer exists). Removed, along with the now-unneeded `FieldExtension: AsBytes` bound on the helper. No more wasted twiddle/Domain work - The old helper built a full `Domain` (trace + LDE roots-of-unity cosets) purely to hand `blowup_factor`/`coset_offset` to the LDE. `commit_lde_columns` builds only the `LdeTwiddles` it needs, once, shared across all columns. Panic -> documented invariant - `commit_preprocessed_columns` previously did `commit_lde_columns(...).unwrap_or_else(|| panic!("failed to commit..."))`, which reads like a runtime error path. `commit_lde_columns` returns `None` *only* for empty input, and a preprocessed table always has a fixed non-empty column set. Replaced with an explicit `assert!` naming the table (the real invariant) plus an `.expect` documenting why the `Option` is then provably `Some`. Behavior-preserving: 125 stark lib tests pass (incl. the commit_lde_columns parity test) and the bitwise preprocessed-soundness tests that drive the test helper. Lint clean.
2 participants
No description provided.