feat: upgrade crypto from bcrypto to noble

[acolytec3]

• Replaces the bcrypto dependency with a combination of @noble/hashes and @noble/secp256k1 (both are audited, modern, JS only ESM modules with no external dependencies)
• Removes bcrypto types
• Adds lint:fix script because eslint is your friend
• Selectively disabled eslint rules where necessary (mainly in tests where typescript would otherwise complain)

[mpetrunic]

I would very much love to drop bcrypto everywhere 😄 but curious to hear from @dapplion and @wemeetagain

[mpetrunic]

Use this api https://nodejs.org/api/buffer.html#static-method-bufferfromarraybuffer-byteoffset-length so that we don't allocate new array but instead just create Buffer view

[paulmillr]

still aes-gcm here instead of aes-ctr

[acolytec3]

Ah, thanks. That's an unused utility but will fix as well.

[acolytec3]

Actually, it's only aes-ctr in the unauthenticated bits of the handshake and then switches to aes-gcm for authenticated sessions per here

[acolytec3]

Okay, the cipher piece is fixed. Will look at some benchmarks later today. @dapplion are you thinking mainly encode/decode packets and then sign/verify since that's where the changes in this PR lie?

[acolytec3]

I added a benchmark script that uses dapplion/benchmark and I must say, the results are somewhat discouraging with regard to sign/verify. I can't say I'm surprised since @noble/secp256k1 is pure Javascript and bcrypto is using native C++ code in NodeJS where these tests run.

With Noble Crypto
  ✔ benchmark aes cipher encryption/decryptions                         23322.53 ops/s    42.87700 us/op        -       2322 runs  0.101 s
  ✔ benchmark sign/verify                                               424.5457 ops/s    2.355459 ms/op        -        512 runs   1.21 s
  ✔ benchmark key derivation                                            51939.96 ops/s    19.25300 us/op        -       5159 runs  0.101 s
  ✔ benchmark secp256k1.generatePrivateKey                              2896.779 ops/s    345.2110 us/op        -        512 runs  0.177 s
  ✔ benchmark keccak hash                                               64926.63 ops/s    15.40200 us/op        -       6447 runs  0.101 s
  ✔ benchmark sha256 hash                                               199481.3 ops/s    5.013000 us/op        -      19205 runs  0.101 s

  With bcrypto
  ✔ benchmark aes cipher encryption/decryptions                         21194.07 ops/s    47.18300 us/op        -       2137 runs  0.102 s
  ✔ benchmark sign/verify                                               6003.590 ops/s    166.5670 us/op        -        604 runs  0.101 s
  ✔ benchmark key derivation                                            125172.1 ops/s    7.989000 us/op        -      12270 runs  0.101 s
  ✔ benchmark secp256k1.generatePrivateKey                              21169.85 ops/s    47.23700 us/op        -       2117 runs  0.101 s
  ✔ benchmark keccak hash                                               533333.3 ops/s    1.875000 us/op        -      48412 runs  0.102 s
  ✔ benchmark sha256 hash                                               291120.8 ops/s    3.435000 us/op        -      27857 runs  0.101 s

[paulmillr]

The pure results on M1 are as such:

generatePrivateKey() x 265,957 ops/sec @ 3μs/op
getPublicKey(generatePrivateKey()) x 6,300 ops/sec @ 158μs/op
sign x 4,888 ops/sec @ 204μs/op
verify x 950 ops/sec @ 1ms/op

Your benchmark also benchmarks other stuff, like Buffer.from(hex).

[acolytec3]

The pure results on M1 are as such:

generatePrivateKey() x 265,957 ops/sec @ 3μs/op
getPublicKey(generatePrivateKey()) x 6,300 ops/sec @ 158μs/op
sign x 4,888 ops/sec @ 204μs/op
verify x 950 ops/sec @ 1ms/op

Your benchmark also benchmarks other stuff, like Buffer.from(hex).

Yup, I'll leave it to @dapplion and @wemeetagain to decide whether to proceed. The only other optimization we can do would be to remove Buffer I think and I'm not sure it'll give that much of an improvement.

[paulmillr]

yeah, i'm just saying that even without buffers, your abstraction layer may be too thick, because verification is ~1k ops/sec, not ~500, so it's useful to compare those raw results; they are totally achievable. Some thinking needs to be applied to what are the bottlenecks and at which process stages.

[wemeetagain]

Haven't had a chance to dig into the benchmarks, but one thing we may be able to do re: the slowdown is make the crypto "pluggable", like we did with noise ChainSafe/js-libp2p-noise#133

Then, for node applications that require highest possible performance, bcrypto or other native implementation can be passed in as configuration. And for browser applications, noble can be passed in.

[acolytec3]

Haven't had a chance to dig into the benchmarks, but one thing we may be able to do re: the slowdown is make the crypto "pluggable", like we did with noise ChainSafe/js-libp2p-noise#133

Then, for node applications that require highest possible performance, bcrypto or other native implementation can be passed in as configuration. And for browser applications, noble can be passed in.

