iden3 / go-circom-prover-verifier

Go implementation of the Groth16 zkSNARK Prover and Verifier compatible with Circom
GNU General Public License v3.0
38 stars 13 forks source link

Feature/tables #8

Closed druiz0992 closed 4 years ago

druiz0992 commented 4 years ago

Table calculation is done only for G1. I show how to use the table pre-calculation in the test in different scenarios

Pending things include:

To do it correctly, we should fork pairing library as there are several improvements that can be done:

druiz0992 commented 4 years ago

I added some additional changes after requesting pull request. Not sure how to proceed.

ed255 commented 4 years ago

This looks very promising! Here are some notes regarding the PR

Format

Go has some canonical formatting rules that we follow strictly thanks to the gofmt tool. I see that your code uses inconsistent levels of indentation and uses different rules of spacing between tokens.

We use gofmt (usually configured as an editor plugin so that every time we save a file it is formatted). This allows having a consistent format across all the code base an removes discussions about formatting (we just follow what gofmt says). So I would ask you to run gofmt over your code.

Number of commits

In every PR we try to stick to a low number of commits. In general we try to organize the commits into meaningful and related packets of work done / features. So we try to avoid implementing a single feature into two commits. In your PR I can see for example that there are 3 commits named Update tables.md which could be merged together. Or a Fixed merge conflicts that could be merged into another commit. I'm not sure if you're familiar with git rebase. It allows you to squash commits (join them), and reorder them. I suggest you try to reorganize the commits a bit; you can make a new branch and try git rebase -i 8c81f5041ec12016abb602f0432651cf183f0088 there and experiment with git rebase.

Git commit subject

This is a very minor thing, but we try to follow the rule of having the commit subject in imperative tone. I won't ask you to change the subjects for this PR, but please be aware of this convention in the future :)

Repeated code in ScalarMultNoDoubleG1 and ScalarMultNoDoubleG2

I will take a look and see if I can come up with a nice solution.

arnaucube commented 4 years ago

I've executed the proof generation for circuits of 1k, 5k, 10k, 20k, 50k constraints in my laptop and in the intel nuc and the results are very nice!! :smile_cat:

constraints before this PR with this PR
1k 185.853367ms 211.110172ms
5k 875.408321ms 488.589314ms
10k 1.724526371s 828.930297ms
20k 3.434832853s 1.517834681s
50k 8.179939441s 3.375135829s
constraints before this PR with this PR
1k 454.637472ms 330.973803ms
5k 2.133982394s 969.211664ms
10k 4.204777476s 1.731214548s
20k 8.541817521s 3.362511834s
50k 20.645559622s 7.931245895s
druiz0992 commented 4 years ago

Switched to #10.