Skip to content
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

Replace bignumber.js+cryptojs with bn.js+elliptic+hash.js #409

Merged
merged 28 commits into from Jul 11, 2014
Merged

Replace bignumber.js+cryptojs with bn.js+elliptic+hash.js #409

merged 28 commits into from Jul 11, 2014

Conversation

ryanxcharles
Copy link
Contributor

I've replaced all of the bignum and elliptic curve crypto in the browser with @indutny's bn.js and elliptic, and also replaced our ripemd160 implementation with his hash.js. The result is much faster and much smaller code.

The mocha browser tests on master take about 45s on my computer. With this PR, they take 17s.

The Copay mocha browser tests, which heavily use bitcore, take 85s with bitcore master. With this PR, they take 22s.

Furthermore, the browser bundle is much smaller with this PR. On master, it is 350kb. With this PR, it is a mere 115kb.

So this PR, roughly speaking, makes bitcore 2.5 times faster and 3 times smaller. And that is actually an underestimate, since that speed test is on all tests, much of which have nothing to do with the changes. On sensitive parts of the code:

HierarchicalKey tests on master take 3.85s, and on this PR take 0.77s. So they are 5 times faster.

TransactionBuilder tests take 25s on master, and 8s on this PR. So they are 3 times faster.

Grepping for "Key" results in a bunch of sensitive tests that take 8s on master and 1.5s on this PR. So they are 4 times faster.

This replaces the previous PR where I only replaced the bignum code: #406

@taariq
Copy link

taariq commented Jul 6, 2014

Wow. Nice work Ryan. Congrats.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) when pulling 83f4105 on ryanxcharles:feature/elliptic into e485613 on bitpay:master.

@matiu
Copy link
Contributor

matiu commented Jul 6, 2014

Wow. Fantastic!!!

On Saturday, July 5, 2014, Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/936860

Coverage increased (+0.14%) when pulling 83f4105
83f4105
on ryanxcharles:feature/elliptic
into e485613
e485613
on bitpay:master
.


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

Matías Alejo Garcia
Skype/Twitter: @EMATIU
Roads? Where we're going, we don't need roads!

@indutny
Copy link

indutny commented Jul 6, 2014

Woooow!

@maraoz
Copy link
Contributor

maraoz commented Jul 6, 2014

:o awesome work both @ryanxcharles and @indutny!!!

On Sat, Jul 5, 2014 at 10:48 PM, Ryan X. Charles notifications@github.com
wrote:

I've replaced all of the bignum and elliptic curve crypto in the browser with @indutny's bn.js and elliptic, and also replaced our ripemd160 implementation with his hash.js. The result is much faster and much smaller code.
The mocha browser tests on master take about 45s on my computer. With this PR, they take 17s.
The Copay mocha browser tests, which heavily use bitcore, take 85s with bitcore master. With this PR, they take 22s.
Furthermore, the browser bundle is much smaller with this PR. On master, it is 350kb. With this PR, it is a mere 115kb.
So this PR, roughly speaking, makes bitcore 2.5 times faster and 3 times smaller. And that is actually an underestimate, since that speed test is on all tests, much of which have nothing to do with the changes. On sensitive parts of the code:
HierarchicalKey tests on master take 3.85s, and on this PR take 0.77s. So they are 5 times faster.
TransactionBuilder tests take 25s on master, and 8s on this PR. So they are 3 times faster.
Grepping for "Key" results in a bunch of sensitive tests that take 8s on master and 1.5s on this PR. So they are 4 times faster.
You can merge this Pull Request by running:
git pull https://github.com/ryanxcharles/bitcore feature/elliptic
Or you can view, comment on it, or merge it online at:
#409
-- Commit Summary --

  • replace bignumber.js with bn.js
  • use elliptic in Point in the browser instead of cryptojs
  • use elliptic for Point.multiply and key regeneration
  • update sign function to use elliptic
  • add "ECKey" to Key test so grepping is easier
  • remove cryptojs dependency from Key
  • remove cryptojs dependency from util
  • remove cryptojs dependency
  • require Point ... woops
    -- File Changes --
    M Gruntfile.js (2)
    M browser/build.js (6)
    M browser/bundle.js (246)
    D browser/concat.sh (7)
    D browser/vendor-bundle.js (2869)
    D browser/vendor/browser-adapter.js (6)
    D browser/vendor/crypto-2.0.js (158)
    D browser/vendor/crypto-3.1.js (31)
    D browser/vendor/ec.js (319)
    D browser/vendor/ecdsa.js (485)
    D browser/vendor/eckey.js (134)
    D browser/vendor/jsbn.js (559)
    D browser/vendor/jsbn2.js (658)
    D browser/vendor/prng4.js (45)
    D browser/vendor/rng.js (70)
    D browser/vendor/sec.js (175)
    D browser/vendor/util.js (229)
    M lib/Base58.js (12)
    M lib/Block.js (12)
    M lib/ScriptInterpreter.js (38)
    M lib/Transaction.js (2)
    M lib/TransactionBuilder.js (26)
    M lib/browser/Bignum.js (2104)
    M lib/browser/Key.js (128)
    M lib/browser/Point.js (105)
    M package.json (3)
    M test/test.Bignum.browser.js (54)
    M test/test.Block.js (2)
    M test/test.Key.js (2)
    M test/test.Point.js (32)
    M test/test.TransactionBuilder.js (15)
    M test/test.misc.js (8)
    M util/util.js (33)
    -- Patch Links --
    https://github.com/bitpay/bitcore/pull/409.patch
    https://github.com/bitpay/bitcore/pull/409.diff

    Reply to this email directly or view it on GitHub:
    Replace bignumber.js+cryptojs with bn.js+elliptic+hash.js #409

