Replace wasm as-sha256 with pure js noble-hashes

[paulmillr]

1. wasm is not supported in all environments. the library is used in ethereumjs, which means the use-cases can be extensive
2. as-sha256 is slower than pure-js audited implementation in https://github.com/paulmillr/noble-hashes. Specifically, on inputs bigger than 64 bytes, Noble always wins. JS is 43% faster than as-sha256. Proof:

SHA256 32B @chainsafe/as-sha256 x 1,615,508 ops/sec @ 619ns/op ± 3.57% (min: 500ns, max: 2ms)
SHA256 32B noble x 1,175,088 ops/sec @ 851ns/op ± 1.42% (min: 667ns, max: 962μs)

SHA256 64B @chainsafe/as-sha256 x 1,257,861 ops/sec @ 795ns/op ± 2.80% (min: 666ns, max: 2ms)
SHA256 64B noble x 822,368 ops/sec @ 1μs/op ± 1.73% (min: 1μs, max: 742μs)

SHA256 1KB @chainsafe/as-sha256 x 122,234 ops/sec @ 8μs/op ± 3.61% (min: 7μs, max: 7ms)
SHA256 1KB noble x 165,837 ops/sec @ 6μs/op

SHA256 8KB @chainsafe/as-sha256 x 16,497 ops/sec @ 60μs/op ± 4.12% (min: 56μs, max: 7ms)
SHA256 8KB noble x 23,789 ops/sec @ 42μs/op

SHA256 1MB @chainsafe/as-sha256 x 132 ops/sec @ 7ms/op
SHA256 1MB noble x 187 ops/sec @ 5ms/op

(it's slightly slower for 32B inputs, but executing 1 million hashes of 32b items at once is not a common case)

3. wasm security is not as good as it can be thought of

[FrederikBolding]

+1 to this! I was going to open a similar issue just now.

At MetaMask we have also had to deal with this dependency being introduced into our dependency tree (via ethereumjs) and essentially breaking builds because arbitrary WASM eval isn't allowed by our CSP:

Refused to compile or instantiate WebAssembly module because neither 'wasm-eval' nor 'unsafe-eval' is an allowed source of script in the following Content Security Policy directive: "script-src 'self'"

We are currently forced to exclude @chainsafe/as-sha256 from our builds entirely and would definitely prefer a more platform agnostic dependency to be used for SHA256.

[dapplion]

I see your concerns and would love to find a setup to benefits both web and nodejs consumers. For Lodestar's beacon node the speed of 64B hashes is crucial for performance since it's one of the most frequent ops we do.

@FrederikBolding would there be some approach like platform aware entrypoints that would fix your build issue? Say a-la https://www.npmjs.com/package/cross-fetch

[FrederikBolding]

@dapplion Yeah. Separate entry points for the browser, React Native and Node.js (e.g. via package.json fields) may alleviate some of the pain points for us. However, the ideal would probably be the possibility to opt-in/out of using WASM at all. Not sure if there is a good pattern for doing that other than publishing two packages 😅

[paulmillr]

Why not allow passing sha implementation as a function? Default can be either no hashing at all or noble-hashes because of compatibility.

[dapplion]

Why not allow passing sha implementation as a function? Default can be either no hashing at all or noble-hashes because of compatibility.

That should work too right

CC @wemeetagain

[FrederikBolding]

That sounds like a good idea to me!

[holgerd77]

That's what we are doing in our trie library (after a similar discussion): https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/trie/src/types.ts#L51

[FrederikBolding]

Looks like the proposed fix has been released for this, but it uses the exports field in the package.json, which not all tools seemingly support yet. Therefore, bumping the version in the EthereumJS monorepo causes failed tests.

I have yet to try bumping in the MetaMask extension, but there is a chance that our build pipeline doesn't support exports either.

[paulmillr]

I agree, using exports field for this particular case does not bring any benefits.

Removing exports should not be complicated:

    ".": "./lib/index.js",
    "./hasher": "./lib/hasher/index.js",
    "./hasher/as-sha256": "./lib/hasher/as-sha256.js",
    "./hasher/noble": "./lib/hasher/noble.js"

will need to be removed; files will need to be moved top-level (lib/hasher/index.js => hasher/index.js) OR export paths will need to be renamed (hashes/noble => lib/hasher/noble.js)

[wemeetagain]

This is not exactly an exotic or new feature, it has been a feature since node v12.7.0. And it's the recommended approach according to the docs.

Unfortunately, as we've seen, there are problems. Collecting links here for posterity.

• Bump @chainsafe/ssz to 0.11.0 ethereumjs/ethereumjs-monorepo#2622

  • Here is where ethereumjs is bumping our ssz library version. First attempts to update break karma tests:

  06 04 2023 22:41:04.633:ERROR [karma-server]: UncaughtException: Error: Unable to resolve module [@chainsafe/persistent-merkle-tree/hasher] from [/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/@chainsafe/ssz/lib/util/merkleize.js]
  

  • Only after adding resolve.alias fields in the karma.conf.js for every subpath export (in every karma.conf.js in their monorepo that uses the ssz library 😿 ), was the issue resolved: https://github.com/ethereumjs/ethereumjs-monorepo/pull/2622/files#diff-1a5ee9e029a125a1b3ba72931fd6c8708f9388ecf0df25091fc9fa197fdb9df1R18-R25

• It seems under the hood, karma-typescript uses browserify's browser-resolve package, which uses browserify's resolve package. Which brings us to:

• add support for the exports package.json attribute browserify/resolve#224

  • Unfortunately, any tooling which uses browserify's resolve fails to properly resolve exports. At least until this PR is merged.

I was hoping that this would be an opportunity to file a bug, make fix in upstream tooling, or to fix some tooling configuration. However it seems that this is a known issue that does not have a positive outlook for being resolved in the near term.

I think we can mitigate this by using the same strategy as is used in the ethereum-cryptography library. It publishes built javascript files under the root directory, and manually ensures that subpath exports match the paths of the files that are exported. So non-compliant resolvers may still resolve everything. This strategy is fitting for us as we would like to soon publish these libraries as ESM as well, and this provides us a path that can also keep backwards compatibility here.

[FrederikBolding]

@wemeetagain It's not as elegant as exports, but this works for Browserify and Webpack: #318

I have verified that this fixes any problems in the MetaMask extension build process when importing @chainsafe/ssz.

[holgerd77]

@wemeetagain It's not as elegant as exports, but this works for Browserify and Webpack: #318

I have verified that this fixes any problems in the MetaMask extension build process when importing @chainsafe/ssz.

🙏

Thanks a lot for the feedback, great timing since I am just preparing release notes for releases today including this fix! 🙂

[FrederikBolding]

@holgerd77 Sorry, just to clarify. We'll need to fix the exports issue before the MetaMask problems are entirely fixed.

But haven't gotten a response on that PR yet.

[holgerd77]

@holgerd77 Sorry, just to clarify. We'll need to fix the exports issue before the MetaMask problems are entirely fixed.

But haven't gotten a response on that PR yet.

Ok, I'll pause our release process (nothing urgent to be released), maybe there is a chance that this can get settled and still included within our release round.

[g11tech]

@paulmillr @FrederikBolding
should this issue be closed off now?