Buffer Abstraction#260
Conversation
|
cc @recmo |
shreyas-londhe
left a comment
shreyas-londhe
left a comment
There was a problem hiding this comment.
Thanks for the PR @zkfriendly. Really like where this lands. Trait split is the right cut, the CPU backend stays a thin wrapper with no second implementation to drift, and the prover got simpler dropping the Any/Cow recycling for mixed_linear_combination and linear_forms_rlc. Left a few inline comments, but the shape is solid.
| assert!( | ||
| !vector.at_index(0).unwrap_or_else(F::one).is_zero(), | ||
| "Proof failed" | ||
| ); |
There was a problem hiding this comment.
issue (blocking): unwrap_or_else(F::one) makes this pass when the buffer is empty.
F::one() is non-zero, so an out-of-bounds at_index(0) falls back to a value that satisfies the check instead of failing it. The covector lines just below (96, 152) already use .expect("Proof failed"). Use that here too. Same fix at line 146.
| /// Build the full node array from leaf hashes, without touching the transcript. | ||
| /// | ||
| /// The returned vector holds the leaf layer first and the root last (at `num_nodes() - 1`). | ||
| pub fn build_nodes(&self, leaves: Vec<Hash>) -> Vec<Hash> { |
There was a problem hiding this comment.
suggestion (non-blocking): build_nodes leaves the root-send as an unenforced caller duty.
Splitting construction from the transcript write is good, but sending the root is now manual (prover_message(&nodes[num_nodes()-1])). A future caller that forgets it produces a commitment whose root never binds, with no compile error and probably no failing test. Consider funneling Witness creation through a method that takes prover_state and sends the root, or returning a #[must_use] wrapper that only a transcript-taking method unwraps. At minimum, document the obligation here.
| fn interleaved_encode( | ||
| &self, | ||
| messages: Messages<'_, F>, | ||
| masks: &ActiveBuffer<F>, | ||
| codeword_length: usize, | ||
| ) -> ActiveBuffer<F>; |
There was a problem hiding this comment.
suggestion: ReedSolomon is a backend-abstraction trait but hardcodes the global ActiveBuffer alias in its signature, which defeats the purpose for the one trait that most needs it.
Give it an associated type Buffer: Buffer<F> and thread it through the registry (type Dyn<F> = dyn ReedSolomon<F, Buffer = ActiveBuffer<F>>); NttEngine then declares type Buffer = CpuBuffer<F>. The payoff is enforcement, not tidiness: today a second backend can't even be expressed here, since the trait forces every impl to speak CpuBuffer. With the associated type, an impl whose buffer doesn't match the build's ActiveBuffer fails to register as a type error instead of sitting as dead code that forces host round-trips if called.
| fn fold_pair(&mut self, other: &mut Self, weight: F) { | ||
| self.fold(weight); | ||
| other.fold(weight); | ||
| } | ||
|
|
||
| fn fold_pair_sumcheck_polynomial(&mut self, other: &mut Self, weight: F) -> (F, F) { | ||
| self.fold_pair(other, weight); | ||
| self.sumcheck_polynomial(other) | ||
| } |
There was a problem hiding this comment.
nitpick (non-blocking): these two overrides duplicate the trait defaults.
fold_pair and fold_pair_sumcheck_polynomial here have the same bodies as the defaults on Buffer. Drop both from the impl and inherit the defaults, unless an override is planned for a reason.
2 participants
Summary
This PR introduces the buffer abstraction layer. All buffer abstraction traits are defined in the buffer crate, with a CPU implementation that essentially wraps the existing in-memory code paths.
At this moment, the buffer crate still depends on existing algebra and protocol crates. Future work can consolidate the relevant logic directly into the buffer crate as the abstraction settles.
Notes
There is also an experimental Metal implementation, but it is not included in this PR.
zk_whiris not fully ported to the buffer abstraction yet. It uses.as_slice()in places to keep the code working, so behavior is essentially unchanged compared to directly using CPU buffers. Since WHIR v3 is coming and is expected to replacezk_whir, that path is intentionally not fully ported for now.