Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to audited and fast version of sha3/keccak #673

Open
paulmillr opened this issue Dec 15, 2022 · 4 comments
Open

Switch to audited and fast version of sha3/keccak #673

paulmillr opened this issue Dec 15, 2022 · 4 comments
Labels
low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it.

Comments

@paulmillr
Copy link

You're using js-sha3, while a better package would be https://github.com/paulmillr/noble-hashes which was audited and is utilized in https://github.com/ethereum/js-ethereum-cryptography.

@cameel
Copy link
Member

cameel commented Dec 15, 2022

Sounds reasonable. We should consider it.

js-sha3 did not have new releases since 2018. I guess something like that does not have to be updated constantly and it has no dependencies to keep up with so it might still be fine but even then I'd expect something to happen in the repo in the last 4 years.

@r0qs can you take a look at the new library when you have a moment?

@cameel cameel added low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. labels Dec 15, 2022
@paulmillr
Copy link
Author

Worth noting the new lib was funded by EF https://blog.ethereum.org/2022/06/01/may-22-grantee-roundup

@r0qs
Copy link
Member

r0qs commented Dec 19, 2022

Hi @paulmillr, thanks for bringing this up. Indeed it would be great to replace js-sha3 with a faster and more actively maintained library, although I agree that this kind of library does not require too much updates, as @cameel pointed out.

However, your implementation does not seem to be faster than js-sha3 as the PR title suggest. I ran your benchmarks and the results show that js-sha3 outperforms noble hashes:

==== SHA3-256, keccak256, shake256 ====
SHA3-256, keccak256, shake256 32B node x 309,310 ops/sec @ 3μs/op ± 14.98% (min: 1μs, max: 40ms)
SHA3-256, keccak256, shake256 32B hash-wasm x 447,828 ops/sec @ 2μs/op ± 1.84% (min: 1μs, max: 2ms)
SHA3-256, keccak256, shake256 32B stablelib x 173,671 ops/sec @ 5μs/op
SHA3-256, keccak256, shake256 32B js-sha3 x 217,770 ops/sec @ 4μs/op ± 1.09% (min: 3μs, max: 4ms)
SHA3-256, keccak256, shake256 32B sha3 x 57,398 ops/sec @ 17μs/op
SHA3-256, keccak256, shake256 32B noble x 87,642 ops/sec @ 11μs/op

SHA3-256, keccak256, shake256 64B node x 254,647 ops/sec @ 3μs/op ± 25.04% (min: 1μs, max: 47ms)
SHA3-256, keccak256, shake256 64B hash-wasm x 438,212 ops/sec @ 2μs/op ± 2.91% (min: 1μs, max: 1ms)
SHA3-256, keccak256, shake256 64B stablelib x 192,566 ops/sec @ 5μs/op ± 7.57% (min: 3μs, max: 37ms)
SHA3-256, keccak256, shake256 64B js-sha3 x 244,319 ops/sec @ 4μs/op ± 1.49% (min: 3μs, max: 1ms)
SHA3-256, keccak256, shake256 64B sha3 x 64,520 ops/sec @ 15μs/op
SHA3-256, keccak256, shake256 64B noble x 97,551 ops/sec @ 10μs/op

I haven't looked at the audits report yet, but I will do it shortly. Were all the audit findings already addressed in your implementation?

@paulmillr
Copy link
Author

paulmillr commented Dec 21, 2022

js-sha3 outperforms noble hashes

Right - I forgot about this. This is because the loops are not unrolled for auditability. You should decide whether the performance degradation is acceptable to your use case. It's acceptable for wallets etc.

Were all the audit findings already addressed in your implementation?

1) The audit was 11 months ago 2) there were no vulnerabilities in noble-hashes, vulns were in other libraries 3) every small "non-vulnerability" item was addressed immediately after it was posted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it.
Projects
None yet
Development

No branches or pull requests

3 participants