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

Adding wNAF multiplication functionality #230

Merged
merged 24 commits into from
Mar 25, 2021

Conversation

ggkitsas
Copy link
Contributor

@ggkitsas ggkitsas commented Mar 13, 2021

Description

Adds wNAF multiplication functionality for Group elements. It doesn't change the way scalar multiplication is currently performed but rather provides an option to use wNAF.

Two functions are added:

  • wnaf_table: generates a lookup table for a given Group element and window size
  • wnaf_mul: performs the scalar multiplication using a lookup table and a scalar in NAF (can be obtained by find_wnaf)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Thanks for PR'ing this! LGTM, left some requests for comments & minor changes

ec/src/group/wnaf.rs Outdated Show resolved Hide resolved
ec/src/group/wnaf.rs Outdated Show resolved Hide resolved
test-templates/src/groups.rs Outdated Show resolved Hide resolved
ec/src/group/wnaf.rs Outdated Show resolved Hide resolved
@ggkitsas ggkitsas requested a review from ValarDragon March 15, 2021 10:18
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM!

Minor note, can a changelog entry be added to ### features.
perhaps:
- #230 (ark-ec) Add wnaf_mul implementation for groups

@ValarDragon ValarDragon requested a review from Pratyush March 16, 2021 18:48
@jon-chuang
Copy link
Contributor

jon-chuang commented Mar 17, 2021

I will be making some PRs from celo-org merge which include a wNAF mul combined with GLV. I think what would be nice would be to have the GLV-compatible curve use wNAF mul with a default window size specified in parameters.

I think we ought to make wNAF (+GLV if applicable) the default mul method, if it is faster.

However, maintaining the underlying methods in a separate file like here is good too, to reduce the single-file bloat and for multiple impls (add_assign for proj and add_assign_mixed for affine).

Unfortunately though, for the current impl, += with affine group would be extremely slow, I think, due to *self = s_proj.into();

The right thing to do would be to convert an affine point into a projective point first, since mul_bits returns a projective point anyway. So in theory, Group is too generic. I think ProjectiveCurve works just fine.

Btw @ValarDragon , do you know where Group is used? Seems it's only used in tests

@Pratyush
Copy link
Member

@jon-chuang how does this PR interact with your upcoming wNAF PR?

@jon-chuang
Copy link
Contributor

@Pratyush it can be used as an impl in a different file, which I am for as it separates duties.

ec/src/group/wnaf.rs Outdated Show resolved Hide resolved
@Pratyush
Copy link
Member

Thanks for the PR! This looks mostly good to me, barring the concern @jon-chuang raised about using Group; it's better to implement it directly for ProjectiveCurve, so that you can take advantage of things like mixed addition.

ggkitsas and others added 2 commits March 21, 2021 12:57
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
ec/src/wnaf.rs Outdated Show resolved Hide resolved
ec/src/wnaf.rs Outdated Show resolved Hide resolved
ec/src/wnaf.rs Outdated Show resolved Hide resolved
ec/src/wnaf.rs Outdated Show resolved Hide resolved
ec/src/wnaf.rs Outdated Show resolved Hide resolved
ec/src/wnaf.rs Outdated Show resolved Hide resolved
ggkitsas and others added 4 commits March 21, 2021 18:35
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
ec/src/wnaf.rs Outdated Show resolved Hide resolved
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
ec/src/wnaf.rs Outdated Show resolved Hide resolved
@Pratyush
Copy link
Member

@ggkitsas I pushed some updates that should now enable working with arbitrary window sizes. Please take a look and let me know if it makes sense.

@ggkitsas
Copy link
Contributor Author

@Pratyush LGTM, thanks!

@Pratyush Pratyush requested a review from ValarDragon March 22, 2021 16:10
@Pratyush
Copy link
Member

This LGTM, so unless folks would like further changes, I'll merge this today.

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM!

@Pratyush Pratyush changed the title adding wNAF multiplication functionality Adding wNAF multiplication functionality Mar 25, 2021
@Pratyush Pratyush merged commit 3c2c4dd into arkworks-rs:master Mar 25, 2021
@Pratyush Pratyush deleted the wnaf-port branch March 25, 2021 15:37
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.

4 participants