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 compile time conversions from & into [u8; 32] #299

Merged
merged 2 commits into from
May 1, 2023

Conversation

ureeves
Copy link
Contributor

@ureeves ureeves commented Apr 12, 2023

It is useful to be able to convert from Hash into [u8; 32] and vice-versa at compile time.

@ureeves
Copy link
Contributor Author

ureeves commented Apr 12, 2023

I hesitate in changing as_bytes to be const, but it could be done as well.

@oconnor663
Copy link
Member

Making .as_bytes() into a const fn sounds like a good idea to me, let's go ahead and do that. With that in place, I don't think a separate into_bytes method is necessary, since you can just dereference the return value of as_bytes.

As far as a const constructor, I wonder if it would make sense to follow the common Rust convention of defining a Self::new method (in our case taking [u8; 32]), which we'd naturally make const. Eventually I suppose Rust will standardize a way to make the From implementation const also, but there are many types that support both new and From/Default, so the duplication doesn't feel to bad to me. What do you think?

@oconnor663
Copy link
Member

Let me know your thoughts about this approach:

diff --git a/src/lib.rs b/src/lib.rs
index b3621c1..18dfd03 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -176,19 +176,19 @@ fn counter_high(counter: u64) -> u32 {
 /// An output of the default size, 32 bytes, which provides constant-time
 /// equality checking.
 ///
-/// `Hash` implements [`From`] and [`Into`] for `[u8; 32]`, and it provides an
-/// explicit [`as_bytes`] method returning `&[u8; 32]`. However, byte arrays
-/// and slices don't provide constant-time equality checking, which is often a
-/// security requirement in software that handles private data. `Hash` doesn't
-/// implement [`Deref`] or [`AsRef`], to avoid situations where a type
-/// conversion happens implicitly and the constant-time property is
-/// accidentally lost.
+/// `Hash` implements [`From`] and [`Into`] for `[u8; 32]`, and also the `const`
+/// functions [`new`] and [`as_bytes`]. However, byte arrays and slices don't
+/// provide constant-time equality checking, which is often a security
+/// requirement in software that handles private data. `Hash` doesn't implement
+/// [`Deref`] or [`AsRef`], to avoid situations where a type conversion happens
+/// implicitly and the constant-time property is accidentally lost.
 ///
 /// `Hash` provides the [`to_hex`] and [`from_hex`] methods for converting to
 /// and from hexadecimal. It also implements [`Display`] and [`FromStr`].
 ///
 /// [`From`]: https://doc.rust-lang.org/std/convert/trait.From.html
 /// [`Into`]: https://doc.rust-lang.org/std/convert/trait.Into.html
+/// [`new`]: #method.new
 /// [`as_bytes`]: #method.as_bytes
 /// [`Deref`]: https://doc.rust-lang.org/stable/std/ops/trait.Deref.html
 /// [`AsRef`]: https://doc.rust-lang.org/std/convert/trait.AsRef.html
@@ -200,11 +200,19 @@ fn counter_high(counter: u64) -> u32 {
 pub struct Hash([u8; OUT_LEN]);
 
 impl Hash {
+    /// Create a new `Hash` from 32 bytes.
+    /// constant-time equality checking, so if  you need to compare hashes,
+    /// prefer the `Hash` type.
+    #[inline]
+    pub const fn new(bytes: [u8; OUT_LEN]) -> Self {
+        Self(bytes)
+    }
+
     /// The raw bytes of the `Hash`. Note that byte arrays don't provide
     /// constant-time equality checking, so if  you need to compare hashes,
     /// prefer the `Hash` type.
     #[inline]
-    pub fn as_bytes(&self) -> &[u8; OUT_LEN] {
+    pub const fn as_bytes(&self) -> &[u8; OUT_LEN] {
         &self.0
     }
 
diff --git a/src/test.rs b/src/test.rs
index 4b6272c..1d9b9aa 100644
--- a/src/test.rs
+++ b/src/test.rs
@@ -603,3 +603,25 @@ fn test_issue_206_windows_sse2() {
         assert_eq!(crate::Hasher::new().update(input).finalize(), expected_hash);
     }
 }
+
+#[test]
+fn test_hash_conversions() {
+    let bytes1 = [99; 32];
+    let hash1: crate::Hash = bytes1.into();
+    let bytes2: [u8; 32] = hash1.into();
+    assert_eq!(bytes1, bytes2);
+    let bytes3 = *hash1.as_bytes();
+    assert_eq!(bytes1, bytes3);
+    let hash2 = crate::Hash::new(bytes1);
+    assert_eq!(hash1, hash2);
+    let hex = hash1.to_hex();
+    let hash3 = crate::Hash::from_hex(hex.as_bytes()).unwrap();
+    assert_eq!(hash1, hash3);
+}
+
+#[test]
+const fn test_hash_const_conversions() {
+    let bytes = [99; 32];
+    let hash = crate::Hash::new(bytes);
+    _ = hash.as_bytes();
+}

The function is `const`, so it is fundamentally different from the
`From` trait implementation by allowing compile-time instantiation of a
`Hash`.
@ureeves
Copy link
Contributor Author

ureeves commented Apr 18, 2023

I'm not very opinionated on it tbh, but a new does make me feel like we're indeed "constructing" a Hash rather than simply converting from its internal representation. Personally I would vote for keeping from_bytes, but either way works.

@oconnor663
Copy link
Member

I think you're right. Landing this as-is.

@oconnor663 oconnor663 merged commit 8176a22 into BLAKE3-team:master May 1, 2023
25 checks passed
@oconnor663
Copy link
Member

Let me know if you need this to get released sooner rather than later? I might like to take one last look at the API at release time before we commit to it forever :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants