Conversation
|
@copilot please review this PR |
0f7f15e to
80d5537
Compare
|
@copilot review this PR |
Reviewed and fixed in commit
|
db93d40 to
80d5537
Compare
|
@copilot review this PR using |
* claude code impl plan * implement the jagged_sumcheck using time-space tradeoff sumcheck prover algorithm * remove debugging codes * ref to the original paper * add jagged sumcheck bench * #32 parallelize (#34) * par wip * check f(z) * g(z) matches expected evaluation * fix clippy * fix clippy: add #[cfg(test)] to test-only method and fix unused import/variable warnings Agent-Logs-Url: https://github.com/scroll-tech/gkr-backend/sessions/4a61316e-cf28-47b8-a43e-fb6ab432701e Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> * refactor test * apply functional programming style * avoid unnecessary BaseField-to-ExtensionField conversion in q_evals access Use E * BaseField multiplication directly instead of converting q_evals elements to extension field with .into() first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * replace col_row binary search with incremental ColRowIter Add ColRowIter that does one binary search at construction and O(1) per step, replacing per-element binary searches in build_m_table, bind_and_materialize, compute_claimed_sum, and final_evaluations_slow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * extend jagged sumcheck benchmark to cover n=25..31 * switch jagged sumcheck benchmark to BabyBearExt4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * remove jagged sumcheck plan doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an initial Jagged PCS adaptor to mpcs, including a commit-time packing of many column polynomials into a single “giga” MLE and a streaming jagged sumcheck prover intended for memory-constrained environments (per PR description and linked #32 context).
Changes:
- Expose a new
mpcs::jaggedmodule and re-export jagged commitment/sumcheck APIs frommpcs::lib. - Implement
jagged_commit(bit-reverse rows + transpose + concatenate) andjagged_sumcheck_prove(epoch-based M-table streaming + fallback to standard sumcheck). - Add a Criterion benchmark for jagged sumcheck and introduce
tracing-forestas a dev-dependency.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mpcs/src/lib.rs | Exports the new jagged module and its public API types/functions. |
| crates/mpcs/src/jagged.rs | Implements jagged commit packing and streaming jagged sumcheck prover, plus unit tests. |
| crates/mpcs/benches/jagged_sumcheck.rs | Adds a Criterion benchmark for the jagged sumcheck prover across n=25..31. |
| crates/mpcs/Cargo.toml | Registers the new benchmark and adds tracing-forest dev-dependency. |
| Cargo.lock | Locks tracing-forest and related transitive dependencies. |
| //! The cumulative heights allow the verifier to succinctly evaluate the indicator function | ||
| //! `g(z_r, z_b, t[i-1], t[i])` needed for the jagged sumcheck. |
There was a problem hiding this comment.
The module docs describe using an indicator function g(z_r, z_b, t[i-1], t[i]) to handle jagged segment boundaries, but the current implementation of f(b) throughout the prover is eq_row[row] * eq_col[col] (no dependence on t beyond (col,row) mapping). Either update the docs to match the implemented (uniform-height) assumption, or implement the g(...) weighting so variable-height segments are handled as described.
| //! The cumulative heights allow the verifier to succinctly evaluate the indicator function | |
| //! `g(z_r, z_b, t[i-1], t[i])` needed for the jagged sumcheck. | |
| //! In this adaptor, `t` is used for segment layout and for recovering the `(column, row)` | |
| //! coordinates of a packed evaluation. The prover logic then uses the standard product-form | |
| //! equality weights induced by those coordinates (effectively `eq_col[column] * eq_row[row]`). | |
| //! This implementation does **not** currently implement the more general boundary-weighting | |
| //! indicator `g(z_r, z_b, t[i-1], t[i])` described in the full jagged-sumcheck construction; | |
| //! accordingly, the documentation here is restricted to the uniform-height packing behavior | |
| //! implemented by this module. |
| |(q_acc, f_acc), ((&eq_r_a, &q), (col, row))| { | ||
| ( | ||
| q_acc + eq_r_a * q, | ||
| f_acc + eq_r_a * (input.eq_row[row] * input.eq_col[col]), | ||
| ) |
There was a problem hiding this comment.
row/col from col_row_iter are used to index input.eq_row[row] and input.eq_col[col] without any validation. If eq_row.len() is smaller than the max polynomial height or eq_col.len() is smaller than num_polys, this will panic. Add an upfront check in jagged_sumcheck_prove (or a constructor for JaggedSumcheckInput) that eq_row.len() >= max_height and eq_col.len() >= num_polys, or change the API to return a Result with a clear error.
| |(q_acc, f_acc), ((&eq_r_a, &q), (col, row))| { | ||
| ( | ||
| q_acc + eq_r_a * q, | ||
| f_acc + eq_r_a * (input.eq_row[row] * input.eq_col[col]), | ||
| ) | ||
| }, |
There was a problem hiding this comment.
Same issue as in build_m_table: input.eq_row[row] and input.eq_col[col] are indexed without checking that the precomputed tables are large enough for the (col,row) pairs implied by cumulative_heights. Without an upfront validation, this can panic for malformed inputs or when polynomials have larger heights than the provided eq_row table.
| fn test_jagged_sumcheck_n25() { | ||
| // n=25: 2^25 = 33M evaluations. Exercises all epochs + 10 rounds of standard sumcheck. | ||
| use ff_ext::FromUniformBytes; | ||
|
|
||
| tracing_forest::init(); | ||
|
|
||
| let mut rng = thread_rng(); | ||
|
|
||
| let num_polys = 1 << 10; // 1024 polynomials | ||
| let poly_height = 1 << 15; // 32768 each, s=15 | ||
| let s = 15; | ||
| let total_evals = num_polys * poly_height; // 2^25 = 33554432 | ||
| let num_giga_vars = 25; | ||
|
|
||
| let q_evals: Vec<F> = (0..total_evals) | ||
| .map(|i| F::from_canonical_u64((i as u64 * 13 + 7) % (1 << 30))) | ||
| .collect(); |
There was a problem hiding this comment.
test_jagged_sumcheck_n25 allocates and processes 2^25 (= 33,554,432) field elements (and additionally builds f_evals in final_evaluations_slow), which is likely to exceed typical CI time/memory budgets and make cargo test flaky/slow. Consider marking this test #[ignore], gating it behind a feature/env var, or moving it to a Criterion bench (keeping unit tests at small sizes that still cover the same code paths).
| tracing_forest::init(); | ||
|
|
There was a problem hiding this comment.
Calling tracing_forest::init() inside a unit test performs global tracing subscriber initialization; this can conflict with other tests that also initialize tracing and can fail/panic depending on the tracing-forest behavior. Prefer a fallible/once-only init (e.g., via a Once guard or a try_init-style API) or remove tracing initialization from the test.
| tracing_forest::init(); |
…rove (#38) * claude code impl plan * implement the jagged_sumcheck using time-space tradeoff sumcheck prover algorithm * remove debugging codes * ref to the original paper * add jagged sumcheck bench * #32 parallelize (#34) * par wip * check f(z) * g(z) matches expected evaluation * fix clippy * fix clippy: add #[cfg(test)] to test-only method and fix unused import/variable warnings Agent-Logs-Url: https://github.com/scroll-tech/gkr-backend/sessions/4a61316e-cf28-47b8-a43e-fb6ab432701e Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> * refactor test * apply functional programming style * avoid unnecessary BaseField-to-ExtensionField conversion in q_evals access Use E * BaseField multiplication directly instead of converting q_evals elements to extension field with .into() first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * replace col_row binary search with incremental ColRowIter Add ColRowIter that does one binary search at construction and O(1) per step, replacing per-element binary searches in build_m_table, bind_and_materialize, compute_claimed_sum, and final_evaluations_slow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * extend jagged sumcheck benchmark to cover n=25..31 * switch jagged sumcheck benchmark to BabyBearExt4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * remove jagged sumcheck plan doc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Initial plan * Make EPOCH_SIZES configurable in jagged_sumcheck_prove via optional parameter Agent-Logs-Url: https://github.com/scroll-tech/gkr-backend/sessions/b18805c7-6e15-44c0-ab03-a1905068d964 Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> * Fix doc spacing and add debug_assert for epoch_sizes validation Agent-Logs-Url: https://github.com/scroll-tech/gkr-backend/sessions/b18805c7-6e15-44c0-ab03-a1905068d964 Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> --------- Co-authored-by: kunxian xia <xiakunxian130@gmail.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: hero78119 <3962077+hero78119@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
This PR includes works in several separate PRs.
q(b)from the input row major matricesq(b)as several smaller multilinear polynomials)