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

secp224r1 not working with pointFromX #21

Closed
dcousens opened this Issue Jun 21, 2014 · 7 comments

Comments

Projects
None yet
2 participants
@dcousens
Member

dcousens commented Jun 21, 2014

As can be seen in the tests we currently skip secp224r1 in the tests because it is not fully supported.
This appears to be because we are using a curve specific optimization in curve.js: https://github.com/cryptocoinjs/ecurve/blob/master/lib/curve.js#L22.

Porting over to bn.js does resolve this issue, but because we aren't using the reduction contexts all the way through, it has a huge performance hit.

For now, it seems better to just flag this as obvious as possible and keep building up tests around the library.

@jprichardson

This comment has been minimized.

Show comment
Hide comment
@jprichardson

jprichardson Jun 23, 2014

Member

How about we just remove secp224r1 curve?

Member

jprichardson commented Jun 23, 2014

How about we just remove secp224r1 curve?

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jun 24, 2014

Member

I think that would be a responsible decision, but maybe also a note in the README that this implementation is only tested for the included curves and contains specific optimizations as such.

Member

dcousens commented Jun 24, 2014

I think that would be a responsible decision, but maybe also a note in the README that this implementation is only tested for the included curves and contains specific optimizations as such.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jun 24, 2014

Member

Found a source for the optimization: http://eprint.iacr.org/2012/309.pdf.
Ref 2.3 Point Compression.

Member

dcousens commented Jun 24, 2014

Found a source for the optimization: http://eprint.iacr.org/2012/309.pdf.
Ref 2.3 Point Compression.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jun 24, 2014

Member

Ok, this is simply because the prime chosen in secp224 is not congruent to 3 (mod 4), and is easily verified by checking the below code against all fields.

console.log(curve.p.and(new BigInteger('3')).toString())

This is mentioned as a pre-condition in the paper referenced above.
This fast case can be seen in bn.js.

I don't see any easy way of getting around this in bigi without introducing new code.

Member

dcousens commented Jun 24, 2014

Ok, this is simply because the prime chosen in secp224 is not congruent to 3 (mod 4), and is easily verified by checking the below code against all fields.

console.log(curve.p.and(new BigInteger('3')).toString())

This is mentioned as a pre-condition in the paper referenced above.
This fast case can be seen in bn.js.

I don't see any easy way of getting around this in bigi without introducing new code.

@jprichardson

This comment has been minimized.

Show comment
Hide comment
@jprichardson

jprichardson Jun 24, 2014

Member

OK, I'd be in favor of getting rid of the curve. I doubt many will use it
anyways and if that changes, we can cross that bridge then.

Thoughts?

On Mon, Jun 23, 2014 at 10:52 PM, Daniel Cousens notifications@github.com
wrote:

Ok, this is simply because the prime chosen in secp224 is not congruent
to 3 (mod 4), and is easily verified by checking the below code against
all fields.

console.log(curve.p.and(new BigInteger('3')).toString())

This is mentioned as a pre-condition in the paper referenced above.
This fast case can be seen in bn.js
https://github.com/indutny/bn.js/blob/master/lib/bn.js#L1607-L1615.

I don't see any easy way of getting around this in bigi without
introducing significant new code.


Reply to this email directly or view it on GitHub
#21 (comment).

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow me on Twitter: http://twitter.com/jprichardson

Member

jprichardson commented Jun 24, 2014

OK, I'd be in favor of getting rid of the curve. I doubt many will use it
anyways and if that changes, we can cross that bridge then.

Thoughts?

On Mon, Jun 23, 2014 at 10:52 PM, Daniel Cousens notifications@github.com
wrote:

Ok, this is simply because the prime chosen in secp224 is not congruent
to 3 (mod 4), and is easily verified by checking the below code against
all fields.

console.log(curve.p.and(new BigInteger('3')).toString())

This is mentioned as a pre-condition in the paper referenced above.
This fast case can be seen in bn.js
https://github.com/indutny/bn.js/blob/master/lib/bn.js#L1607-L1615.

I don't see any easy way of getting around this in bigi without
introducing significant new code.


Reply to this email directly or view it on GitHub
#21 (comment).

Simple & Secure Bitcoin Wallet: https://www.coinbolt.com
Bitcoin / JavaScript: http://cryptocoinjs.com
Follow me on Twitter: http://twitter.com/jprichardson

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jun 24, 2014

Member

Now that we're aware of the underlying issue, I'm perfectly OK with removing the curve and just making a note, at least until a better solution is found.

Member

dcousens commented Jun 24, 2014

Now that we're aware of the underlying issue, I'm perfectly OK with removing the curve and just making a note, at least until a better solution is found.

@jprichardson

This comment has been minimized.

Show comment
Hide comment
@jprichardson

jprichardson Jun 25, 2014

Member

Removed the curve in 1.0.0.

Member

jprichardson commented Jun 25, 2014

Removed the curve in 1.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment