-
Notifications
You must be signed in to change notification settings - Fork 138
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
Add edge case handling for batch_add
#169
Conversation
05c4179
to
b2671ef
Compare
a8c0e45
to
a5a18fe
Compare
Wait for #168 to be merged. |
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.
Overall, I think the PR looks good.
I have 2 comments.
- Is there any use case of
batch_add_nonexceptional
?
I think we can get rid of this function if not used.
It will reduce the code.
Also, we can keep the original name -batch_add
, just add the missing logic. - How about
multiexp_serial
/multiexp_parallel
, instead ofserial_multiexp
/parallel_multiexp
?
I just believe thatmultiexp_*
is more meaningful than*_multiexp
.
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.
Regarding the naming, I think we should use msm
, instead of multiexp
.
Since we are handling the EC points and scalars.
I'm still on the fence about the naming.
We could have |
524a5ba
to
806f47e
Compare
Yes, I believe they are better. |
806f47e
to
6ede410
Compare
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.
lgtm :)
This PR adds a new version of
batch_add
that accounts for edge cases. (Closes #153)The old function is now
batch_add_nonexceptional
( as suggested #130 (comment))The new version is
batch_add_exceptional
The decrease in performance after the change is minimal:
The msm functions have been renamed:
multiexp_serial
->serial_multiexp
best_multiexp
->parallel_multiexp
best_multiexp_indepenedent_points
->best_multiexp
I still think this names are quite confusing, so suggestions are welcome. ( Maybe
cyclone_
instead ofbest_
andmsm
instead ofmultiexp
).The old
batch_add
has been kept but is now unsused. We can either remove it, or add another msm or argument tobest_multiexp
that enables its use.