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

Groth16 prover #463

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

Groth16 prover #463

wants to merge 49 commits into from

Conversation

Vindaar
Copy link
Collaborator

@Vindaar Vindaar commented Aug 22, 2024

Edit: The PR previously contained a long description of my debugging of the Groth16 prover implementation. For anyone interested, it can still be found here.

This PR implements a Groth16 prover and verifier, which requires SnarkJS produced .wtns, .zkey and .r1cs binary files.

  • Implement verifier

@Vindaar
Copy link
Collaborator Author

Vindaar commented Jan 12, 2025

I just had another look at SnarkJS to see if I can figure out the double Montgomery encoding situation.

As it turns out at least I could identify the SnarkJS code that writes the coefficients as doubly Montgomery encoded. This happens here:

const nR2 = curve.Fr.mul(n, R2r);
curve.Fr.toRprLE(buffCoeff, 12, nR2);

where R2r is:

const R2r = curve.Fr.e(Scalar.mod(Scalar.mul(Rr,Rr), primeR));

Which at the very least finally ends the question of whether I'm imagining things or not.

I still have no clue why this is done to be honest. The commits touching these lines are not very illuminating either.

@Vindaar Vindaar changed the title Groth16 prover buggy draft Groth16 prover Jan 25, 2025
Previously these were generated from the wrong branch of the
`{.booldefine.}` variables we had in the code when the PR was still a
draft. Instead of parsing the data as doubly Montgomery encoded as
they actually are, we parsed them as regular Montgomery encoded
resulting in wrong test vectors.
@Vindaar Vindaar marked this pull request as ready for review January 25, 2025 17:51
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.

1 participant