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

refactor: generic KZG and Groth16 verifier #840

Merged
merged 36 commits into from
Oct 16, 2023
Merged

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Sep 27, 2023

Description

This PR introduces a generic KZG and Groth16 verifier gadget in gnark. Currently we had typed gadgets for every 2-chain but with future introduction of PLONK verifier it would be good to have a single implementation for KZG, Groth16 and PLONK. This change still requires some setting up per curve (instantiating the new Curve and Pairing generic interfaces), but otherwise KZG/Groth16 are completely curve-agnostic.

In order to achieve it, I also changed the curve operations API for the 2-chain curves to match non-native implementations.

This is a breaking change as existing typed implementations are removed.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

  • KZG verifier for BN254 using field emulation
  • KZG verifier for BLS12377 using 2-chains
  • Groth16 verifer for BN254-in-BN254 using field emulation
  • Groth16 verifier for BLS12-377-in-BW6-633 using field emulation
  • witness creation tests

How has this been benchmarked?

  • verified compiled circuit sizes is not changed compared to native 2-chain kzg verifier

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

cc @yelhousni

@ivokub
Copy link
Collaborator Author

ivokub commented Oct 4, 2023

@yelhousni - the PR is not fully done yet, but I think its kind of ready for review (to validate the design). There are still some todos:

  • also extend all the assignment stuff to BLS24 and rest of the emulated implementations (BLS12-381 and when BW6 emulation comes, then also for that).
  • implement hash-to-field to get proper Groth16 verification (but I think I will do it in different PR)
  • remove existing KZG and Groth16 implementations in favor of the generic one.
  • add examples for the users.

Particularly, have a look at the generic interfaces algebra.Curve and algebra.Pairing if they make sense. I tried to follow the current implementation of sw_bn254 and sw_bls12381 packages. For two-chains there was a mismatch but I implemented a wrapper. If you are good with that, then I would make the current wrapped version the default one (and also removing the methods where the group ops are defined on group elements instead of the Curve/Pairing instance).

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Summary

✅ Passed: 5737
❌ Failed: 0
🚧 Skipped: 21

🚧 Skipped

  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls12-377/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls12-377/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls12-381/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls12-381/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls24-315/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls24-315/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bls24-317/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bls24-317/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bn254/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bw6-633/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bw6-633/mpcsetup)
  • TestContributionSerialization (github.com/consensys/gnark/backend/groth16/bw6-761/mpcsetup)
  • TestSetupCircuit (github.com/consensys/gnark/backend/groth16/bw6-761/mpcsetup)
  • TestCircuitInclusionProof (github.com/consensys/gnark/examples/rollup)
  • TestCircuitUpdateAccount (github.com/consensys/gnark/examples/rollup)
  • TestCircuitFull (github.com/consensys/gnark/examples/rollup)
  • TestFrobeniusFp12 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestMulByNonResidueFp2 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestMulByFp2Fp6 (github.com/consensys/gnark/std/algebra/native/fields_bls12377)
  • TestIssue348UnconstrainedLimbs (github.com/consensys/gnark/std/math/emulated)
  • TestSolverConsistency (github.com/consensys/gnark/test)

@yelhousni
Copy link
Contributor

@yelhousni - the PR is not fully done yet, but I think its kind of ready for review (to validate the design). There are still some todos:

  • also extend all the assignment stuff to BLS24 and rest of the emulated implementations (BLS12-381 and when BW6 emulation comes, then also for that).
  • implement hash-to-field to get proper Groth16 verification (but I think I will do it in different PR)
  • remove existing KZG and Groth16 implementations in favor of the generic one.
  • add examples for the users.

Particularly, have a look at the generic interfaces algebra.Curve and algebra.Pairing if they make sense. I tried to follow the current implementation of sw_bn254 and sw_bls12381 packages. For two-chains there was a mismatch but I implemented a wrapper. If you are good with that, then I would make the current wrapped version the default one (and also removing the methods where the group ops are defined on group elements instead of the Curve/Pairing instance).

This looks clean, nice work! I think the design and interfaces make sense so we can ditch the old implementations and make the new wrapped one default. Once we have that, I would like to add BLS24 (2-chain) and BW6 (emulated) to familiarize myself with the code.

@ivokub ivokub marked this pull request as ready for review October 13, 2023 13:02
@ivokub
Copy link
Collaborator Author

ivokub commented Oct 13, 2023

@yelhousni - the PR is not fully done yet, but I think its kind of ready for review (to validate the design). There are still some todos:

  • also extend all the assignment stuff to BLS24 and rest of the emulated implementations (BLS12-381 and when BW6 emulation comes, then also for that).
  • implement hash-to-field to get proper Groth16 verification (but I think I will do it in different PR)
  • remove existing KZG and Groth16 implementations in favor of the generic one.
  • add examples for the users.

Particularly, have a look at the generic interfaces algebra.Curve and algebra.Pairing if they make sense. I tried to follow the current implementation of sw_bn254 and sw_bls12381 packages. For two-chains there was a mismatch but I implemented a wrapper. If you are good with that, then I would make the current wrapped version the default one (and also removing the methods where the group ops are defined on group elements instead of the Curve/Pairing instance).

These tasks done. I think from my side the PR is ready.

Additionally, I rebased on top of master to take use of the refactoring in the gnark-crypto and updated dependencies.

I'll leave cleaning up the native EC gadgets as a separate PR. I think we can have other discussions there i.e. we can try to implement Jacobian operations such that it also implements Curve interface etc.

@ivokub
Copy link
Collaborator Author

ivokub commented Oct 13, 2023

And requesting re-review. Nothing much has changed (except adding documentation, examples more compatibility etc.), and I'm confident it didn't break anything after the last review. But just in case if you want to. Otherwise will merge as-is.

Copy link
Contributor

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@yelhousni yelhousni merged commit 33db4ee into master Oct 16, 2023
7 checks passed
@yelhousni yelhousni deleted the refactor/generic-kzg branch October 16, 2023 15:16
@yelhousni yelhousni mentioned this pull request Oct 16, 2023
9 tasks
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.

2 participants