-
Notifications
You must be signed in to change notification settings - Fork 299
Conversation
The warning is related to the change in #108 - it's logged when encountering the memory issue described in this PR comment. |
My understanding is that the Ethereum ecosystem can't use the native PBKDF2 implementation because it was previously using a non-standard implementation. Is this still the case, @cgewecke ? |
@cgewecke Ah, thanks, first thought that CI failing here would also be due to the repeated warning, but CI runs through and the one test failing seems unrelated. Not sure if CI got slower after the merge of #108, there might be some tendency but also hard to read from the CI run times. Wonder if we should test this more thoroughly before we release. |
Would be proceeding with axic/scrypt.js#8 a more robust respectively better solution? Then I would try to knock at @axic 's door again on this. |
I think that suffers from the problem I described. When I created that PR I wasn't aware of it. |
@alcuadrado I am not certain...read through this stuff again and last week someone in the Node issue reported they've filed errata in the scrypt RFC, implying that the native impl. needs a fix. This looks like it's up in the air a little? There's a fix for the memory issue @michaelsbradleyjr was ambivalent about because of the RFC problem which may be viable again. (Will ping him on Gitter asking for advice) |
0fa10db
to
81ac212
Compare
Some updates: Node Tests Coveralls Failure So for now we just need to look over this. Browser Tests Firefox 74.0 (Ubuntu 0.0.0) .fromExtendedKey() should work with public FAILED
r._strip is not a function
ireduce@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:12471:9
imod@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:12643:39
mul@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:12709:17
sqr@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:12717:17
redSqr@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:4632:21
loadCompressedPublicKey@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:14939:13
loadPublicKey@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:14971:14
global.wrappers["/home/runner/work/ethereumjs-wallet/ethereumjs-wallet/node_modules/hdkey/node_modules/secp256k1/lib/elliptic/index.js"]</exports.publicKeyVerify@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:15042:10
publicKeyVerify@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:30494:24
set@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:30702:22
global.wrappers["/home/runner/work/ethereumjs-wallet/ethereumjs-wallet/node_modules/hdkey/lib/hdkey.js"]</HDKey.fromExtendedKey@/tmp/karma-typescript-bundle-3248fRnuQwxF00sj.js:30861:5
global.wrappers["/home/runner/work/ethereumjs-wallet/ethereumjs-wallet/src/hdkey.ts"]</EthereumHDKey</EthereumHDKey.fromExtendedKey@src/hdkey.ts:20:36 <- src/hdkey.js:41:52
global.wrappers["/home/runner/work/ethereumjs-wallet/ethereumjs-wallet/test/hdkey.spec.ts"]</</<@test/hdkey.spec.ts:39:34 <- test/hdkey.spec.js:27:38 Not sure yet if this is a caching problem or something more substantial. This likely has something to do with updated |
Ok, tests are still failing here. Some observations / further context: Before (when tests were passing) I just updated by After the complete reinstall with then failing browser tests 🤔 |
The MPrime.prototype.ireduce = function ireduce (num) {
// Assumes that `num` is less than `P^2`
// num = HI * (2 ^ N - K) + HI * K + LO = HI * K + LO (mod P)
var r = num;
var rlen;
do {
this.split(r, this.tmp);
r = this.imulK(r);
r = r.iadd(this.tmp);
rlen = r.bitLength();
} while (rlen > this.n);
var cmp = rlen < this.n ? -1 : r.ucmp(this.p);
if (cmp === 0) {
r.words[0] = 0;
r.length = 1;
} else if (cmp > 0) {
r.isub(this.p);
} else {
r._strip();
}
return r;
}; |
Running a test run to see if downgrading the BN.js version to a standalong v4.11.8 version will solve the issue at #122 |
Ah, this (downgrading BN.js) would actually solve our problem here. I would suggest that we consider merging this in and doing the release with this fix, eventually also depending how quickly it might be possible to integrate a fix on the BN.js side. But we generally can remove the dependency again along some subsequent patch release. |
@ryanio thanks for the thumbs up, also still think this is a viable way to go (merge #122 and continue here) but let's give this 2-3 days more and see how things/discussion evolve along ethereumjs/ethereumjs-util#250, would generally not so beautiful if we let the Util v7 library out in the wild and then eventually directly deprecate the version ourselves. |
…, some Buffer type assertions
…h new msg from updated secp2561 v4 library
81ac212
to
61487a1
Compare
Ok, this is now failing again sigh (non-deterministically, on random runs) but instead of giving the This is a strong indicator that this wasn't actually an interoperability issue but triggered by some other condition. Will try to give this some further investigation. |
Do you have any information in the non-standard PBKDF2 thing? I use node’s in node and my own adapted version in the browser, and have never had any problems. Are you thinking of the axic testcase? (If you test against the ethers testcases, it’s present there too)? That should’ve only been a problem for libraries using bitpay’s HD implementation... Or is there another issue? |
Ok, I got at least a bit further. The above is the moment before this errors. What was confusing at first was that there are actually only BN Following is the ethereumjs-wallet@0.6.3 [PATH]/ethereumjs-wallet
├─┬ ethereumjs-util@7.0.1
│ ├── bn.js@4.11.8
│ └─┬ rlp@2.2.4
│ └── bn.js@4.11.8 deduped
├─┬ ethers@4.0.47
│ ├── bn.js@4.11.8 deduped
│ └─┬ elliptic@6.5.2
│ └── bn.js@4.11.8 deduped
├─┬ hdkey@1.1.2
│ └─┬ secp256k1@3.8.0
│ └── bn.js@4.11.8 deduped
└─┬ karma-typescript@5.0.2
└─┬ crypto-browserify@3.12.0
├─┬ browserify-sign@4.1.0
│ ├── bn.js@5.1.1
│ ├─┬ browserify-rsa@4.0.1
│ │ └── bn.js@4.11.8 deduped
│ └─┬ parse-asn1@5.1.5
│ └─┬ asn1.js@4.10.1
│ └── bn.js@4.11.8 deduped
├─┬ create-ecdh@4.0.3
│ └── bn.js@4.11.8 deduped
├─┬ diffie-hellman@5.0.3
│ ├── bn.js@4.11.8 deduped
│ └─┬ miller-rabin@4.0.1
│ └── bn.js@4.11.8 deduped
└─┬ public-encrypt@4.0.3
└── bn.js@4.11.8 deduped So turns out the So good so far. |
Ah, here this actually happens (respectively one line before in ethereumjs-wallet@0.6.3 [PATH]/ethereumjs-wallet
├─┬ bs58check@2.1.2
│ └─┬ create-hash@1.2.0
│ └── ripemd160@2.0.2
└─┬ karma-typescript@5.0.2
└─┬ crypto-browserify@3.12.0
├─┬ create-hmac@1.1.7
│ └── ripemd160@2.0.2 deduped
└─┬ pbkdf2@3.0.17
└── ripemd160@2.0.2 deduped So this instantiates a BN |
This problem will likely only get bigger of time. So it might be worth to suggest two - in it's scope both pretty small - bug fix releases over on the BN side for both the v5 and v4 branch. so a simple: if (r.strip !== undefined) {
r.strip();
} else {
r._strip();
} would already do it for both versions. |
@holgerd77 great investigation, I agree that this problem may only get worse as some libs start upgrading to v5 and others stay on v4. Instead of checking for i.e. at https://github.com/indutny/bn.js/blob/master/lib/bn.js#L3059: - var r = num
+ var r = new BN(num) edit: regarding the karma failure, how weird is it that this exact branch running on my fork passes fine? ryanio#1 |
…eed for karma-typescript-es6-transform)
@ricmoo Sorry, read over your comment above at first: this is another issue with problems on the interoperability of BN.js |
@ryanio I couldn't imagine how this goes so I made a test: $ node
Welcome to Node.js v12.15.0.
Type ".help" for more information.
> const BN4 = require('BN.js')
undefined
> const BN5 = require('ethereumjs-util').BN
undefined
> let num4 = new BN4(3)
undefined
> num4.strip
[Function: strip]
> num4._strip
undefined
> let num5 = new BN5(4)
undefined
> num5.strip
undefined
> num5._strip
[Function: strip]
> let w5num4 = new BN5(num4)
undefined
> w5num4.strip
[Function: strip]
> w5num4._strip
undefined This doesn't seem to work, the wrapped |
@ryanio I've now provided a summary of our findings here over on the BN.js issue and suggested to apply two fixes (now took the conditional versions) for BN.js v4 and v5. I would suggest that me move on here independently and continue on the release. For now I have removed the three failing browser test runs together with a comment with the Karma check you already applied on the VM. So this is now ready for review. Phew. 😄 |
ee9dc74
to
073319c
Compare
Update: there were releases with fixes today on the BN.js side for both v4 and v5 (thanks @fanatid), so I re-added the browser tests again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, that was fast! LGTM!
@ryanio What a journey. 😄 |
@alcuadrado having problems running the test locally (on Node
v12.15.0
) getting"Memory limit exceeded for Node's built-in crypto.scrypt, falling back to scryptsy (times: 1), if this happens frequently you can improve the performance of scrypt when running Node.js versions older than 12.0.0 by installing the (deprecated) scrypt package in your project"
This occurs after the
.toV3() ✓ should work with PBKDF2 (2463ms)
test execution.
Any idea what this might hint to? I made a clean install with
package-lock.json
deleted.This does occur along the comparison with Ethers wallet results, so might also be some problem on the integrated Ethers version? //cc @ricmoo