Merge rust-bitcoin/rust-bitcoin#4028: hashes: several cleanups

a5d96ceb39 hashes: remove private `internal_new` method (Andrew Poelstra)
0aeff359f5 hashes: reformat (Andrew Poelstra)
aadd7df266 hashes: move `from_engine` fn impl out of macro body (Andrew Poelstra)

Pull request description:

  This is a followup to https://github.com/rust-bitcoin/rust-bitcoin/pull/4010 which does some simple cleanups to the hash macros and some function signatures.

ACKs for top commit:
  tcharding:
    ACK a5d96ceb39

Tree-SHA512: e8c3d8770fe3a49da4eb2d548af86cbe6e0e17efcac419815f4953b7dafffbd3e5c0be65574e08c86d09fe594d95512dfd7837534be429b1490fea973ec4d5e6

hashes/src/hash160/mod.rs

@@ -15,13 +15,16 @@ crate::internal_macros::general_hash_type! {
     "Output of the Bitcoin HASH160 hash function. (RIPEMD160(SHA256))"
 }
 
-fn from_engine(e: HashEngine) -> Hash {
+impl Hash {
+    /// Finalize a hash engine to produce a hash.
+    pub fn from_engine(e: HashEngine) -> Self {
         let sha2 = sha256::Hash::from_engine(e.0);
         let rmd = ripemd160::Hash::hash(sha2.as_byte_array());
 
         let mut ret = [0; 20];
         ret.copy_from_slice(rmd.as_byte_array());
         Hash(ret)
+    }
 }
 
 /// Engine to compute HASH160 hash function.

hashes/src/internal_macros.rs

@@ -15,10 +15,7 @@
 ///
 /// Restrictions on usage:
 ///
-/// * There must be a free-standing `fn from_engine(HashEngine) -> Hash` in the scope
-/// * `fn internal_new([u8; $bits / 8]) -> Self` must exist on `Hash`
-///
-/// `from_engine` obviously implements the finalization algorithm.
+/// * The hash type must implement the `GeneralHash` trait.
 macro_rules! hash_trait_impls {
     ($bits:expr, $reverse:expr $(, $gen:ident: $gent:ident)*) => {
         $crate::impl_bytelike_traits!(Hash, { $bits / 8 } $(, $gen: $gent)*);
@@ -62,8 +59,9 @@ pub(crate) use hash_trait_impls;
 /// * `$reverse` - `true` if the hash should be displayed backwards, `false` otherwise
 /// * `$doc` - doc string to put on the type
 ///
-/// The `from_engine` free-standing function is still required with this macro. See the doc of
-/// [`hash_trait_impls`].
+/// Restrictions on usage:
+///
+/// * The hash type must implement the `GeneralHash` trait.
 macro_rules! general_hash_type {
     ($bits:expr, $reverse:expr, $doc:literal) => {
         /// Hashes some bytes.
@@ -93,9 +91,6 @@ macro_rules! general_hash_type {
         $crate::internal_macros::hash_type_no_default!($bits, $reverse, $doc);
 
         impl Hash {
-            /// Produces a hash from the current state of a given engine.
-            pub fn from_engine(e: HashEngine) -> Hash { from_engine(e) }
-
             /// Constructs a new engine.
             pub fn engine() -> HashEngine { Default::default() }
 
@@ -135,12 +130,8 @@ macro_rules! hash_type_no_default {
         }
 
         impl Hash {
-            const fn internal_new(arr: [u8; $bits / 8]) -> Self { Hash(arr) }
-
             /// Constructs a new hash from the underlying byte array.
-            pub const fn from_byte_array(bytes: [u8; $bits / 8]) -> Self {
-                Self::internal_new(bytes)
-            }
+            pub const fn from_byte_array(bytes: [u8; $bits / 8]) -> Self { Hash(bytes) }
 
             /// Copies a byte slice into a hash object.
             #[deprecated(since = "0.15.0", note = "use `from_byte_array` instead")]
@@ -156,7 +147,7 @@ macro_rules! hash_type_no_default {
                 } else {
                     let mut ret = [0; $bits / 8];
                     ret.copy_from_slice(sl);
-                    Ok(Self::internal_new(ret))
+                    Ok(Self::from_byte_array(ret))
                 }
             }
 

hashes/src/ripemd160/mod.rs

@@ -20,8 +20,10 @@ crate::internal_macros::general_hash_type! {
     "Output of the RIPEMD160 hash function."
 }
 
-#[cfg(not(hashes_fuzz))]
-fn from_engine(mut e: HashEngine) -> Hash {
+impl Hash {
+    /// Finalize a hash engine to produce a hash.
+    #[cfg(not(hashes_fuzz))]
+    pub fn from_engine(mut e: HashEngine) -> Self {
         // pad buffer with a single 1-bit then all 0s, until there are exactly 8 bytes remaining
         let n_bytes_hashed = e.bytes_hashed;
 
@@ -38,13 +40,15 @@ fn from_engine(mut e: HashEngine) -> Hash {
         debug_assert_eq!(incomplete_block_len(&e), 0);
 
         Hash(e.midstate())
-}
+    }
 
-#[cfg(hashes_fuzz)]
-fn from_engine(e: HashEngine) -> Hash {
+    /// Finalize a hash engine to produce a hash.
+    #[cfg(hashes_fuzz)]
+    pub fn from_engine(e: HashEngine) -> Self {
         let mut res = e.midstate();
         res[0] ^= (e.bytes_hashed & 0xff) as u8;
         Hash(res)
+    }
 }
 
 const BLOCK_SIZE: usize = 64;

hashes/src/sha1/mod.rs

@@ -20,7 +20,9 @@ crate::internal_macros::general_hash_type! {
     "Output of the SHA1 hash function."
 }
 
-fn from_engine(mut e: HashEngine) -> Hash {
+impl Hash {
+    /// Finalize a hash engine to produce a hash.
+    pub fn from_engine(mut e: HashEngine) -> Self {
         // pad buffer with a single 1-bit then all 0s, until there are exactly 8 bytes remaining
         let n_bytes_hashed = e.bytes_hashed;
 
@@ -37,6 +39,7 @@ fn from_engine(mut e: HashEngine) -> Hash {
         debug_assert_eq!(incomplete_block_len(&e), 0);
 
         Hash(e.midstate())
+    }
 }
 
 const BLOCK_SIZE: usize = 64;

hashes/src/sha256/mod.rs

@@ -22,37 +22,6 @@ crate::internal_macros::general_hash_type! {
     "Output of the SHA256 hash function."
 }
 
-#[cfg(not(hashes_fuzz))]
-fn from_engine(mut e: HashEngine) -> Hash {
-    // pad buffer with a single 1-bit then all 0s, until there are exactly 8 bytes remaining
-    let n_bytes_hashed = e.bytes_hashed;
-
-    let zeroes = [0; BLOCK_SIZE - 8];
-    e.input(&[0x80]);
-    if incomplete_block_len(&e) > zeroes.len() {
-        e.input(&zeroes);
-    }
-    let pad_length = zeroes.len() - incomplete_block_len(&e);
-    e.input(&zeroes[..pad_length]);
-    debug_assert_eq!(incomplete_block_len(&e), zeroes.len());
-
-    e.input(&(8 * n_bytes_hashed).to_be_bytes());
-    debug_assert_eq!(incomplete_block_len(&e), 0);
-
-    Hash(e.midstate_unchecked().bytes)
-}
-
-#[cfg(hashes_fuzz)]
-fn from_engine(e: HashEngine) -> Hash {
-    let mut hash = e.midstate_unchecked().bytes;
-    if hash == [0; 32] {
-        // Assume sha256 is secure and never generate 0-hashes (which represent invalid
-        // secp256k1 secret keys, causing downstream application breakage).
-        hash[0] = 1;
-    }
-    Hash(hash)
-}
-
 const BLOCK_SIZE: usize = 64;
 
 /// Engine to compute SHA256 hash function.
@@ -141,6 +110,39 @@ impl crate::HashEngine for HashEngine {
 }
 
 impl Hash {
+    /// Finalize a hash engine to obtain a hash.
+    #[cfg(not(hashes_fuzz))]
+    pub fn from_engine(mut e: HashEngine) -> Self {
+        // pad buffer with a single 1-bit then all 0s, until there are exactly 8 bytes remaining
+        let n_bytes_hashed = e.bytes_hashed;
+
+        let zeroes = [0; BLOCK_SIZE - 8];
+        e.input(&[0x80]);
+        if incomplete_block_len(&e) > zeroes.len() {
+            e.input(&zeroes);
+        }
+        let pad_length = zeroes.len() - incomplete_block_len(&e);
+        e.input(&zeroes[..pad_length]);
+        debug_assert_eq!(incomplete_block_len(&e), zeroes.len());
+
+        e.input(&(8 * n_bytes_hashed).to_be_bytes());
+        debug_assert_eq!(incomplete_block_len(&e), 0);
+
+        Hash(e.midstate_unchecked().bytes)
+    }
+
+    /// Finalize a hash engine to obtain a hash.
+    #[cfg(hashes_fuzz)]
+    pub fn from_engine(e: HashEngine) -> Self {
+        let mut hash = e.midstate_unchecked().bytes;
+        if hash == [0; 32] {
+            // Assume sha256 is secure and never generate 0-hashes (which represent invalid
+            // secp256k1 secret keys, causing downstream application breakage).
+            hash[0] = 1;
+        }
+        Hash(hash)
+    }
+
     /// Iterate the sha256 algorithm to turn a sha256 hash into a sha256d hash
     #[must_use]
     pub fn hash_again(&self) -> sha256d::Hash { sha256d::Hash::from_byte_array(hash(&self.0).0) }

hashes/src/sha256d/mod.rs

@@ -10,13 +10,16 @@ crate::internal_macros::general_hash_type! {
     "Output of the SHA256d hash function."
 }
 
-fn from_engine(e: HashEngine) -> Hash {
+impl Hash {
+    /// Finalize a hash engine to produce a hash.
+    pub fn from_engine(e: HashEngine) -> Self {
         let sha2 = sha256::Hash::from_engine(e.0);
         let sha2d = sha256::Hash::hash(sha2.as_byte_array());
 
         let mut ret = [0; 32];
         ret.copy_from_slice(sha2d.as_byte_array());
         Hash(ret)
+    }
 }
 
 /// Engine to compute SHA256d hash function.

hashes/src/sha256t/mod.rs

@@ -62,10 +62,8 @@ impl<T> Hash<T>
 where
     T: Tag,
 {
-    const fn internal_new(arr: [u8; 32]) -> Self { Hash(PhantomData, arr) }
-
     /// Constructs a new hash from the underlying byte array.
-    pub const fn from_byte_array(bytes: [u8; 32]) -> Self { Self::internal_new(bytes) }
+    pub const fn from_byte_array(bytes: [u8; 32]) -> Self { Self(PhantomData, bytes) }
 
     /// Copies a byte slice into a hash object.
     #[deprecated(since = "0.15.0", note = "use `from_byte_array` instead")]
@@ -83,7 +81,7 @@ where
     }
 
     /// Produces a hash from the current state of a given engine.
-    pub fn from_engine(e: HashEngine<T>) -> Hash<T> {
+    pub fn from_engine(e: HashEngine<T>) -> Self {
         Hash::from_byte_array(sha256::Hash::from_engine(e.0).to_byte_array())
     }
 

hashes/src/sha384/mod.rs

@@ -10,10 +10,13 @@ crate::internal_macros::general_hash_type! {
     "Output of the SHA384 hash function."
 }
 
-fn from_engine(e: HashEngine) -> Hash {
+impl Hash {
+    /// Finalize a hash engine to produce a hash.
+    pub fn from_engine(e: HashEngine) -> Self {
         let mut ret = [0; 48];
-    ret.copy_from_slice(&sha512::from_engine(e.0).as_byte_array()[..48]);
+        ret.copy_from_slice(&sha512::Hash::from_engine(e.0).as_byte_array()[..48]);
         Hash(ret)
+    }
 }
 
 /// Engine to compute SHA384 hash function.

hashes/src/sha512/mod.rs

@@ -20,8 +20,10 @@ crate::internal_macros::general_hash_type! {
     "Output of the SHA512 hash function."
 }
 
-#[cfg(not(hashes_fuzz))]
-pub(crate) fn from_engine(mut e: HashEngine) -> Hash {
+impl Hash {
+    /// Finalize a hash engine to produce a hash.
+    #[cfg(not(hashes_fuzz))]
+    pub fn from_engine(mut e: HashEngine) -> Self {
         // pad buffer with a single 1-bit then all 0s, until there are exactly 16 bytes remaining
         let n_bytes_hashed = e.bytes_hashed;
 
@@ -39,13 +41,15 @@ pub(crate) fn from_engine(mut e: HashEngine) -> Hash {
         debug_assert_eq!(incomplete_block_len(&e), 0);
 
         Hash(e.midstate())
-}
+    }
 
-#[cfg(hashes_fuzz)]
-pub(crate) fn from_engine(e: HashEngine) -> Hash {
+    /// Finalize a hash engine to produce a hash.
+    #[cfg(hashes_fuzz)]
+    pub fn from_engine(e: HashEngine) -> Self {
         let mut hash = e.midstate();
         hash[0] ^= 0xff; // Make this distinct from SHA-256
         Hash(hash)
+    }
 }
 
 pub(crate) const BLOCK_SIZE: usize = 128;

hashes/src/sha512_256/mod.rs

@@ -15,10 +15,13 @@ crate::internal_macros::general_hash_type! {
     "Output of the SHA512/256 hash function.\n\nSHA512/256 is a hash function that uses the sha512 algorithm but it truncates the output to 256 bits. It has different initial constants than sha512 so it produces an entirely different hash compared to sha512. More information at <https://eprint.iacr.org/2010/548.pdf>."
 }
 
-fn from_engine(e: HashEngine) -> Hash {
+impl Hash {
+    /// Finalize a hash engine to produce a hash.
+    pub fn from_engine(e: HashEngine) -> Self {
         let mut ret = [0; 32];
-    ret.copy_from_slice(&sha512::from_engine(e.0).as_byte_array()[..32]);
+        ret.copy_from_slice(&sha512::Hash::from_engine(e.0).as_byte_array()[..32]);
         Hash(ret)
+    }
 }
 
 /// Engine to compute SHA512/256 hash function.

hashes/src/siphash24/mod.rs

@@ -12,15 +12,6 @@ crate::internal_macros::hash_type_no_default! {
     "Output of the SipHash24 hash function."
 }
 
-#[cfg(not(hashes_fuzz))]
-fn from_engine(e: HashEngine) -> Hash { Hash::from_u64(Hash::from_engine_to_u64(e)) }
-
-#[cfg(hashes_fuzz)]
-fn from_engine(e: HashEngine) -> Hash {
-    let state = e.state.clone();
-    Hash::from_u64(state.v0 ^ state.v1 ^ state.v2 ^ state.v3)
-}
-
 macro_rules! compress {
     ($state:expr) => {{
         compress!($state.v0, $state.v1, $state.v2, $state.v3)
@@ -176,7 +167,14 @@ impl Hash {
     pub fn engine(k0: u64, k1: u64) -> HashEngine { HashEngine::with_keys(k0, k1) }
 
     /// Produces a hash from the current state of a given engine.
-    pub fn from_engine(e: HashEngine) -> Hash { from_engine(e) }
+    #[cfg(not(hashes_fuzz))]
+    pub fn from_engine(e: HashEngine) -> Self { Hash::from_u64(Hash::from_engine_to_u64(e)) }
+
+    #[cfg(hashes_fuzz)]
+    pub fn from_engine(e: HashEngine) -> Self {
+        let state = e.state.clone();
+        Hash::from_u64(state.v0 ^ state.v1 ^ state.v2 ^ state.v3)
+    }
 
     /// Hashes the given data with an engine with the provided keys.
     pub fn hash_with_keys(k0: u64, k1: u64, data: &[u8]) -> Hash {