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

std/sw: use faster double-and-add for scalar multiplication and add constant scalar multiplication fast-path #222

Merged
merged 5 commits into from
Jan 5, 2022

Conversation

ivokub
Copy link
Collaborator

@ivokub ivokub commented Jan 4, 2022

See #181 - I wasn't able to change the branch for this PR and I couldn't get CI running for my fork.

Copy link
Contributor

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

LGTM, few remarks:

  • To be consistant with the literature nomenclature and with gnark-crypto, main/companion curves are outer/inner curves. e.g. inner: BLS12-377 and outer:BW6-761. Also, in gnark-crypto, we coined "companion curves" to be the twisted Edwards curves for SNARK-friendly ops. Can you please do the renaming?
  • the inner-outer (companion-main) mapping is a 1-1 mapping at the sw_XXX package level, so not sure it helps at this level.
  • this GLV improvement works also for G2. So the same code should be used in g2.go (as in gnark-crypto). That is said, we can have a 4-dimensional GLV (GLS) in sw_bls12377/g2 and 8-dimensional in in sw_bls24315/g2 (instead of 2-dimensional in g1). This is low priority as it is not used in groth16 verifier gadget and it's not implemented in gnark-crypto yet. But it might be worth it once we implement PlonK verifier gadget (or just KZG verifier) as it involves a single scalar mul in G2.

std/algebra/sw_bls24315/g1.go Outdated Show resolved Hide resolved
std/algebra/sw_bls24315/g1.go Outdated Show resolved Hide resolved
std/algebra/sw_bls24315/g1.go Outdated Show resolved Hide resolved
@ivokub
Copy link
Collaborator Author

ivokub commented Jan 5, 2022

LGTM, few remarks:

  • To be consistant with the literature nomenclature and with gnark-crypto, main/companion curves are outer/inner curves. e.g. inner: BLS12-377 and outer:BW6-761. Also, in gnark-crypto, we coined "companion curves" to be the twisted Edwards curves for SNARK-friendly ops. Can you please do the renaming?

Renamed "companion" to "inner" curves. Renamed "main" to "outer" curves.

  • the inner-outer (companion-main) mapping is a 1-1 mapping at the sw_XXX package level, so not sure it helps at this level.

It is not too useful right now, but I was at some point in the middle of merging sw_bls24315 and sw_bls12377. Then it would make a bit more sense as the only curve-specific parameters are in the mapping, not in the multiplication implementation. However, if it is too premature right now, then I can remove in this PR and reintroduce when I get back to merging. What do you think?

  • this GLV improvement works also for G2. So the same code should be used in g2.go (as in gnark-crypto). That is said, we can have a 4-dimensional GLV (GLS) in sw_bls12377/g2 and 8-dimensional in in sw_bls24315/g2 (instead of 2-dimensional in g1). This is low priority as it is not used in groth16 verifier gadget and it's not implemented in gnark-crypto yet. But it might be worth it once we implement PlonK verifier gadget (or just KZG verifier) as it involves a single scalar mul in G2.

I'll open a new issue for that if there isn't anymore.

@yelhousni
Copy link
Contributor

yelhousni commented Jan 5, 2022

It is not too useful right now, but I was at some point in the middle of merging sw_bls24315 and sw_bls12377. Then it would make a bit more sense as the only curve-specific parameters are in the mapping, not in the multiplication implementation. However, if it is too premature right now, then I can remove in this PR and reintroduce when I get back to merging. What do you think?

That makes sense, let's keep it then. Also from a theoretical perspective, there isn't a unique outer BW6 curve. Moreover, there are also (different) outer CP8 competitive with BW6 (over BLS24), e.g. implemented here (a fork of gnark-crypto). If this gets merged sometime, then this mapping is doubly worth it.

I'll open a new issue for that if there isn't anymore.

sounds good

@ivokub ivokub merged commit 9778dc4 into develop Jan 5, 2022
@ivokub ivokub deleted the perf/std-sw-glv branch January 5, 2022 23: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.

2 participants