chore: Doc, unit test, minors #289

adria0 · 2024-02-26T16:11:13Z

This PR adds some documentation, improves code readeability and adds an unit test for GraphEvaluator

CPerezz

LGTM! Thanks for the great addition to the docs!

halo2_backend/src/plonk.rs

halo2_backend/src/plonk/evaluation.rs

Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com>

ed255

I've left a lot of comments, most of them are suggestions or discussions.
In particular many comments refer to bind vs blind. After seeing so many instances of bind I'm no longer sure if bind is a thing, or it's a common typo.

The documentation is just great! Specially all the details in prover/verifier, and the references to follow how the transcript is written in proving and then read in verifying, great job!

ed255 · 2024-02-29T14:56:09Z

halo2_backend/src/plonk/evaluation.rs

@@ -169,46 +173,59 @@ impl Calculation {
 #[derive(Clone, Default, Debug)]
 pub struct Evaluator<C: CurveAffine> {
    ///  Custom gates evalution
-    pub custom_gates: GraphEvaluator<C>,
+    custom_gates: GraphEvaluator<C>,


These were pub before the frontend-backend split:

halo2/halo2_proofs/src/plonk/evaluation.rs

Lines 169 to 173 in 73408a1

pub custom_gates: GraphEvaluator<C>,

/// Lookups evalution

pub lookups: Vec<GraphEvaluator<C>>,

/// Shuffle evalution

pub shuffles: Vec<GraphEvaluator<C>>,

I wonder why...

I checked this and it comes from an improvement that Brecht did, so this is not inherited from zcash. I guess this was unneeded pub that was unnoticed.

halo2_backend/src/plonk/prover.rs

ed255 · 2024-02-29T15:31:38Z

halo2_backend/src/plonk/prover.rs

+    /// - 4. Generate commited shuffle polys
+    /// - 5. Commit to the vanishing argument's random polynomial
+    /// - 6. Generate the advice polys
+    /// - 7. Evaluate the h(X) polynomial


This is confusing for me (even if this is a reference to the original code, which has the function evaluate_h, and there was even a comment saying Evaluate the h(X) polynomial).

The way I see it, we're not evaluating h(X), instead we're calculating h(X). I say this because in my mind, if we evaluate a polynomial, the result is a field value. But the result of step 7 is a polynomial.

ed255 · 2024-02-29T15:35:43Z

halo2_backend/src/plonk/prover.rs

@@ -563,14 +643,19 @@ impl<
            )
            .collect();

-        // Evaluate the h(X) polynomial
+        // 7. Evaluate the h(X) polynomial -----------------------------------------------------------


I would change this to the following

// 7. Given `h(X) = g(X) / t(X)` where: // - `g(X)` is the gates and arguments polynomial // - `t(X)` is the vanishing polynomial // - `h(X)` is the quotient polynomial // Calculate the g(X) polynomial

The reason I say g(X) instead of h(X) is because if you look at the implementation of evaluate_h it actually returns g

It was added here 70cfe70#diff-6fd234fc283be7a2c59f305cdd5f15f4bb074fa6b56866452a06b34c6803fc1fR274

I agree on changing it, and to also update the MD book with a reference to g(X)

@davidnevadoc @CPerezz @han0110 ?

I think we should ref the paper naming always.

Aside from that, happy to adapt to anything you consider better! :)

For reference, I took h and t from the halo2 book https://zcash.github.io/halo2/design/proving-system/vanishing.html
As for g, it's not called like that in the halo2 book, it just doesn't have a name.

Then the plonk paper uses different names.

Alternatively we can say also evaluate_h_dividend

Far from helping, I come to add more confusion 😅
I like the notation of the original Plonk paper, in which t is the quotient polynomial, and z_H (zero over the set H) is the vanishing poly.

I would say we should stick to the halo2 book names, h for the quotient and t for the vanishing.
With respect to g, I'm not sure if this polynomial ever needs to be computed explicitly. In any case, it could be h_times_t.

ed255 · 2024-02-29T15:36:24Z

halo2_backend/src/plonk/prover.rs

+        // Obtain challenge for keeping all separate gates linearly independent
+        // [TRANSCRIPT-13]
+        let y: ChallengeY<_> = self.transcript.squeeze_challenge_scalar();
+
        let h_poly = pk.ev.evaluate_h(


I would change this to:

let g_poly = pk.ev.calculate_g(

Please if somebody knows why this was called evaluate_h, can they explain it to me?

H is usually how the Vanishing polynomial is called in the Plonk paper.

Maybe it's due to that?

hm, but in the code this variable holds the polynomial that contains gate_1(X) + alpha * gate_2(X) + alpha^2 * gate_3(X) + ..., which is not the vanishing polynomial right?

Agree with @ed255, I would rename this to g. Currently h is both the quotient poly and the quotient poly * vanishing.

halo2/halo2_backend/src/plonk/vanishing/prover.rs

Lines 112 to 116 in 13fa01f

// Divide by t(X) = X^{params.n} - 1.

let h_poly = domain.divide_by_vanishing_poly(h_poly);

// Obtain final h(X) polynomial

let h_poly = domain.extended_to_coeff(h_poly);

ed255 · 2024-02-29T15:45:06Z

halo2_backend/src/plonk/verifier.rs

-                ),
-                shuffles,
-            )| {
+        .flat_map(|((((((instance_commitments, instance_evals), advice_commitments),advice_evals),permutation),lookups),shuffles)| {


This is madness 😆
This must feel like home to fans of LISP.

ed255

There are still some unresolved conversations but they are all about the naming around the quotient polynomial. It may take some discussion to settle that. I think we can defer that to the future and merge this PR already :)

davidnevadoc · 2024-03-04T14:56:48Z

halo2_backend/src/plonk/prover.rs

@@ -355,7 +394,7 @@ impl<
                    }
                }

