Fast amortized KZG commitments #79

joebebel · 2021-05-04T10:57:25Z

Description

This is an implementation of fast amortized KZG commitments with additional details from here and here and here

Definitely a "work in progress" but early-stage review, feedback, critique is appreciated.

Fix MarlinPC's CommitterKey to return the correct supported_degree (arkworks-rs#78)

Pratyush · 2021-05-04T21:55:25Z

src/kzg10/data_structures.rs

+    /// Combine opening proofs onto a subset of the domain
+    pub fn combine_at_evals(
+        &self,
+        range: &std::ops::Range<usize>, // Domain is omega^{start}, ..., omega^{end-1}


maybe just replacing this with start: usize, end: usize would work better? Or do we need it to be a Range?

Sure, I think that makes sense. Ideally it seems more rustic to use something like Range but it has it's own issues.

src/kzg10/fastpoly.rs

Pratyush · 2021-05-04T21:59:35Z

src/kzg10/fastpoly.rs

+use ark_poly::polynomial::univariate::DensePolynomial;
+
+type P<E: PairingEngine> = DensePolynomial<E::Fr>;


Maybe just import this is as ark_poly::polynomial::univariate::DensePolynomial as Poly? Also, not sure if it's more ergonomic to make it generic over a pairing engine, instead of directly over the scalar field.

That sounds good, and also it may be better to merge the polynomial operations into algebra rather than poly-commit

Yeah, that seems reasonable!

Let's merge the polynomial operations here for the time being, and then in a follow up PR we can move them to ark-poly?

src/kzg10/fastpoly.rs

src/kzg10/mod.rs

Pratyush · 2021-05-04T22:03:56Z

src/kzg10/mod.rs

@@ -191,6 +218,30 @@ where
        Ok((Commitment(commitment.into()), randomness))
    }

+    /// Outputs a commitment to `polynomial` in G2
+    pub fn G2_commit(


We should also rename commit to commit_g1.

Suggested change

pub fn G2_commit(

pub fn commit_g2(

This part is also a little awkward, because to check a KZG proof you have to commit the witness/evaluation polynomial in G1 and the subproduct domain polynomial in G2, both of which in the case of a single reveal can happen implicitly, but with a multi reveal should probably call commit_g1 and commit_g2 which requires the UniversalParams.

Or, the VerifierKey needs to include a lot more, and actually might need to include more powers than the committer key (in the case where the multi reveal domain is larger than the degree of the committed polynomial)

Or, the VerifierKey needs to include a lot more, and actually might need to include more powers than the committer key (in the case where the multi reveal domain is larger than the degree of the committed polynomial)

That's fine, as long as we make it parameterized, so that when produce_g2_powers (or whatever the appropriate flag is) is false, we don't produce the powers in the verifier key.

src/kzg10/mod.rs

Pratyush · 2021-05-04T22:08:04Z

Thanks for the PR! Left some comments

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>

joebebel · 2021-05-13T00:34:06Z

src/kzg10/subproductdomain.rs

+        circulant[size + 1..size + 1 + coeffs.len() - 2]
+            .copy_from_slice(&coeffs[1..coeffs.len() - 1]);
+    } else {
+        circulant[size + 1..size + 1 + coeffs.len() - 1].copy_from_slice(&coeffs[1..]);


Unfortunately there is a bug somewhere in here which prevents arbitrary circulant matrices/toeplitz matrices from being constructed (it is not triggered by the KZG opening, but is triggered by other tests where the diagonal of the toeplitz matrix is all zeros)

What's the bug that you face?

it doesn't give the correct result for circulant in the "else" case (e.g. zeros on the diagonal of the Toeplitz matrix). The "else" is never triggered by the fast KZG open so all of those tests pass.

I suspect it's just some indexing error, off by one or something, but I haven't debugged it yet.

joebebel · 2021-05-13T00:34:38Z

I added more comments to the SubproductDomain algorithms

src/error.rs

Pratyush · 2021-05-28T21:40:42Z

src/kzg10/mod.rs

+mod subproductdomain;
+pub use subproductdomain::*;


Stylistic nit, but usually Rust uses snake-case for module names, which means this should be something like

Suggested change

mod subproductdomain;

pub use subproductdomain::*;

mod subproduct_domain;

pub use subproduct_domain::*;

src/kzg10/subproductdomain.rs

Pratyush · 2021-05-28T21:43:25Z

src/kzg10/subproductdomain.rs

+        let mut i = 2usize;
+        // Use Newton iteration which converges to the inverse mod x^l
+        for _ in 0..r {
+            g = &(&g + &g) - &(f * &(&g * &g)); //TODO: is g*2 better than g+g?


You can do something like g.double() and g.square(). These should be slightly faster than computing the addition and multiplication directly.

Oh, that makes sense, I hadn't realized there were API calls for this

src/kzg10/subproductdomain.rs

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>

Pratyush · 2021-05-28T21:57:15Z

this looks great, modulo the nits and the bug fix!

Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>

alxiong · 2023-05-25T15:34:18Z

I stumble onto this stale PR yesterday, for those who want to use this FK23 today(Fast amortized KZG https://eprint.iacr.org/2023/033.pdf),
we have an implementation in Jellyfish library.
(relevant PR: initial work, follow-up refactor)

Essentially, you can go to: jellyfish/primitives/pcs/univariate_kzg/mod.rs::UnivariateKzgPCS::multi_open() for general multi-points opening, and the multi_open_rou_*() APIs in the same file that are specialized for roots of unity (faster)

Of independent interest, you can find

Toeplitz Matrix multiplication implementation in jellyfish/primitives/toeplitz.rs
our generalization of DensePolynomial in arkwork in jellyfish/primitives/pcs/poly.rs::GeneralDensePolynomial<T: GroupCoeff<F>, F: Field>
- this allows polynomial whose coeffs can be both fields and groups elements. (this is useful when computing multi-proofs in the FK23)
- imo, this actually should be eventually be pushed to upstream arkwork lib, since the underlying fft-related API in arkwork already implicitly support trait GroupCoeffs

(we are open-sourced under permissive license, so feel free to grab whichever subcomponent you find useful.)

joebebel and others added 2 commits May 3, 2021 23:24

Merge pull request #1 from arkworks-rs/master

2e85c9a

Fix MarlinPC's CommitterKey to return the correct supported_degree (arkworks-rs#78)

fast amortized kzg commitments

5a92f33