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

refactor: eddsa factorizing and code cleaning #285

Merged
merged 15 commits into from
Mar 22, 2022

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Mar 18, 2022

  • This PR merges the std/ code for bandersnatch and twistededwards in one single package.
  • Refactors the tests.
  • Uses latest gnark-crypto to simplify eddsa signature;

Twisted edwards curve usage is now, in a circuit (including bandersnatch);

curve, err := twistededwards.NewEdCurve(api, tedwards.BN254) // or tedwards.BLS12_381_BANDERSNATCH, ...

The curve object has point arithmetic defined on it, and if an endomorphism is available, will use scalarMulGLV.

eddsa usage is now:

err = eddsa.Verify(curve, signature, message, publicKey, hashFunction)

Both std/.../eddsa and std/.../twistededwards packages test all twisted edwards curves.

Fixes #262

@gbotrel gbotrel added this to the v0.7.0 milestone Mar 18, 2022
@gbotrel gbotrel added the consolidate strengthen an existing feature label Mar 18, 2022
@gbotrel gbotrel requested review from yelhousni and ivokub March 18, 2022 16:45
@gbotrel gbotrel linked an issue Mar 18, 2022 that may be closed by this pull request
std/algebra/twistededwards/twistededwards.go Outdated Show resolved Hide resolved
@@ -150,7 +145,7 @@ func (p *Point) ScalarMul(api frontend.API, p1 *Point, scalar frontend.Variable,
// DoubleBaseScalarMul computes s1*P1+s2*P2
// where P1 and P2 are points on a twisted Edwards curve
// and s1, s2 scalars.
func (p *Point) DoubleBaseScalarMul(api frontend.API, p1, p2 *Point, s1, s2 frontend.Variable, curve EdCurve) *Point {
func (p *Point) DoubleBaseScalarMul(api frontend.API, p1, p2 *Point, s1, s2 frontend.Variable, curve *CurveParams) *Point {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this also use GLV?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a @yelhousni question --> but this branch should be merged in his current WIP branch, not in develop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could but constraint-wise it's worse. It would need a Lookup4. The analysis is similar to a 2-bit window vs. a 4-bit window.

@ivokub
Copy link
Collaborator

ivokub commented Mar 22, 2022

I'm not sure if it makes sense in the current PR context, but maybe should initialise the hint in init() method. Otherwise makes the documentation quite verbose:
image

And it seems that if Curve would provide ScalarMulBasePoint method, then do not need to expose public methods on Point at all?

But otherwise, looks good.

@gbotrel
Copy link
Collaborator Author

gbotrel commented Mar 22, 2022

re hint declaration, will revisit in separate PR before v0.7.0 release (see also #287 ) .

Good observation for Point ; the only method so far not in the Curve interface is Neg. But I think you may be right; all package user could do with the Curve object only.

@gbotrel gbotrel merged commit 3adc130 into perf/EdDSA-Bandersnatch Mar 22, 2022
@gbotrel gbotrel deleted the refactor/eddsa branch March 22, 2022 15:01
gbotrel added a commit that referenced this pull request Mar 24, 2022
* perf(std/tEd): first bit in ScalarMul handled separately

* perf(std/tEd): rearrange Double --> less constraints

* perf(std/EdDSA): rearrange eddsa verify (-1 addtion, -1 MustBeOnCurve)

* perf(std/tEd): Lookup2 for first 2 bits in ScalarMulFixedBase

* perf(std/tEd): FixedPoint should be hidden by the API

* test(tEd): test scalarMul for all curves and schemes

* fix(tEd): case when scalar size is odd

* fix(tEd): case when scalar size is odd

* refactor(eddsa): rearrange eddsa verif as cofactor clearing counts

* feat(tEd): implements double-base scalar mul

* perf(EdDSA): eddsa gadget using double-base scalar mul

* perf(bandersnatch): apply tEd perf changes to Bandersnatch

* fix: fixed wrong bigInt op in plonk api

* style(eddsa, tEd): no benchmarks

* style(eddsa, tEd): no benchmarks

* perf(bandersnatch): GLV scalar mul in-circuit

* test(twistededwards): randomise test

* refactor(bandersnatch): review PR 263

* fix(bandersnatch): curveID in hint not checked

* fix(bandersnatch): check curveID for endomorphism availability

* style(bandersnatch): correct comment

* style(bandersnatch): correct comment about negative scalars

* fix(bandersnatch): increase scalars size bound to 129 + comments

* fix: hint signature in bandersnatch matches new format

* refactor: eddsa factorizing and code cleaning (#285)

* build: updated to latest gnark-crypto

* build: updated to latest gnark-crypto

* refactor: introduce Curve interface in std/ and updated eddsa tests

* feat: added std/eddsa publicKey and signature assign helpers

* refactor(std): merged twistededwards and bandersnatch. IsOnCurve failing for bandersnatch

* fix: closes #283. ensure test.Assert compile cache handles different object of same type

* fix: use UnsafeAddr instead of UnsafePointer to be retro compatible

* fix: fix previous commit

* test: test all twisted ed curve operations

* Fixes #283 : ensure test.Assert compile cache handles different objects of same type (#284)

* fix: closes #283. ensure test.Assert compile cache handles different object of same type

* fix: use UnsafeAddr instead of UnsafePointer to be retro compatible

* fix: fix previous commit

* fix: apply pr patch

* style: make twistededwards/Point methods package private

* style: fix gosec errors in std/eddsa

* feat: disable GLV mul in bandersnatch until #268 is fixed

Co-authored-by: Thomas Piellard <[email protected]>
Co-authored-by: Gautam Botrel <[email protected]>
@drakstik
Copy link

I'm new to Gnark and zk-proofs in general and I am trying to implement some of your examples, but for some reason they are full of little complexities that seem related to the fast-moving pace of this project.

Just wanted to comment that we noobs would greatly appreciate the examples to remain up to date and easily runnable. As fun as it is to rummage through the Issues tab, it is frustrating at times, considering how the topic itself is difficult to comprehend, let alone having to debug examples while you try to bring your ideas to life.

Thanks for all your hard work! It is because of people like you that noobs can even begin to dream up ideas for the new world.

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

Successfully merging this pull request may close these issues.

std/signature/eddsa is unnecessary complicated
4 participants