WIP: Remove scrypt.js dependency (help wanted) #91

Leibniz137 · 2019-06-16T20:07:32Z

I'm trying to remove scrypt.js, which is no longer maintained as a dependency. The primary reason is that scrypt.js isn't compatible with node12. A secondary benefit is that we can remove a dependency.

Deprecation

scrypt.js isn't compatible with node12, see:

Testing errors

When testing these changes, I'm hitting out of memory errors:


> ethereumjs-wallet@0.6.3 test /Users/nathaniel/src/ethereumjs/ethereumjs-wallet
> mocha ./src/test/*.js



  .fromMasterSeed()
    ✓ should work

  .privateExtendedKey()
    ✓ should work

  .publicExtendedKey()
    ✓ should work

  .fromExtendedKey()
    ✓ should work with public
    ✓ should work with private

  .deriveChild()
    ✓ should work

  .derivePath()
    ✓ should work with m
    ✓ should work with m/44'/0'/0/1

  .getWallet()
    ✓ should work
    ✓ should work with public nodes

  .getPrivateKey()
    ✓ should work
    ✓ should fail

  .getPrivateKeyString()
    ✓ should work

  .getPublicKey()
    ✓ should work

  .getPublicKeyString()
    ✓ should work

  .getAddress()
    ✓ should work

  .getAddressString()
    ✓ should work

  .getChecksumAddressString()
    ✓ should work

  public key only wallet
    ✓ .fromPublicKey() should work
    ✓ .fromPublicKey() should not accept compressed keys in strict mode
    ✓ .fromPublicKey() should accept compressed keys in non-strict mode
    ✓ .getAddress() should work
    ✓ .getPrivateKey() should fail
    ✓ .toV3() should fail

  .fromExtendedPrivateKey()
    ✓ should work

  .fromExtendedPublicKey()
    ✓ should work

  .generate()
    ✓ should generate an account
    ✓ should generate an account compatible with ICAP Direct

  .generateVanityAddress()
    ✓ should generate an account with 000 prefix (object) (257ms)
    ✓ should generate an account with 000 prefix (string) (41ms)

  .getV3Filename()
    ✓ should work

  .toV3()
    ✓ should work with PBKDF2 (119ms)
    1) should work with Scrypt
    2) should work without providing options
    ✓ should fail for unsupported kdf

  .fromV3()
    ✓ should work with PBKDF2 (122ms)
    3) should work with Scrypt
    4) should work with 'unencrypted' wallets
    ✓ should fail with invalid password (121ms)
    ✓ should work with (broken) mixed-case input files (119ms)
    ✓ shouldn't work with (broken) mixed-case input files in strict mode
    ✓ should fail for wrong version
    ✓ should fail for wrong kdf
    ✓ should fail for wrong prf in pbkdf2

  .fromEthSale()
    ✓ should work with short password (8 characters)
    ✓ should work with long password (19 characters)
    ✓ should work with pyethrecover's wallet

  .fromEtherWallet()
    ✓ should work with unencrypted input
    ✓ should work with encrypted input

  .fromEtherCamp()
    ✓ should work with seed text

  .fromKryptoKit()
    ✓ should work with basic input (d-type)
    ✓ should work with encrypted input (q-type) (50ms)

  .fromQuorumWallet()
    ✓ should work

  raw new Wallet() init
    ✓ should fail when both priv and pub key provided


  50 passing (872ms)
  4 failing

  1) .toV3()
       should work with Scrypt:
     Error: error:060B50AC:digital envelope routines:EVP_PBE_scrypt:memory limit exceeded
      at handleError (internal/crypto/scrypt.js:62:14)
      at Object.scrypt (internal/crypto/scrypt.js:47:3)
      at Wallet.toV3 (src/index.js:132:25)
      at Wallet.toV3String (src/index.js:189:30)
      at Context.<anonymous> (src/test/index.js:158:38)
      at processImmediate (internal/timers.js:439:21)

  2) .toV3()
       should work without providing options:
     Error: error:060B50AC:digital envelope routines:EVP_PBE_scrypt:memory limit exceeded
      at handleError (internal/crypto/scrypt.js:62:14)
      at Object.scrypt (internal/crypto/scrypt.js:47:3)
      at Wallet.toV3 (src/index.js:132:25)
      at Context.<anonymous> (src/test/index.js:162:38)
      at processImmediate (internal/timers.js:439:21)

  3) .fromV3()
       should work with Scrypt:
     Error: error:060B50AC:digital envelope routines:EVP_PBE_scrypt:memory limit exceeded
      at handleError (internal/crypto/scrypt.js:62:14)
      at Object.scrypt (internal/crypto/scrypt.js:47:3)
      at Function.Wallet.fromV3 (src/index.js:264:25)
      at Context.<anonymous> (src/test/index.js:190:25)
      at processImmediate (internal/timers.js:439:21)

  4) .fromV3()
       should work with 'unencrypted' wallets:
     Error: error:060B50AC:digital envelope routines:EVP_PBE_scrypt:memory limit exceeded
      at handleError (internal/crypto/scrypt.js:62:14)
      at Object.scrypt (internal/crypto/scrypt.js:47:3)
      at Function.Wallet.fromV3 (src/index.js:264:25)
      at Context.<anonymous> (src/test/index.js:196:25)
      at processImmediate (internal/timers.js:439:21)



npm ERR! Test failed.  See above for more details.

For crypto.scrypt and scrypto.scryptSync, there is a maxmem optional argument that can be passed. I tried a couple different values but keep hitting "memory limit exceeded" errors. I'm not sure what the memory limit should be.

callback?

One thing I noticed when reading the code was that was a mention in the comments about using a callback.

Note that the asynchonous version of the crypto.scrypt function requires a callback be passed.
I don't know what the definition of the callback would be if we passed it. Suggestions welcome!

crypto.scrypt vs crypto.scryptSync?

Should these function calls be synchronous or async? I don't know what the callback defs should be, so I'm using the synchronous version at this point. But if the callback question is answered, perhaps we can switch to the asynch version?

cryptographic gotchas/concerns?

I'm not a cryptographer and I don't have a good understanding of these libraries. Obviously before merging someone with some background in cryptography should OK these changes.

holgerd77 · 2019-06-22T06:21:13Z

We are currently in the discussion if we move over the scrypt.js repository over to EthereumJS and maintain from here, maybe wait with this PR.

Updating for Node 12 compatibility of the library shouldn't be such a thing (I suppose).

//cc @axic

michaelsbradleyjr · 2019-07-17T18:52:42Z

A shorter-term option could be to implement a shim like this from PR #2938 for web3.js:

https://github.com/michaelsbradleyjr/web3.js/blob/1.x-node-12-support/packages/web3-eth-accounts/src/scrypt.js

See also #2952.

The warning message would just need ethereumjs-wallet swapped in for web3; also scrypt.js would be dropped as a dependency of ethereumjs-wallet and in its place would be:

"semver": "6.2.0",
"scryptsy": "2.1.0",

I'm happy to put up a PR, just let me know.

I don't think there's an easy way around the memory exhaustion problem with Node's built-in crypto.scrypt (available since 10.5.0). Even if the maxmem option is set to its absolute max, it still throws when n is large enough, though the deprecated scrypt package doesn't have a problem with the same n and the pure-JS implementations work with it too. As far as I can tell, the Node.js team made different decisions regarding maxmem; it might be worthwhile opening an issue with them to see if it would be possible for common large n values that work with the scrypt package to be made usable with the built-in.

Longer-term, I think having a unified crypto-shims package for the community would helpful, as this technique can also be applied to secp256k1. Things are trickier with keccak since there is no Node.js built-in. Besides shims, it should be possible for a foundation to pull some packages "in house" and outfit them with proper pre-built binaries that can be downloaded during npm install, provide updates related to Node's ABI changes across versions, etc. (cc @nivida).

MicahZoltu · 2019-07-21T06:46:25Z

Naive question: Is there a TL;DR someone can provide for why an Ethereum library needs scrypt in the first place? Normally it is used as a PoW algorithm, so I'm not sure what purpose it serves in an Ethereum library.

Generally speaking, I hold a burning hatred for any library that has a node-gyp dependency as it causes no end of dev-time and build-time problems, especially when it is deep in a transitive dependency that you cannot easily change (like this one is in many projects).

michaelsbradleyjr · 2019-07-21T17:19:19Z

@MicahZoltu the scrypt function is used to derive the private key when unencrypting a wallet, and used also used when generating an encrypted wallet from one that's not encrypted.

michaelsbradleyjr · 2019-07-21T17:22:51Z

A few days ago I filed an issue with the Node.js maintainers: nodejs/node#28755.

The initial response seems positive.

michaelsbradleyjr · 2019-07-21T20:19:19Z

An increased maxmem will be part of an upcoming Node.js release 🎉 : nodejs/node#28799.

gjgd · 2019-11-14T23:56:46Z

Hi! What's the status on removing the scrypt.js dep? Any way I can help move this forward?

scmilee · 2019-12-11T21:28:53Z

Has there been any further discussion surrounding either moving scrypt into this repo or removing it altogether?

dmihal · 2020-03-23T12:59:02Z

I'm running into this issue as well, would love to help get this resolved

dmihal · 2020-03-26T20:30:22Z

I created #108, which uses the same package used in Web3.js

If anyone needs to use this package right now on Node 12, you can use @dmihal/ethereumjs-wallet

dmihal · 2020-04-20T15:58:42Z

This can be closed now that #108 has been merged

Leibniz137 added 5 commits June 16, 2019 09:46

implement naive removal of scrypt module

c0526cf

update crypto.scrypt calls to include dummy callback

45c5098

increment standard patch version to 12.0.1

cdc1875

apply standard --fix

3c9bfa2

use scryptSync instead scrypt to remove the need to pass callback

a13f398

This was referenced Jul 17, 2019

remove scrypt.js dependency for node 12, use native node 12 instead. #92

Closed

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

Merged

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

Merged

gjgd mentioned this pull request Nov 14, 2019

Not usable on Node.js 12 decentralized-identity/element#23

Open

holgerd77 closed this Apr 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Remove scrypt.js dependency (help wanted) #91

WIP: Remove scrypt.js dependency (help wanted) #91

Leibniz137 commented Jun 16, 2019

holgerd77 commented Jun 22, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 17, 2019 •

edited

Loading

MicahZoltu commented Jul 21, 2019

michaelsbradleyjr commented Jul 21, 2019

michaelsbradleyjr commented Jul 21, 2019

michaelsbradleyjr commented Jul 21, 2019 •

edited

Loading

gjgd commented Nov 14, 2019

scmilee commented Dec 11, 2019

dmihal commented Mar 23, 2020

dmihal commented Mar 26, 2020

dmihal commented Apr 20, 2020

WIP: Remove scrypt.js dependency (help wanted) #91

WIP: Remove scrypt.js dependency (help wanted) #91

Conversation

Leibniz137 commented Jun 16, 2019

Deprecation

Testing errors

callback?

crypto.scrypt vs crypto.scryptSync?

cryptographic gotchas/concerns?

holgerd77 commented Jun 22, 2019 • edited Loading

michaelsbradleyjr commented Jul 17, 2019 • edited Loading

MicahZoltu commented Jul 21, 2019

michaelsbradleyjr commented Jul 21, 2019

michaelsbradleyjr commented Jul 21, 2019

michaelsbradleyjr commented Jul 21, 2019 • edited Loading

gjgd commented Nov 14, 2019

scmilee commented Dec 11, 2019

dmihal commented Mar 23, 2020

dmihal commented Mar 26, 2020

dmihal commented Apr 20, 2020

holgerd77 commented Jun 22, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 17, 2019 •

edited

Loading

michaelsbradleyjr commented Jul 21, 2019 •

edited

Loading