paulmillr / noble-secp256k1

Fastest 4KB JS implementation of secp256k1 signatures and ECDH
https://paulmillr.com/noble
MIT License
757 stars 114 forks source link

Switches to using webcrypto everywhere. #96

Closed MicahZoltu closed 1 year ago

MicahZoltu commented 1 year ago

globalThis.crypto is available in all major browsers, deno, and NodeJS 19+. It is also available behind experimental flag in NodeJS 16-18, and exists at import {webcrypto} from 'node:crypto'.

Updated tests to polyfill webcrypto so they can run in NodeJS 16+ without experimental flags.

Bumped @types/node to v18 so it has the latest subtlecrypto definitions.

paulmillr commented 1 year ago

The tests run properly for me.

MicahZoltu commented 1 year ago

Updated with failing tests. Current branch results in a timeout for me. If I increase the timeout from default 5 seconds to 20 seconds the tests pass:

√ should handle RFC 6979 vectors (12213 ms)

paulmillr commented 1 year ago

What are your benchmark results?

paulmillr commented 1 year ago

3401ms for the RFC test on my machine, compared to 3406ms in the main branch.

paulmillr commented 1 year ago

3 notes

  1. fails on node 19, so need to at least do if (!globalThis.crypto)
  2. will prob fail in web workers, which have self instead of globalThis
  3. main branch tests took 12 seconds in CI, and did not fail - not sure why it timeouts now
MicahZoltu commented 1 year ago

Hmm, it seems the test takes about as long for me on 1.7.1 as it does on this branch, but it only times out on this branch and doesn't timeout on 1.7.1. Tests should timeout after 5 seconds according to jest defaults, so this test should timeout on both branches and in CI, but for some reason it doesn't.

Hypothesis

Perhaps a switch from sync to async when moving to webcrypto results in jest correctly timing out where previously it would not have timed out? It is possible that jest can only trigger a timeout when it gets an opportunity to execute, and if everything is sync then jest will not get an opportunity to execute the timeout until after the test has passed.

Supporting evidence for this hypothesis, this test doesn't timeout:

it('timeout', () => {
  const startTime = Date.now()
  while (Date.now() < startTime + 6000) {}
})

Recommendation

Either split the test into individual tests or increase the timeout to ~20 seconds so it can pass on a variety of hardware (including CI).

Data

This Branch:

Benchmarking
Initialized: 51ms
RAM: rss=45.0mb heap=11.4mb used=7.4mb ext=0.5mb

getPublicKey(utils.randomPrivateKey()) x 1,717 ops/sec @ 582μs/op ± 1.48% (min: 409μs, max: 2ms)
sign x 1,099 ops/sec @ 909μs/op
signSync (@noble/hashes) x 1,586 ops/sec @ 630μs/op
verify x 315 ops/sec @ 3ms/op
recoverPublicKey x 311 ops/sec @ 3ms/op
getSharedSecret aka ecdh x 196 ops/sec @ 5ms/op
getSharedSecret (precomputed) x 2,229 ops/sec @ 448μs/op
Point.fromHex (decompression) x 4,484 ops/sec @ 223μs/op
schnorr.sign x 238 ops/sec @ 4ms/op
schnorr.verify x 348 ops/sec @ 2ms/op

RAM: rss=62.3mb heap=27.3mb used=9.8mb ext=0.5mb
√ should handle RFC 6979 vectors (12016 ms)

1.7.1:

getPublicKey(utils.randomPrivateKey()) x 1,536 ops/sec @ 650μs/op ± 1.70% (min: 412μs, max: 2ms)
sign x 1,384 ops/sec @ 722μs/op ± 1.05% (min: 569μs, max: 2ms)
signSync (@noble/hashes) x 1,515 ops/sec @ 659μs/op
verify x 315 ops/sec @ 3ms/op
recoverPublicKey x 307 ops/sec @ 3ms/op
getSharedSecret aka ecdh x 195 ops/sec @ 5ms/op
getSharedSecret (precomputed) x 1,889 ops/sec @ 529μs/op ± 1.43% (min: 428μs, max: 1ms)
Point.fromHex (decompression) x 4,088 ops/sec @ 244μs/op ± 1.22% (min: 203μs, max: 1ms)
schnorr.sign x 193 ops/sec @ 5ms/op ± 4.38% (min: 3ms, max: 13ms)
schnorr.verify x 273 ops/sec @ 3ms/op ± 3.80% (min: 2ms, max: 12ms)

RAM: rss=62.5mb heap=28.4mb used=12.2mb ext=0.5mb
√ should handle RFC 6979 vectors (11816 ms)
paulmillr commented 1 year ago

Either split the test into individual tests

Jest is a mess and it should be replaced with micro-should, which supports ESM correctly, doesn't use dependencies, and is just a few hundred LOC.

noble-curves already executed the switch. The amount of test vectors has been increased massively.

If you care about supply chain security, not using jest is as important as not using rollup/etc.

Increasing timeout sounds fine.

MicahZoltu commented 1 year ago
  1. fails on node 19, so need to at least do if (!globalThis.crypto)
  2. will prob fail in web workers, which have self instead of globalThis
  3. main branch tests took 12 seconds in CI, and did not fail - not sure why it timeouts now

Fixed (1) with globalThis.crypto || (globalThis.crypot = webcrypto) in the tests and benchmark.

For (2) I believe workers have globalThis. The whole point of globalThis was to specifically resolve the problem of global vs window vs self being differently available in various environments. globalThis should be available in all JS execution environments.

See above post about (3).

paulmillr commented 1 year ago

I believe workers have globalThis

Nope.

paulmillr commented 1 year ago

Hmm, i'm mistaken: seems like workers have it...

MicahZoltu commented 1 year ago

If you care about supply chain security, not using jest is as important as not using rollup/etc.

Yeah, I 💯% agree on this. I have plans to slowly migrate my projects to this over time (at least new projects will use it).

MicahZoltu commented 1 year ago

Increasing timeout sounds fine.

Pushed update with 20 second timeout.

coolaj86 commented 1 year ago

As of today Bun also exposes WebCrypto for both globalThis.crypto and require("crypto") / import from "node:crypto".

(I just filed the bug earlier and it was fixed immediately, so now this works with that too :) )

paulmillr commented 1 year ago

We won't break backwards compatibility in v1.0. For v2.0 which is in a pr now, that seems like a good option. However the pr switched to "require" so not sure if it's still needed.

Not having any stable node.js version supporting it is a big one.

paulmillr commented 1 year ago

=> gh-92