Node >= v8.0.0 support for web3 1.0 #2938

michaelsbradleyjr · 2019-07-12T01:02:50Z

Helps with #2913.

This PR is presently incomplete because it was necessary to disable two test scripts after removing ethereumjs-wallet as a development dependency, since that package has the Node 12 incompatible scrypt package as a transitive dependency.

Gaining support for Node versions from 8 to 12 inclusive seems like a very good thing, so maybe there is another testing strategy that can be used to restore the test files without needing to install ethereumjs-wallet?

coveralls · 2019-07-12T01:06:47Z

Coverage decreased (-0.04%) to 83.03% when pulling d7adf70 on michaelsbradleyjr:1.x-node-12-support into 54feed8 on ethereum:1.x.

michaelsbradleyjr · 2019-07-12T03:06:59Z

I'm attempting to swap ethers for ethereumjs-wallet, so as to resurrect the test suites I disabled, but have to stop for now. It's mostly going well but I've run into a few problems re: fine-points of scrypt options; I hope to get them sorted out tomorrow.

WIP branch that would be merged back into this PR's branch:

https://github.com/michaelsbradleyjr/web3.js/blob/1.x-node-12-support-WIP/test/eth.accounts.encrypt-decrypt.js

The tests that are skipped (.skip) need to be fixed and not-skipped. But even with that changeset there are still errors when running npm test in the root... something to do with scrypt option parameters, I think.

levino

Imo this can wait until 1.0 has been released.

michaelsbradleyjr · 2019-07-13T01:30:01Z

The base branch for this PR will be changed once #2905 has been merged (or it can be closed and a new PR opened, whatever is preferable). It turns out there are some subtle but non-trivial behavioral differences between the scrypt package and Node's built-in crypto.scrypt(), so these changes should definitely not land in web3 1.0.0.

michaelsbradleyjr · 2019-07-16T22:51:54Z

The test suite has been fully revised to use the ethers package instead of ethereumjs-wallet.

Also, a bug was fixed that prevented web3 accounts from being decrypted if the user provides their own salt parameter — previously, the salt was not consistently converted to a Buffer during encryption.

michaelsbradleyjr · 2019-07-16T22:53:46Z

I introduced an additional fallback behavior and console warning in the case that Node's built-in scrypt experiences memory exhaustion, as was happening in the test suite with the static tests. A large n parameter is the culprit, but that's not the case when using the 3rd party (deprecated) scrypt package; also scryptsy can handle the large n but of course performance suffers considerably.

This commit introduces an additional fallback behavior and console warning in the case that Node's built-in scrypt experiences memory exhaustion, as was discovered in the 1.x test suite with the static tests (see web3#2938). A large `n` parameter is the culprit, but that's not the case when using the 3rd party (deprecated) scrypt package. The `scryptsy` package can handle a large `n`, but performance does suffer considerably.

