added usage of node-secp256k1 bindings module as optional module

[rubensayshi]

https://github.com/wanderer/secp256k1-node provides nodejs bindings for https://github.com/bitcoin/secp256k1

it's a pretty decent performance gain using this module instead of the current js implementation.

in this PR it will switch to using it if it's installed, because I couldn't really think of a nice way to 'configure' bitcoinjs-lib to enable / disable it...
just putting this 'out there' to see what you think of it

[coveralls]

Coverage decreased (-0.28%) to 97.97% when pulling 322ada4 on blocktrail:node-secp256k1 into 0c451b5 on bitcoinjs:master.

[rubensayshi]

hmm, I asumed that optionalDependancies worked like the suggest in composer and wouldn't install but only hint that it should be installed by default.
but npm install will install them unless you --no-optional

[weilu]

it's a pretty decent performance gain using this module instead of the current js implementation.

Is there a benchmark we can look at?

There's also "This library is experimental, so use at your own risk." on their readme. We should at least make sure wherever we use it, we run it against our existing test cases and they all pass. Technically it's doable if we push the if (secp256k1) ... else switch up to ecdsa.js or its own wrapper, but I have no idea how to guarantee that Jenkins runs it.

For browserify support we will also need to add something like "browser": { "secp256k1": false } in package.json

[jprichardson]

This seems like a bad idea as Bitcore originally started down this path of having native bindings in Node.js and different code in the browser; AFAIK there were subtle bugs. Long-term (short-term?), the elliptic crypto code should drop ecurve and move to elliptic.

[dcousens]

I think we should move to elliptic first.

[weilu]

@jprichardson @dcousens I hear both of you saying we should move to elliptic. What are the motivations behind that? If it's performance, do we have benchmarks?

I think we should move to elliptic first.

How does moving to elliptic block us from considering node-secp256k1?

[dcousens]

@weilu its not hard to run the benchmarks yourself: https://github.com/indutny/elliptic/blob/master/benchmarks/index.js

It is a lot faster, and given that this PR is about moving to native bindings because performance is better, it would seem that we should first see if switching to elliptic doesn't resolve the issue adequately enough first.

[weilu]

@dcousens thanks for the link.

Here's my benchmark result off elliptic master (https://github.com/indutny/elliptic/releases/tag/v2.0.2)

±  |master ✗| → node benchmarks/index.js 
Benchmarking: sign
elliptic#sign x 367 ops/sec ±1.23% (179 runs sampled)
sjcl#sign x 62.45 ops/sec ±2.14% (129 runs sampled)
openssl#sign x 1,743 ops/sec ±1.53% (187 runs sampled)
ecdsa#sign x 38.06 ops/sec ±1.71% (102 runs sampled)
secp256k1#sign x 13,320 ops/sec ±1.57% (183 runs sampled)
------------------------
Fastest is secp256k1#sign
========================
Benchmarking: verify
elliptic#verify x 163 ops/sec ±1.09% (167 runs sampled)
sjcl#verify x 52.81 ops/sec ±2.30% (136 runs sampled)
openssl#verify x 1,610 ops/sec ±0.99% (189 runs sampled)
ecdsa#verify x 26.86 ops/sec ±1.13% (94 runs sampled)
secp256k1#verify x 9,239 ops/sec ±2.02% (182 runs sampled)
------------------------
Fastest is secp256k1#verify
========================
Benchmarking: gen
elliptic#gen x 281 ops/sec ±6.36% (135 runs sampled)
sjcl#gen x 64.63 ops/sec ±2.35% (132 runs sampled)
------------------------
Fastest is elliptic#gen
========================
Benchmarking: ecdh
elliptic#ecdh x 133 ops/sec ±5.66% (115 runs sampled)
------------------------
Fastest is elliptic#ecdh
========================
Benchmarking: curve25519
elliptic#curve25519 x 65.51 ops/sec ±5.27% (113 runs sampled)
jodid#curve25519 x 81.31 ops/sec ±2.45% (140 runs sampled)
------------------------
Fastest is jodid#curve25519
========================

[weilu]

Put things into perspective for the two functions we care about:

Switching from ecdsa to elliptic is expected to give us 10x and 6x performance improvement. For 36x and 57x speed gain (secp256k1 over elliptic), I don't think we should dismiss secp256k1 so quickly. And I don't see why switching to elliptic should stand in the way of considering secp256k1.

[dcousens]

I don't disagree, I'm just saying the improvement of elliptic over the
current is so large that it may be likely that ECDSA will no longer be your
bottleneck.
On 8 Feb 2015 12:31 pm, "Wei Lu" notifications@github.com wrote:

Put things into perspective for the two functions we care about:

[image: image]
https://cloud.githubusercontent.com/assets/412533/6095074/8cf86734-af85-11e4-88c1-085d48bf326d.png

[image: image 1]
https://cloud.githubusercontent.com/assets/412533/6095075/8f2e1472-af85-11e4-9e08-75630acd0835.png

