jolt-openings crate API cutover#1521
Conversation
quangvdao
changed the title
|
Claude spec review session started: https://claude.ai/code/session_016zYwqpvB8AFnhWrGjedjok |
|
Spec Analysis:
Status: Approved — The spec is clear enough for one-shot implementation. Verification against the current tree:
Summary of what will be built:
Key invariants to preserve (all named in the spec):
Critical evaluation criteria:
Minor soft spots (not blocking):
Next step: Run
Generated by Claude Code |
moodlezoup
added
claude-spec-approved
Claude
moodlezoup
left a comment
moodlezoup
left a comment
There was a problem hiding this comment.
this looks good to me, just a couple of minor comments.
Also, while you're refactoring the PCS traits, perhaps you could also take a look at the relationship between the streaming and ZK traits? There's an awkward interaction there for Dory which I flagged here
| 2. `jolt-openings` depends only on reusable leaf crates plus generic dependencies: `jolt-field`, `jolt-poly`, `jolt-transcript`, `jolt-crypto`, `serde`, `thiserror`, and `tracing`. | ||
| 3. The base verifier trait does not expose prover-only associated types such as `ProverSetup`, `Polynomial`, `OpeningHint`, or `SetupParams`. | ||
| 4. The base PCS traits expose fused batched openings through `prove_batch` and `verify_batch`. | ||
| 5. Single-claim `open` and `verify` are not on the base PCS trait. |
There was a problem hiding this comment.
why? seems like they would make more sense on the base triat
| 1. `Field = Fr`. | ||
| 2. `VerifierSetup = DoryVerifierSetup`. | ||
| 3. `Proof = DoryProof`. | ||
| 4. `BatchProof = Vec<DoryProof>`. |
There was a problem hiding this comment.
the relationship between BatchProof and the AdditivelyHomomorphic traits is a little unclear –– below it says that prove_batch and verify_batch delegate to their homomorphic counterparts, but then shouldn't BatchProof = DoryProof?
|
@moodlezoup thanks for the comments! I updated the spec to reflect the resolution, and the spec now has a redesign of the CommitmentScheme trait that bakes in streaming via a generic "polynomial source" rather than having a separate trait. Below is GPT-5.5's response after discussion: __ First, agreed on single-claim openings: Second, the Dory batch-proof shape is now spelled out more explicitly. For Dory, On the streaming/ZK interaction: I think the right fix is to remove
So the ZK distinction moves to the PCS method boundary ( One subtlety: Stage 8 has a different “streaming” path from witness commitment streaming. Stage 8 currently builds a joint RLC polynomial directly from the trace/hints before opening, and we should preserve that optimized construction for this PR. The new API still applies there: Stage 8 should feed the resulting joint polynomial/claim through Net-net: I think this addresses the awkward Dory ZK/streaming interaction you flagged by making streaming a property of sources and batch commitment, not a separate commitment scheme trait. |
I guess I'm not convinced that streaming can be fully quarantined to the source implementation. It seems to me like streaming tends to have implications on the commitment algorithm itself, e.g. how we parallelize |
|
@moodlezoup yeah, I agree with this. I overstated it by saying streaming lives in the source implementation. The distinction I want is closer to the compute/backend split in the Bolt work on So the intended split is:
So no, streaming is not fully quarantined inside the source. The source gives the PCS enough structure to avoid materialization; the PCS/backend decides how to consume that structure. This also seems compatible with the Bolt direction. Bolt has compute-level obligations like I updated the spec to call this out explicitly. The goal is that this PR gives Bolt a Rust PCS boundary it can lower into, without baking Dory’s current row-major streaming schedule into Posted by Cursor assistant (model: GPT-5) on behalf of the user (Quang Dao) with approval. |
quangvdao
changed the title
|
Claude code review session started: https://claude.ai/code/session_01QPkTEyadrKJEohTo3cymWD |
moodlezoup
left a comment
moodlezoup
left a comment
There was a problem hiding this comment.
Reviewed the jolt-openings cutover. The Stage 8 prover/verifier symmetry holds, transcript binding order is preserved, and the ZK/standard mode split looks consistent end-to-end. Most flags below are footguns introduced by the new trait surface (silent defaults and duplicated helpers) that will bite a future PCS backend or a future SourceRow variant rather than the current Dory-only path.
Generated by Claude Code
| @@ -78,23 +219,8 @@ where | |||
| } | |||
There was a problem hiding this comment.
Silent default that returns OpeningHint::default() will silently produce wrong opening proofs for any backend that implements AdditivelyHomomorphic but forgets to override combine_hints. homomorphic_prove_batch (homomorphic.rs:80) consumes the result without checks, and the verifier just sees a bad proof. Verifier-side combine has no default (good); making this required matches.
| fn combine_hints( | |
| hints: Vec<Self::OpeningHint>, | |
| scalars: &[Self::Field], | |
| ) -> Self::OpeningHint; |
Generated by Claude Code
| @@ -277,166 +1060,600 @@ impl AdditivelyHomomorphic for DoryScheme { | |||
| .map(|row| { | |||
| let mut acc = Bn254G1::default(); | |||
| for (hint, &scalar) in hints.iter().zip(scalars.iter()) { | |||
| acc += hint.row_commitments[row].scalar_mul(&scalar); | |||
| if let Some(row_commitment) = hint.row_commitments.get(row) { | |||
| acc += row_commitment.scalar_mul(&scalar); | |||
| } | |||
| } | |||
| acc | |||
| }) | |||
| .collect(); | |||
|
|
|||
| DoryHint::new(combined, combined_blind) | |||
| let chunk_len = hints | |||
| .iter() | |||
| .map(|hint| hint.chunk_len) | |||
| .max() | |||
| .unwrap_or_default(); | |||
| DoryHint::new(combined, combined_blind, chunk_len) | |||
| } | |||
There was a problem hiding this comment.
Body is textually identical to combine_hint_refs at line 657 (modulo Vec<&DoryHint> vs Vec<DoryHint>). Any future change to hint combination has to be made twice. Express one via the other — either have the trait impl call the helper with .iter().collect(), or take impl IntoIterator<Item: Borrow<DoryHint>> in the helper.
Generated by Claude Code
| fn source_to_evaluations<F, S>(source: &S) -> Vec<F> | ||
| where | ||
| F: Field, | ||
| S: CommitmentSource<F> + ?Sized, | ||
| { | ||
| let mut evaluations = Vec::with_capacity(1 << source.num_vars()); | ||
| let chunk_len = source | ||
| .natural_chunk_len() | ||
| .unwrap_or_else(|| 1usize << source.num_vars()); | ||
| source.for_each_row(chunk_len, |_, row| match row { | ||
| SourceRow::FieldElements(values) => evaluations.extend_from_slice(values), | ||
| SourceRow::StridedFieldElements { | ||
| values, | ||
| column_stride, | ||
| } => { | ||
| for value in values { | ||
| evaluations.push(*value); | ||
| evaluations.extend(repeat_n(F::zero(), column_stride.saturating_sub(1))); | ||
| } | ||
| } | ||
| SourceRow::I128(values) => { | ||
| evaluations.extend(values.iter().map(|&value| F::from_i128(value))); | ||
| } | ||
| SourceRow::StridedI128 { | ||
| values, | ||
| column_stride, | ||
| } => { | ||
| for value in values { | ||
| evaluations.push(F::from_i128(*value)); | ||
| evaluations.extend(repeat_n(F::zero(), column_stride.saturating_sub(1))); | ||
| } | ||
| } | ||
| SourceRow::U64(values) => { | ||
| evaluations.extend(values.iter().map(|&value| F::from_u64(value))); | ||
| } | ||
| SourceRow::StridedU64 { | ||
| values, | ||
| column_stride, | ||
| } => { | ||
| for value in values { | ||
| evaluations.push(F::from_u64(*value)); | ||
| evaluations.extend(repeat_n(F::zero(), column_stride.saturating_sub(1))); | ||
| } | ||
| } | ||
| SourceRow::OneHot(row) => { | ||
| let domain_size = 1usize << row.log_domain_size; | ||
| match row.entries { | ||
| OneHotEntries::OnePerColumn(indices) => { | ||
| for hot_index in indices { | ||
| let mut dense = vec![F::zero(); domain_size]; | ||
| dense[hot_index.get()] = F::from_u64(1); | ||
| evaluations.extend(dense); | ||
| } | ||
| } | ||
| OneHotEntries::MaybeZero(indices) => { | ||
| for hot_index in indices { | ||
| let mut dense = vec![F::zero(); domain_size]; | ||
| if let Some(hot_index) = hot_index { | ||
| dense[hot_index.get()] = F::from_u64(1); | ||
| } | ||
| evaluations.extend(dense); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| evaluations | ||
| } |
There was a problem hiding this comment.
This re-implements the same SourceRow enum dispatch as materialize_source_evaluations in jolt-openings/src/sources.rs:438, but with a different one-hot layout (column-major inline here vs hot-index-major accumulated in jolt-openings). Adding a new SourceRow variant requires updating two places, and the divergent one-hot handling means a multi-row one-hot source produces a different polynomial through HyperKZG vs the canonical helper. Easiest fix is making materialize_source_evaluations pub and deleting this copy; if the column-major shape was intentional for HyperKZG, leave a comment explaining why.
Generated by Claude Code
| if rebuild { | ||
| let mut advice_polys = self | ||
| .advice_polys | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
| let advice_polys = advice_polys.take().unwrap_or_default(); | ||
| let (poly_ids, coeffs): (Vec<_>, Vec<_>) = terms | ||
| .iter() | ||
| .map(|term| (term.source_id, term.coefficient)) | ||
| .unzip(); | ||
| let source = MultilinearPolynomial::RLC(RLCPolynomial::new_streaming( | ||
| self.one_hot_params.clone(), | ||
| Arc::clone(&self.streaming_data), | ||
| self.trace_source.clone(), | ||
| poly_ids, | ||
| &coeffs, | ||
| advice_polys, | ||
| )); | ||
| *cache = Some(Stage8CachedRlc { | ||
| terms: terms.to_vec(), | ||
| chunk_len, | ||
| source, | ||
| }); | ||
| } |
There was a problem hiding this comment.
advice_polys.take() permanently moves the advice map out of the Mutex<Option<...>> on the first rebuild. If fold_linear_rows is ever invoked a second time with a different terms or chunk_len (cache miss), the second rebuild gets None and unwrap_or_default() silently substitutes an empty HashMap — advice contributions vanish from the RLC with no error.
Current Dory path is safe because prove_source_backed_batch_with_mode calls vector_matrix_product with a single fixed (terms, chunk_len). But Stage8OpeningSource::fold_rows at line 326 builds a single-element terms per source, so anything that routes through the default prove_batch_opening and hits this from multiple sources will silently drop advice starting on the second call. Worth either making the advice map cloneable so rebuilds remain consistent, or debug_assert!(cache.is_none()) here so a second rebuild fails loudly.
Generated by Claude Code
| fn evaluate(&self, _point: &[F]) -> F { | ||
| panic!("Stage8OpeningSource is only used through source-backed batch opening") | ||
| } | ||
|
|
||
| fn for_each_row<V>(&self, _chunk_len: usize, _visit: V) | ||
| where | ||
| V: for<'row> FnMut(usize, SourceRow<'row, F>), | ||
| { | ||
| panic!("Stage8OpeningSource is only used through source-backed batch opening") | ||
| } |
There was a problem hiding this comment.
Panicking evaluate/for_each_row means any PCS that uses the default prove_batch_opening against Stage8OpeningSourceBatch (which routes per-id through source(id)) will crash at runtime instead of failing at compile time. Dory overrides prove_batch_opening, so this is latent today, but it's a non-obvious coupling between this source impl and the chosen backend. Worth a doc comment noting the requirement that the backend implement source-backed batch opening directly.
Generated by Claude Code
quangvdao
changed the title
quangvdao
force-pushed
the
quang/jolt-openings-crate
branch
from
7f569b3 to
37ddc5c
Compare
Scope the extracted PCS cutover to jolt-openings, jolt-dory, and related crate-level backends. Keep legacy jolt-core unchanged as the reference implementation while documenting the generated-role integration path.
quangvdao
force-pushed
the
quang/jolt-openings-crate
branch
from
bc7a13c to
8ee3947
Compare
| } | ||
| } | ||
|
|
||
| impl CanonicalSerialize for Bn254GT { |
There was a problem hiding this comment.
can we just #[derive(CanonicalSerialize, CanonicalDeserialize)] on Bn254GT instead of manually implementing these?
| /// proof algorithm. Most schemes set both fields equal; Dory uses `proof` for | ||
| /// its private row-major opening order while Jolt keeps `public` in protocol | ||
| /// order. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] |
There was a problem hiding this comment.
I don't really like this; I feel like the PCS implementation should just expect one coordinate order, and it should be responsible for any reordering it might need
| /// Expression describing how a PCS output is derived from raw claim terms. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum BatchOutputExpression<F: Field, ClaimId> { | ||
| /// Linear relation `output = Σ coefficient_i * claim_i`. | ||
| Linear(Vec<(ClaimId, F)>), | ||
| } |
There was a problem hiding this comment.
can we fold this into BatchOutputRelation instead of introducing a new type?
| materialize_source_evaluations(source) | ||
| } | ||
|
|
||
| type ProverPointGroup<F, PCS, S> = Vec<(Vec<F>, Vec<ProverClaimWithHint<F, PCS, S>>)>; |
There was a problem hiding this comment.
i feel like a map makes more sense here?
| groups | ||
| } | ||
|
|
||
| type OpeningPointGroup<F, PCS> = Vec<(Vec<F>, Vec<OpeningClaim<F, PCS>>)>; |
| /// cannot: their verifier setup contains trapdoor-derived elements generated | ||
| /// during setup, so verifier-only code should receive the verifier setup as an | ||
| /// input rather than pretending it can derive it from public generators. | ||
| pub trait PublicVerifierSetup: CommitmentSchemeVerifier { |
There was a problem hiding this comment.
why have a separate trait here?
|
|
||
| fn verifier_setup(prover_setup: &Self::ProverSetup) -> Self::VerifierSetup; | ||
| /// Projects prover setup down to verifier setup. | ||
| fn project_verifier_setup(prover_setup: &Self::ProverSetup) -> Self::VerifierSetup; |
There was a problem hiding this comment.
| fn project_verifier_setup(prover_setup: &Self::ProverSetup) -> Self::VerifierSetup; | |
| fn prover_to_verifier_setup(prover_setup: &Self::ProverSetup) -> Self::VerifierSetup; |
| /// A dense row of unsigned 64-bit integers embedded canonically into the field. | ||
| /// | ||
| /// This preserves the common compact-polynomial benchmark and commitment | ||
| /// path without paying the cost of first converting every row entry into a | ||
| /// full-width field element. | ||
| U64(&'a [u64]), | ||
|
|
||
| /// A strided unsigned-integer row embedded canonically into the field. | ||
| StridedU64 { | ||
| values: &'a [u64], | ||
| column_stride: usize, | ||
| }, |
There was a problem hiding this comment.
I think I slightly prefer to collapse this into a single variant, since U64 is just the special case of StridedU64 with a stride of 1. Same for FieldElements and I128. wdyt?
2 participants
Summary
This PR now scopes the cutover to the extracted PCS crates only. The earlier
jolt-coreintegration experiment was useful for finding the right abstraction boundary, but the merge target leaves legacyjolt-coreuntouched so it can remain the end-to-end reference implementation.The intended consumer for this API is the upcoming Bolt-generated prover/verifier split, such as #1514. Generated role crates should be able to use
jolt-openings/jolt-dorydirectly without going through the old in-core PCS trait family.What Changed
crates/jolt-openings.natural_chunk_lenand separate linear-combination opening support.jolt-openings.jolt-dory, preserving source-backed streaming behavior and ZK openings.DoryScheme::BatchProof = Vec<DoryProof>, with one proof per opening-point group.jolt-hyperkzgaligned with the same base trait family.jolt-core, SDK, example, transpiler, CLI, book, and extractor diff surface from the PR.Validation
cargo fmt -q --checkcargo check -p jolt-openings -q --features test-utilscargo check -p jolt-dory -qcargo check -p jolt-hyperkzg -qcargo nextest run -p jolt-openings --cargo-quiet --features test-utilscargo nextest run -p jolt-dory --cargo-quietcargo nextest run -p jolt-hyperkzg --cargo-quietcargo clippy -p jolt-openings -q --features test-utils --all-targets -- -D warningscargo clippy -p jolt-dory -q --all-targets -- -D warningscargo clippy -p jolt-hyperkzg -q --all-targets -- -D warningsPosted by Cursor assistant (model: GPT-5.5) on behalf of the user (Quang Dao) with approval.