Merge rust-bitcoin#3113: Push up the Default bound on HashEngine in order to better support keyed hash functions

2969b03 Push up the Default bound on HashEngine (Nick Johnson)

Pull request description:

  Backstory for this patch is in rust-bitcoin#3084, and this might be enough to close that issue, but I think some follow up work might be required.

  I took Kixunil's advice and pushed up the `Default` bound on `HashEngine` all the way to the default methods themselves on the `GeneralHash` trait. I initially placed the bound just on the assocated type `Engine` of `GeneralHash`, which is a tad cleaner and keeps the bound from "leaking" downstream into things like `Hmac` and `Hkdf` implementations. However, that restricts `GeneralHash` implementations to just unkeyed hash functions and I believe there will be value in having `Poly1305` and `SipHash24` still leverage the interface. I struggled a bit on the best spots to put these bounds (even found the syntax a little jarring at times, I found [RFC2289](https://rust-lang.github.io/rfcs/2289-associated-type-bounds.html) helpful if others do too).

  Refactored the existing keyed hash function, `SipHash24`, to no longer have `Default` functions with zero-value keys. I kept the test coverage though by just hardcoding the zero-value keys over in the tests.

  Refactoring the `hash_type` macro for keyed hashes was getting a little hairy, so backed off and just wrote the bare minimum for `SipHash24` inline. Once `Poly1305` lands there will be two keyed hash functions and I think it will make more sense to then generalize over them.

ACKs for top commit:
  Kixunil:
    ACK 2969b03
  apoelstra:
    ACK 2969b03

Tree-SHA512: 9ca8f8baa6d60c36627eb3564f5faafcddd0a69fed5e29965404e405d3d09cb08bec2e05f14e177b8bfa2a488efba9947c0e0c158db6b00b44c8c11869328f5a

Author: apoelstra

hashes/src/hkdf.rs

@@ -37,7 +37,10 @@ pub struct Hkdf<T: GeneralHash> {
     prk: Hmac<T>,
 }
 
