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

Add BLAKE3 hashing algorithm #12379

Merged
merged 2 commits into from
Feb 6, 2025
Merged

Add BLAKE3 hashing algorithm #12379

merged 2 commits into from
Feb 6, 2025

Conversation

silvanshade
Copy link
Contributor

@silvanshade silvanshade commented Jan 29, 2025

This uses the single-threaded C-based routines from libblake3.

This is not optimal performance-wise but should be a good starting point for nix compatibility with BLAKE3 hashing until a more performant implementation based on the multi-threaded BLAKE3 routines (written in Rust) can be developed.

@Ericson2314

Motivation

See below.

Context

#10600 #11999

EDIT: Before merging this please see the companion PR with BLAKE3 via Rust interop: #12416

@silvanshade silvanshade requested a review from edolstra as a code owner January 29, 2025 20:44
@roberth
Copy link
Member

roberth commented Jan 29, 2025

Nice!
Have you cross-validated this with tvix's use of BLAKE3 in their store implementation?

@silvanshade
Copy link
Contributor Author

silvanshade commented Jan 29, 2025

Have you cross-validated this with tvix's use of BLAKE3 in their store implementation?

No, but I'm also not sure if that is a meaningful thing we can do in this context. Maybe you can elaborate if you have something specific in mind?

I don't know much about tvix but from scanning the documentation and source code it appears that their use of BLAKE3 is primarily for their custom data model and protocols around interacting with the content addressable store, which as far as I know is not directly comparable to the equivalent in C++ nix.

It would theoretically be possible to re-implement what they have done for the castore here but it would be a lot more work, beyond the scope of this PR, and certainly beyond my current knowledge of nix internals.

What this PR does is just add BLAKE3 as another hashing algorithm, which could presumably be used for anything nix does internally related to hashing, but at the moment is only really exposed for SRI hashes and via the CLI interface for nix hash.

Surprisingly, tvix doesn't seem to support BLAKE3 for SRI hashes though: https://github.com/tvlfyi/tvix/blob/f8325a64845500916f1e40f5d04466d4c7d1bef6/eval/src/builtins/hash.rs#L19-L29

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@Ericson2314
Copy link
Member

I asked in #tvix-dev

@Kranzes
Copy link
Member

Kranzes commented Jan 30, 2025

Surprisingly, tvix doesn't seem to support BLAKE3 for SRI hashes though: https://github.com/tvlfyi/tvix/blob/f8325a64845500916f1e40f5d04466d4c7d1bef6/eval/src/builtins/hash.rs#L19-L29

Because we're trying to be compatible with with Nix.

@edolstra
Copy link
Member

edolstra commented Feb 5, 2025

Team meeting notes:

  • Decided to merge this as an experimental feature. We can drop the experimental feature status if there is evidence of actual use. @silvanshade Can you add the experimental feature?

@silvanshade
Copy link
Contributor Author

@edolstra Okay, I can add the experimental feature. What should I use as the tracking URL?

This uses the single-threaded C-based routines from libblake3.

This is not optimal performance-wise but should be a good starting point
for nix compatibility with BLAKE3 hashing until a more performant
implementation based on the multi-threaded BLAKE3 routines
(written in Rust) can be developed.
@silvanshade
Copy link
Contributor Author

I made some additional changes:

  • Added the blake3-hashes experimental feature
  • Added an optional ExperimentalFeatureSettings argument to Hash and hashString
  • Switched from TEST to TEST_F to allow enabling blake3-hashes feature in tests
  • Added an example BLAKE3 hash digest from https://datatracker.ietf.org/doc/draft-aumasson-blake3/
  • Added blake3 to the documentation (blake3 will already show in error messages so might as well put it in docs)

@Ericson2314
Copy link
Member

We can add the milestone after, thanks!

@Ericson2314 Ericson2314 merged commit fc83c6c into NixOS:master Feb 6, 2025
12 checks passed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-12-nix-team-meeting-minutes-212-5/60216/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants