Replace all uses of SHA-256 with BLAKE2b.#37439
Conversation
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Looks great to me! r=me with just a few nits
There was a problem hiding this comment.
Just to confirm, we don't have to worry about endianness here, right?
There was a problem hiding this comment.
That's correct, blake2b_finish makes sure that Blake2bCtx::h contains the bytes in little endian order, which is what blake2b defines.
src/librustc_driver/driver.rs
Outdated
There was a problem hiding this comment.
Could you add a comment why 256 is used here?
There was a problem hiding this comment.
There's no real reason other than that we had a 256 bit hash here. 128 bits would probably be more than enough.
There was a problem hiding this comment.
Hm, I thought it doesn't how with this hash was as long as it would avoid collisions. We are only storing it once per crate, so I thought there is no harm in making it as wide as possible. But since it has been introduced a few things have been added and now we are feeding this hash into many other hashes, often multiple times (symbol hashes, type id hashes, incr. comp. hashes). So, making it smaller should actually have a positive impact on runtime.
I figure, 128 bits should be enough to make collisions unlikely enough.
There was a problem hiding this comment.
Sounds good to me! r=me with a rebase
There was a problem hiding this comment.
I shared this patch with one of the blake2 designers and he was concerned about the use of 128 bits. See this titter thread: https://twitter.com/zooko/status/794972598853627904?s=09
src/librustc_driver/Cargo.toml
Outdated
There was a problem hiding this comment.
I think this should start with "librustc" (travis is complaining)
michaelwoerister
force-pushed
the
remove-sha256
branch
from
db4df45 to
558f761
Compare
|
☔ The latest upstream changes (presumably #37400) made this pull request unmergeable. Please resolve the merge conflicts. |
michaelwoerister
force-pushed
the
remove-sha256
branch
from
558f761 to
885c924
Compare
michaelwoerister
force-pushed
the
remove-sha256
branch
from
885c924 to
9ef9194
Compare
|
@bors r=alexcrichton |
|
📌 Commit 9ef9194 has been approved by |
|
⌛ Testing commit 9ef9194 with merge 4497196... |
Replace all uses of SHA-256 with BLAKE2b. Removes the SHA-256 implementation and replaces all uses of it with BLAKE2b, which we already use for debuginfo type guids and incremental compilation hashes. It doesn't make much sense to have two different cryptographic hash implementations in the compiler and Blake has a few advantages over SHA-2 (computationally less expensive, hashes of up to 512 bits).
brson
added
the
relnotes
7 participants
Removes the SHA-256 implementation and replaces all uses of it with BLAKE2b, which we already use for debuginfo type guids and incremental compilation hashes. It doesn't make much sense to have two different cryptographic hash implementations in the compiler and Blake has a few advantages over SHA-2 (computationally less expensive, hashes of up to 512 bits).