Skip to content

Commit

Permalink
curves: optimize Maxwell's trick
Browse files Browse the repository at this point in the history
  • Loading branch information
@indutny
indutny committed Jun 14, 2016
1 parent 7b1c732 commit e102a69
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 0 deletions.
5 changes: 5 additions & 0 deletions lib/elliptic/curve/edwards.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,13 @@ Point.prototype.eqXToP = function eqXToP(x) {
if (this.x.cmp(rx) === 0)
return true;

var xc = x.clone();
var t = this.curve.redN.redMul(this.z);
for (var i = 1; i <= this.curve._maxwellAdjust; i++) {
xc.iadd(this.curve.n);
if (xc.cmp(this.curve.p) >= 0)
return false;

rx.redIAdd(t);
if (this.x.cmp(rx) === 0)
return true;
Expand Down
5 changes: 5 additions & 0 deletions lib/elliptic/curve/short.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -910,8 +910,13 @@ JPoint.prototype.eqXToP = function eqXToP(x) {
if (this.x.cmp(rx) === 0)
return true;

var xc = x.clone();
var t = this.curve.redN.redMul(zs);
for (var i = 1; i <= this.curve._maxwellAdjust; i++) {
xc.iadd(this.curve.n);
if (xc.cmp(this.curve.p) >= 0)
return false;

rx.redIAdd(t);
if (this.x.cmp(rx) === 0)
return true;
Expand Down

4 comments on commit e102a69

@briansmith
Copy link

Choose a reason for hiding this comment

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

I believe you don't need the value _maxwellAdjust any more. Instead, the loop will terminate when xc exceeds the field modulus value.

It seems like you could have combined the top part of the function into the loop to reduce duplicate code. Maybe you didn't do that because you want to avoid the x.clone() for performance? If so it would be good to add comments to that effect.

Keeping in mind that I'm completely unfamiliar with this code and I'm not a JS coder, this seems to do the right thing. I'm working on adding the test vectors to ring. I'll ping you when they're ready so you can use them here. Note that libsecp256k1 has a test vector for that curve: see bitcoin-core/secp256k1@32600e5#diff-4655d106bf03045a3a50beefc800db21R1003.

@briansmith
Copy link

Choose a reason for hiding this comment

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

Also, you titled the commit "optimize Maxwell's trick", but my suggestion isn't about performance. It's about making sure that the code using the trick has exactly the same semantics as the normal code that does Jacobian-to-affine conversion.

In particular, consider the following two cases, using n == 5 and q == 7.

Case 1: r < q - n. In this case, it is possible that the original affine x coordinate was either r or r + n, both of which reduce to r mod n. For example, if r == 1 then x might be either 1 (r) or 6 (r + n) for a valid signature.

Case 2: r >= q - n. In this case, the affine x coordinate must have been r. There is no other value of x where x % n == r. For example, if r == 3 then it must be the case that x == 3 if the signature is valid. Note that r + n (mod q) == 3 + 5 (mod 7) == 8 (mod 7) == 1! (This is based on my understanding that rx.redIAdd(t); in your code is reducing mod q, not mod n.)

@indutny
Copy link
Owner Author

@indutny indutny commented on e102a69 Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@briansmith thank you for the comment! I believe you a right. Here is my reasoning:

Original comparison:

(p.x) % curve.order === r

This means that technically p.x could be:

p.x = r + i * curve.order

Where i is either positive or negative integer with the only requirement that the 0 <= p.x <= curve.prime. Imposing this requirement and observing that r is always r < curve.order - i can be only positive. Therefore we need to check all values of i from 0 to whatever it takes to get r + i * order maximally close to the curve's prime. Checking it further than that will have a different semantics indeed.

Going to remove the ._maxwellAdjust comparison, thank you!

@indutny
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is what I have in mind for the patch:

diff --git a/lib/elliptic/curve/base.js b/lib/elliptic/curve/base.js
index b011481..c39577a 100644
--- a/lib/elliptic/curve/base.js
+++ b/lib/elliptic/curve/base.js
@@ -32,10 +32,10 @@ function BaseCurve(type, conf) {
   // Generalized Greg Maxwell's trick
   var adjustCount = this.n && this.p.div(this.n);
   if (!adjustCount || adjustCount.cmpn(100) > 0) {
-    this._maxwellAdjust = 0;
+    this._maxwellTrick = false;
     this.redN = null;
   } else {
-    this._maxwellAdjust = adjustCount.toNumber();
+    this._maxwellTrick = true;
     this.redN = this.n.toRed(this.red);
   }
 }
diff --git a/lib/elliptic/curve/edwards.js b/lib/elliptic/curve/edwards.js
index 9bfc854..6e9fb77 100644
--- a/lib/elliptic/curve/edwards.js
+++ b/lib/elliptic/curve/edwards.js
@@ -416,7 +416,7 @@ Point.prototype.eqXToP = function eqXToP(x) {

   var xc = x.clone();
   var t = this.curve.redN.redMul(this.z);
-  for (var i = 1; i <= this.curve._maxwellAdjust; i++) {
+  for (;;) {
     xc.iadd(this.curve.n);
     if (xc.cmp(this.curve.p) >= 0)
       return false;
diff --git a/lib/elliptic/curve/short.js b/lib/elliptic/curve/short.js
index 290e26a..0a09d24 100644
--- a/lib/elliptic/curve/short.js
+++ b/lib/elliptic/curve/short.js
@@ -912,7 +912,7 @@ JPoint.prototype.eqXToP = function eqXToP(x) {

   var xc = x.clone();
   var t = this.curve.redN.redMul(zs);
-  for (var i = 1; i <= this.curve._maxwellAdjust; i++) {
+  for (;;) {
     xc.iadd(this.curve.n);
     if (xc.cmp(this.curve.p) >= 0)
       return false;
diff --git a/lib/elliptic/ec/index.js b/lib/elliptic/ec/index.js
index 9994590..b7fa666 100644
--- a/lib/elliptic/ec/index.js
+++ b/lib/elliptic/ec/index.js
@@ -166,7 +166,7 @@ EC.prototype.verify = function verify(msg, signature, key, enc) {
   var u1 = sinv.mul(msg).umod(this.n);
   var u2 = sinv.mul(r).umod(this.n);

-  if (this.curve._maxwellAdjust === 0) {
+  if (!this.curve._maxwellTrick) {
     var p = this.g.mulAdd(u1, key.getPublic(), u2);
     if (p.isInfinity())
       return false;

Regarding merging stuff, I'll iterate on it a bit later. Thank you again!

Please sign in to comment.