Sumcheck IOP for LogUp-GKR #295

Al-Kindi-0 · 2024-08-07T06:19:36Z

Adds a sumcheck crate containing the necessary tools for the LogUp-GKR protocol.

irakliyk

Thank you! This looks great! This not a full review by any means, but I left a few preliminary comments inline (mostly nits).

sumcheck/Cargo.toml

sumcheck/src/utils/univariate.rs

sumcheck/src/prover/mod.rs

sumcheck/src/utils/mod.rs

sumcheck/src/prover/mod.rs

sumcheck/src/verifier/mod.rs

air/src/air/mod.rs

irakliyk

Looks good! Thank you! I left some more comments inline. Most of the comments are minor and the rest are things we can do in the future (i.e., adding benchmarks).

Also, let's add a simple README.md for the wintersum check crate.

Cargo.toml

air/src/air/logup_gkr.rs

sumcheck/src/prover/high_degree.rs

sumcheck/src/verifier/mod.rs

Al-Kindi-0 · 2024-08-19T15:29:31Z

I have added benchmarks for all the critical components and I am seeing between 3x and 4x improvements for the multi-linear related methods on an 8 core laptop.

For the sum-check, which is the main target, I am seeing the following

and

Could you confirm on yours?

air/src/air/logup_gkr.rs

sumcheck/src/multilinear.rs

sumcheck/README.md

sumcheck/benches/bind_variable.rs

sumcheck/benches/eq_function.rs

sumcheck/benches/sum_check_high_degree.rs

sumcheck/benches/sum_check_plain.rs

sumcheck/src/prover/high_degree.rs

irakliyk

All looks good! Thank you!

irakliyk · 2024-08-20T00:03:22Z

sumcheck/src/lib.rs

