a generic Trait for both ml and uni KZG #43

zhenfeizhang · 2022-07-19T12:14:48Z

No description provided.

zhenfeizhang · 2022-07-20T18:35:39Z

Would also like some input from @alxiong on the APIs for PCS trait

chancharles92

@zhenfeizhang I only checked the multilinear KZG part, do I also need to check the univariate KZG part?

pcs/src/multilinear_kzg/batching.rs

pcs/src/multilinear_kzg/mod.rs

zhenfeizhang · 2022-07-20T20:45:49Z

@zhenfeizhang I only checked the multilinear KZG part, do I also need to check the univariate KZG part?

Yes please.

pcs/src/multilinear_kzg/util.rs

chancharles92 · 2022-07-20T20:54:54Z

pcs/src/univariate_kzg/mod.rs

+
+    /// Input a list of polynomials, and a same number of points,
+    /// compute a multi-opening for all the polynomials.
+    fn multi_open(


This is a naive approach, do we want to add a TODO to implement the more efficient batch opening algorithm? (e.g., the appendix C.4 in https://eprint.iacr.org/2020/1536.pdf)

I think we do (BTW our Jellyfish didn't use this either)

fixed in ff2e7a9

Thanks, can you also add an issue for this?

pcs/src/univariate_kzg/srs.rs

alxiong · 2022-07-22T09:47:37Z

pcs/src/param.rs

-use ark_ec::{msm::FixedBaseMSM, AffineCurve, PairingEngine, ProjectiveCurve};
-use ark_ff::{Field, PrimeField};
-use ark_poly::DenseMultilinearExtension;
+use ark_ec::{AffineCurve, PairingEngine};


why do we still keep this file param.rs?

nice catch. removed in #39

alxiong · 2022-07-22T09:59:11Z

pcs/src/structs.rs

+pub struct Commitment<E: PairingEngine> {
+    /// the actual commitment is an affine point.
+    pub commitment: E::G1Affine,
+}


Personally, I don't think defining this struct is necessary. (maybe just a matter of preference)

in xxx-kzg/mod.rs we could directly type Commitment = E::G1Affine. The indirection introduced by this struct is not useful

sounds good to me

alxiong · 2022-07-22T12:44:31Z

pcs/src/lib.rs


    /// Generate a commitment for a polynomial
    fn commit(
        prover_param: &Self::ProverParam,
-        poly: &impl MultilinearExtension<E::Fr>,
+        poly: &Self::Polynomial,
    ) -> Result<Self::Commitment, PCSErrors>;


We should have associated type Error instead of concrete PCSError

Our enum PCSError::PolyIOPError(_) seems strange.

Nice catch. It use PolyIOPError because IOPTranscript is implemented in PolyIOP. I now think we should have a separate crate and trait for Transcript and that should simplify things.

fixed with #46

#46 only fixed the second point, right?

The first point is still yet to be addressed.

alxiong · 2022-07-22T13:11:31Z

pcs/src/lib.rs

+    // Parameters
    type ProverParam;
    type VerifierParam;
    type SRS;


This can shorten as type SRS: StructuredReferenceString<E> ?
and Self::ProverParam is just <Self::SRS as StructuredReferenceString>::ProverParam.

no we cannot.
SRS for multilinear KZG is a tuple (MultiliearUniversalParams, UnivariateUniversalParams) and it does not have <Self::SRS as StructuredReferenceString>::ProverParam

alxiong · 2022-07-22T13:12:25Z

pcs/src/lib.rs


-pub trait MultilinearCommitmentScheme<E: PairingEngine> {
+/// This trait defines APIs for polynomial commitment schemes.
+/// Note that for our usage of PCS, we do not require the hiding property.


interesting, if we want a general API/traits, why not consider the cases where we do need hiding?

If we consider hiding, then two things:

our Polynomial better be a LabeledPolynomial (at least something of similar spirit?)

we need to pass in an rng to commit() etc.

I think we can leave this as an TODO for future. We do not currently need hiding property.

alxiong · 2022-07-22T13:15:15Z

pcs/src/lib.rs

    /// WARNING: THIS FUNCTION IS FOR TESTING PURPOSE ONLY.
    /// THE OUTPUT SRS SHOULD NOT BE USED IN PRODUCTION.
-    fn setup<R: RngCore>(rng: &mut R, num_vars: usize) -> Result<Self::SRS, PCSErrors>;
+    fn gen_srs_for_testing<R: RngCore>(


Not sure why we use RngCore rather than Rng

can provide a default implementation: by invoking SRS's gen_srs_for_testing

can provide a default implementation: by invoking SRS's gen_srs_for_testing

no. SRS may not be a single universal parameter.

alxiong · 2022-07-22T13:18:11Z

pcs/src/lib.rs

+    // Polynomial and its associated types
+    type Polynomial;
+    type Point;
+    type Evaluation;


why is type Evaluation necessary? isn't it always E::Fr?
especially when our verify() and batch_verify() taking a value: E::Fr or values: &[E::Fr] directly.

alxiong · 2022-07-22T13:27:12Z

pcs/src/lib.rs

+    fn batch_verify<R: RngCore>(
        verifier_param: &Self::VerifierParam,
-        multi_commitment: &Self::Commitment,
-        points: &[&[E::Fr]],
+        multi_commitment: &Self::BatchCommitment,
+        points: &[Self::Point],
+        values: &[E::Fr],
        batch_proof: &Self::BatchProof,
-        transcript: &mut IOPTranscript<E::Fr>,
+        rng: &mut R,


it's strange to only have batch_verify() taking an rng as input.
imo either both verify() and batch_verify() takes an optional rng, or both takes rngs but not necessarily use it.

I understand that usually only during batching, you need a randomizer to combine all opening proofs/statements. But one could implement a deterministic batch verification just by verifying one by one as well.
Therefore imo, the two variants of verify API should resemble each other in this regard.

@chancharles92 Can we remove rng and use FS to deterministically derive randomness?
it makes more sense to have deterministic verification algorithm.

@zhenfeizhang mentioned that altho heuristically it should be fine, we don't have a rigorous proof.

zhenfeizhang added 10 commits July 18, 2022 15:29

refactoring

0270137

wip

c4963be

wip

881172b

wip

d463d5a

impl kzg

291e351

update api

eb8e589

good so far

9670b7b

ok so far

fc2b02e

wip

4140056

e2e works

f4c7b47

zhenfeizhang force-pushed the impl_kzg branch from e042e33 to f4c7b47 Compare July 20, 2022 15:36

zhenfeizhang added 3 commits July 20, 2022 13:15

wip

d63ed6a

finish

360cfec

fmt

1f2b8c3

zhenfeizhang requested a review from chancharles92 July 20, 2022 18:35

zhenfeizhang marked this pull request as ready for review July 20, 2022 18:35

zhenfeizhang requested a review from alxiong July 20, 2022 18:35

chancharles92 requested changes Jul 20, 2022

View reviewed changes

chancharles92 reviewed Jul 20, 2022

View reviewed changes

zhenfeizhang added 3 commits July 20, 2022 17:54

partial address comments

ff2e7a9

address comments

a7c6431

fix clippy and print-trace

73b1a94

zhenfeizhang mentioned this pull request Jul 21, 2022

implement batch KZG for univariate #44

Closed

fix bug in qx degree

f014961

chancharles92 reviewed Jul 21, 2022

View reviewed changes

pcs/src/univariate_kzg/srs.rs Outdated Show resolved Hide resolved

address comments and improve verification

d441cde

chancharles92 approved these changes Jul 21, 2022

View reviewed changes

zhenfeizhang merged commit 17cff52 into main Jul 21, 2022

zhenfeizhang deleted the impl_kzg branch July 21, 2022 17:01

alxiong reviewed Jul 22, 2022

View reviewed changes

zhenfeizhang mentioned this pull request Jul 22, 2022

add hiding property for PCS #45

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a generic Trait for both ml and uni KZG #43

a generic Trait for both ml and uni KZG #43

zhenfeizhang commented Jul 19, 2022

zhenfeizhang commented Jul 20, 2022

chancharles92 left a comment

zhenfeizhang commented Jul 20, 2022

chancharles92 Jul 20, 2022

zhenfeizhang Jul 20, 2022

zhenfeizhang Jul 20, 2022

chancharles92 Jul 21, 2022

alxiong Jul 22, 2022

zhenfeizhang Jul 22, 2022

alxiong Jul 22, 2022

zhenfeizhang Jul 22, 2022

alxiong Jul 22, 2022

zhenfeizhang Jul 22, 2022

zhenfeizhang Jul 26, 2022

alxiong Jul 27, 2022

alxiong Jul 22, 2022

zhenfeizhang Jul 22, 2022

alxiong Jul 22, 2022

zhenfeizhang Jul 22, 2022

alxiong Jul 22, 2022

zhenfeizhang Jul 22, 2022 •

edited

Loading

alxiong Jul 22, 2022

alxiong Jul 22, 2022

alxiong Jul 22, 2022 •

edited

Loading

a generic Trait for both ml and uni KZG #43

a generic Trait for both ml and uni KZG #43

Conversation

zhenfeizhang commented Jul 19, 2022

zhenfeizhang commented Jul 20, 2022

chancharles92 left a comment

Choose a reason for hiding this comment

zhenfeizhang commented Jul 20, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenfeizhang Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxiong Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

zhenfeizhang Jul 22, 2022 •

edited

Loading

alxiong Jul 22, 2022 •

edited

Loading