-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Signed-digit based ecmult_const algorithm #1184
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
Signed-digit based ecmult_const algorithm #1184
Conversation
real-or-random
left a comment
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.
Whoa nice! The skew correction is really annoying to reason about...
I just don't know where to get all the review power from.
a0c7934 to
f16c500
Compare
|
Added a commit to remove |
|
@peterdettman Randomly tagging you, you may find this interesting. |
|
Very cool! I wonder if the explanation might be clearer if you said something like "without any further optimizations, k0 would be 2^n - 1 with n = 256; however we are going to combine this scheme with the GLV endomorphism and do a windowed rather than bitwise approach. So instead we solve for k0" rather than "for some constant k0". In the end I was able to understand what you were doing without much trouble, so what you've written is fine, but it was a bit intimidating to see k0, k1, k2 all introduced at once just as unknowns. |
apoelstra
left a comment
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.
ACK f16c500
f16c500 to
b4ff4eb
Compare
|
@apoelstra I've changed the explanation to introduce the offset terms a bit more gently. I've also dropped the |
|
@sipa this looks way better, thanks! |
apoelstra
left a comment
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.
ACK b4ff4eb
b4ff4eb to
4de39ef
Compare
|
EDIT: no, doesn't work |
4de39ef to
1c48861
Compare
|
Oops, my previous rebase accidentally reverted the changes I made to address @apoelstra's comment. Re-rebased. |
4436ce2 to
a3b1a90
Compare
Are you able to explain why that doesn't work? |
|
Nice idea, simple and clear.
|
a3b1a90 to
dd21e2c
Compare
|
I've switched to offset |
|
I think if _scalar_split_lambda just output 129-bit two's complement values (and then sign-extend to ECMULT_CONST_BITS), then they would be directly usable in the comb, no offset needed for s1, s2. Something for the future, perhaps. |
f773744 to
c8eb787
Compare
|
Rebased and addressed the comments above. |
jonasnick
left a comment
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.
real-or-random
left a comment
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.
utACK mod these nits
also utACK on jonas' fixups (except a typo and one further comment here: jonasnick@cdf545a )
I believe I've done a thorough review, but I really have only nits.
stratospher
left a comment
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.
ACK c8eb787.
was able to follow the useful comments and code! also observed a 2% speedup on my machine.
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com> Co-authored-by: Tim Ruffing <crypto@timruffing.de>
* add test case for a=infinity The corresponding ecmult_const branch was not tested before this commit. * add test for edge cases
c8eb787 to
355bbdf
Compare
jonasnick
left a comment
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.
ACK 355bbdf
siv2r
left a comment
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.
ACK 355bbdf
real-or-random
left a comment
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.
ACK 355bbdf
Using some insights learned from #1058, this replaces the fixed-wnaf ecmult_const algorithm with a signed-digit based one. Conceptually both algorithms are very similar, in that they boil down to summing precomputed odd multiples of the input points. Practically however, the new algorithm is simpler because it's just using scalar operations, rather than relying on wnaf machinery with skew terms to guarantee odd multipliers.
The idea is that we can compute$q \cdot A$ as follows:
secp256k1_scalar_lambda_split.The comments in$f(q) = (q + (1+\lambda)(2^n - 2^{129} - 1))/2 \mod n$ , the result will equal $q \cdot A$ .
ecmult_const_impl.hshow that ifThis last step can be performed in groups of multiple bits at once, by looking up entries in a precomputed table of odd multiples of$A$ and $\lambda A$ , and then multiplying by a power of two before proceeding to the next group.
The result is slightly faster (I measure ~2% speedup), but significantly simpler as it only uses scalar arithmetic to determine the table lookup values. The speedup is due to the fact that no skew corrections at the end are needed, and less overhead to determine table indices. The precomputed table sizes are also made independent from the
ecmultones, after observing that the optimal table size is bigger here (which also gives a small speedup).