Repository: rust-bitcoin/rust-bitcoin
Pull request #4010 (2025-02-05 01:42:20)

Add a tagged hash engine
Add a new hash engine to the `sha256t` module and put the tag on it.

Note the issue suggests adding the tag to `sha256::HashEngine` but this PR adds a new type to `sha256t` and adds the tag on it.

Resolve: #1552

github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
apoelstra: concept ACK 73f2defa34e1a945fd0154433a6526d1427d82a1


apoelstra: > Chasing concept ack first please for the logic in this PR. If/when they convince me I can move the tag.

What does this mean? It seems like this PR is a mostly complete solution to the issue. (There are some other cleanups I'd like to do to these macros but they're not necessary.)
tcharding: Oh I thought that you'd say put the tag on `sha256::HashEngine` instead of creating a new type. I rekon its good to go as is.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
Kixunil: > Oh I thought that you'd say put the tag on `sha256::HashEngine` instead of creating a new type. I rekon its good to go as is

I think it doesn't matter much and we can most likely change it using type alias if we find it important. Oh, maybe the advantage is simply having less impls in the code.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
Kixunil: Since this should fix `GeneralHash::engine` problem, we could also add a test for it, fine to do in a followup PR.
apoelstra: Frustratingly this conflicts with #3989. (Tried it locally as well and it did not work.)
tcharding: Manually re-did the whole thing.

tcharding: > Tried it locally as well and it did not work.

Ha! Lost your confidence in GitHub huh. This one was bound to conflict. That's on me.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.
github-actions[bot]: :rotating_light: API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

Patch:
diff --git a/bitcoin/src/taproot/mod.rs b/bitcoin/src/taproot/mod.rs
index 50ea6c25..775357f0 100644
--- a/bitcoin/src/taproot/mod.rs
+++ b/bitcoin/src/taproot/mod.rs
@@ -1564,7 +1564,6 @@ impl Sealed for super::TapNodeHash {}
 #[cfg(test)]
 mod test {
     use hashes::sha256;
-    use hashes::sha256t::Tag;
     use hex::{DisplayHex, FromHex};
     use secp256k1::VerifyOnly;
 
@@ -1590,18 +1589,18 @@ fn tag_engine(tag_name: &str) -> sha256::HashEngine {
 
     #[test]
     fn midstates() {
-        use sha256t::Hash;
+        use sha256t::{Hash, Tag};
         // test that engine creation roundtrips
-        assert_eq!(tag_engine("TapLeaf").midstate(), TapLeafTag::engine().midstate());
-        assert_eq!(tag_engine("TapBranch").midstate(), TapBranchTag::engine().midstate());
-        assert_eq!(tag_engine("TapTweak").midstate(), TapTweakTag::engine().midstate());
-        assert_eq!(tag_engine("TapSighash").midstate(), TapSighashTag::engine().midstate());
+        assert_eq!(tag_engine("TapLeaf").midstate().unwrap(), TapLeafTag::MIDSTATE);
+        assert_eq!(tag_engine("TapBranch").midstate().unwrap(), TapBranchTag::MIDSTATE);
+        assert_eq!(tag_engine("TapTweak").midstate().unwrap(), TapTweakTag::MIDSTATE);
+        assert_eq!(tag_engine("TapSighash").midstate().unwrap(), TapSighashTag::MIDSTATE);
 
         // check that hash creation is the same as building into the same engine
         fn empty_hash(tag_name: &str) -> [u8; 32] {
             let mut e = tag_engine(tag_name);
             e.input(&[]);
-            Hash::<TapBranchTag>::from_engine(e).to_byte_array()
+            sha256::Hash::from_engine(e).to_byte_array()
         }
         assert_eq!(empty_hash("TapLeaf"), Hash::<TapLeafTag>::hash(&[]).to_byte_array());
         assert_eq!(empty_hash("TapBranch"), Hash::<TapBranchTag>::hash(&[]).to_byte_array());
@@ -1618,19 +1617,19 @@ fn vectors_core() {
         //   CHashWriter writer = HasherTapLeaf;
         //   writer.GetSHA256().GetHex()
         assert_eq!(
-            Hash::<TapLeafTag>::from_engine(TapLeafTag::engine()).to_string(),
+            Hash::<TapLeafTag>::from_engine(Hash::<TapLeafTag>::engine()).to_string(),
             "5212c288a377d1f8164962a5a13429f9ba6a7b84e59776a52c6637df2106facb"
         );
         assert_eq!(
-            Hash::<TapBranchTag>::from_engine(TapBranchTag::engine()).to_string(),
+            Hash::<TapBranchTag>::from_engine(Hash::<TapBranchTag>::engine()).to_string(),
             "53c373ec4d6f3c53c1f5fb2ff506dcefe1a0ed74874f93fa93c8214cbe9ffddf"
         );
         assert_eq!(
-            Hash::<TapTweakTag>::from_engine(TapTweakTag::engine()).to_string(),
+            Hash::<TapTweakTag>::from_engine(Hash::<TapTweakTag>::engine()).to_string(),
             "8aa4229474ab0100b2d6f0687f031d1fc9d8eef92a042ad97d279bff456b15e4"
         );
         assert_eq!(
-            Hash::<TapSighashTag>::from_engine(TapSighashTag::engine()).to_string(),
+            Hash::<TapSighashTag>::from_engine(Hash::<TapSighashTag>::engine()).to_string(),
             "dabc11914abcd8072900042a2681e52f8dba99ce82e224f97b5fdb7cd4b9c803"
         );
 
diff --git a/hashes/src/internal_macros.rs b/hashes/src/internal_macros.rs
index 959ccdb4..5ae79f11 100644
--- a/hashes/src/internal_macros.rs
+++ b/hashes/src/internal_macros.rs
@@ -27,9 +27,9 @@ macro_rules! hash_trait_impls {
         $crate::impl_debug_only!(Hash, { $bits / 8 }, $reverse $(, $gen: $gent)*);
 
         impl<$($gen: $gent),*> $crate::GeneralHash for Hash<$($gen),*> {
-            type Engine = HashEngine;
+            type Engine = HashEngine<$($gen),*>;
 
-            fn from_engine(e: HashEngine) -> Hash<$($gen),*> { Self::from_engine(e) }
+            fn from_engine(e: Self::Engine) -> Hash<$($gen),*> { Self::from_engine(e) }
         }
 
         #[cfg(feature = "serde")]
diff --git a/hashes/src/macros.rs b/hashes/src/macros.rs
index dc134577..12ca0152 100644
--- a/hashes/src/macros.rs
+++ b/hashes/src/macros.rs
@@ -33,11 +33,7 @@ macro_rules! sha256t_tag {
         $crate::sha256t_tag_struct!($tag_vis, $tag, stringify!($hash_name), $(#[$($tag_attr)*])*);
 
         impl $crate::sha256t::Tag for $tag {
-            #[inline]
-            fn engine() -> $crate::sha256::HashEngine {
-                const MIDSTATE: $crate::sha256::Midstate = $crate::sha256t_tag_constructor!($constructor, $($tag_value)+);
-                $crate::sha256::HashEngine::from_midstate(MIDSTATE)
-            }
+            const MIDSTATE: $crate::sha256::Midstate = $crate::sha256t_tag_constructor!($constructor, $($tag_value)+);
         }
     }
 }
diff --git a/hashes/src/sha256t/mod.rs b/hashes/src/sha256t/mod.rs
index 6c62846c..5d28b9d9 100644
--- a/hashes/src/sha256t/mod.rs
+++ b/hashes/src/sha256t/mod.rs
@@ -5,14 +5,14 @@
 use core::cmp;
 use core::marker::PhantomData;
 
+#[cfg(doc)]
+use crate::sha256::Midstate;
 use crate::{sha256, FromSliceError, HashEngine as _};
 
-type HashEngine = sha256::HashEngine;
-
 /// Trait representing a tag that can be used as a context for SHA256t hashes.
-pub trait Tag {
-    /// Returns a hash engine that is pre-tagged and is ready to be used for the data.
-    fn engine() -> sha256::HashEngine;
+pub trait Tag: Clone {
+    /// The [`Midstate`] after pre-tagging the hash engine.
+    const MIDSTATE: sha256::Midstate;
 }
 
 /// Output of the SHA256t hash function.
@@ -57,10 +57,12 @@ pub fn from_slice(sl: &[u8]) -> Result<Hash<T>, FromSliceError> {
     }
 
     /// Produces a hash from the current state of a given engine.
-    pub fn from_engine(e: HashEngine) -> Hash<T> { from_engine(e) }
+    pub fn from_engine(e: HashEngine<T>) -> Hash<T> {
+        Hash::from_byte_array(sha256::Hash::from_engine(e.0).to_byte_array())
+    }
 
     /// Constructs a new engine.
-    pub fn engine() -> HashEngine { T::engine() }
+    pub fn engine() -> HashEngine<T> { HashEngine::default() }
 
     /// Hashes some bytes.
     #[allow(clippy::self_named_constructors)] // Hash is a noun and a verb.
@@ -133,112 +135,33 @@ fn hash<H: core::hash::Hasher>(&self, h: &mut H) { self.0.hash(h) }
 
 crate::internal_macros::hash_trait_impls!(256, false, T: Tag);
 
-fn from_engine<T>(e: sha256::HashEngine) -> Hash<T>
-where
-    T: Tag,
-{
-    Hash::from_byte_array(sha256::Hash::from_engine(e).to_byte_array())
-}
-
-/// Macro used to define a newtype tagged hash.
-///
-/// This macro creates two types:
-///
-/// * a tag struct
-/// * a hash wrapper
-///
-/// The syntax is:
-///
-/// ```
-/// # #[allow(deprecated)] {
-/// # use bitcoin_hashes::sha256t_hash_newtype;
-/// sha256t_hash_newtype! {
-///     /// Optional documentation details here.
-///     /// Summary is always generated.
-///     pub struct FooTag = hash_str("foo");
-///
-///     /// A foo hash.
-///     // Direction works just like the hash_newtype! macro.
-///     #[hash_newtype(backward)]
-///     pub struct FooHash(_);
-/// }
-/// # }
-/// ```
-///
-/// The structs must be defined in this order - tag first, then hash type. `hash_str` marker
-/// says the midstate should be generated by hashing the supplied string in a way described in
-/// BIP-341. Alternatively, you can supply `hash_bytes` to hash raw bytes. If you have the midstate
-/// already pre-computed and prefer **compiler** performance to readability you may use
-/// `raw(MIDSTATE_BYTES, HASHED_BYTES_LENGTH)` instead.
-///
-/// Both visibility modifiers and attributes are optional and passed to inner structs (excluding
-/// `#[hash_newtype(...)]`). The attributes suffer same compiler performance limitations as in
-/// [`hash_newtype`] macro.
-///
-/// [`hash_newtype`]: crate::hash_newtype
-#[macro_export]
-#[deprecated(since = "0.15.0", note = "use `sha256_tag!` combined with `hash_newtype!` instead")]
-macro_rules! sha256t_hash_newtype {
-    ($(#[$($tag_attr:tt)*])* $tag_vis:vis struct $tag:ident = $constructor:tt($($tag_value:tt)+); $(#[$($hash_attr:tt)*])* $hash_vis:vis struct $hash_name:ident($(#[$($field_attr:tt)*])* _);) => {
-        $crate::sha256t_tag_struct!($tag_vis, $tag, stringify!($hash_name), $(#[$($tag_attr)*])*);
-
-        impl $crate::sha256t::Tag for $tag {
-            #[inline]
-            fn engine() -> $crate::sha256::HashEngine {
-                const MIDSTATE: $crate::sha256::Midstate = $crate::sha256t_tag_constructor!($constructor, $($tag_value)+);
-                $crate::sha256::HashEngine::from_midstate(MIDSTATE)
-            }
-        }
-
-        $crate::hash_newtype! {
-            $(#[$($hash_attr)*])*
-            $hash_vis struct $hash_name($(#[$($field_attr)*])* $crate::sha256t::Hash<$tag>);
-        }
-
-        impl $hash_name {
-            /// Constructs a new engine.
-            #[allow(unused)] // the user of macro may not need this
-            pub fn engine() -> <$hash_name as $crate::GeneralHash>::Engine {
-                <$hash_name as $crate::GeneralHash>::engine()
-            }
-
-            /// Produces a hash from the current state of a given engine.
-            #[allow(unused)] // the user of macro may not need this
-            pub fn from_engine(e: <$hash_name as $crate::GeneralHash>::Engine) -> Self {
-                <$hash_name as $crate::GeneralHash>::from_engine(e)
-            }
-
-            /// Hashes some bytes.
-            #[allow(unused)] // the user of macro may not need this
-            pub fn hash(data: &[u8]) -> Self {
-                <$hash_name as $crate::GeneralHash>::hash(data)
-            }
-
-            /// Hashes all the byte slices retrieved from the iterator together.
-            #[allow(unused)] // the user of macro may not need this
-            pub fn hash_byte_chunks<B, I>(byte_slices: I) -> Self
-            where
-                B: AsRef<[u8]>,
-                I: IntoIterator<Item = B>,
-            {
-                <$hash_name as $crate::GeneralHash>::hash_byte_chunks(byte_slices)
-            }
-        }
+/// Engine to compute SHA256t hash function.
+#[derive(Debug, Clone)]
+pub struct HashEngine<T>(sha256::HashEngine, PhantomData<T>);
 
-        impl $crate::GeneralHash for $hash_name {
-            type Engine = <$crate::sha256t::Hash<$tag> as $crate::GeneralHash>::Engine;
-
-            fn engine() -> Self::Engine {
-                <$crate::sha256t::Hash<$tag> as $crate::GeneralHash>::engine()
-            }
-
-            fn from_engine(e: Self::Engine) -> $hash_name {
-                Self::from(<$crate::sha256t::Hash<$tag> as $crate::GeneralHash>::from_engine(e))
-            }
-        }
+impl<T: Tag> Default for HashEngine<T> {
+    fn default() -> Self {
+        let tagged = sha256::HashEngine::from_midstate(T::MIDSTATE);
+        HashEngine(tagged, PhantomData)
     }
 }
 
+impl<T: Tag> crate::HashEngine for HashEngine<T> {
+    const BLOCK_SIZE: usize = 64; // Same as sha256::HashEngine::BLOCK_SIZE;
+    fn input(&mut self, data: &[u8]) { self.0.input(data) }
+    fn n_bytes_hashed(&self) -> u64 { self.0.n_bytes_hashed() }
+}
+
+crate::internal_macros::impl_io_write!(
+    HashEngine<T>,
+    |us: &mut HashEngine<T>, buf| {
+        us.input(buf);
+        Ok(buf.len())
+    },
+    |_us| { Ok(()) },
+    T: crate::sha256t::Tag
+);
+
 // Workaround macros being unavailable in attributes.
 #[doc(hidden)]
 #[macro_export]
@@ -289,11 +212,7 @@ mod tests {
     pub struct TestHashTag;
 
     impl sha256t::Tag for TestHashTag {
-        fn engine() -> sha256::HashEngine {
-            // The TapRoot TapLeaf midstate.
-            let midstate = sha256::Midstate::new(TEST_MIDSTATE, 64);
-            sha256::HashEngine::from_midstate(midstate)
-        }
+        const MIDSTATE: sha256::Midstate = sha256::Midstate::new(TEST_MIDSTATE, 64);
     }
 
     // We support manually implementing `Tag` and creating a tagged hash from it.


Test patch:
diff --git a/hashes/tests/regression.rs b/hashes/tests/regression.rs
index 0b685679..a4ffddc2 100644
--- a/hashes/tests/regression.rs
+++ b/hashes/tests/regression.rs
@@ -40,10 +40,7 @@ fn $test_name() {
 pub struct RegHashTag;
 
 impl sha256t::Tag for RegHashTag {
-    fn engine() -> sha256::HashEngine {
-        let midstate = sha256::Midstate::new([0xab; 32], 64);
-        sha256::HashEngine::from_midstate(midstate)
-    }
+    const MIDSTATE: sha256::Midstate = sha256::Midstate::new([0xab; 32], 64);
 }
 
 type RegHash = sha256t::Hash<RegHashTag>;