Merge main into feat/shrink-cpu-byte-alu#662
Conversation
* Add warn, rename function, add tets * zero init page commitments * change the preprocessed-commitment mismatch check from debug_assert_eq! to a real error * simplify VmAirs page-AIR construction into two branches Zero-init and ELF-data pages took separate arms that ended identically and re-checked init_values.is_none(), which preprocessed_commitment already does. Merge them: non-private pages now prefer a caller-supplied page_commitments entry and otherwise fall back to preprocessed_commitment (static const for zero-init, recompute for ELF data). Behavior-preserving; the lookup stays a linear scan (no hashing, which is costly in the recursion guest). * hoist shared zero-init page commitment; assert uniform page size All zero-init pages share one preprocessed commitment (page-relative OFFSET, all-zero INIT) fixed by (page_size, blowup, coset). Compute it once before the page loop instead of once per zero-init page — this also collapses the 'not static' warning from per-page to once. Assert every page is DEFAULT_PAGE_SIZE so the shared value is valid for all of them; page_size is verifier-derived (never from the proof), so the assert can't be attacker-tripped. A follow-up will remove the per-page page_size field, making uniform size structural and the assert unnecessary. * cleanup: split page preprocessed-commitment wrapper, fix generator output (#656) * refactor: split page preprocessed-commitment wrapper; fix generator output - Replace the dual-duty page::preprocessed_commitment(config, options) with a config-free zero_init_preprocessed_commitment(options); ELF data pages call compute_precomputed_commitment directly. The is-zero-init branch lived inside the wrapper but both call sites already knew the answer, so it was dead weight; each function now has one observable behavior (zero-init: static const or warn+recompute; ELF data: always recompute, never warns). Behavior-identical. - Drop the #[doc(hidden)] herding on compute_precomputed_commitment — calling it directly is now the production path for ELF data pages. - Fix compute_static_commitments: the zero_page arm printed '(DEFAULT_PAGE_SIZE, blowup) => ...', a tuple pattern that does not compile when pasted into 'match blowup_factor' (leftover from the pre-#653 per-page page_size keying). Print 'blowup => ...' like the bitwise/keccak arms. - Drift test now asserts static_zero_page_commitment(blowup).is_some() so a deleted match arm can't pass trivially via the fallback path (the doc comment previously overclaimed this). * test: pin static zero-page bytes directly against recompute; fix stale docs Review feedback on #656: - The drift test compared the static bytes to the recompute only through the wrapper, so a broken dispatch gate (e.g. a coset-gate typo) would silently turn the comparison into recompute-vs-recompute and stop guarding the constants. Now the bytes are read from static_zero_page_commitment directly and compared to the recompute unconditionally; the wrapper gets its own separate equality check. The is_some assert is subsumed by the direct read (panics if the arm is missing) and is removed. - Module doc still described the suite as covering only bitwise/keccak_rc and referenced the old page wrapper name; updated for page and zero_init_preprocessed_commitment. - The drift test's doc no longer overclaims what the (removed) is_some assert guaranteed; the coset-gate coverage note points at the ignored page_non_three_coset test. --------- Co-authored-by: MauroFab <[email protected]> Co-authored-by: Mauro Toscano <[email protected]>
* Add a --cycles flag to cli execute that prints a program's dynamic instruction count * Update the execute loop comment to reflect that it always counts cycles, not only when generating a flamegraph * Fix docs --------- Co-authored-by: MauroFab <[email protected]> Co-authored-by: Mauro Toscano <[email protected]>
* simple ethrex tx * add full test * fix test * fix print ecall * add ethrex with tx bench * add keccak patch * Fix ethrex fixture reproducibility (#661) * Fix ethrex fixture reproducibility * Tighten ethrex benchmark rebuild logging --------- Co-authored-by: Mauro Toscano <[email protected]>
|
Benchmark Results for modified programs 🚀
|
Codex Code ReviewFindings
No other concrete issues found in the reviewed diff. |
MauroToscano
merged commit cade3ff
into
feat/shrink-cpu-byte-alu
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .flat_map(|commitment| { | ||
| std::iter::repeat_n((*commitment).into(), CELLS_PER_EXT_BLOB) | ||
| }) | ||
| .collect::<Vec<_>>(), |
There was a problem hiding this comment.
Medium – Missing length validation before batch verification
cells has length blobs.len() * CELLS_PER_EXT_BLOB, but the caller-supplied commitments and cell_proof slices are not validated against that expected length before the c_kzg call. A mismatch (e.g. commitments.len() != blobs.len() or cell_proof.len() != blobs.len() * CELLS_PER_EXT_BLOB) will surface as an opaque c_kzg::Error rather than an actionable message.
| .collect::<Vec<_>>(), | |
| debug_assert_eq!( | |
| commitments.len(), | |
| blobs.len(), | |
| "commitments.len() must equal blobs.len()" | |
| ); | |
| debug_assert_eq!( | |
| cell_proof.len(), | |
| blobs.len() * CELLS_PER_EXT_BLOB, | |
| "cell_proof.len() must equal blobs.len() * CELLS_PER_EXT_BLOB" | |
| ); | |
| c_kzg::KzgSettings::verify_cell_kzg_proof_batch( |
Also worth noting: the function name verify_cell_kzg_proof_batch implies callers supply the cells themselves (as in EIP-7594), but this function internally recomputes cells from blobs and ignores any caller-supplied cells. The signature should either take cells directly or be renamed (e.g. verify_blob_cells_kzg_proof_batch) to reflect that it takes blobs.
| let rem = SHA3_absorb(&mut self.state, data.as_ptr(), data.len(), BLOCK_SIZE); | ||
| self.tail_len = rem; | ||
| if rem != 0 { | ||
| let tail_data = data.get_unchecked(data.len() - rem..); |
There was a problem hiding this comment.
Low – Implicit safety invariant on SHA3_absorb return value
let tail_data = data.get_unchecked(data.len() - rem..);This is UB if rem > data.len(). The safety argument relies on the OpenSSL SHA3_absorb contract that it returns the number of leftover bytes after absorbing full blocks, i.e. rem < BLOCK_SIZE ≤ data.len() (since this arm requires data.len() >= BLOCK_SIZE). That contract holds for the known implementation, but the assumption is implicit.
At minimum add a debug assertion:
| let tail_data = data.get_unchecked(data.len() - rem..); | |
| let rem = SHA3_absorb(&mut self.state, data.as_ptr(), data.len(), BLOCK_SIZE); | |
| debug_assert!(rem <= data.len(), "SHA3_absorb returned rem > data.len()"); | |
| self.tail_len = rem; |
| } | ||
|
|
||
| #[inline] | ||
| pub fn update(&mut self, data: impl AsRef<[u8]>) -> Self { |
There was a problem hiding this comment.
Low – Confusing API: &mut self + -> Self both update state
update mutates self in-place AND returns a clone. This means callers who write:
let h2 = h.update(data);
// h and h2 are now both at the same state — h was mutated, h2 is a clone
h.update(more_data); // advances h
// h2.state still at the state after first update — callers may not expect thisThe RISC-V implementation accumulates into a Vec (no mutation until finalize), making the two implementations subtly different in how chaining behaves when both the binding and the original handle are used.
Consider making update take self (consuming) and return Self — consistent with the builder pattern — or just &mut self returning &mut Self, which is the standard Digest-trait style.
3 participants
Resolves the current PR #644 conflict with main by merging origin/main into feat/shrink-cpu-byte-alu and preserving the PR's shrunk decode tests while updating verify_with_options call sites for the new page_commitments argument. Validation: cargo test -p lambda-vm-prover decode_tests --lib; cargo test -p lambda-vm-prover decode_layout_tests --lib.