Correct secp256k1_gej_add_ge with 3% performance hit on signing
#261
Conversation
There is zero functionality or opcount changes here; I need to do this to make sure both R and M are computed before they are used, since a future patch will replace either none or both of them. Also compute r->y directly in terms of r->x, which again will be used in a future patch.
| secp256k1_fe_negate(&rr_alt, &s2, 1); /* rr = -Y2*Z1^3 */ | ||
| secp256k1_fe_add(&rr_alt, &s1); /* rr = Y1*Z2^3 - Y2*Z1^3 */ | ||
| secp256k1_fe_negate(&m_alt, &u2, 1); /* m = -X2*Z1^2 */ | ||
| secp256k1_fe_add(&m_alt, &u1); /* m = X1*Z2^2 - Y1*Z2^2 */ |
There was a problem hiding this comment.
/* m = X1*Z2^2 - X2*Z1^2 */
|
Push to fix everyone's comments (comment fixes and Peter's awesome speedup). |
| * This means either x1 == beta*x2 or beta*x1 == x2, where beta is | ||
| * a nontrivial cube root of one. In either case, an alternate | ||
| * non-indeterminate expression for lambda is (x1 - x2)/(y1 - y2), | ||
| * so we set R/M equal to this. */ |
There was a problem hiding this comment.
Oh, I had the expression for lambda right the first time :) it should be (y1 - y2)/(x1 - x2).
There was a problem hiding this comment.
Is it (y1 - y2)/(x1 - x2) or the inverse? The comment still says the inverse.
|
We might want to update the citation above to point out that the additive law given in the citation is actually not complete, lest some unwary traveler see the citation and go there rather than reading the code. |
| /* These two lines use a clever observation of Peter Dettman's, | ||
| * that either M == Malt or M == 0. So M^3 * Malt is either Malt^4 | ||
| * (which is computed by squaring), or zero (which is computed by | ||
| * the cmov). So the cost is one squaring versus two multiplications. */ |
There was a problem hiding this comment.
It's of course nice to be appreciated, but I think code comments would be better kept to the immediate technical details.
There was a problem hiding this comment.
Direct experience shows that people do find attribution in commit messages just fine. E.g. I had people contact me on the inversion free R == r trick.
There was a problem hiding this comment.
If you'd like me to remove your name, I will. But I agree with @gmaxwell that it's probably good for readers that it be there.
There was a problem hiding this comment.
Actually I was saying that people will find it if it's just in the commit message rather than a comment in the code.
There was a problem hiding this comment.
Oh :) well if you're both against me I'll take it out of the source comment.
There was a problem hiding this comment.
I think we eventually want some AUTHORS document that explains who contributed what - that's easier to access than digging through commit messages, and less invasive than putting it in the code.
Some documentation in the source tree that explains the various tricks and optimizations implemented would be even nicer.
|
For what it's worth, the citation does say that it's assuming the y1 = -y2 case does not occur. |
apoelstra
force-pushed
the
add-fix
branch
2 times, most recently
from
4ad13c0
to
e22b66a
Compare
|
Push with comment fixes and a much-expanded comment about our high-level strategy for obtaining constant-time multiplication for all pairs of points (well, except those with a = infinity). We might be the first to have produced a fast, constant-time algorithm which works for all pairs of points. When developing this fix I checked both Trezor and OpenSSL, and their constant-time additions both have degenerate cases (specifically both don't work when asked to double) which they avoid by being careful about how constadds are used in their larger algorithms. Edit: We did discuss using this strategy; however we were uncertain how it interacted with our existing blinding and optimization tricks throughout the codebase (which OpenSSL and Trezor do not have to worry about) and none of us wanted to have to keep "oh, provably make sure you avoid the cases that break constadd" in our heads for all future work. |
apoelstra
changed the title
secp256k1_gej_add_ge with 2M - S performance hitsecp256k1_gej_add_ge with 3% performance hit on signing
If two points (x1, y1) and (x2, y2) are given to gej_add_ge with x1 != x2 but y1 = -y2, the function gives a wrong answer since this causes it to compute "lambda = 0/0" during an intermediate step. (Here lambda refers to an auxiallary variable in the point addition formula, not the cube-root of 1 used by the endomorphism optimization.) This commit catches the 0/0 and replaces it with an alternate expression for lambda, cmov'ing it in place if necessary.
|
ACK |
7657420 Add tests for adding P+Q with P.x!=Q.x and P.y=-Q.y (Pieter Wuille) 8c5d5f7 tests: Add failing unit test for #257 (bad addition formula) (Andrew Poelstra) 5de4c5d gej_add_ge: fix degenerate case when computing P + (-lambda)P (Andrew Poelstra) bcf2fcf gej_add_ge: rearrange algebra (Andrew Poelstra)
Fixes #257 by catching the degenerate case, which appears as an intermediate variable being 0/0, and using an alternate expression for that variable in this case.
This results in only a ~3% perf hit on signing. There are no changes in the multiplication or squaring count; this is entirely due to additions and cmovs. "I doubt we can do better" but I said that about an earlier revision with a 6% hit and Peter Dettman found a way to beat it, so my doubts aren't worth much :)