Refactor/new init #1129
Refactor/new init #1129
Conversation
moodlezoup
force-pushed
the
refactor/new-init
branch
from
56d26b8 to
b719c67
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the initialization pattern for sumcheck provers and verifiers across the codebase. The main changes introduce a new SumcheckInstanceParams trait that encapsulates parameters shared between provers and verifiers, separating parameter creation from instance initialization.
Key changes:
- Introduces
SumcheckInstanceParamstrait withdegree(),num_rounds(),input_claim(), andnormalize_opening_point()methods - Renames prover/verifier constructors from
gen()toinitialize()to distinguish from parameter generation - Extracts parameter structs to be public and reusable between prover and verifier
- Adds
UnivariateSkipvariant toVirtualPolynomialenum for univariate skip tracking - Moves
ValFinalSumcheckimplementation to separateval_final.rsfile
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| jolt-core/src/zkvm/witness.rs | Adds UnivariateSkip variant to VirtualPolynomial enum |
| jolt-core/src/zkvm/verifier.rs | Updates imports and changes parameter passing to use new params pattern |
| jolt-core/src/zkvm/spartan/*.rs | Refactors shift, product, outer sumcheck to use params pattern |
| jolt-core/src/zkvm/ram/*.rs | Refactors RAM sumchecks and extracts val_final to separate file |
| jolt-core/src/zkvm/registers/*.rs | Updates register sumchecks to use params pattern |
| jolt-core/src/zkvm/instruction_lookups/*.rs | Updates instruction lookup sumchecks to use params pattern |
| jolt-core/src/zkvm/bytecode/*.rs | Updates bytecode sumchecks to use params pattern |
| jolt-core/src/subprotocols/*.rs | Adds SumcheckInstanceParams trait and updates univariate skip |
| jolt-core/src/zkvm/prover.rs | Updates prover to use new initialization pattern throughout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Allocative)] | ||
| pub struct ValFinalSumcheckProver<F: JoltField> { | ||
| inc: MultilinearPolynomial<F>, | ||
| wa: MultilinearPolynomial<F>, | ||
| #[allocative(skip)] | ||
| params: ValFinalSumcheckParams<F>, | ||
| } | ||
|
|
||
| impl<F: JoltField> ValFinalSumcheckProver<F> { |
There was a problem hiding this comment.
ValFinal moved into its own file
quangvdao
left a comment
quangvdao
left a comment
There was a problem hiding this comment.
Thanks for the PR! Just one comment
danielwlz
left a comment
danielwlz
left a comment
There was a problem hiding this comment.
Thanks for the PR! How would you feel about making members of structs like XXXXSumcheckParams public by default? I suggested changes only for those sumchecks that I knew needed it, but I figure other sumchecks will also require visibility changes.
| } | ||
|
|
||
| #[derive(Allocative)] | ||
| pub(crate) struct ValEvaluationSumcheckProver<F: JoltField> { |
There was a problem hiding this comment.
| pub(crate) struct ValEvaluationSumcheckProver<F: JoltField> { | |
| pub struct ValEvaluationSumcheckProver<F: JoltField> { |
| struct ReadRafSumcheckParams<F: JoltField> { | ||
| pub struct ReadRafSumcheckParams<F: JoltField> { | ||
| /// Index `i` stores `gamma^i`. | ||
| gamma_powers: Vec<F>, |
There was a problem hiding this comment.
| gamma_powers: Vec<F>, | |
| pub gamma_powers: Vec<F>, |
| gamma_powers: Vec<F>, | ||
| /// RLC of stage rv_claims and RAF claims (per Stage1/Stage3) used as the sumcheck LHS. | ||
| rv_claim: F, | ||
| input_claim: F, |
There was a problem hiding this comment.
| input_claim: F, | |
| pub input_claim: F, |
| rv_claim: F, | ||
| input_claim: F, | ||
| /// RaParams | ||
| one_hot_params: OneHotParams, |
There was a problem hiding this comment.
| one_hot_params: OneHotParams, | |
| pub one_hot_params: OneHotParams, |
| /// RaParams | ||
| one_hot_params: OneHotParams, | ||
| /// Bytecode length. | ||
| K: usize, |
There was a problem hiding this comment.
| pub K: usize, |
| log_K: usize, | ||
| log_T: usize, | ||
| /// Number of address chunks (and RA polynomials in the product). | ||
| d: usize, | ||
| /// Stage Val polynomials evaluated over address vars. | ||
| val_polys: [MultilinearPolynomial<F>; N_STAGES], | ||
| /// Stage rv claims. | ||
| rv_claims: [F; N_STAGES], | ||
| raf_claim: F, | ||
| raf_shift_claim: F, | ||
| /// Identity polynomial over address vars used to inject RAF contributions. | ||
| int_poly: IdentityPolynomial<F>, | ||
| r_cycles: [Vec<F::Challenge>; N_STAGES], |
There was a problem hiding this comment.
Need to make all members pub
Splits the
genfunction for each sumcheck prover instance intonewandinitialize.The general principle:
*::newis a relatively cheap operation, and mainly involves pulling values out of the opening accumulator and/or transcript*::initializeis a relatively expensive operation, and typically involves reading the trace.initializeshould not need the opening accumulator or transcript.