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: Groth16 Solidity contract with commitments #1063

Merged
merged 17 commits into from Apr 5, 2024

Conversation

ahmetyalp
Copy link
Contributor

@ahmetyalp ahmetyalp commented Feb 19, 2024

Description

This PR adds commitment verification to groth16 solidity verifier contract. Contract does not do foldings so uses first commitment for pairing check and uses sha256 hash as hash to field function. I added warning logs about these assumptions

Resolves: #524

Type of change

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

How has this been tested?

How has this been benchmarked?

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

backend/groth16/bn254/solidity.go Show resolved Hide resolved
backend/groth16/bn254/solidity.go Show resolved Hide resolved
backend/groth16/bn254/solidity.go Show resolved Hide resolved
backend/groth16/bn254/solidity.go Show resolved Hide resolved
backend/groth16/bn254/solidity.go Show resolved Hide resolved
internal/backend/circuits/commit.go Outdated Show resolved Hide resolved
test/assert_solidity.go Outdated Show resolved Hide resolved
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.

Looks good, thanks!

I'll try to think maybe we can omit the --commitment argument for gnark-solidity-checker. For now it works, but I'm quite certain it would haunt us in the future trying.

And also one part in the Solidity template -- I think it can be simplified a little to be more readable. Otherwise imo looks perfect. I tested locally and works end to end.

@ahmetyalp
Copy link
Contributor Author

@ivokub I tried to address some comments you left. Thanks for the review

@ivokub
Copy link
Collaborator

ivokub commented Apr 2, 2024

I pushed some changes to simplify tests (have options modifying the hash functions for Solidity target automatically). I'll go about running the solidity checks when Consensys/gnark-solidity-checker#2 has been merged and lets see how the tests behave.

@ivokub ivokub self-requested a review April 4, 2024 18:03
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.

Lets wait for the CI pipeline to finish and I'll also create two new issues for tracking improvement effors, but I think the PR is now good to merge. We really appreciate the contribution and the implementation has been very good! Good work!

@ivokub
Copy link
Collaborator

ivokub commented Apr 5, 2024

I created new issues for tracking improvements: #1094 and #1095.

@ivokub ivokub merged commit 9761428 into Consensys:master Apr 5, 2024
4 of 5 checks passed
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.

Update solidity verifiers with optional BSB22 commit part
5 participants