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

feat: added HashToG2 and BLS G2 signature verification circuit for BLS12-381 #1040

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

weijiguo
Copy link

@weijiguo weijiguo commented Feb 7, 2024

Description

This PR implemented circuit for HashToG2 and BLS G2 signature verification for the BLS12-381 curve. The implementation is based on affine coordinations.

Along with the said functionalities, it also added G2.addUnified(p, q) function which can handle the case that p == q. And as an optimization, it also adopted a new hint to calculate the sqrtRatio function with the gnark-crypto library to save constraints. Therefore this PR depends on an update to gnark-crypto

Fixes # (issue)
#648

Type of change

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

How has this been tested?

Added unit tests to cover:

  • G2.addUnified for BLS12-381
  • HashToG2 for BLS12-381

How has this been benchmarked?

Added test cases for HashToG2. Results: 2761896 constraints with SCS and 779198 with R1CS for simple message ("abcd").

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

@weijiguo weijiguo marked this pull request as draft February 7, 2024 20:24
@weijiguo
Copy link
Author

weijiguo commented Feb 7, 2024

L'et also make it a draft for now as I have a related pending question here: #1041

The background is that I want to ensure that the Domain Separation Tag is constant.

@weijiguo weijiguo marked this pull request as ready for review February 8, 2024 01:18
@yelhousni
Copy link
Contributor

Hi @weijiguo, thank you for this contribution! this is a great work! As you would imagine, it takes a bit of time to review all of this. But it's definitely a great PR that we are looking to merge.

@gbotrel gbotrel requested review from yelhousni and ivokub May 4, 2024 01:41
@weijiguo
Copy link
Author

Hi @yelhousni @ivokub now that we have the 0.11 version. Can you spare some time to review yet? Really appreciate your time among your tight schedule.

@weijiguo
Copy link
Author

static check fails due to needing to merge Consensys/gnark-crypto#481 first

@ivokub
Copy link
Collaborator

ivokub commented Sep 27, 2024

Hi @weijiguo - actually I started reviewing the PR about a month ago, but then realized it would require a bit of work to understand completely and postponed. But indeed, it would be a great addition and it would be good to review. I'll retry again soon, hopefully being more successful.

For example - the first issue I encountered with Consensys/gnark-crypto#481 is that it is implemented only for the BLS12-381 in the code generated path, so would have to add to the code generation. And I think exposing the individual method may overload the ecc/bls12-381 package, so maybe there is a better way for exposing the required functionality.

@weijiguo
Copy link
Author

@ivokub Understood. Thanks again.

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

Successfully merging this pull request may close these issues.

3 participants