Hmm, that's an interesting idea, also how we did for the transport layer. I'm game to rework things down that route, though we'll have to think through the best approach given that it's really 4 different underlying dependencies (secp256k1, sha256, keccak, aes ciphers). To my way of thinking, the aes ciphers is the least challenging one since I just subbed in the node crypto library implementations which seem to be actually slightly faster based on the couple of benchmarks I ran. And, those are simple to polyfill in browser.

[wemeetagain]

have to think through the best approach given that it's really 4 different underlying dependencies

Good point! I'm hoping to avoid spreading this to the ENR layer (I'm hoping to pull it out into its own package at some point)

The way it was done in noise was to add some interface (in noise called ICryptoInterface) that looks like:

export interface ICryptoInterface {
  hashSHA256: (data: Uint8Array) => Uint8Array
  ...
}

Then something that fulfills that interface gets passed in when the main object is constructed.

I guess some decisions could be made about which functions are synchronous vs asynchronous. And which crypto methods need / should be pluggable.

[acolytec3]

have to think through the best approach given that it's really 4 different underlying dependencies

Good point! I'm hoping to avoid spreading this to the ENR layer (I'm hoping to pull it out into its own package at some point)

The way it was done in noise was to add some interface (in noise called ICryptoInterface) that looks like:

export interface ICryptoInterface {
  hashSHA256: (data: Uint8Array) => Uint8Array
  ...
}

Then something that fulfills that interface gets passed in when the main object is constructed.

I guess some decisions could be made about which functions are synchronous vs asynchronous. And which crypto methods need / should be pluggable.

Make sense. I've already half-way done that by consolidating the secp export in the util directory. Shouldn't be too much of a lift to take it one step further to a full interface.

Would you be good with just letting the aes ciphers stand on their own using the node standard library dependencies? That would simplify things (and I think we can remove those aesCtr helpers unless you want to use them somewhere).

[paulmillr]

For aes, we're using this approach in EF repo: https://github.com/ethereum/js-ethereum-cryptography/blob/master/src/aes.ts

It uses node or web browser native built-ins. I'm sure there is zero need in polyfilling AES when web browsers support it.

[acolytec3]

For aes, we're using this approach in EF repo: https://github.com/ethereum/js-ethereum-cryptography/blob/master/src/aes.ts

It uses node or web browser native built-ins. I'm sure there is zero need in polyfilling AES when web browsers support it.

I'll take a look. The polyfill approach is nice because we only have to use nodejs builtins API and not maintain separate code branches for the two environments but it also does introduce that secondary dependency (since crypto-browserify is all pure javascript and doesn't depend on builtins). Will think on it as I work through implementing a new crypto interface for this stuff.

[paulmillr]

Crypto-browserify is garbage that was last updated in 2017. It's not one dependency - it's at least 12. And I don't think it was thoroughly audited, like our approach.

[dapplion]

I guess some decisions could be made about which functions are synchronous vs asynchronous

@wemeetagain please don't make the API async 🙏 we are already doing enough Promises everywhere

[acolytec3]

I guess some decisions could be made about which functions are synchronous vs asynchronous

@wemeetagain please don't make the API async 🙏 we are already doing enough Promises everywhere

As long as it's follows roughly what I've done so far in this PR, I shouldn't need to make anything else async that isn't already but will keep that in mind as I work on the interface.

[acolytec3]

I'm starting to dig into an interface based approach to the session/keys/crypto stuff and this is kind of what I'm thinking. I'd like to remove the v4 file and enrKeyPair class entirely and leverage the existing IKeypair and/or IKeypairClass interfaces for all of the methods. It seems like unnecessary layers of abstraction and duplication of methods to have verify/sign/hash methods in both v4 and in the keypair classes. I think it makes sense to have a single set of utility methods for the hashing (keccak and sha256 as applicable), and then a single keypair interface that each keypair type implements.

[acolytec3]

@wemeetagain and I met today and talked through this PR and at this point, there doesn't seem like a viable path forward for implementing a crypto interface for the secp256k1 code that won't also require a much deeper rewrite of the whole ENR implementation so I'm not going to continue more work on this specific PR going forward but will rather explore how to get Ultralight working with the existing master branch and try and solve the import issues related to bcrypto browser bindings in the near term. I think there are some specific items that can be pulled out that would be fruitful next steps:

• Replace the bcrypto aes method calls with Nodejs builtins. That should be relatively easy to pull into a standalone PR as it is isolated from the other crypto work contained herein.
• Also, the sha256 implementation could likely be switched for something else if desired (either your own AS version or the noble/hashes version used here) without too much work.
• Switching secp256k1 to an interface that would allow any conforming implementation to be brought would require a deeper rewrite though, likely entirely disposing of the existing ENR class and replacing it with a collection of static method calls that use some global secp25k1 interface that could be provided at build time by the consuming application.

[acolytec3]

One more note on this. I was finally able to get webpack to find the correct bcrypto bindings in the browser by adding a new environmental variable to explicitly tell bcrypto which binding to provide. I'm not sure if there is some industry wide environmental variable named NODE_BACKEND but I found it being used throughout the bcrypto code base and telling webpack to explicitly set it to js allows Ultralight to compile using the master branch and run in the browser without having issues locating the right bcrypto backend.

[acolytec3]

I still vote to drop bcrypto at the earliest opportunity but that may be tricky with secp256k1 until you allow for an interface to be passed in.