HDPublicKey: need better error handling when deriving child key

[pinheadmz]

https://github.com/handshake-org/hsd/blob/107ed2be9280bb4292d21330e7d1ace62da9a002/lib/hd/public.js#L162-L167

This code is here, I believe to satisfy this requirement of the BIP32 spec

In case parse256(IL) ≥ n or Ki is the point at infinity, the resulting key is invalid, and one should proceed with the next value for i.

However I just encountered an issue trying to derive RECEIVE branch (0) from an account, and the error that was swallowed there was actually: Error: Entropy source not available. which resulted in the code deriving the CHANGE branch (1) which for whatever reason did not encounter the same error.

I'm still trying to figure out what happened to the entropy source and why the second iteration succeeded at all - but for hsd, I think we need to be more careful about checking the error thrown by the crypto library before just incrementing the index and trying again.

[pinheadmz]

Found a crazy work-around: https://github.com/pinheadmz/hip2-server/commit/05ca2c2c55a73d3beb15282923f69da66ede1cfc

I call account.derive(0) twice. The first time it fails and I don't care, after that it works fine.

The entropy source is needed in the ECDSA precomputation here: https://github.com/bcoin-org/bcrypto/blob/a0e2635eb5bd50671703853391589c5bc428f51a/lib/js/elliptic.js#L1302-L1325

I still have no idea why it works after failing once. It could simply be that the way bpkg packages all the required modules, that the const HAS_CRYPTO is maybe not set at the correct time because something is not in the right order?

[Anunayj]

So, bcrypto's JS implementation assumes that all the cryptography will be running in a browser, which exposes the WebCrypto API.

Since bpkg here targets node, but forces the use of browser field in package.json, window.crypto is not available.

This works second time because here [1] bcrypto doesn't do precalculations again if this._curve is defined, no further errors show up later cause the code used here doesn't really depend on the functions that got left undefined. This is kinda bad in terms of error handling, but that's a issue for another day.

New versions of nodejs (15+) include the WebCrypto Api [2], but the current LTS (14) doesn't.

The best solution for now is using Polyfills for WebCrypto api when targeting node.js and wanting to use JS shims. (I don't know how to do this with bpkg)

• polyfill.js :

const { Crypto } = require("@peculiar/webcrypto"); const crypto = new Crypto(); global.crypto = crypto;

- webpack.config.js
```js
const path = require('path');

module.exports = {
  entry: ['./src/polyfills.js','./src/index.js'],
  target: 'node',
  mode: 'production',
  output: {
    filename: 'bundle',
    path: path.resolve(__dirname, 'build'),
  },
  resolve: {
    modules: ['node_modules'],
    extensions: ['-browser.js', '.js', '.json']
  },
};

Better longterm solution is making a PR to bcrypto and using NodeJS's require('crypto') properly, bundlers will replace it with a appropriate polyfill when targeting browsers.

[1]. https://github.com/bcoin-org/bcrypto/blob/b73dbc69884ecc790864e8ac513d627cdb0318d7/lib/js/ecdsa.js#L62-L66 [2]. https://developer.mozilla.org/en-US/docs/Web/API/Web_Crypto_API

[pinheadmz]

Thanks @Anunayj I think for hip2-server since I'm already doing something wrong (using browser bundling for nodejs app) the only real action needed is to fix the error handling in hsd. I think we should test the error from secp and only derive the next index if an invalid public key is derived (as specified in BIP32). Otherwise, any other error should be thrown.

Writing a test for this will be a real bitch!

(Note: this has probability lower than 1 in 2^127.)

UPDATE: we should probably just throw there instead of trying to catch. From IRC regarding this particular line of code:

sipa: Arguably, it doesn't matter, because it is computationally infeasible to even a create a case where it matters.

[Anunayj]

Actually I feel like we should also do a PR to bcrypto and remove it's dependency on the WebCrypto API (use require("crypto")), cause in it's current state the JS implementation is incomptible with nodejs <17