Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FK23 batch opening of univariate polynomials #231

Merged
merged 30 commits into from
Apr 28, 2023
Merged

Add FK23 batch opening of univariate polynomials #231

merged 30 commits into from
Apr 28, 2023

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Apr 10, 2023

Description

Partially completes: #118

Major changes include:

  • add trait bound to associated type of the PCS trait: PolynomialCommitmentScheme::SRS: StructuredReferenceString.
  • define Toeplitz and Circulant matrix, impl conversion between them.
  • Implement fast mul with a vector for circulant matrix
  • implement fast mul with a vector for toeplitz by expanding into a 2N circulant matrix and use above subroutine
  • add correctness test for matrix multiplications.
  • Define a GeneralDensePolynomial that allows its coeffs to be either field or group, and implement batch_evaluate
  • FK23 methods using all above, with unit tests.

More important changes when addressing comments:

  • hide gen_srs_for_testing under test-srs feature flag (and also available for test) so that downstream and bench code can still use it, but won't be in the default or production-ready binary.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@alxiong alxiong self-assigned this Apr 10, 2023
@alxiong
Copy link
Contributor Author

alxiong commented Apr 11, 2023

dev log (safely skip for reviewer):

I made the wrong decision to make my Toeplitz APIs constant generic rather than just accepting a vector and checking their lengths inside. (the former is good when we know the size at compile-time, the latter is better for dynamic and run-time variables)

I will need to update my code cuz the rigidity of const-generic propagate up to where I try to use my own APIs in batch_open, and it's a hustle.


Update: fixed in 8e239e1

@alxiong alxiong marked this pull request as ready for review April 14, 2023 14:52
utilities/src/lib.rs Outdated Show resolved Hide resolved
utilities/src/lib.rs Outdated Show resolved Hide resolved
primitives/src/toeplitz.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@chancharles92 chancharles92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alxiong! Generally LGTM but added a few minor comments.

primitives/src/pcs/mod.rs Show resolved Hide resolved
primitives/src/pcs/poly.rs Show resolved Hide resolved
primitives/src/pcs/poly.rs Outdated Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/mod.rs Outdated Show resolved Hide resolved
primitives/src/toeplitz.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ggutoski ggutoski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whew that was big. Minor comments only.

primitives/src/pcs/errors.rs Show resolved Hide resolved
primitives/src/pcs/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/mod.rs Show resolved Hide resolved
primitives/src/pcs/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/srs.rs Show resolved Hide resolved
primitives/src/toeplitz.rs Outdated Show resolved Hide resolved
utilities/src/lib.rs Show resolved Hide resolved
primitives/src/pcs/multilinear_kzg/srs.rs Outdated Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ggutoski ggutoski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm only a couple minor tweaks

plonk/src/proof_system/snark.rs Outdated Show resolved Hide resolved
primitives/src/pcs/mod.rs Outdated Show resolved Hide resolved
ggutoski
ggutoski previously approved these changes Apr 25, 2023
Copy link
Contributor

@ggutoski ggutoski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment, but we can push that to a future issue if we decide it's worth addressing. lgtm.

primitives/src/pcs/univariate_kzg/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/mod.rs Show resolved Hide resolved
primitives/src/toeplitz.rs Show resolved Hide resolved
primitives/src/pcs/univariate_kzg/mod.rs Show resolved Hide resolved
primitives/src/pcs/poly.rs Show resolved Hide resolved
@alxiong
Copy link
Contributor Author

alxiong commented Apr 27, 2023

Log for future reference

I shall document briefly on the slight complication of computing the degree/coeffs for FK23 to work for future reference.

Firstly, we are opening a polynomial f(X) = f_0 + f_1 * X + ... + f_d * X^d, (degree: d, num_coeffs: d+1),
the resulting h(X) = h_1 + h_2 * X + ... + h_d * X^d-1 (degree: d-1, num_coeffs: d).

Now we accept f of any degree, and in order for FK23 to work, we need to internally use Toeplitz matrix which works only for degree d power of two, therefore some degree padding is required.
Furthermore, we also need to make sure the roots of unity applied in evaluating h(X) (which are proofs) and in evaluating f(X) (which are evaluations) are the same.

$$d = 2^k$$ $$d = 2^k - 1$$ $$d = 2^k + 1$$
FFT(n)/rou (i.e. padded h(X).coeffs.len() ) $$2^{k+1}$$ $$2^k$$ $$2^{k+1}$$
Teoplitz padded degree(i.e. next_pow_of_two) $$2^k$$ $$2^k$$ $$2^{k+1}$$

and for trim_fft_size() API, it should accomondate the FFT size (which is no smaller than the padded degree).

Copy link
Contributor

@chancharles92 chancharles92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Hooray!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants