perf: page preprocessed commitment#645
Conversation
Codex Code ReviewFindings
No other issues found in the changed diff. I could not run the targeted tests because rustup tried to write to |
Review: perf: page preprocessed commitmentOverviewThis PR lands two complementary optimizations:
The ~43× recursion-guest cycle reduction is substantial and the design is clean. SecurityThe trust model is sound and mirrors the existing Minor: linear scan in the ELF-data-page lookup// prover/src/lib.rs ~line 440
let commitment = page_commitments
.and_then(|list| {
list.iter()
.find(|(pb, _)| *pb == config.page_base)
.map(|(_, c)| *c)
})
.unwrap_or_else(|| page::preprocessed_commitment(config, proof_options));This is O(pages × list_len). With Nit: naming inconsistencyThe low-level function retained its old name OverallLGTM. The approach is correct, the trust model is properly documented, and the test coverage is thorough. The static constant table for zero-init pages is a clean replacement for the old |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| list.iter() | ||
| .find(|(pb, _)| *pb == config.page_base) | ||
| .map(|(_, c)| *c) | ||
| }) |
There was a problem hiding this comment.
The find scan is O(pages × list_len). Fine today — with DEFAULT_PAGE_SIZE = 256 KB, typical ELF programs have only a handful of data pages so the total work is tiny. If the ELF-page count ever grows materially, a pre-built HashMap<u64, Commitment> outside the iterator would be the fix.
| /// drift-detection tests in `static_commitments_tests`. Production callers | ||
| /// should go through [`preprocessed_commitment`] so the static const-table | ||
| /// shortcut is used when applicable. | ||
| #[doc(hidden)] |
There was a problem hiding this comment.
Minor naming nit: this function is compute_precomputed_commitment while the new public wrapper below is preprocessed_commitment — "precomputed" vs "preprocessed". The inconsistency is understandable (preserving the old name as #[doc(hidden)]), but a reader encountering both names for the first time may wonder if they compute different things. A brief one-liner comment on this function noting it's the raw computation behind preprocessed_commitment would make the relationship explicit.
Codex Code ReviewFound one issue: Performance: significant fallback regression
Actionable fix: keep the static fast path, but cache fallback zero-page commitments keyed by No security vulnerabilities found in the changed verifier commitment path. Incorrect supplied page commitments appear to reject rather than silently pass. I could not run |
| let commitment = page_commitments | ||
| .and_then(|list| { | ||
| list.iter() | ||
| .find(|(pb, _)| *pb == config.page_base) |
There was a problem hiding this comment.
Low – undocumented first-match-wins behaviour
If page_commitments contains two entries with the same page_base (e.g. a caller accidentally duplicates an entry), .find() silently uses the first one. A wrong commitment will still cause Fiat-Shamir rejection, so this can't silently accept a bad proof, but it can cause a head-scratcher where a caller updates the second copy of a duplicate and wonders why nothing changes.
Consider documenting the contract ("each page_base must appear at most once") in the verify_with_options / VmAirs::new doc-comments, or adding a debug_assert! that the slice has no duplicate page_base values.
Review: perf: page preprocessed commitmentSummaryGood, focused optimization. The two changes (static const commitments for zero-init pages, caller-supplied commitments for ELF data pages) are well-separated, and the trust model is correctly documented and tested. The 43× cycle reduction for the recursion guest is significant. Security properties hold: a wrong IssuesLow – duplicate Low – missing partial-coverage test: // supply only the first page; the rest recompute from ELF
let partial = &list[..1];
let ok = verify_with_options(&vm_proof, &elf_bytes, &options, None, Some(partial))
.expect("partial page_commitments should not error");
assert!(ok, "partial page_commitments must still verify");Notes
|
…q! to a real error
2 participants
Description
This PR optimizes the PAGE preprocessed commitment in two complementary ways:
Static zero-init page commitments: Zero-init pages (stack, heap, runtime —
init_values.is_none()) have an INIT column of uniformly zero bytes and an OFFSET column of0..page_size-1, neither depending on the program being proven.Optional ELF-data-page commitments:
verify_with_optionsandVmAirs::newnow accept an optionalpage_commitments: Option<&[(u64, Commitment)]>. When supplied, the verifier looks up each ELF data page bypage_baseand uses the value directly, skipping the in-verifier FFT + Merkle build for that page.Verifier cycles reduction
Measured on the recursion guest (the verifier compiled as a RISC-V program verifying an empty inner proof, blowup=2, 1 query):
Baseline (Bitwise + keccak_rc + decode optional, page recompute): 2,895 M
This PR (+ Page static + Page optional): 67 M (~43× reduction)
Each ELF data page passed via
page_commitmentssaves the cost of one 256 KB page's FFT + Merkle build in the recursion guest (~2.8 B cycles per page, measured here).