@matiu
Copy link
Contributor

matiu commented Jul 10, 2014

We should starting testing copay with this PR. I am really excited about this work!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.14%) when pulling 69b5c8a on ryanxcharles:feature/elliptic into 3f0dd8d on bitpay:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling b504b0b on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling dbaeb04 on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling dbaeb04 on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling 992e1cf on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.19%) when pulling 3f75bea on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.19%) when pulling 823d021 on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 823d021 on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@matiu
Copy link
Contributor

matiu commented Jul 11, 2014

what is happening with Coveralls?

On Thu, Jul 10, 2014 at 11:17 PM, Coveralls notifications@github.com
wrote:

[image: Coverage Status] https://coveralls.io/builds/955173

Coverage increased (+0.19%) when pulling 823d021
823d021
on ryanxcharles:feature/elliptic
into 0b33869
0b33869
on bitpay:master
.


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

Matías Alejo Garcia
Skype/Twitter: @EMATIU
Roads? Where we're going, we don't need roads!

@ryanxcharles
Copy link
Contributor Author

I keep adding more commits, so it keeps recalculating the coverage.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling c75de96 on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

Ryan X. Charles added 2 commits July 11, 2014 11:52
...i.e., bignums, numbers, and strings. Also, ensure that if you try to
multiply a buffer, it should be exactly 32 bytes. Eventually this "multiply"
function will be replaced with a more conventional "mul" function, but not yet.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.21%) when pulling 0f0a1b1 on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

@ryanxcharles ryanxcharles changed the title WIP: Replace bignumber.js+cryptojs with bn.js+elliptic+hash.js Replace bignumber.js+cryptojs with bn.js+elliptic+hash.js Jul 11, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.19%) when pulling 92ac073 on ryanxcharles:feature/elliptic into 0b33869 on bitpay:master.

ryanxcharles pushed a commit that referenced this pull request Jul 11, 2014
Replace bignumber.js+cryptojs with bn.js+elliptic+hash.js
@ryanxcharles ryanxcharles merged commit baf31e5 into bitpay:master Jul 11, 2014
@ryanxcharles
Copy link
Contributor Author

I pulled this in. Here's some code you can run to do a performance comparison:

var bitcore = require('bitcore');
var key = bitcore.Key.generateSync();

console.log('Pre-generating data');
var data = [];
for (var i = 0; i <= 5000; i++) {
  data[i] = bitcore.util.sha256('data ' + i);
};
console.log('Generating signatures');
var time1 = Date.now();
for (var i = 0; i<= 5000; i++) {
  var sig = key.signSync(data[i]);
};
var time2 = Date.now();
console.log('Time: ' + (time2-time1)/1000);

5000 signatures:

  • openssl (node): 2s
  • new crypto (firefox): 8s
  • old crypto (firefox): 125s

So the new crypto is about 20 times faster at performing signatures in the browser. And the browser code now is only about 4 times slower than node/openssl.

@ryanxcharles ryanxcharles added this to the Improve Crypto Interface milestone Jul 15, 2014
slowfx pushed a commit to slowfx/bitcore-mona that referenced this pull request Jul 14, 2015
fix link next block if you are on the last block
micahriggan pushed a commit to micahriggan/bitcore that referenced this pull request Dec 27, 2018
Handle invalid xpub on wallet join
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants