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

Dependency updates, support node v12 #777

Merged
merged 5 commits into from
May 25, 2019
Merged

Conversation

nodech
Copy link
Member

@nodech nodech commented May 18, 2019

Updates dependencies, dotfiles and other minor updates.

Dotfiles and others.

  • minimum required node engine in bdb is 8.6.0, so bcoin will have the same.
  • Switch from istanbul@alpha (deprecated) to nyc.
  • {git,npm}ignore more things
  • update eslint checks, uses babel-eslint.
  • update ci
    • fix tools
    • use package-lock instead of package.json
    • coverage will count files that were not touched. (better coverage)
  • remove chacha benchmarks, it's out of scope of bcoin (part of bcrypto). Also not used anymore, it was part of bip151.

Dependency updates

NOTE: even though patch updates does not necessarily need change in package.json, because we allow patch level changes, I still included those.

tl;dr; nothing needs to change in bcoin.

incomplete list of updates

major update:

  • bmocha - Even though major version was updated, it's usage for bcoin is the same.

minor updates:

  • bcrypto - notes below
  • bfile

patch updates:

  • Everything else is patch update, that either fix bug or updates dependency and dotfiles.

BFile update:

  • Feature detection - based on nodejs version it can either use native versions
    (when available) or provide wrappers and has more consistent API across nodejs versions.

Bcrypto updates to bcoin related apis:
General:

  • node v12 fixes for native modules.

Diffs

  • secp256k1:
  • ripemd160:
    • bcrypto/node version will throw error if not supported (node.crypto does not have the hash).
  • bn.js:
    • moved vendor and was rewritten
    • has native implementation (not used from lib/bn.js, for now ?)
    • has js BigInt implementation (This can be used with env variable BCRYPTO_FORCE_BIGINT when
      when NODE_BACKEND is js)
  • scrypt:
    • changes to internal functions.
  • pbkdf2:
    • more safety checks
    • changes to internal functions
  • random:
    • internal changes, buffer allocations happen in C instead of js.
    • "proper" randomInt
  • aes:
    • internal changes.

v12 fixes only:

  • sha256
  • hash256
  • sha512
  • sha1

No change:

  • cleanse
  • safe
  • merkle

@nodech nodech changed the title Dependency updates, support node v12 [wip\Dependency updates, support node v12 May 18, 2019
@nodech nodech changed the title [wip\Dependency updates, support node v12 [wip] Dependency updates, support node v12 May 18, 2019
@nodech
Copy link
Member Author

nodech commented May 18, 2019

It seems NYC takes twice as much time for some tests (mining) and also consumes more memory that sometimes causes out of memory issues.

nyc seems to monkey patch child_process.spawn and that causes a lot of issues with our workers. Sometimes workers don't spawn anymore or system runs out of memory.

This workaround worked: https://gist.github.com/nodar-chkuaselidze/23b1221cabcb3ac61842d15a4b1fb3ae, but I will be skipping nyc change for now..

@codecov-io

This comment has been minimized.

bench: remove chacha that is out of the scope, it was part of the bip151 implementation.
test: increase timeouts.
@nodech nodech changed the title [wip] Dependency updates, support node v12 Dependency updates, support node v12 May 19, 2019
@nodech
Copy link
Member Author

nodech commented May 19, 2019

I checked why there was a problem from the workers, so there are couple of things that cause the problems:

  • nyc wraps workers
  • CI Machine has bunch of cores which gives us bunch of workers (36 cores)
  • Chain, Wallet and Indexer test suites don't close their workers (fixed)
  • Indexer starts creates it's workers but then Indexer->HTTP test suite will also create bunch of workers, the result being Indexer test case by the end of it will create NumCores * 2 workers.

Addressing these:

  • Fixed Chain, Wallet and Indexer
  • Best option would be to limit test workers size to 2 or 3, they don't optimize test benchmarks much, because they take time to boot and also tests are not intensive enough to be actually useful.
  • We could also separated Indexer and Indexer->HTTP (call Indexer HTTP, even in the same file, so Indexer can call after).. After configuring to 2 workers, it will just spawn 4 workers, so not a big deal.

Having NYC back, would be much better in that case, because it also covers workers (with some issues)

@nodech nodech requested a review from braydonf May 19, 2019 16:54
@braydonf
Copy link
Contributor

It also looks like some additional changes:

  • To use loady instead of bindings
  • And a fix for build for node@^9.3.0

@braydonf
Copy link
Contributor

Other changes not mentioned:

  • bweb - Now has patch method, and cjs and mjs mime type.
  • bsert - Support for symbols, and safeguards and cleanup.
  • bsock - Fix unhook, rebuild of faye-websocket, upgrade now has try/catch, name included in errors,
    and bufferization/stringification improvements and fixes.

@braydonf
Copy link
Contributor

bcrypto has a lot of implementation changes as relating to bcoin, specifically around BN and ECDSA and secp256k1.

@braydonf
Copy link
Contributor

Also a related Node.js v12 change that is relevant nodejs/node#25576

@braydonf braydonf added the ready for review Ready to be reviewed label May 23, 2019
@braydonf
Copy link
Contributor

Looks good, would be good to have some more review and testing in relation to the bfile and bcrypto updates.

@braydonf
Copy link
Contributor

General time for running the tests is faster.

Node.js v12.3.1 (master branch, native build fails):

$ time npm run test
real	0m51.470s
user	0m52.914s
sys	0m2.664s

Node.js v12.3.0 (depupdates branch):

$ time npm run test
real	0m18.312s
user	0m20.428s
sys	0m1.656s

Node.js v10.13.0 (master branch):

$ time npm run test
real	0m25.942s
user	0m24.424s
sys	0m2.451s

Node.js v10.13.0 (depupdates branch):

$ time npm run test
real	0m19.288s
user	0m22.104s
sys	0m1.422s

Node.js v8.11.2 (master branch):

$ time npm run test
real	0m26.511s
user	0m24.043s
sys	0m2.320s

Node.js v8.11.2 (depupdates branch):

$ time npm run test
real	0m19.868s
user	0m21.380s
sys	0m1.488s

@braydonf
Copy link
Contributor

General test time related to different bcrypto backends with Node.js v12.3.1:

time npm run test
real	0m18.641s
user	0m21.096s
sys	0m1.486s
export NODE_BACKEND=js
time npm run test
real	1m17.876s
user	1m27.895s
sys	0m1.953s
export NODE_BACKEND=js
export BCRYPTO_FORCE_BIGINT=1
time npm run test
real	1m11.606s
user	1m19.309s
sys	0m1.878s

@braydonf
Copy link
Contributor

braydonf commented May 24, 2019

There are a few duplicate packages:

  • bsert is duplicated in:
    • bevent
    • bdns
    • bdb
    • bcfg
    • bclient
  • bsock is duplicated in:
    • bcurl
  • brq is duplicated in:
    • bcurl

This can be easily resolved by running npm run dedupe. The versions are the same for bsert and bsock (brq isn't in package.json).

@braydonf
Copy link
Contributor

braydonf commented May 24, 2019

In regards to bcrypto:

There are minimal changes to bcrypto/src/secp256k1. As mentioned earlier, bcrypto/vendor/bn.js and bcrypto/vendor/elliptic have been moved to lib and rewritten. And bn now has a native implementation at bcrypto/src/bn.cc (not hooked in yet) in addition to bcrypto/lib/js/bn.js and bcrypto/lib/node/bn.js.

And for bsert:

The methods throws and rejects, and testError can likely replace some of the test specific assertions, can be another PR.

@braydonf braydonf merged commit 6d2aca6 into bcoin-org:master May 25, 2019
braydonf added a commit that referenced this pull request May 25, 2019
Dependency updates, support node v12
@braydonf braydonf removed the ready for review Ready to be reviewed label May 25, 2019
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.

3 participants