feat(jolt-witness): add witness crate and modular helpers#1512
feat(jolt-witness): add witness crate and modular helpers#1512markosg04 wants to merge 1 commit into
Conversation
|
Warning This PR has more than 500 changed lines and does not include a spec. Large features and architectural changes benefit from a spec-driven workflow. If this PR is a bug fix, refactor, or doesn't warrant a spec, feel free to ignore this message. |
e5ea5bb to
76c5b22
Compare
01d21cc to
8818148
Compare
8818148 to
c6aa875
Compare
Benchmark comparison (crates) |
|
Claude code review session started: https://claude.ai/code/session_019WRFqns5WTTMS4Teyas7Jw |
moodlezoup
left a comment
There was a problem hiding this comment.
Reviewed the new jolt-witness crate and the supporting helpers in jolt-poly/jolt-r1cs/jolt-transcript. The migrated GruenSplitEqPolynomial and ExpandingTable look algorithmically faithful to the jolt-core originals. A few items below — the biggest one is that MockTranscript is exposed unconditionally as a public re-export, which is a soundness footgun if anything ever instantiates it outside tests.
Generated by Claude Code
| pub use digest::DigestTranscript; | ||
| pub use domain::{Label, LabelWithCount, U64Word}; | ||
| pub use keccak::KeccakTranscript; | ||
| pub use mock::MockTranscript; |
There was a problem hiding this comment.
MockTranscript is exposed as an unconditional pub re-export. Its append_* are no-ops, state() is a fixed seed, and the default new() ignores the label — anything that constructs it instead of Blake2bTranscript/KeccakTranscript silently collapses Fiat-Shamir and produces unsound proofs. Worth gating both mod mock (line 54) and this re-export behind #[cfg(any(test, feature = "test-utils"))] so misuse is impossible at link time.
Generated by Claude Code
| /// Evaluates a `u64`-valued multilinear extension at `point`. | ||
| pub fn mle_eval_u64<F: Field>(values: &[u64], point: &[F]) -> F { | ||
| EqPolynomial::<F>::evals(point, None) | ||
| .iter() | ||
| .zip(values) | ||
| .map(|(&weight, &value)| weight * F::from_u64(value)) | ||
| .sum() |
There was a problem hiding this comment.
.iter().zip(values) silently stops at the shorter side. If a caller passes values whose length isn't exactly 1 << point.len(), the result is a partial MLE with no warning — and stage4_ram_val_init_opening just below relies on the caller matching them. An explicit length check would surface miscalibrations instead of returning a wrong field element.
| /// Evaluates a `u64`-valued multilinear extension at `point`. | |
| pub fn mle_eval_u64<F: Field>(values: &[u64], point: &[F]) -> F { | |
| EqPolynomial::<F>::evals(point, None) | |
| .iter() | |
| .zip(values) | |
| .map(|(&weight, &value)| weight * F::from_u64(value)) | |
| .sum() | |
| /// Evaluates a `u64`-valued multilinear extension at `point`. | |
| pub fn mle_eval_u64<F: Field>(values: &[u64], point: &[F]) -> F { | |
| assert_eq!( | |
| values.len(), | |
| 1 << point.len(), | |
| "mle_eval_u64 expected {} values, got {}", | |
| 1 << point.len(), | |
| values.len() | |
| ); | |
| EqPolynomial::<F>::evals(point, None) | |
| .iter() | |
| .zip(values) | |
| .map(|(&weight, &value)| weight * F::from_u64(value)) | |
| .sum() | |
| } |
Generated by Claude Code
| instruction_ra_booleanity: Vec::new(), | ||
| bytecode_ra_booleanity: Vec::new(), | ||
| ram_ra_booleanity: Vec::new(), | ||
| bytecode_ra_read_raf: Vec::new(), |
There was a problem hiding this comment.
stage6_witness_polynomials leaves instruction_ra_booleanity, bytecode_ra_booleanity, ram_ra_booleanity, and bytecode_ra_read_raf as Vec::new(), but Stage6WitnessPolynomials::slices() happily folds those empties into booleanity_chunks and bytecode_ra_read_raf_chunks. A caller that takes the returned struct and immediately calls slices() gets misleadingly-aligned empty slices with no type-level signal that the witness is half-built. Worth either documenting the two-phase contract on the struct/function or splitting into Stage6WitnessPartial + Stage6WitnessComplete so the type system enforces ordering.
Generated by Claude Code
| } else { | ||
| assert!(!e_out.is_empty(), "split eq e_out invariant"); | ||
| std::process::abort(); | ||
| } |
There was a problem hiding this comment.
std::process::abort() here is unreachable in practice (the assert! on the line above panics first) and, if it ever did run, would kill the whole process without unwinding or a backtrace — worse than panic!/unreachable!, and inconsistent with the .expect("E_in_vec is never empty") pattern in jolt-core/src/poly/split_eq_poly.rs.
| } else { | |
| assert!(!e_out.is_empty(), "split eq e_out invariant"); | |
| std::process::abort(); | |
| } | |
| } else { | |
| unreachable!("split eq e_out invariant violated: e_out is empty"); | |
| } |
Generated by Claude Code
| } | ||
|
|
||
| fn state(&self) -> &[u8; 32] { | ||
| &self.seed |
There was a problem hiding this comment.
state() returns the original seed regardless of how many challenges have been squeezed, so two mock transcripts that have advanced to different counters still compare equal — which defeats the trait's documented "synchronization-detection" purpose. Either fold counter into the returned digest (e.g. cache H(seed || counter) on each squeeze) or document the limitation explicitly so callers don't trust state() for divergence checks.
Generated by Claude Code
| pub ram_addresses: Vec<Option<u128>>, | ||
| pub bytecode_indices: Vec<Option<u128>>, |
There was a problem hiding this comment.
this file is a bit bloated, let's split it up
| impl CycleInput { | ||
| pub const PADDING: Self = Self { | ||
| dense: [0; NUM_DENSE_TRACE_COLUMNS], | ||
| one_hot: [Some(0), Some(0), None], |
| pub struct CycleInput { | ||
| pub dense: [i128; NUM_DENSE_TRACE_COLUMNS], | ||
| pub one_hot: [Option<u128>; NUM_ONE_HOT_TRACE_SOURCES], | ||
| } |
There was a problem hiding this comment.
why this shape, as opposed to:
| pub struct CycleInput { | |
| pub dense: [i128; NUM_DENSE_TRACE_COLUMNS], | |
| pub one_hot: [Option<u128>; NUM_ONE_HOT_TRACE_SOURCES], | |
| } | |
| pub struct CycleInput { | |
| pub rd_inc: i128, | |
| pub ram_inc: i128, | |
| pub instruction_keys: Option<u128>, | |
| pub ram_addresses: Option<u128>, | |
| pub bytecode_indices: Option<u128>, | |
| } |
| } | ||
|
|
||
| /// Returns a dense trace source by its generated oracle source name. | ||
| pub fn dense_cycle_source(cycle_inputs: &[CycleInput], source: &str) -> Vec<i128> { |
There was a problem hiding this comment.
I'm lacking context here, but why &str as opposed to enum variants for source labels?
| /// Deterministic placeholder data for optional advice oracles in synthetic tests. | ||
| pub fn deterministic_oracle_data<F: Field>(oracle: &str, num_vars: usize) -> Vec<F> { |
There was a problem hiding this comment.
put this behind #[cfg(test)] if it's only used for tests
| EqPolynomial::<F>::evals(point, None) | ||
| .iter() | ||
| .zip(values) | ||
| .map(|(&weight, &value)| weight * F::from_u64(value)) |
There was a problem hiding this comment.
| .map(|(&weight, &value)| weight * F::from_u64(value)) | |
| .map(|(&weight, &value)| weight.mul_u64(value)) |
Adds the jolt-witness crate and the supporting modular crate APIs needed before landing the Bolt and generated-code stack.
The new witness crate centralizes Bolt-facing witness/oracle helpers for commitment trace inputs, one-hot chunk materialization, sparse stage 4/5 trace data, stage 6 bytecode/read-RAF/booleanity inputs, and small deterministic oracle helpers used by generated tests.
This also moves the primitive modular crates onto the semantic surface expected by Bolt-generated code: a simplified jolt-field trait bundle, transcript byte-length accounting and MockTranscript, reusable polynomial/source helpers, R1CS row-dot helpers, trace extraction over CycleRow, canonical lookup-table ordering/names, and sumcheck verifier helper modules.