+///
+/// This is the result of batching the `p_k` and `q_k` of section 3.2 in
+/// https://eprint.iacr.org/2023/1284.pdf.
+fn comb_func<E: FieldElement>(p0: E, p1: E, q0: E, q1: E, eq: E, r_batch: E) -> E {


Given how frequently this function is used, I'd probably add #[inline(always)] to it.

irakliyk · 2024-08-20T00:05:29Z

Oh - could you fix the couple of lints to make the CI green? (I think one of them can be disabled).

irakliyk · 2024-08-20T00:22:43Z

I have added benchmarks for all the critical components and I am seeing between 3x and 4x improvements for the multi-linear related methods on an 8 core laptop.

I'm seeing roughly similar numbers - though absolute numbers are faster while relative improvements between serial and concurrent are smaller (but seems to get better with size).

Al-Kindi-0 · 2024-08-20T09:24:03Z

Oh - could you fix the couple of lints to make the CI green? (I think one of them can be disabled).

Fixed!

plafer

(partial review)

plafer · 2024-08-20T13:44:19Z

sumcheck/src/prover/plain.rs

+    p0: &mut MultiLinearPoly<E>,
+    p1: &mut MultiLinearPoly<E>,
+    q0: &mut MultiLinearPoly<E>,
+    q1: &mut MultiLinearPoly<E>,
+    eq: &mut MultiLinearPoly<E>,


I would change these variables to be

p_next: mut MultiLinearPoly<E>, q_next: mut MultiLinearPoly<E>, eq: mut MultiLinearPoly<E>,

and run the ML::bind_least_significant_variable() inside this function. This is closer to the math in the docs, where we have 2 polynomials p and q (not 4).

Note also that we now take ownership of the multilinear polys, since AFAIK we never need to mutated result (in the GKR PR).

plafer · 2024-08-20T13:45:47Z

sumcheck/src/prover/high_degree.rs

+    claim: E,
+    r_sum_check: E,
+    log_up_randomness: Vec<E>,
+    mls: &mut [MultiLinearPoly<E>],


Similarly, this can be a mut Vec<MultiLinearPoly<E>>

plafer · 2024-08-20T13:49:39Z

air/src/air/logup_gkr.rs

+    fn evaluate_query<F, E>(
+        &self,
+        query: &[F],
+        rand_values: &[E],


nit: would rename this to logup_randomness

plafer · 2024-08-20T13:50:57Z

air/src/air/logup_gkr.rs

+
+use super::EvaluationFrame;
+
+pub trait LogUpGkrEvaluator: Clone + Sync {


Can we add a docstring here? This trait does a lot of things and it's a little bit hard to get a handle on exactly what

plafer · 2024-08-20T14:35:12Z

sumcheck/src/multilinear.rs

+    pub fn project_least_significant_variable(&self) -> (Self, Self) {
+        let mut p0 = Vec::with_capacity(self.num_evaluations() / 2);
+        let mut p1 = Vec::with_capacity(self.num_evaluations() / 2);
+        for chunk in self.evaluations.chunks_exact(2) {
+            p0.push(chunk[0]);
+            p1.push(chunk[1]);
+        }
+
+        (MultiLinearPoly::from_evaluations(p0), MultiLinearPoly::from_evaluations(p1))
+    }


We could try rewriting this function as:

pub fn project_least_significant_variable(mut self) -> (Self, Self) { let odds: Vec<E> = self .evaluations .iter() .enumerate() .filter_map(|(idx, x)| if idx % 2 == 1 { Some(*x) } else { None }) .collect(); // Builds the evens multilinear from the current `self.evaluations` buffer, which saves an // allocation. let evens = { let evens_size = self.num_evaluations() / 2; for write_idx in 0..evens_size { let read_idx = write_idx * 2; self.evaluations[write_idx] = self.evaluations[read_idx]; } self.evaluations.truncate(evens_size); self.evaluations }; (Self::from_evaluations(evens), Self::from_evaluations(odds)) }

This version saves an allocation compared to the current version. I'd be curious how this affects the sum-check benchmark (after we moved the project_least_significant_variable() call into the sum-check, as mentioned in another comment).

Done, thank you for the suggestion!
I haven't noticed any noticeable difference on machine (x86) but maybe it is different on other archs.

plafer

review of sumcheck_prove_plain

plafer · 2024-08-20T14:39:52Z

sumcheck/src/prover/plain.rs

+) -> Result<SumCheckProof<E>, SumCheckProverError> {
+    let mut round_proofs = vec![];
+
+    let mut claim = claim;


we can make the claim function argument mut and remove this statement

plafer · 2024-08-20T15:25:10Z

sumcheck/src/prover/plain.rs

+            transcript.draw().map_err(|_| SumCheckProverError::FailedToGenerateChallenge)?;
+
+        // compute the new reduced round claim
+        let new_claim = round_poly_coefs.evaluate_using_claim(&claim, &round_challenge);


this statement can be moved after the calls to bind_least_significant_variable() (and assigned to claim directly)

plafer · 2024-08-20T15:26:46Z

sumcheck/src/prover/plain.rs

+        let len = p0.num_evaluations() / 2;
+
+        #[cfg(not(feature = "concurrent"))]
+        let (eval_point_1, eval_point_2, eval_point_3) =


Suggested change

let (eval_point_1, eval_point_2, eval_point_3) =

let (round_poly_at_1, round_poly_at_2, round_poly_at_3) =

I would make it explicit that the evaluations are ultimately for the round poly. It makes it more clear at this point in the function that we are computing the evaluations of the round polynomial.

Also, I would be helpful to make it explicit in the docstring that the degree of this sumcheck's g is 3

plafer · 2024-08-20T15:29:58Z

sumcheck/src/prover/plain.rs

+
+        #[cfg(not(feature = "concurrent"))]
+        let (eval_point_1, eval_point_2, eval_point_3) =
+            (0..len).fold((E::ZERO, E::ZERO, E::ZERO), |(a, b, c), i| {


Suggested change

(0..len).fold((E::ZERO, E::ZERO, E::ZERO), |(a, b, c), i| {

(0..len).fold((E::ZERO, E::ZERO, E::ZERO), |(acc_point_1, acc_point_2, acc_point_3), i| {

I would use more descriptive names for a, b and c (and similarly for the concurrent version)

plafer · 2024-08-20T15:53:52Z

sumcheck/src/prover/plain.rs

+                    p1[2 * i + 1],
+                    q0[2 * i + 1],
+                    q1[2 * i + 1],
+                    eq[2 * i + 1],


Reading this index-based evaluation is a little bit hard to read. I would be curious to see if explicitly representing the bits that we're querying improves readability, and if it affects performance.

Obviously not in this PR though

plafer · 2024-08-20T15:57:10Z

sumcheck/src/prover/plain.rs

+                let mut p0_evx = p0[2 * i + 1] + p0_delta;
+                let mut p1_evx = p1[2 * i + 1] + p1_delta;
+                let mut q0_evx = q0[2 * i + 1] + q0_delta;
+                let mut q1_evx = q1[2 * i + 1] + q1_delta;
+                let mut eq_evx = eq[2 * i + 1] + eq_delta;


Would rename those to e.g. p0_eval_at_x or just p0_at_x. I never remember what evx stands for

plafer · 2024-08-20T15:57:37Z

sumcheck/src/prover/plain.rs

+                |(a0, b0, c0), (a1, b1, c1)| (a0 + a1, b0 + b1, c0 + c1),
+            );
+
+        let evals = smallvec![eval_point_1, eval_point_2, eval_point_3,];


Suggested change

let evals = smallvec![eval_point_1, eval_point_2, eval_point_3,];

let compressed_round_poly_evals = smallvec![eval_point_1, eval_point_2, eval_point_3,];

plafer · 2024-08-20T15:58:10Z

sumcheck/src/prover/plain.rs

+            );
+
+        let evals = smallvec![eval_point_1, eval_point_2, eval_point_3,];
+        let poly = CompressedUnivariatePolyEvals(evals);


Suggested change

let poly = CompressedUnivariatePolyEvals(evals);

let compressed_round_poly = CompressedUnivariatePolyEvals(evals);

plafer · 2024-08-20T15:58:21Z

sumcheck/src/prover/plain.rs

+
+        let evals = smallvec![eval_point_1, eval_point_2, eval_point_3,];
+        let poly = CompressedUnivariatePolyEvals(evals);
+        let round_poly_coefs = poly.to_poly(claim);


Suggested change

let round_poly_coefs = poly.to_poly(claim);

let round_poly = poly.to_poly(claim);

plafer

All LGTM modulo some nits that we can take care of in subsequent PRs!

Al-Kindi-0 · 2024-08-20T17:23:39Z

I think I have addressed all of the comments above.

Al-Kindi-0 added 5 commits August 6, 2024 14:49

feat: math utilities needed for sum-check protocol

8ac25c7

feat: add sum-check prover and verifier

5e06378

tests: add sanity tests for utils

16389d6

doc: document sumcheck_round

380aa1a

feat: use SmallVec

7a1a99e

facebook-github-bot added the cla signed label Aug 7, 2024

docs: improve documentation of sum-check

1901066

irakliyk reviewed Aug 9, 2024

View reviewed changes

Al-Kindi-0 added 5 commits August 9, 2024 09:34

feat: add remaining functions for sum-check verifier

8a57216

chore: move prover into sub-mod

ff9e6fa

chore: remove utils mod

7e24f8f

chore: remove utils mod

23044e8

chore: move logup evaluator trait to separate file

ad0497d

Al-Kindi-0 mentioned this pull request Aug 9, 2024

GKR backend for LogUp-GKR #296

Merged

Al-Kindi-0 added 3 commits August 15, 2024 15:36

feat: add multi-threading support and simplify input sum-check

d721bc2

chore: fix Sync issue

2e5a704

chore: pacify clippy

06454d9

irakliyk approved these changes Aug 19, 2024

View reviewed changes

Al-Kindi-0 added 5 commits August 19, 2024 11:23

chore: fix toml formatting

2c07857

feat: add benchmarks and address feedback

2e02f1f

feat: address feedback and add benchmarks

e7a50c0

feat: simplify sum-check bench

2b64dbf

feat: add sum-check benchmarks

ea7cb84

irakliyk reviewed Aug 19, 2024

View reviewed changes

doc: improve documentation

dae27e6

irakliyk approved these changes Aug 20, 2024

View reviewed changes

chore: pacify clippy

9543e1a

Al-Kindi-0 added 3 commits August 20, 2024 11:14

chore: pacify clippy

60c8591

chore: pacify clippy

7338caf

chore: pacify clippy

43c7441

plafer reviewed Aug 20, 2024

View reviewed changes

Al-Kindi-0 added 3 commits August 20, 2024 18:53

feat: reduce memory allocation in bind least signi

f85e44f

doc: add docs to logup evaluator trait

be0c2cd

chore: improve var naming

1847d0d

doc: improve

3adc855

irakliyk changed the base branch from next to logup-gkr August 21, 2024 06:15

irakliyk merged commit d507eb9 into facebook:logup-gkr Aug 21, 2024
8 checks passed

plafer mentioned this pull request Aug 21, 2024

LogUp-GKR proposition to simplify input layer polynomials 0xPolygonMiden/miden-vm#1417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sumcheck IOP for LogUp-GKR #295

Sumcheck IOP for LogUp-GKR #295

Al-Kindi-0 commented Aug 7, 2024

irakliyk left a comment

irakliyk left a comment

Al-Kindi-0 commented Aug 19, 2024

irakliyk left a comment

irakliyk Aug 20, 2024

Al-Kindi-0 Aug 20, 2024

irakliyk commented Aug 20, 2024

irakliyk commented Aug 20, 2024

Al-Kindi-0 commented Aug 20, 2024

plafer left a comment

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

Al-Kindi-0 Aug 20, 2024

plafer left a comment

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer Aug 20, 2024

plafer left a comment

Al-Kindi-0 commented Aug 20, 2024


		use super::EvaluationFrame;

		pub trait LogUpGkrEvaluator: Clone + Sync {

	let (eval_point_1, eval_point_2, eval_point_3) =
	let (round_poly_at_1, round_poly_at_2, round_poly_at_3) =

	(0..len).fold((E::ZERO, E::ZERO, E::ZERO), \|(a, b, c), i\| {
	(0..len).fold((E::ZERO, E::ZERO, E::ZERO), \|(acc_point_1, acc_point_2, acc_point_3), i\| {

	let evals = smallvec![eval_point_1, eval_point_2, eval_point_3,];
	let compressed_round_poly_evals = smallvec![eval_point_1, eval_point_2, eval_point_3,];

	let poly = CompressedUnivariatePolyEvals(evals);
	let compressed_round_poly = CompressedUnivariatePolyEvals(evals);

	let round_poly_coefs = poly.to_poly(claim);
	let round_poly = poly.to_poly(claim);

Sumcheck IOP for LogUp-GKR #295

Sumcheck IOP for LogUp-GKR #295

Conversation

Al-Kindi-0 commented Aug 7, 2024

irakliyk left a comment

Choose a reason for hiding this comment

irakliyk left a comment

Choose a reason for hiding this comment

Al-Kindi-0 commented Aug 19, 2024

irakliyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irakliyk commented Aug 20, 2024

irakliyk commented Aug 20, 2024

Al-Kindi-0 commented Aug 20, 2024

plafer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Al-Kindi-0 commented Aug 20, 2024