-
Notifications
You must be signed in to change notification settings - Fork 2
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
Complete Curdleproof migration to generic group backend #5
Conversation
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
z.inner = bls12381.GT{} | ||
z.inner.SetOne() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default bls12381.GT{}
is the neutral element in the additive group. But we live in a multiplicative subgroup, so the correct neutral element is one, not zero.
@@ -45,15 +47,27 @@ func (z *GtElement) AddAssign(e Element) Element { | |||
return z | |||
} | |||
|
|||
func (z *GtElement) SubAssign(e Element) Element { | |||
ee := e.(*GtElement).inner | |||
z.inner.Div(&z.inner, &ee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SubAssign
it's a new one needed. Prob a good idea to have in mind.
return &GtElement{} | ||
res := &GtElement{} | ||
res.inner.SetOne() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved for correctness, because we're in a multiplicative subgroup of F_12.
var byts [48 * 12]byte | ||
for i := 0; i < 6; i++ { | ||
a, err := r.GetG1Affine() | ||
if err != nil { | ||
return bls12381.GT{}, nil | ||
} | ||
abytes := a.RawBytes() | ||
copy(byts[i*(48*2):], abytes[:]) | ||
} | ||
var randElem bls12381.GT | ||
if _, err := randElem.SetRandom(); err != nil { | ||
if err := randElem.SetBytes(byts[:]); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to the test verifiers complaining... we were using SetRandom()
, where we should use the randomness from r *Rand
to get deterministic setup generation.
This PR completes migrating to the generic backend that can run G1 and GT (or any group with order
Fr
field size).TL;DR - Show me the numbers
Here’s the benchmark for a complete curdleproofs run in various setups:
Prover:
Verifier (please see the "Notes and caveats" section):
Internal arguments:
Note: the
(Ams+Bms)
refers toAms
for the verifier logic, andBms
for the accumulated MSM check.Note: the setups for these are scaled down; IIRC length is usually 128. We could do "more serious" benchmarks for different sizes if needed.
Notes and caveats
The implementation for G1 is the same as the original implementation, so nothing is interesting to say here. There're some inefficiencies due to the generic group abstraction layer. In any case, if G1 is decided to be used we can remove that an probably avoid those inefficiencies (i.e: the original impl).
The implementation for GT is fine but naive/optimizable. The most naive and inefficient implementation is doing MSMs in GT. Mostly because this is implemented now as a sum of scalar multiplications, which could be better. This was done now since the plan was to have this working for a generic group so that we can improve on that. We could use a Pippenger variant over GT, apply any trick for the GT algebraic structure (if any?), or any other idea. This means that the "GT verifier" numbers should be taken with a grain of salt, so adjust your interpretation accordingly.
I'm using a version of gnark-crypto that has a patch I did some days ago to improve the performance of G1 equality checks, which became relevant for the new way the MSM accumulator works. The gnark team still has to review that, but I'm taking that out of the way so it doesn't interfere with the results. When (and if) gets merged, I'll switch back to
gnark-crypto@master
, just to avoid pointing to a my fork.Correctness
I also migrated all existing tests, which exist for completeness and soundness. I mostly did this to gain some confidence that things were working as expected; like, the chance of all this working with all that passing and having a mistake is quite low. (And actually, I got into some bug-fixing rabbit holes “thanks” to that).
Run with
go test ./... -v
. (I won't paste the output; it's too long).Reproduce in your machine
For curdleproofs:
For tests and internal arguments ~benchs, run:
go test ./... -v -parallel=1
Review notes
I'll leave this PR open for a day or two, and then I'll merge it into the base PR and copy this same PR description.
I'm not doing much commenting on the PR since I'm not expecting a review since it's quite massive... but I got into some rabbit holes catching bugs. It's not the nicest thing to see a proof verification failing; that's finding a needle in a haystack... in any case, this had a happy ending.