fix isPoint inconsistency with native library

[dcousens]

Two things:

• [ ] We need more tests (adding in this PR)
• [ ] We need to be ensuring external modules run their unit tests in both a native and forced JS environment

The elliptic point that passes with isPoint, throws immediately in decodeFrom (and therefore isn't usable for any ECDSA operations).

[dcousens]

(note: this is not a fix for https://github.com/bitcoinjs/tiny-secp256k1/issues/6)

[fanatid]

@dcousens any idea why elliptic throws error? code in library looks valid, but this is not work... (y**0.5)**2 != y (?)

  var y2 = x.redSqr().redMul(x).redIAdd(x.redMul(this.a)).redIAdd(this.b);
  var y = y2.redSqrt();
  if (y.redSqr().redSub(y2).cmp(this.zero) !== 0)
    throw new Error('invalid point');

[dcousens]

@fanatid the tl;dr is we used to have curve.validate AND ECPoint.decodeFrom, but secp256k1 native doesn't have the two, it merges them.

elliptic does equivalent to curve.validate... in decodeFrom. We weren't doing a curve.validate with our optimized isPoint. That optimization is leading to this differentiation, for isPoint.

See https://github.com/bitcoinjs/bitcoinjs-lib/blob/f4caaf42e7b58332d74c1540f88bcda7e55b82e6/src/hdnode.js#L107-L112

[fanatid]

Just realized that sqrt can give 2 numbers, so probably valid code should be:

  var y2 = x.redSqr().redMul(x).redIAdd(x.redMul(this.a)).redIAdd(this.b);
  var y2c = y2.redSqrt().redSqr();
  if (y2c.redSub(y2).cmp(this.zero) !== 0 && y2c.redNeg().redSub(y2).cmp(this.zero) !== 0)
    throw new Error('invalid point');

@dcousens does this fix make sense? should we fix elliptic?

[dcousens]

@fanatid elliptic throws invalid point because the point is not on the curve?

[fanatid]

I have no idea currently what give to us check (y^0.5)^2 = y (which generate invalid point error) in current code...

[dcousens]

Added two failing cases in 83ce5e4 - I intend to follow up with more tests, but this seems OK for patch? @junderw @jprichardson

[dcousens]

Any review?