-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix: change hashing, curve and cipher libraries to @noble #322
Conversation
Signed-off-by: Marin Petrunic <[email protected]>
Signed-off-by: Marin Petrunic <[email protected]>
@paulmillr x25519 curve is a lot slower than stablelib implementation. It causes ~35% worse benchmark performance. I've created #323 to update hashing libs. They seems to be pretty much the same in terms of performance |
Not sure why 35%, from my low-level benchmark of x25519 it's 21%: noble x 1,510 ops/sec @ 661μs/op Nevertheless, the slowdown is easily explained.
Stablelib is extremely hard to audit because it uses unrolled loops with 5x? code. My philosophy is that cryptography should be easy to audit and reason about. |
Probably because it's benchmarking negotiating connections between two peers and that negotiation is not parallel so x25519 methods are called on both source and destination peer sequentially |
@ChainSafe/lodestar ^^ |
lodestar has to struggle a lot due to performance issue, we have a lot of benchmark tests there as part of CI, everything is "performance first" in this case I'd go with #323 if it has comparable performance to the latest release, please "~35% worse benchmark performance" is not acceptable to us 🙏 |
hkdf and sha256 are faster or similar to what you're having right now. |
This PR is worth dusting off considering #321 (comment) The lodestar case shouldn't be a blocker here since we override noise crypto implementation there anyways. |
…-libs Signed-off-by: Marin Petrunic <[email protected]>
@paulmillr @wemeetagain I'm hitting errors:
On other notes, memory wise, it would be better if we could pass destination Uint8Array to chacha-poly1305 decrypt to save on memory allocation. TODO: I could try to cache chachapoly function^^ |
This makes sense and pure-non-poly-chacha has this feature. Chapoly doesn't. Chapoly ciphertext is 16 bytes longer than plaintext, which means we can't reuse plaintext u8a, for one. So, it's not radically simpler. Also i'm not sure if it will be much more performant. |
Regarding the error, i've cloned the repository, but it tries to access |
I agree for encryption it doesn't make sense but when decrypting we are passing ciphertext as destination not plaintext |
Here you go: paulmillr/noble-ciphers#2 |
fixed now |
Signed-off-by: Marin Petrunic <[email protected]>
@paulmillr do you have plans of enabling that? @wemeetagain maybe worth checking perf of assemblyscript version vs pure js. Although I imagine not reusing buffers when decrypting are gonna be blocker^^ In any case, this should be ready to go. |
tnx! |
Lodestar will probably keep using our own implementation, as you mentioned, we rely on perf benefits of avoiding allocation as described in #242 Would definitely love to bench against noble (esp if it gets this feature too) |
chainsafe/as-chacha20poly1305 is slower than noble-ciphers on meaningful (1KB+) inputs. Proof (will commit benchmark to ciphers repo):
|
Thanks @paulmillr for the benchmark, it looks great. Lodestar would definitely switch to noble if it supports an optional param |
@tuyennhv why? What's the need? noble implements
In chacha-poly, |
@paulmillr it's because having to allocate new memory in I know it's not ideal to mix plaintext with ciphertext, it took us a lot of time to review and test, finally we decided to get that PR in given a lot of performance issues we've had to deal with. in the end I think that's not a concern of a library, you only need to provide an optional |
@tuyennhv if the performance difference is really that big, as with the 242 pull request you've mentioned, I can definitely add the I assume you'll do similar CPU tests before noble deployment. |
@tuyennhv |
It's not on npm yet? |
@mpetrunic hmm, seems like GitHub CI auto-publish did not go through. I've pinged it one more time and now it's on NPM. |
@paulmillr thanks for this. When I add noble to benchmark, not sure why I get different results, the test is here ChainSafe/as-chacha20poly1305#7 according to this, noble is 5% faster than |
Because your benchmark software is 1000 lines of code, and each additional feature could influence the result? How could you be sure that running inside of mocha context does not influence V8 garbage collection strategy, or just-in-time compiler? You should try this instead:
and using micro-bmark
This is not just about perf (even though the benchmarks are wrong). You are using wasm, wasm is unsupported on many platforms, which makes your library unsupported on many platforms. Remember ChainSafe/ssz#313: metamask was not able to use your wasm sha256 library. |
Seems like types aren't updated: paulmillr/noble-ciphers#4 |
@paulmillr I followed your instruction, the result is quite similar to my benchmark: noble starts getting better at 8kb:
|
Interesting -- thanks for the test. I wonder why my m2 has parity on 1kb. Still, it's wasm (ChainSafe/ssz#313). |
If noble or any libraries, at some point, is consistently better than |
This is not about lodestar - this is about libp2p. In ssz, you've added noble as default, but kept using native, performant version for lodestar. Seems like a good idea. |
It's same here, noble (or previously stablelib) is default but lodestar uses as-chacha20poly1305^^ |
Great! 👏 So we're good here. |
resolves #321
resolves #255