-                // Compute commitments to advice column polynomials
+                // Compute affine commitments to advice column polynomials.


I think the explicit distinction between affine and projective in this section is nice, but I'd leave this comment as is. Mainly because "affine commitment" doesn't sound quite right, they are unrelated concepts.

Reverted in 9e29391.

davidnevadoc · 2024-03-04T14:57:29Z

halo2_backend/src/plonk/prover.rs

@@ -392,6 +435,9 @@ impl<
                Ok(())
            };

+        // Update bindings for each advice column


blinding or blinding factors maybe?

halo2_backend/src/plonk/prover.rs

davidnevadoc · 2024-03-04T15:29:01Z

halo2_backend/src/plonk/prover.rs

@@ -203,6 +217,8 @@ impl<
                    }
                }

+                // Convert the lagrange polynomials to coefficients


Same comment as before, I wouldn't call these Lagrange polynomials.
Convert from evaluation to coefficient form. maybe?

davidnevadoc · 2024-03-04T15:41:18Z

halo2_backend/src/plonk/prover.rs

@@ -563,14 +643,19 @@ impl<
            )
            .collect();

-        // Evaluate the h(X) polynomial
+        // 7. Evaluate the h(X) polynomial -----------------------------------------------------------


Far from helping, I come to add more confusion 😅
I like the notation of the original Plonk paper, in which t is the quotient polynomial, and z_H (zero over the set H) is the vanishing poly.

I would say we should stick to the halo2 book names, h for the quotient and t for the vanishing.
With respect to g, I'm not sure if this polynomial ever needs to be computed explicitly. In any case, it could be h_times_t.

davidnevadoc · 2024-03-04T15:48:50Z

halo2_backend/src/plonk/prover.rs

+        // Obtain challenge for keeping all separate gates linearly independent
+        // [TRANSCRIPT-13]
+        let y: ChallengeY<_> = self.transcript.squeeze_challenge_scalar();
+
        let h_poly = pk.ev.evaluate_h(


Agree with @ed255, I would rename this to g. Currently h is both the quotient poly and the quotient poly * vanishing.

halo2/halo2_backend/src/plonk/vanishing/prover.rs

Lines 112 to 116 in 13fa01f

// Divide by t(X) = X^{params.n} - 1.

let h_poly = domain.divide_by_vanishing_poly(h_poly);

// Obtain final h(X) polynomial

let h_poly = domain.extended_to_coeff(h_poly);

adria0 and others added 2 commits February 26, 2024 17:07

chore: Doc, unit test, minors

4402df7

Merge branch 'main' into fix/improve1

f920773

CPerezz self-requested a review February 26, 2024 19:14

Fix merge

f91c9ea

CPerezz approved these changes Feb 29, 2024

View reviewed changes

adria0 commented Feb 29, 2024

View reviewed changes

halo2_backend/src/plonk/evaluation.rs Outdated Show resolved Hide resolved

Apply @CPerezz suggestions from code review

69f0971

Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com>

ed255 reviewed Feb 29, 2024

View reviewed changes

adria0 and others added 2 commits February 29, 2024 18:00

Apply @ed255 suggestions from code review

7b73cf2

@ed255 review + fmt

0c5b227

ed255 mentioned this pull request Feb 29, 2024

Add LagrangeBasis trait #279

Closed

ed255 approved these changes Mar 7, 2024

View reviewed changes

Merge branch 'main' into fix/improve1

ba4827a

ed255 mentioned this pull request Mar 7, 2024

Remove dependency to common ConstraintSystem in the backend #290

Merged

davidnevadoc reviewed Mar 7, 2024

View reviewed changes

Address @davidnevadoc comments

9e29391

adria0 merged commit d6f7020 into main Mar 7, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Doc, unit test, minors #289

chore: Doc, unit test, minors #289

adria0 commented Feb 26, 2024

CPerezz left a comment

ed255 left a comment

ed255 Feb 29, 2024

ed255 Feb 29, 2024

ed255 Feb 29, 2024

adria0 Feb 29, 2024

CPerezz Mar 1, 2024

ed255 Mar 1, 2024

adria0 Mar 1, 2024

davidnevadoc Mar 4, 2024

ed255 Feb 29, 2024

CPerezz Mar 1, 2024

ed255 Mar 1, 2024

davidnevadoc Mar 4, 2024

ed255 Feb 29, 2024

ed255 left a comment

davidnevadoc Mar 4, 2024

adria0 Mar 7, 2024

davidnevadoc Mar 4, 2024

adria0 Mar 7, 2024

davidnevadoc Mar 4, 2024

adria0 Mar 7, 2024

davidnevadoc Mar 4, 2024

davidnevadoc Mar 4, 2024

	pub custom_gates: GraphEvaluator<C>,
	/// Lookups evalution
	pub lookups: Vec<GraphEvaluator<C>>,
	/// Shuffle evalution
	pub shuffles: Vec<GraphEvaluator<C>>,

	// Divide by t(X) = X^{params.n} - 1.
	let h_poly = domain.divide_by_vanishing_poly(h_poly);

	// Obtain final h(X) polynomial
	let h_poly = domain.extended_to_coeff(h_poly);

chore: Doc, unit test, minors #289

chore: Doc, unit test, minors #289

Conversation

adria0 commented Feb 26, 2024

CPerezz left a comment

Choose a reason for hiding this comment

ed255 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

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

ed255 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