-impl<T: GeneralHash> Hkdf<T> {
+impl<T: GeneralHash> Hkdf<T>
+where
+    <T as GeneralHash>::Engine: Default,
+{
     /// Initialize a HKDF by performing the extract step.
     pub fn new(salt: &[u8], ikm: &[u8]) -> Self {
         let mut hmac_engine: HmacEngine<T> = HmacEngine::new(salt);

hashes/src/hmac.rs

@@ -42,7 +42,10 @@ pub struct HmacEngine<T: GeneralHash> {
     oengine: T::Engine,
 }
 
-impl<T: GeneralHash> Default for HmacEngine<T> {
+impl<T: GeneralHash> Default for HmacEngine<T>
+where
+    <T as GeneralHash>::Engine: Default,
+{
     fn default() -> Self { HmacEngine::new(&[]) }
 }
 
@@ -54,7 +57,10 @@ impl<T: GeneralHash> HmacEngine<T> {
     /// # Panics
     ///
     /// Larger hashes will result in a panic.
-    pub fn new(key: &[u8]) -> HmacEngine<T> {
+    pub fn new(key: &[u8]) -> HmacEngine<T>
+    where
+        <T as GeneralHash>::Engine: Default,
+    {
         debug_assert!(T::Engine::BLOCK_SIZE <= 128);
 
         let mut ipad = [0x36u8; 128];

hashes/src/impls.rs

@@ -153,8 +153,6 @@ mod tests {
         "a9608c952c8dbcc20c53803d2ca5ad31d64d9313",
     );
 
-    write_test!(siphash24, "d70077739d4b921e", "3a3ccefde9b5b1e3", "ce456e4e4ecbc5bf",);
-
     #[test]
     fn hmac() {
         let mut engine = hmac::HmacEngine::<sha256::Hash>::new(&[0xde, 0xad, 0xbe, 0xef]);
@@ -178,4 +176,19 @@ mod tests {
             "30df499717415a395379a1eaabe50038036e4abb5afc94aa55c952f4aa57be08"
         );
     }
+
+    #[test]
+    fn siphash24() {
+        let mut engine = siphash24::HashEngine::with_keys(0, 0);
+        engine.write_all(&[]).unwrap();
+        assert_eq!(format!("{}", siphash24::Hash::from_engine(engine)), "d70077739d4b921e");
+
+        let mut engine = siphash24::HashEngine::with_keys(0, 0);
+        engine.write_all(&[1; 256]).unwrap();
+        assert_eq!(format!("{}", siphash24::Hash::from_engine(engine)), "3a3ccefde9b5b1e3");
+
+        let mut engine = siphash24::HashEngine::with_keys(0, 0);
+        engine.write_all(&[99; 64000]).unwrap();
+        assert_eq!(format!("{}", siphash24::Hash::from_engine(engine)), "ce456e4e4ecbc5bf");
+    }
 }

hashes/src/lib.rs

@@ -177,7 +177,7 @@ pub type HkdfSha256 = Hkdf<sha256::Hash>;
 pub type HkdfSha512 = Hkdf<sha512::Hash>;
 
 /// A hashing engine which bytes can be serialized into.
-pub trait HashEngine: Clone + Default {
+pub trait HashEngine: Clone {
     /// Length of the hash's internal block size, in bytes.
     const BLOCK_SIZE: usize;
 
@@ -189,20 +189,31 @@ pub trait HashEngine: Clone + Default {
 }
 
 /// Trait describing hash digests which can be constructed by hashing arbitrary data.
+///
+/// Some methods have been bound to engines which implement Default, which is
+/// generally an unkeyed hash function.
 pub trait GeneralHash: Hash {
     /// A hashing engine which bytes can be serialized into. It is expected
     /// to implement the `io::Write` trait, and to never return errors under
     /// any conditions.
     type Engine: HashEngine;
 
     /// Constructs a new engine.
-    fn engine() -> Self::Engine { Self::Engine::default() }
+    fn engine() -> Self::Engine
+    where
+        Self::Engine: Default,
+    {
+        Self::Engine::default()
+    }
 
     /// Produces a hash from the current state of a given engine.
     fn from_engine(e: Self::Engine) -> Self;
 
     /// Hashes some bytes.
-    fn hash(data: &[u8]) -> Self {
+    fn hash(data: &[u8]) -> Self
+    where
+        Self::Engine: Default,
+    {
         let mut engine = Self::engine();
         engine.input(data);
         Self::from_engine(engine)
@@ -213,6 +224,7 @@ pub trait GeneralHash: Hash {
     where
         B: AsRef<[u8]>,
         I: IntoIterator<Item = B>,
+        Self::Engine: Default,
     {
         let mut engine = Self::engine();
         for slice in byte_slices {

hashes/src/siphash24.rs

@@ -6,13 +6,13 @@ use core::ops::Index;
 use core::slice::SliceIndex;
 use core::{cmp, mem, ptr};
 
+use crate::internal_macros::arr_newtype_fmt_impl;
 use crate::HashEngine as _;
 
-crate::internal_macros::hash_type! {
-    64,
-    false,
-    "Output of the SipHash24 hash function."
-}
+#[doc = "Output of the SipHash24 hash function."]
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[repr(transparent)]
+pub struct Hash([u8; 8]);
 
 #[cfg(not(hashes_fuzz))]
 fn from_engine(e: HashEngine) -> Hash { Hash::from_u64(Hash::from_engine_to_u64(e)) }
@@ -106,10 +106,6 @@ impl HashEngine {
         }
     }
 
-    /// Creates a new SipHash24 engine.
-    #[inline]
-    pub const fn new() -> HashEngine { HashEngine::with_keys(0, 0) }
-
     /// Retrieves the keys of this engine.
     pub fn keys(&self) -> (u64, u64) { (self.k0, self.k1) }
 
@@ -128,10 +124,6 @@ impl HashEngine {
     }
 }
 
-impl Default for HashEngine {
-    fn default() -> Self { HashEngine::new() }
-}
-
 impl crate::HashEngine for HashEngine {
     const BLOCK_SIZE: usize = 8;
 
@@ -186,6 +178,9 @@ impl Hash {
         Hash::from_engine(engine)
     }
 
+    /// Produces a hash from the current state of a given engine.
+    pub fn from_engine(e: HashEngine) -> Hash { from_engine(e) }
+
     /// Hashes the given data directly to u64 with an engine with the provided keys.
     pub fn hash_to_u64_with_keys(k0: u64, k1: u64, data: &[u8]) -> u64 {
         let mut engine = HashEngine::with_keys(k0, k1);
@@ -215,8 +210,39 @@ impl Hash {
 
     /// Creates a hash from its (little endian) 64-bit integer representation.
     pub fn from_u64(hash: u64) -> Hash { Hash(hash.to_le_bytes()) }
+
+    fn from_slice(sl: &[u8]) -> Result<Self, crate::FromSliceError> {
+        let mut ret = [0; 8];
+        ret.copy_from_slice(sl);
+        Ok(Hash(ret))
+    }
+}
+
+impl crate::Hash for Hash {
+    type Bytes = [u8; 8];
+
+    const LEN: usize = 8;
+    const DISPLAY_BACKWARD: bool = false;
+
+    fn from_slice(sl: &[u8]) -> Result<Self, crate::FromSliceError> { Self::from_slice(sl) }
+
+    fn to_byte_array(self) -> Self::Bytes { self.0 }
+
+    fn as_byte_array(&self) -> &Self::Bytes { &self.0 }
+
+    fn from_byte_array(bytes: Self::Bytes) -> Self { Hash(bytes) }
 }
 
+impl<I: SliceIndex<[u8]>> Index<I> for Hash {
+    type Output = I::Output;
+
+    #[inline]
+    fn index(&self, index: I) -> &Self::Output { &self.0[index] }
+}
+
+arr_newtype_fmt_impl!(Hash, 8);
+borrow_slice_impl!(Hash);
+
 /// Load an u64 using up to 7 bytes of a byte slice.
 ///
 /// Unsafe because: unchecked indexing at `start..start+len`.
@@ -341,7 +367,7 @@ mod benches {
 
     #[bench]
     pub fn siphash24_1ki(bh: &mut Bencher) {
-        let mut engine = siphash24::Hash::engine();
+        let mut engine = siphash24::HashEngine::with_keys(0, 0);
         let bytes = [1u8; 1024];
         bh.iter(|| {
             engine.input(&bytes);
@@ -351,7 +377,7 @@ mod benches {
 
     #[bench]
     pub fn siphash24_64ki(bh: &mut Bencher) {
-        let mut engine = siphash24::Hash::engine();
+        let mut engine = siphash24::HashEngine::with_keys(0, 0);
         let bytes = [1u8; 65536];
         bh.iter(|| {
             engine.input(&bytes);

hashes/tests/regression.rs

@@ -30,7 +30,6 @@ impl_regression_test! {
     regression_sha384, sha384, "f545bd83d297978d47a7f26b858a54188499dfb4d7d570a6a2362c765031d57a29d7e002df5e34d184e70b65a4f47153";
     regression_sha512, sha512, "057d0a37e9e0ac9a93acde0752748da059a27bcf946c7af00692ac1a95db8d21f965f40af22efc4710f100f8d3e43f79f77b1f48e1e400a95b7344b7bc0dfd10";
     regression_sha512_256, sha512_256, "e204244c429b5bca037a2a8a6e7ed8a42b808ceaff182560840bb8c5c8e9a2ec";
-    regression_siphash24, siphash24, "e823ed82311d601a";
 }
 
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Default, Hash)]
@@ -90,3 +89,14 @@ fn regression_hmac_sha512_with_key() {
     let want = "8511773748f89ba22c07fb3a2981a12c1823695119de41f4a62aead6b848bd34939acf16475c35ed7956114fead3e794cc162ecd35e447a4dabc3227d55f757b";
     assert_eq!(got, want);
 }
+
+#[test]
+fn regression_siphash24_with_key() {
+    let mut engine = siphash24::HashEngine::with_keys(0, 0);
+    engine.input(DATA.as_bytes());
+    let hash = siphash24::Hash::from_engine(engine);
+
+    let got = format!("{}", hash);
+    let want = "e823ed82311d601a";
+    assert_eq!(got, want);
+}