Add `ParamsVerifierKZG` #318

davidnevadoc · 2024-04-24T21:50:26Z

This is a small refactor around the PCS parameters, proposed as an alternative to this PR : #301

Changes

Add new struct: ParamsVerifierKZG.
This struct only contains s_g2 and the domain size k. Since it does not contain the SRS the verifier that uses these params is not able to commit to the public inputs. Commitment to the public inputs is currently disabled for both kzg-based versions of the verifier gwc and shplonk).
In case the verifier wants to commit to the PI it should use the original ParamsKZG, which remain unchanged.
Attempt at cleaning up.
Methods that were exclusive for the verifier or the prover have been moved from Params to ParamsVerifier and ParamsProver respectively.
Some structs and traits no longer contain the parameters and as a result, no longer need the explicit 'params lifetime.

Closes #280

ed255

I've done an initial review. I've left some comments on points I'd like to discuss further.

ed255 · 2024-04-26T07:44:39Z

halo2_backend/src/poly/commitment.rs

-pub trait ParamsVerifier<'params, C: CurveAffine>: Params<'params, C> {}
+pub trait ParamsVerifier<'params, C: CurveAffine>: Params<C> {
+    /// Multiscalar multiplication engine
+    type MSM: MSM<C> + 'params;


I wonder why the verifier needs an MSM engine 🤔
I guess this is the verifier that commits?

This is not obvious at all. I believe this is inherited from the original only-IPA design. At that point, it made sense to provide the MSM engine together with the CRS for IPA. When KZG was introduced, this was left here because KZG also uses an MSM, not for the final verification like IPA, but for the accumulation of partial verifications.

ed255 · 2024-04-26T07:46:53Z

halo2_backend/src/poly/commitment.rs

@@ -190,7 +178,7 @@ pub trait Verifier<'params, Scheme: CommitmentScheme> {
    const QUERY_INSTANCE: bool;

    /// Creates new verifier instance