Previously, if a `salt` option was supplied as a string it would be passed to `pbkdf2Sync`/`scrypt` as a string, but during decryption it was always converted to a Buffer instance such that supplying a `salt` option as a string resulted in output that could not be decrypted. This commit fixes the bug, and also makes encrypted output match up with the output of other wallet libraries, e.g. `ethers`, whenever the equivalent encryption options are used consistently across libraries. See the following `web3.js` PRs: [#2950][2950] and [#2938][2938] [2950]: web3/web3.js#2950 [2938]: web3/web3.js#2938

michaelsbradleyjr · 2019-07-17T19:01:44Z

See also: ethereumjs/ethereumjs-wallet#95.

michaelsbradleyjr · 2019-07-17T19:02:56Z

See also: ethereumjs/ethereumjs-wallet#91 (comment).

Helps with web3#2913

michaelsbradleyjr · 2019-07-23T14:46:25Z

I've rebased this PR against 1.x following the release of 1.2.0 and also changed the base of the PR to 1.x.

levino · 2019-07-25T12:45:41Z

test/eth.accounts.encrypt-decrypt.js

-                // increase the test timeout
-                this.timeout(6000);
+                // disable the test timeout
+                this.timeout(0);


This should be removed not set to 0.

Setting to 0 is the way to say "no timeout" in mocha: https://mochajs.org/#suite-level.

Some of the static tests have large n values: https://github.com/michaelsbradleyjr/web3.js/blob/d7adf702f815537376e7b8adb8fbdacfece17435/test/eth.accounts.encrypt-decrypt.js#L32.

In the case that Node.js falls back to scryptsy, which will be the case in CI for Node <10.5.0 (since no scrypt) and also >=10.5.0 because of maxmem errors with large n (until PR #28799 makes it into releases of 10.x/12.x), then those tests will be prone to timeouts (scryptsy is much, much slower for large n). So for these particular tests, it makes sense to disable the timeout.

levino · 2019-07-25T12:48:19Z

package.json

@@ -77,7 +77,7 @@
        "coveralls": "^3.0.4",
        "crypto-js": "^3.1.9-1",
        "del": "^4.1.1",
-        "ethereumjs-wallet": "^0.6.3",
+        "ethers": "4.0.33",


Putting quite some trust into the devs of ether.js. Also why is this version pinned here? The pinning should happen via yarn.lock or package-lock.json files. I think ^4.0.33 would be more appropriate.

It's just a devDep, used to test that web3's wallet matches up with an independent implementation. ethereumjs-wallet was serving that purpose but it also depends on scrypt.js so npm i in the repo still resulted in scrypt package being installed and compiled.

As for pinning vs. ^, I've been bitten so many times by 3rd party patch bumps that violate semver (i.e. have breaking changes) and/or bugs that I spec exact version deps without thinking about it. However, using ^ would be more consistent with what's spec'd for the other devDeps.

levino · 2019-07-25T12:48:52Z

packages/web3-eth-accounts/src/index.js

@@ -307,7 +307,7 @@ Accounts.prototype.decrypt = function (v3Keystore, password, nonStrict) {
        kdfparams = json.crypto.kdfparams;

        // FIXME: support progress reporting callback


What about this "FIXME" comment? Is it still valid?

I think it can be removed since there's not much point in trying to make use of the progress callback of scryptsy and Node's built-in doesn't have such a thing.

levino · 2019-07-25T12:50:18Z

packages/web3-eth-accounts/src/index.js

@@ -351,13 +351,13 @@ Accounts.prototype.encrypt = function (privateKey, password, options) {
    if (kdf === 'pbkdf2') {
        kdfparams.c = options.c || 262144;
        kdfparams.prf = 'hmac-sha256';
-        derivedKey = cryp.pbkdf2Sync(Buffer.from(password), salt, kdfparams.c, kdfparams.dklen, 'sha256');
+        derivedKey = cryp.pbkdf2Sync(Buffer.from(password), Buffer.from(kdfparams.salt, 'hex'), kdfparams.c, kdfparams.dklen, 'sha256');


Quite confusing that we are not using salt any more but kdfparams.salt all of a sudden. When I read the code above it should be the same, but still confusing.

See above:

https://github.com/michaelsbradleyjr/web3.js/blob/d7adf702f815537376e7b8adb8fbdacfece17435/packages/web3-eth-accounts/src/index.js#L348

var kdfparams = { dklen: options.dklen || 32, salt: salt.toString('hex') };

That code was already there and normalized the salt whether it was a string or buffer. The problem was we weren't using the normalized value and it resulted in encrypted output that couldn't be decrypted. ethereumjs-wallet has the same bug and I made a PR to fix it (still open).

levino · 2019-07-25T12:51:10Z

packages/web3-eth-accounts/src/scrypt.js

+if (isNode) {
+    var NODE_MIN_VER_WITH_BUILTIN_SCRYPT = '10.5.0';
+    var NODE_MIN_VER_INCOMPAT_SCRYPT_PKG = '12.0.0';
+    var semver = require('semver');


That is quite painful in webpack I fear... One can have webpack and node (e.g. lambda functions).

Can you explain a bit more? If this broke something in a node FaaS context, I will be glad to work on a fix.

I am just not sure whether or not webpack correctly supports this dynamic require. Needs to be tested.

It doesn't. Webpack fails at dynamic require at [email protected]. Any idea how to workaround it?

I've seen the warning too, but it didn't cause my build to fail, and my build worked as expected. Are you sure you actually got a failed build?

In any case, the web3-eth-accounts module should be refactored to make use of "browser" in package.json instead of using runtime isNode detection. I'll work on that very soon.

As I said in my previous comment, this should be refactored. But in the meantime I don't think the warning will result in broken builds. Here's what I tried:

$ cd ~/temp $ npx create-react-app foo $ cd foo && yarn add web3

After that I edited src/App.js to import Web3 from 'web3' and I set window.Web3 = Web3. Then:

$ yarn start

Once the dev server was available at localhost:3000 I was able to use window.Web3 without any problems, though I did see the webpack warning in the console output of yarn start.

I checked that I could use web3.eth.accounts to create/encrypt/decrypt — worked as expected, so while the webpack warning isn't nice, it doesn't seem to prevent browser builds (w/ webpack) from working.

I'm not 100% sure about webpack builds targeting Node.js; but shouldn't be a problem because such builds couldn't pack up the scrypt package in any case (it's a compiled C++ thing).

Hey @michaelsbradleyjr you are absolutely right. I had other issue and critical dependency got me triggered. Thanks for detailed answer and check. Really appreciated.

0mkara · 2019-08-08T06:28:51Z

Lots of love that this feature got merged. ❤️

michaelsbradleyjr mentioned this pull request Jul 12, 2019

Release - 1.0 #2905

Merged

11 tasks

levino suggested changes Jul 12, 2019

View reviewed changes

nivida added 1.x 1.0 related issues Enhancement Includes improvements or optimizations In Progress Currently being worked on labels Jul 12, 2019

nivida mentioned this pull request Jul 12, 2019

Fixes deprecation of scrypt.js on >10.x node versions #2903

Closed

michaelsbradleyjr mentioned this pull request Jul 12, 2019

Release - 2.0.0-alpha #2906

Merged

13 tasks

0mkara mentioned this pull request Jul 16, 2019

nodejs >=v8.0.0 support 7finney/ethcode#25

Closed

michaelsbradleyjr force-pushed the 1.x-node-12-support branch 4 times, most recently from 4098ba3 to 8ef305e Compare July 16, 2019 22:43

michaelsbradleyjr force-pushed the 1.x-node-12-support branch 2 times, most recently from 217965e to 1510c53 Compare July 17, 2019 01:46

michaelsbradleyjr mentioned this pull request Jul 17, 2019

implement fallback from built-in scrypt to scryptsy/scrypt packages #2952

Merged

michaelsbradleyjr force-pushed the 1.x-node-12-support branch from 1510c53 to ede0b5a Compare July 17, 2019 02:03

This was referenced Jul 17, 2019

Normalize salt, iv, uuid params of .toV3() before encrypting ethereumjs/ethereumjs-wallet#95

Merged

WIP: Remove scrypt.js dependency (help wanted) ethereumjs/ethereumjs-wallet#91

Closed

michaelsbradleyjr changed the base branch from release/1.0 to 1.x July 23, 2019 14:17

Node >= v8.0.0 support for web3 1.0

d7adf70

Helps with web3#2913

michaelsbradleyjr force-pushed the 1.x-node-12-support branch from ede0b5a to d7adf70 Compare July 23, 2019 14:45

nivida approved these changes Jul 25, 2019

View reviewed changes

nivida removed the In Progress Currently being worked on label Jul 25, 2019

nivida merged commit bb65e56 into web3:1.x Jul 25, 2019

levino reviewed Jul 25, 2019

View reviewed changes

michaelsbradleyjr mentioned this pull request Jul 25, 2019

Node 12 support #2913

Closed

This was referenced Aug 2, 2019

Installation Error: make failed with exit code: 2 #2992

Closed

web3.eth.accounts.decrypt() Error #2996

Closed

Release - 1.2.1 #3004

Merged

Release - 2.0.0-alpha.1 #3006

Merged

This was referenced Aug 14, 2019

webpack scrypt.js: Critical dependency: the request of a dependency is an expression #3018

Closed

Avoid using IIFE to require scrypt #3026

Closed

michaelsbradleyjr deleted the 1.x-node-12-support branch August 15, 2019 18:27

schmidsi mentioned this pull request Aug 22, 2019

Upgrade web3.js for latest node.js (<11) support aragon/radspec#76

Merged

agbilotia1998 mentioned this pull request Jan 11, 2020

support "yarn global add leap-node" with node -v >= 12 leapdao/leap-node#346

Closed

snyk-bot mentioned this pull request Feb 4, 2020

[Snyk] Upgrade web3 from 1.0.0-beta.35 to 1.2.4 shiplco/shipl-sdk#6

Open

snyk-bot mentioned this pull request Mar 16, 2020

[Snyk] Upgrade web3 from 1.0.0-beta.34 to 1.2.6 shahinalzahrani/smart-contracts#1

Open

This was referenced Apr 9, 2020

[Snyk] Upgrade web3 from 1.0.0-beta.37 to 1.2.6 lshicks13/starNotaryv2#2

Open

[Snyk] Upgrade web3 from 1.0.0-beta.37 to 1.2.6 lshicks13/P2v2-CryptoStarDapp#5

Open

[Snyk] Upgrade web3 from 1.0.0-beta.37 to 1.2.6 lshicks13/starNotaryv1#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node >= v8.0.0 support for web3 1.0 #2938

Node >= v8.0.0 support for web3 1.0 #2938

michaelsbradleyjr commented Jul 12, 2019 •

edited

Loading

coveralls commented Jul 12, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 12, 2019 •

edited

Loading

levino left a comment

michaelsbradleyjr commented Jul 13, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 16, 2019

michaelsbradleyjr commented Jul 16, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 17, 2019

michaelsbradleyjr commented Jul 17, 2019

michaelsbradleyjr commented Jul 23, 2019

levino Jul 25, 2019

michaelsbradleyjr Jul 25, 2019 •

edited

Loading

levino Jul 25, 2019

michaelsbradleyjr Jul 25, 2019

michaelsbradleyjr Jul 25, 2019 •

edited

Loading

levino Jul 25, 2019

michaelsbradleyjr Jul 25, 2019

levino Jul 25, 2019

michaelsbradleyjr Jul 25, 2019

levino Jul 25, 2019

michaelsbradleyjr Jul 25, 2019

levino Jul 25, 2019

ylv-io Aug 7, 2019

michaelsbradleyjr Aug 7, 2019

michaelsbradleyjr Aug 7, 2019 •

edited

Loading

ylv-io Aug 7, 2019

0mkara commented Aug 8, 2019

		@@ -307,7 +307,7 @@ Accounts.prototype.decrypt = function (v3Keystore, password, nonStrict) {
		kdfparams = json.crypto.kdfparams;

		// FIXME: support progress reporting callback

Node >= v8.0.0 support for web3 1.0 #2938

Node >= v8.0.0 support for web3 1.0 #2938

Conversation

michaelsbradleyjr commented Jul 12, 2019 • edited Loading

coveralls commented Jul 12, 2019 • edited Loading

michaelsbradleyjr commented Jul 12, 2019 • edited Loading

levino left a comment

Choose a reason for hiding this comment

michaelsbradleyjr commented Jul 13, 2019 • edited Loading

michaelsbradleyjr commented Jul 16, 2019

michaelsbradleyjr commented Jul 16, 2019 • edited Loading

michaelsbradleyjr commented Jul 17, 2019

michaelsbradleyjr commented Jul 17, 2019

michaelsbradleyjr commented Jul 23, 2019

Choose a reason for hiding this comment

michaelsbradleyjr Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsbradleyjr Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsbradleyjr Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0mkara commented Aug 8, 2019

michaelsbradleyjr commented Jul 12, 2019 •

edited

Loading

coveralls commented Jul 12, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 12, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 13, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 16, 2019 •

edited

Loading

michaelsbradleyjr Jul 25, 2019 •

edited

Loading

michaelsbradleyjr Jul 25, 2019 •

edited

Loading

michaelsbradleyjr Aug 7, 2019 •

edited

Loading