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

Perf: plonk verifier gadget #949

Merged
merged 87 commits into from
Dec 13, 2023
Merged

Perf: plonk verifier gadget #949

merged 87 commits into from
Dec 13, 2023

Conversation

yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Dec 6, 2023

Description

(first review #874 and Consensys/gnark-crypto#471)

Some optimizations for the PLONK verifier gadget:

  • precomputed lines in pairing (Perf: KZG verify gadget #874 merged here)
  • JointScalarMul (s1*p1+s2*p2 using Shamir's trick)
  • scalarBitsMul (scalar binary decomposition only once)
  • MultiScalarMul to use JointScalarMul and scalarBitsMul in the folding case
  • Revisit Karabina cyclotomic square (a variant that is more efficient to decompress in-circuit for some relevant sizes)
  • re-arrange addition chains to make optimal use of the new Karabina square
  • rewrite Hayashida et al. hard part for BLS12 (observe that (x-1)^2 has a better addition chain than x or x^2)
  • addition vs. unified addition when relevant
  • nonzero scalar and point handling only when relevant
  • avoid emulated negations and handle trivial mul/add by 0/1
  • use of endomorphism for ScalarMul and MultiScalarMul (https://eprint.iacr.org/2019/1021, sec. 6.2)
  • use 1/x as a bijection in bw6 MiMC instead of x^5(needs perf: use inverse as a bijection for bw6-761 mimc gnark-crypto#471)

TODO:

  • Augment the verifying key with some pre-computed data related to the fixed points:
    • binary decompositions for MarshalG1
    • doubles of points for ScalarMul and MultiScalarMul

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

All current tests pass.

How has this been benchmarked?

  • 342,254 SCS for the bls12-377 in bw6-761 case.
  • 44,095,764 SCS for the bw6-761 in bn254 case.

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Nice! All seems good. I already pushed some cosmetic changes. I think there are still a few issues:

  • for two-chain we have wrapper for MillerLoop which goes from []G2Affine argument to []*G2Affine. But we compute the lines in the method which uses []G2Affine. However, as the slice contains of values then inside a method we are working on a copy of G2Affine value and we are not updating the lazy lines of the input. I think we can either change MillerLoop to take []*G2Affine so that can modify inline or precompute the lines in Pairing wrapper method Pairing.MillerLoop to already precompute. I like the first approach better because then we always will use lazy line computation.
  • I would still like to get PLONK verification without commitment working. I'm actually thinking about adding new algopt option algopts.ForceSafe which performs safe arithmetic inside MSM. It is suboptimal (we could also only omit the selector we know is 0), but atleast would cover different cases. And I think we already know inside the circuit if there is commitment or not.
  • there are a few unused functions. I didn't want to remove yet as maybe could be useful in the future - for example Pairing.generators() in packages or addStep() in some packages. If you say they are good to remove then I can.
  • I'm not sure, but isn't there regression for ScalarMulBase in 2-chains? Previously we had computed powers of twos, but now we're using generic scalar multiplication (with GLV nontheless). Have you run any comparisons which is better for base scalar mul - generic with GLV or double-and-add with precomputed points.

std/algebra/native/sw_bls12377/g1.go Show resolved Hide resolved
std/algebra/native/sw_bls12377/g1.go Show resolved Hide resolved
std/algebra/native/sw_bls12377/pairing.go Show resolved Hide resolved
std/commitments/kzg/verifier.go Show resolved Hide resolved
std/commitments/kzg/verifier.go Show resolved Hide resolved
std/recursion/plonk/verifier_test.go Show resolved Hide resolved
@ivokub ivokub self-requested a review December 13, 2023 14:37
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

The PR is good to merge. We can address my other issues in other PR. I think this PR needs to be merged to unblock #960.

@yelhousni
Copy link
Contributor Author

The PR is good to merge. We can address my other issues in other PR. I think this PR needs to be merged to unblock #960.

Okay let's merge this. Do you want to merge #874 first and then this or just this and close 874?

@ivokub
Copy link
Collaborator

ivokub commented Dec 13, 2023

The PR is good to merge. We can address my other issues in other PR. I think this PR needs to be merged to unblock #960.

Okay let's merge this. Do you want to merge #874 first and then this or just this and close 874?

It seems all changes are already incorporated here. I'd merge only this and then close 874.

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

Successfully merging this pull request may close these issues.

None yet

2 participants