-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix isPoint inconsistency with native library #28
Conversation
(note: this is not a fix for #6) |
@dcousens any idea why elliptic throws error? code in library looks valid, but this is not work... 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'); |
@fanatid the tl;dr is we used to have
|
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 |
@fanatid |
I have no idea currently what give to us check |
Added two failing cases in 83ce5e4 - I intend to follow up with more tests, but this seems OK for patch? |
Any review? |
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
Two things:
The elliptic point that passes with
isPoint
, throws immediately indecodeFrom
(and therefore isn't usable for any ECDSA operations).