For 36x and 57x speed gain (secp256k1 over elliptic), I don't think we
should dismiss secp256k1 so quickly.

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

[jprichardson]

Do we want bitcoinjs-lib to be a library where performance is the priority? If so, then it makes sense to accept this PR and go in the direction as such. I'm not suggesting that we shouldn't care about performance. I'm acknowledging that we only have so much programmer muscle and introducing two code paths that run differently depending upon the environment may invite unforeseen consequences in the future. It'd probably be prudent to research why Bitcore started this way and then subsequently dropped the Node.js/C++ specific code.

[dcousens]

@jprichardson agreed. I'd much rather stay stable for now, move to elliptic after tests are merged and then move to looking at secp256k bindings if we can prove consistency and safety.

[rubensayshi]

secp526k1 is what bitcoin core uses, we can't be more stable than that really ... or at least if we have the same bugs are bitcoin core then we're following consensus xD

by relying on the javascript code you're preferring to potentially have differences from bitcoin core just because you want those differences to be consistent between browser and nodejs.

but I get that! I was expecting you to make this argument, and I'd make the same call if I were you, I made the PR just to get a discussion going ;-)

and switching to elliptic already sounds like a nice improvement.

however, maybe there could be an easy way of making it configurable?
either an ENV var that switches to using secp256k1, or creating a bitcoinjs-secp256k1 package which can be installed to start using it.
that way the usage of secp256k1 will be 100% explicit and a choice of the developer, not an excidental side effect of installing secp256k1 (which someone might have installed for another purpose).

[Polve]

Thanks for evaluating this PR and opening the discussion.

I'm also in favour to having an option to use the sep256k1 implementation: real field usage has shown that developers are manually patching the library due to the huge performance improvements, even if this means less tested code.

So, an option to have it cleany available would be certainly be an improvement.

[weilu]

Again, my stand on this isn't that we should absolutely move to support secp526k1 right now. If you read my comment above, I'm suggesting that 1) we need to make sure the performance gain is significant enough to justify this. AND 2) we should at least make sure whatever we decide to switch over, we should run it against our existing test cases and make sure they all pass. I think the benchmark gives us a green light on 1).

Point 2) deserves a bit more discussion here. To @jprichardson's point:

I'm acknowledging that we only have so much programmer muscle and introducing two code paths that run differently depending upon the environment may invite unforeseen consequences in the future.

That's exactly why I proposed that we make sure we have Travis constantly running the same set of test cases against both code paths. That will provide us the level of confidence as low (or as high) as our test cases could cover. We will notice (and fix) any divergence when either code path changes. Until we have such a CI setup, I wouldn't be comfortable supporting two code paths either, regardless of the explicit option to switch sep256k1 implementation as proposed by @rubensayshi.

[weilu]

I'm just saying the improvement of elliptic over the
current is so large that it may be likely that ECDSA will no longer be your
bottleneck.

I'm still not convinced that moving to elliptic stops us from considering node-secp256k1. No one will complain that code runs too fast.

[ryanxcharles]

On bitcore: We had some C++ code for "key", which had a completely different interface from the pure javascript code in the browser. We ended up implementing many things twice, even pure javascript things that merely depended on the elliptic curve stuff. It was such a pain to deal with this that it was better to remove the C++ code than try to make the interface the same as the pure javascript code.

We also updated the pure javascript crypto from cryptojs to elliptic for significant speed gains of around 5x.

IMO, the right long-term approach is to use the new bitcoin core consensus library that is being developed in node, which obviously includes (or will include) libsecp256k1. Try to wrap that code in an interface that is exactly the same as the interface exposed in the browser so that you don't have to implement a bunch of things twice.

Also, elliptic has not been thoroughly audited. One of the BitPay developers found a money-losing bug in elliptic merely by running a bunch of random tests. There are probably more errors. IMO the right way to deal with this is to rewrite the secp256k1 part of elliptic, determine all edge cases, write tests for those, confirm the same tests pass elliptic, have many people review this code, confirm the performance is better or at least the same, then use that code instead of elliptic. We don't need all the other curves supported by elliptic. We need a bitcoin-only pure javascript implementation of secp256k1. A pure javascript version of libsecp256k1.

[rubensayshi]

in #361 it's suggested to use emscriptem for the browser version which is already faster than the baseline bitcoinjs-lib and then we'd have consistency between nodejs and browser!

[coveralls]

Coverage decreased (-0.26%) to 97.86% when pulling 1036dee on blocktrail:node-secp256k1 into 3a15f0c on bitcoinjs:master.

[dcousens]

I don't think we should use both modules simultaneously, it should be a full switch-over if libsecp256k1 is present.

[dcousens]

@fanatid, you might be able to rebase this work with the 3.0.0 API.
Though, I'd make the dependency non-optional.

[dcousens]

Related to #623

[dcousens]

Closing (for now) in favour of #737, as it depends on how we answer that question