-    fn new(params: &'params Scheme::ParamsVerifier) -> Self;
+    fn new() -> Self;


I would like to understand this change better.

This is part of a series of changes that remove the parameters from the traits and structs it was not used in.
The motivation originally was to try to get rid of the 'params lifetime where possible. This led me to realize that the parameters where being passed around but not used in many places: MSM and Verifier are the principal examples.
I should remark that, somewhat counter-intuitively, Verifier never verifies (and hence doesn't need the parameters). It just makes partial verifications and accumulations.

In short, it changes nothing from a functionality standpoint, it's just an attempt to make things a bit cleaner.

ed255 · 2024-04-26T07:48:25Z

halo2_backend/src/poly/commitment.rs

-    fn get_g(&self) -> &[C];
-
-    /// Returns verification parameters.
-    fn verifier_params(&'params self) -> &'params Self::ParamsVerifier;


How do we transform ParamsProver to ParamsVerifier now?

The new signature is :
fn verifier_params(&self) -> Self::ParamsVerifier
Since the new ParamsVerifier is just s_g2, no issue creating it from scratch.

ed255 · 2024-04-26T07:53:05Z

halo2_backend/src/poly/kzg/commitment.rs

+        1 << self.k
+    }
+
+    fn commit_lagrange(


👀
Is there any way where we could have a clean separation between KZG prover and verifier, and then on top of that build an object that can verify large commitments and commit small vectors?

I think there is and it is very desirable!
The question is if we want to refactor that here, or work on a new version of the backend altogether.

ed255 · 2024-04-26T07:53:32Z

halo2_backend/src/poly/kzg/commitment.rs

+    pub fn into_verifier_params(self) -> ParamsVerifierKZG<E> {
+        ParamsVerifierKZG {
+            k: self.k,
+            g_lagrange: self.g_lagrange,


What's the length of this vector? Where does ParamsVerifierKZG become small?

Now, it is 0 😁

davidnevadoc · 2024-05-13T17:21:02Z

I've addressed the comments and updated the PR description with the changes. Can you give this another pass, pls? @ed255

halo2_backend/src/poly/kzg/commitment.rs

halo2_backend/src/poly/kzg/msm.rs

halo2_backend/src/poly/kzg/strategy.rs

codecov-commenter · 2024-05-20T17:00:23Z

Codecov Report

Attention: Patch coverage is 64.05229% with 55 lines in your changes are missing coverage. Please review.

Project coverage is 80.86%. Comparing base (0513fb4) to head (61f55e6).

Files	Patch %	Lines
halo2_backend/src/poly/kzg/commitment.rs	21.66%	47 Missing ⚠️
halo2_backend/src/poly/kzg/strategy.rs	64.70%	6 Missing ⚠️
halo2_backend/src/plonk/verifier/batch.rs	0.00%	1 Missing ⚠️
halo2_backend/src/poly/kzg/msm.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #318      +/-   ##
==========================================
- Coverage   80.98%   80.86%   -0.13%     
==========================================
  Files          80       80              
  Lines       16532    16567      +35     
==========================================
+ Hits        13389    13397       +8     
- Misses       3143     3170      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ed255

After a discussion about possible improvements on the codebase we determined that they're out of the scope of this PR, which already achieves the main goal of reducing the size of the verifier KZG params.

Remove params use from `verify_proof`. It only uses the generator of G1, and this is fixed in the curve. refactor: rm params from GWC, SHPLONK Verifiers The verifier just takes care of partial verification, the pairing is not checked until the last phase. As a result, params are not needed at that stage. refactor: rm params from IPA Verifier refactor: rm params from Verifier::new() refactor: rm KZGParams from DualMSM, GuardKZG ParamsVerifierKZG are now in the structs that implmentent VerifierStrategy: SingleStrategy and AccumulatorStrategy. ParamsVerifierKZG is no longer reference with explicit lifetime. This struct should be small, only the s_g2 point, so this is fine. refactor: rm get_g from Params refactor: add ParamsVerifierKZG refactor: move downsize to ParamsProver refactor: move empty_msm to ParamsVerifier refactor: remove 'params from Params/ProverParams refactor: fix TODOs & leftover refactor: verfier_params -> into_verfier_params

chore: remove leftovers fix: remove needless From impl chore: remove leftover TODO chore: remove unused import fix:compress_selector with new verifier_params

davidnevadoc changed the title ~~Nev@refactor pcs params~~ Refactor PCS params Apr 25, 2024

davidnevadoc force-pushed the nev@refactor-pcs-params branch from f533336 to af68bf3 Compare April 25, 2024 15:40

davidnevadoc marked this pull request as ready for review April 25, 2024 17:03

davidnevadoc force-pushed the nev@refactor-pcs-params branch 2 times, most recently from 1d67d87 to d9958e2 Compare April 25, 2024 17:08

davidnevadoc requested a review from ed255 April 25, 2024 17:09

ed255 reviewed Apr 26, 2024

View reviewed changes

davidnevadoc mentioned this pull request May 13, 2024

Add ParamsVerifierKZG #301

Closed

davidnevadoc changed the title ~~Refactor PCS params~~ Add ParamsVerifierKZG May 13, 2024

ed255 reviewed May 15, 2024

View reviewed changes

halo2_backend/src/poly/kzg/commitment.rs Show resolved Hide resolved

halo2_backend/src/poly/kzg/msm.rs Outdated Show resolved Hide resolved

halo2_backend/src/poly/kzg/strategy.rs Outdated Show resolved Hide resolved

halo2_backend/src/poly/kzg/strategy.rs Show resolved Hide resolved

davidnevadoc force-pushed the nev@refactor-pcs-params branch from c6d7f71 to 0e196a5 Compare May 20, 2024 16:34

ed255 approved these changes May 23, 2024

View reviewed changes

davidnevadoc force-pushed the nev@refactor-pcs-params branch 2 times, most recently from b16baab to 61f55e6 Compare May 23, 2024 16:57

davidnevadoc added 3 commits May 23, 2024 18:58

add: COMMIT_INSTANCE in ParamsVerifier

651f669

chore: back to verifier_params()

61f55e6

chore: remove leftovers fix: remove needless From impl chore: remove leftover TODO chore: remove unused import fix:compress_selector with new verifier_params

davidnevadoc merged commit 2458654 into main May 23, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ParamsVerifierKZG` #318

Add `ParamsVerifierKZG` #318

davidnevadoc commented Apr 24, 2024 •

edited

Loading

ed255 left a comment

ed255 Apr 26, 2024 •

edited

Loading

davidnevadoc May 9, 2024

ed255 Apr 26, 2024

davidnevadoc May 9, 2024

ed255 Apr 26, 2024

davidnevadoc May 9, 2024

ed255 Apr 26, 2024 •

edited

Loading

davidnevadoc May 9, 2024

ed255 Apr 26, 2024

davidnevadoc May 9, 2024

davidnevadoc commented May 13, 2024

codecov-commenter commented May 20, 2024 •

edited

Loading

ed255 left a comment

Add ParamsVerifierKZG #318

Add ParamsVerifierKZG #318

Conversation

davidnevadoc commented Apr 24, 2024 • edited Loading

Changes

ed255 left a comment

Choose a reason for hiding this comment

ed255 Apr 26, 2024 • edited Loading

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 Apr 26, 2024 • edited Loading

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

davidnevadoc commented May 13, 2024

codecov-commenter commented May 20, 2024 • edited Loading

Codecov Report

ed255 left a comment

Choose a reason for hiding this comment

Add `ParamsVerifierKZG` #318

Add `ParamsVerifierKZG` #318

davidnevadoc commented Apr 24, 2024 •

edited

Loading

ed255 Apr 26, 2024 •

edited

Loading

ed255 Apr 26, 2024 •

edited

Loading

codecov-commenter commented May 20, 2024 •

edited

Loading