-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: Switch from SHA3-256 to BLAKE3-256 #306
Conversation
Codecov Report
@@ Coverage Diff @@
## main #306 +/- ##
==========================================
- Coverage 56.25% 56.22% -0.04%
==========================================
Files 43 43
Lines 3180 3189 +9
Branches 769 770 +1
==========================================
+ Hits 1789 1793 +4
- Misses 900 904 +4
- Partials 491 492 +1
|
I haven't actually like plotted the benchmarks or anything, but just looking at the raw numbers it looks like this PR mostly runs as-fast-or-faster than the previous PR 🙌 |
Actually, do we have a tool that plots the benches set up? Save me from tabbing back and forth between the plaintext results 😛 |
We have this: https://wnfs-wg.github.io/rs-wnfs/dev/bench/ But I don't think we have anything like that for branches. |
Oh these benchmarks are for native code though, right? Do we have Wasm benches? |
No WASM benches currently :/ |
(I really dislike the lack of threading in main thread PR comments) We should at minimum run some manual tests for this PR I think, especially given that GH Issue on the BLAKE3 repo. Setting up Wasm benches would be nice in general (we have wasm-js tests, so maybe extend that for rough microbenches?) |
Was chatting with @zeeshanlakhani about this, and he says that the right way to bench Wasm is via JS wrappers. It would be nice to have them for all targets (so... two for now, but who knows down the road) |
Remaining TODOs:
|
This ensures you can re-generate all block labels, even if you don't have access to the PrivateNodeHeader`, e.g. when you only have snapshot access.
I set up this very crude benchmark: Code
And the variance between runs is bigger than the variance between EDIT: LOL should've added the benchmark results: Two runs on c17f6bb (main):
Two runs on 05529f5 (this PR):
|
@appcypher This is ready for review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
sha3
dependencies out forblake3
with thetraits-preview
feature. That feature made the switch pretty seamless.prime_hash
impl to using little-endian big integer encoding, otherwise some tests would run out of stack space.BigUInt::from_bytes_be
actually simply copies over the bytes into a new array that it reverses and runsBigUint::from_bytes_le
on, which causes quite a lot of allocation.prime_hash
don't unintentionally change hash results.base_name
toExternalFileContent
. This allows users with snapshot access only to derive the necessary block labels (they wouldn't be able to access theheader.name
).keys.rs
, making the API slimmer (theSnapshotKey
andTemporalKey
newtypes are now private), and removingFrom<>
impls in favor of documented, (hopefully hard to misuse?) special-purpose functions.Still having the "tests run out of stack" issue every now and then. After a couple of complicated debugging sessions with
lldb
, it turns out the issue is mostly having functions with lots of await points (e.g. unit tests!). All of the Futures are allocated on the stack, blowing it. There's no good solution that I'm aware of, except forRUST_MIN_STACK=2500000 cargo test
.