Update WHIR version and integrate buffer abstraction - #482
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CSP benchmarks
Prover time, peak RSS, peak heap, and verifier time are arithmetic means across the iterations. Peak heap comes from the largest Each metric cell shows the current value followed by the percentage delta against the latest successful ResultsNo benchmark results were produced. |
6ac5dc7 to
1ee05a6
Compare
zkfriendly
changed the title
BornPsych
left a comment
BornPsych
left a comment
There was a problem hiding this comment.
All and all very good PR, minor perf/nit changes here and there.
one thing to note that this PR touches several protocol-facing paths (whir bump, NTT registration, SPARK setup/prove flow) where regressions wouldn’t necessarily show up in existing unit tests. If it’s easy to fold in here or as a quick follow-up, a bit of targeted coverage would help — e.g. asserting NTT is registered before use, and a small SPARK prepare → prove → verify smoke test so future whir bumps don’t depend on manual pipeline runs. No blocker from my side; mostly noting where I had to validate by hand.
| hash_config: HashConfig, | ||
| ) -> Self { | ||
| provekit_backend_bn254::register(); | ||
|
|
There was a problem hiding this comment.
correctness: Registering here makes sense but in provekit-common two functions actually reach whir's Config::new, new_witness_config_for_size and new_blinding_config_for_size, and neither registers or warns you that you need to. Could FieldHash carry a register()? Both backends already have an idempotent one, so the constructors could just call P::register() instead of us chasing this at call sites.
| vec![Cow::Borrowed(p1.as_slice())], | ||
| vec![Cow::Owned(w1)], | ||
| &[&p1], | ||
| vec![&w1], |
There was a problem hiding this comment.
perf: this can be dropped earlier, since it is still resident through w2's prove
| .flat_map(|message| { | ||
| message | ||
| .to_slice() | ||
| .chunks_exact(messages.message_length) |
There was a problem hiding this comment.
nit: Could this use whir::utils::chunks_exact_or_empty instead? chunks_exact panics when the chunk size is 0(defensive).
| impl Serialize for ArkVecRef<'_> { | ||
| fn serialize<S: Serializer>(&self, s: S) -> Result<S::Ok, S::Error> { | ||
| serde_ark_vec::serialize(self.0, s) | ||
| serde_ark_vec::serialize(&Vec::from(self.0), s) |
There was a problem hiding this comment.
I think it's only here because to_slice() gives you a &[T] and serde_ark_vec::serialize still wants a &Vec<T>. That function only calls len() and iterates though, so switching it to &[T] would kill the copy, and the existing #[serde(with = ...)] fields keep compiling since deref covers it.
| data.resize(total, Fr::ZERO); | ||
|
|
||
| let messages: Vec<&[Fr]> = data.chunks(message_length).collect(); | ||
| let messages: Vec<Buffer<Fr>> = data.chunks(message_length).map(|c| Buffer::from(c)).collect(); |
There was a problem hiding this comment.
nit: cargo clippy warning
| merlin, | ||
| evaluation_randomness, | ||
| &[config.final_timestamp], | ||
| &[&Buffer::from(config.final_timestamp)], |
There was a problem hiding this comment.
perf: final_timestamp is a &[FieldElement], so Buffer::from hits the copying From<&[T]>. The caller already owns the data: final_row_field() returns a Vec, prover.rs borrows it and drops it (same for col).
Fix: change AxisConfig.final_timestamp to Vec<FieldElement>, move the caller's vec into it, pass that owned vec to Buffer::from so WHIR reuses the allocation instead of copying a slice the caller was about to drop anyway; add & to multilinear_extend.
|
|
||
| let whir_params = ProtocolParameters { | ||
| unique_decoding: false, | ||
| decoding_regime: whir::protocols::params::DecodingRegime::Johnson, |
There was a problem hiding this comment.
maintainability: This duplicates whir_protocol_params in common/whir_r1cs.rs — same 128/10/3/3/2/Johnson literals — which is why the regime change had to land in two places in this diff.
DecodingRegime now has a third option that didn't exist under the old bool. If someone moves the main path to Capacity and misses this copy, SPARK's commitments get proven at a different soundness level than the proof they back, and nothing would catch it.
Fix: hoist the existing private whir_protocol_params in common/whir_r1cs.rs to a public whir_protocol_params(hash_id, batch_size), have WhirR1CSScheme call it with batch_size: 1, and replace the inline literal in new_whir_config_for_size with that shared constructor. One source of truth for the security profile.
2 participants
Summary
This PR updates the
whirdependency and refactors ProveKit's interfaces to work with Whir's new buffer abstraction.The changes adapt the BN254 backend, prover, R1CS compiler, Spark integration, and related tooling to the updated Whir APIs. They also update the affected NTT, witness serialization, setup, memory, and benchmark code paths.
Buffer abstraction status
At the Whir boundaries, ProveKit currently converts data using
Buffer::fromandBuffer::to_slice. ProveKit's own operations have not yet been ported to operate directly on the buffer abstraction.These conversions have no meaningful cost on CPU because the buffers remain CPU-backed. On GPU, however, converting back to slices would force device-to-host readbacks that could otherwise be avoided. A follow-up will port the relevant ProveKit operations to work directly with buffers so data can remain on the GPU across the ProveKit/Whir boundary.