Add `ParamsVerifierKZG` #301

davidnevadoc · 2024-03-20T15:01:48Z

Description

The aim of this PR is to allow the verification of a KZG proof without having to deal with the complete KZG parameters.
ParamsKZG contains 2 G2 elements and 2 vectors of n-sized vectors of G1 elements, where n is the size of the circuit.
The verifier only needs the 2 G2 elements, and as many G1 elements as public inputs in the circuit.

Changes

Add the struct ParamsVerifierKZG.
This struct contains the bare minimum info needed to verify a proof.
Remove get_g from ParamsProver.
Modify verifier_params -> into_verifier_params.
I'm not 100% convinced how this ended up, so any suggestion is greatly appreciated.
Adjust to function calls to use verifier parameters when needed, instead of the full parameters.

Closes #280

- remove `get_g` from ParamsProver - modify `verifier_params` -> `into_verifier_params` - Adjust to function calls to use verifier parameters when needed, instead of the full parameters.

CPerezz

Where's all this read/write impls comming from??

CPerezz · 2024-03-21T07:22:50Z

halo2_backend/src/poly/commitment.rs

-    fn verifier_params(&'params self) -> &'params Self::ParamsVerifier;
+    fn into_verifier_params(self) -> Self::ParamsVerifier;


Why removimg the return of the ref? You can always clone later if you need to own the data.

Thanks for bringing this up! This is the change I'm the least happy with.

Before this PR, the verfier params and the prover params where the same struct. As a result, this function could just return self without issue.
The problem is that now, the verifier and prover params are different. Moreover, the prover parameters do not contain the verifier parameters directly. As a result, the ParamsVerifier struct must be created here, and we cannot return a reference.

I reasoned that in most cases, when you create the verifier params you are done with all the proving, so it is acceptable to consume the prover parameters. ( I removed an unnecessary clone in the From implementation 3b5b8a5). So in most cases, the user can generate the verifier parameters without cloning.

Can we at least get &self as input?
I say it as usually, when we test, we do both prove and verify within the same scope. And this will force a clone of a quite big struct most likely.

In any case, not strongly against this change though. Just wanted to explore the options.

I also would have liked to keep the &self as input, but it run into a big issue: A lot of cloning.
For example, in IPA, where the verifier parameters are the same as the input parameters, this forced to clone the whole struct (with its n sized vectors and all...).
I found that by getting self, the user could decide to clone in advance to keep the parameters, or just consume them to create the verifier params, which is the most usual case.

Here is an attempt to go back to the original function: 4e2f44e

The g and g_lagrange fields of ParmamsIPA have been changed from Vec<C> to Arc<[C]> ( as suggested by @ed255). As a result, the clone that that would happen in verifier_params is very cheap.

Despite this, I'm not sure if this is the right choice. There are a couple of points where I've been forced to initialize a vector first, and then convert it to Arc. I'd like to avoid these re-allocations, but I'm not sure if it is possible.

davidnevadoc · 2024-03-21T09:36:29Z

Where's all this read/write impls comming from??

These are adaptations from the implementations of ParamsKZG, they are equal just with different fields in the struct.

CPerezz · 2024-03-21T11:23:42Z

Where's all this read/write impls comming from??

These are adaptations from the implementations of ParamsKZG, they are equal just with different fields in the struct.

We should consider this as related to #176

adria0 · 2024-04-01T17:21:55Z

halo2_backend/src/poly/kzg/commitment.rs

+    // Domain size.
+    pub(crate) n: u64,
+    // This is the maximum size of PI supported.
+    pub(crate) trimed_size: u32,


What is the point of storing trimed_size here if AFIS it's just g_lagrange.len()?
Makes sense to store it serialized, but maybe it's not necessary to be kept in memory.

adria0 · 2024-04-01T17:26:11Z

halo2_backend/src/poly/kzg/commitment.rs

@@ -186,6 +204,8 @@ where
    where
        E::G2Affine: SerdeCurveAffine,
    {
+        // TODO: 4 bytes is too much for k, this is the log-size of the params.


Maybe it's just enough with the check you added later.

// This is a generous bound on the size of the domain. debug_assert!(k < 32);

Yes, I think that check is definitely necessary, even with just 1 byte for storing k. This check was already added in the verifier keys, as it could lead to the attempt of allocating a 2 ^ (2 ^ 32) len vector. ( #244 ). In this PR, we also reduced the number of bytes from 4 to 1, so I think it would be good to reduce it here to keep things coherent. WDYT?

adria0 · 2024-04-01T17:28:49Z

halo2_backend/src/poly/kzg/commitment.rs

+    }
+
+    /// Reads verifier parameters from a buffer.
+    pub fn read_custom<R: io::Read>(reader: &mut R, format: SerdeFormat) -> io::Result<Self>


There's some code that is repeated in ParamsKZG::read_custom, maybe it can be reused via shared functions.

Go back to `verifier_params` method in `ParamsProver`. To avoid duplciating the IPA parameters when calling this function, `g`, `g_lagrange` has been change from `Vec<C>` to `Arc<[C]>`.

davidnevadoc · 2024-05-13T17:22:01Z

Overridden by #318

davidnevadoc force-pushed the nev@vk-kzg-params branch from f1bc892 to b11077b Compare March 20, 2024 17:41

add: ParamsVerifierKZG

b11077b

- remove `get_g` from ParamsProver - modify `verifier_params` -> `into_verifier_params` - Adjust to function calls to use verifier parameters when needed, instead of the full parameters.

davidnevadoc marked this pull request as ready for review March 20, 2024 18:13

fix: check instance len against trimed size

6a6d215

CPerezz reviewed Mar 21, 2024

View reviewed changes

fix: remove needless clone

3b5b8a5

davidnevadoc requested review from ed255 and duguorong009 March 21, 2024 09:36

adria0 reviewed Apr 1, 2024

View reviewed changes

feat: into_verifier_params -> verifier_params

4e2f44e

Go back to `verifier_params` method in `ParamsProver`. To avoid duplciating the IPA parameters when calling this function, `g`, `g_lagrange` has been change from `Vec<C>` to `Arc<[C]>`.

davidnevadoc marked this pull request as draft April 10, 2024 18:22

davidnevadoc mentioned this pull request Apr 25, 2024

Add ParamsVerifierKZG #318

Merged

davidnevadoc closed this May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ParamsVerifierKZG` #301

Add `ParamsVerifierKZG` #301

davidnevadoc commented Mar 20, 2024 •

edited

Loading

CPerezz left a comment

CPerezz Mar 21, 2024

davidnevadoc Mar 21, 2024

CPerezz Mar 21, 2024

davidnevadoc Mar 25, 2024

davidnevadoc Apr 10, 2024 •

edited

Loading

davidnevadoc commented Mar 21, 2024

CPerezz commented Mar 21, 2024

adria0 Apr 1, 2024

adria0 Apr 1, 2024

davidnevadoc Apr 10, 2024

adria0 Apr 1, 2024

davidnevadoc commented May 13, 2024

		fn verifier_params(&'params self) -> &'params Self::ParamsVerifier;
		fn into_verifier_params(self) -> Self::ParamsVerifier;

Add ParamsVerifierKZG #301

Add ParamsVerifierKZG #301

Conversation

davidnevadoc commented Mar 20, 2024 • edited Loading

Description

Changes

CPerezz 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

davidnevadoc Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

davidnevadoc commented Mar 21, 2024

CPerezz commented Mar 21, 2024

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

Add `ParamsVerifierKZG` #301

Add `ParamsVerifierKZG` #301

davidnevadoc commented Mar 20, 2024 •

edited

Loading

davidnevadoc Apr 10, 2024 •

edited

Loading