Repository: rust-bitcoin/rust-bitcoin
Pull request #1914 (2023-06-20 01:09:18)

Move type definitions of pubkey/script hash types
Total re-write since review. Now this PR moves the hash type definitions out of `hash_types`. Please see https://github.com/rust-bitcoin/rust-bitcoin/issues/1909#issuecomment-1603634440 for more.

No longer adds unit tests.

Fix: #1909 

yancyribbens: Also From is defined in [key.rs](https://github.com/rust-bitcoin/rust-bitcoin/blob/master/bitcoin/src/crypto/key.rs#L287) so the test could go there?  I don't really understand why the impl PubkeyHash and the struct are in different files along with the traits.
tcharding: > I don't really understand why the impl PubkeyHash and the struct are in different files along with the traits.

BOOM! Thanks man.
apoelstra: You say `new` does not make sense for `Script` since it implies ownership, but both `Path` and `PathBuf` have `new` constructors (and neither have `empty` one).

I think we should use the name `new` for both.
apoelstra: ack b798939fde1d9890a93d41abe31eaf23e4a5356e other than the `new`/`empty` thing. Also, I'm unconvinced that moving the hash types is a breaking change, but it's fine, since the `Script` constructor rename definitely will be.
tcharding: Will fix  emty/new thing as suggested.

> I'm unconvinced that moving the hash types is a breaking change

Well after this one can no longer write `use bitcoin::hash_types::PubkeyHash` and before this they can. That is a breaking change, right? IIRC I mentioned it because I intentionally did not add a re-export in `hash_types` since we are moving loads of things out of there.
tcharding: I just realized the new/empty change should not even be in this PR, its totally unrelated to pubkey/script hash - will pull it out into a separate PR.
tcharding: Added a patch `9216ab11 Move re-export of newtypes::*` to stop rustfmt messing with the code layout.
tcharding: Rebased on master (to pick up #1927). No other changes.

tcharding: Removed the first two patches (because we now have https://github.com/rust-bitcoin/rust-bitcoin/pull/1925) and rebased. 
tcharding: Pulled master and rebased, woops.

Patch:
diff --git a/bitcoin/src/address.rs b/bitcoin/src/address.rs
index ced80341..81354dee 100644
--- a/bitcoin/src/address.rs
+++ b/bitcoin/src/address.rs
@@ -43,9 +43,8 @@
 };
 use crate::blockdata::script::witness_program::{self, WitnessProgram};
 use crate::blockdata::script::witness_version::{self, WitnessVersion};
-use crate::blockdata::script::{self, Script, ScriptBuf};
-use crate::crypto::key::{PublicKey, TapTweak, TweakedPublicKey, UntweakedPublicKey};
-use crate::hash_types::{PubkeyHash, ScriptHash};
+use crate::blockdata::script::{self, Script, ScriptBuf, ScriptHash};
+use crate::crypto::key::{PubkeyHash, PublicKey, TapTweak, TweakedPublicKey, UntweakedPublicKey};
 use crate::network::Network;
 use crate::prelude::*;
 use crate::taproot::TapNodeHash;
diff --git a/bitcoin/src/blockdata/script/borrowed.rs b/bitcoin/src/blockdata/script/borrowed.rs
index 9fd12b27..64a57abc 100644
--- a/bitcoin/src/blockdata/script/borrowed.rs
+++ b/bitcoin/src/blockdata/script/borrowed.rs
@@ -14,9 +14,9 @@
 use crate::blockdata::script::witness_version::WitnessVersion;
 use crate::blockdata::script::{
     bytes_to_asm_fmt, Builder, Instruction, InstructionIndices, Instructions, ScriptBuf,
+    ScriptHash, WScriptHash,
 };
 use crate::consensus::Encodable;
-use crate::hash_types::{ScriptHash, WScriptHash};
 use crate::key::{PublicKey, UntweakedPublicKey};
 use crate::policy::DUST_RELAY_TX_FEE;
 use crate::prelude::*;
diff --git a/bitcoin/src/blockdata/script/mod.rs b/bitcoin/src/blockdata/script/mod.rs
index ee45ed45..2a341eef 100644
--- a/bitcoin/src/blockdata/script/mod.rs
+++ b/bitcoin/src/blockdata/script/mod.rs
@@ -55,13 +55,13 @@
 use core::fmt;
 use core::ops::{Deref, DerefMut};
 
+use hashes::{hash160, sha256};
 #[cfg(feature = "serde")]
 use serde;
 
 use crate::blockdata::opcodes::all::*;
 use crate::blockdata::opcodes::{self};
 use crate::consensus::{encode, Decodable, Encodable};
-use crate::hash_types::{ScriptHash, WScriptHash};
 use crate::prelude::*;
 use crate::{io, OutPoint};
 
@@ -81,6 +81,38 @@
 pub use self::owned::*;
 pub use self::push_bytes::*;
 
+hashes::hash_newtype! {
+    /// A hash of Bitcoin Script bytecode.
+    pub struct ScriptHash(hash160::Hash);
+    /// SegWit version of a Bitcoin Script bytecode hash.
+    pub struct WScriptHash(sha256::Hash);
+}
+crate::hash_types::impl_asref_push_bytes!(ScriptHash, WScriptHash);
+
+impl From<ScriptBuf> for ScriptHash {
+    fn from(script: ScriptBuf) -> ScriptHash { script.script_hash() }
+}
+
+impl From<&ScriptBuf> for ScriptHash {
+    fn from(script: &ScriptBuf) -> ScriptHash { script.script_hash() }
+}
+
+impl From<&Script> for ScriptHash {
+    fn from(script: &Script) -> ScriptHash { script.script_hash() }
+}
+
+impl From<ScriptBuf> for WScriptHash {
+    fn from(script: ScriptBuf) -> WScriptHash { script.wscript_hash() }
+}
+
+impl From<&ScriptBuf> for WScriptHash {
+    fn from(script: &ScriptBuf) -> WScriptHash { script.wscript_hash() }
+}
+
+impl From<&Script> for WScriptHash {
+    fn from(script: &Script) -> WScriptHash { script.wscript_hash() }
+}
+
 /// Encodes an integer in script(minimal CScriptNum) format.
 ///
 /// Writes bytes into the buffer and returns the number of bytes written.
@@ -304,30 +336,6 @@ impl From<ScriptBuf> for Vec<u8> {
     fn from(v: ScriptBuf) -> Self { v.0 }
 }
 
-impl From<ScriptBuf> for ScriptHash {
-    fn from(script: ScriptBuf) -> ScriptHash { script.script_hash() }
-}
-
-impl From<&ScriptBuf> for ScriptHash {
-    fn from(script: &ScriptBuf) -> ScriptHash { script.script_hash() }
-}
-
-impl From<&Script> for ScriptHash {
-    fn from(script: &Script) -> ScriptHash { script.script_hash() }
-}
-
-impl From<ScriptBuf> for WScriptHash {
-    fn from(script: ScriptBuf) -> WScriptHash { script.wscript_hash() }
-}
-
-impl From<&ScriptBuf> for WScriptHash {
-    fn from(script: &ScriptBuf) -> WScriptHash { script.wscript_hash() }
-}
-
-impl From<&Script> for WScriptHash {
-    fn from(script: &Script) -> WScriptHash { script.wscript_hash() }
-}
-
 impl AsRef<Script> for Script {
     #[inline]
     fn as_ref(&self) -> &Script { self }
diff --git a/bitcoin/src/blockdata/script/owned.rs b/bitcoin/src/blockdata/script/owned.rs
index 24f8f83f..baebebd0 100644
--- a/bitcoin/src/blockdata/script/owned.rs
+++ b/bitcoin/src/blockdata/script/owned.rs
@@ -9,9 +9,12 @@
 use crate::blockdata::opcodes::{self};
 use crate::blockdata::script::witness_program::WitnessProgram;
 use crate::blockdata::script::witness_version::WitnessVersion;
-use crate::blockdata::script::{opcode_to_verify, Builder, Instruction, PushBytes, Script};
-use crate::hash_types::{PubkeyHash, ScriptHash, WPubkeyHash, WScriptHash};
-use crate::key::{PublicKey, TapTweak, TweakedPublicKey, UntweakedPublicKey};
+use crate::blockdata::script::{
+    opcode_to_verify, Builder, Instruction, PushBytes, Script, ScriptHash, WScriptHash,
+};
+use crate::key::{
+    PubkeyHash, PublicKey, TapTweak, TweakedPublicKey, UntweakedPublicKey, WPubkeyHash,
+};
 use crate::prelude::*;
 use crate::taproot::TapNodeHash;
 
diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs
index fcfe14e7..5ac93873 100644
--- a/bitcoin/src/crypto/key.rs
+++ b/bitcoin/src/crypto/key.rs
@@ -17,7 +17,6 @@
 pub use secp256k1::{self, constants, KeyPair, Parity, Secp256k1, Verification, XOnlyPublicKey};
 
 use crate::crypto::ecdsa;
-use crate::hash_types::{PubkeyHash, WPubkeyHash};
 use crate::network::Network;
 use crate::prelude::*;
 use crate::taproot::{TapNodeHash, TapTweakHash};
@@ -299,10 +298,22 @@ fn from_str(s: &str) -> Result<PublicKey, Error> {
     }
 }
 
+hashes::hash_newtype! {
+    /// A hash of a public key.
+    pub struct PubkeyHash(hash160::Hash);
+    /// SegWit version of a public key hash.
+    pub struct WPubkeyHash(hash160::Hash);
+}
+crate::hash_types::impl_asref_push_bytes!(PubkeyHash, WPubkeyHash);
+
 impl From<PublicKey> for PubkeyHash {
     fn from(key: PublicKey) -> PubkeyHash { key.pubkey_hash() }
 }
 
+impl From<&PublicKey> for PubkeyHash {
+    fn from(key: &PublicKey) -> PubkeyHash { key.pubkey_hash() }
+}
+
 /// A Bitcoin ECDSA private key
 #[derive(Copy, Clone, PartialEq, Eq)]
 #[cfg_attr(feature = "std", derive(Debug))]
diff --git a/bitcoin/src/hash_types.rs b/bitcoin/src/hash_types.rs
index b9b9f989..54e0d636 100644
--- a/bitcoin/src/hash_types.rs
+++ b/bitcoin/src/hash_types.rs
@@ -46,13 +46,13 @@ fn from(hash: $hashtype) -> Self {
         )*
     };
 }
-
+pub(crate) use impl_asref_push_bytes;
 // newtypes module is solely here so we can rustfmt::skip.
 pub use newtypes::*;
 
 #[rustfmt::skip]
 mod newtypes {
-    use hashes::{sha256, sha256d, hash160, hash_newtype};
+    use hashes::{sha256d, hash160, hash_newtype};
 
     hash_newtype! {
         /// A bitcoin transaction hash/transaction ID.
@@ -69,15 +69,6 @@ mod newtypes {
         /// A bitcoin block hash.
         pub struct BlockHash(sha256d::Hash);
 
-        /// A hash of a public key.
-        pub struct PubkeyHash(hash160::Hash);
-        /// A hash of Bitcoin Script bytecode.
-        pub struct ScriptHash(hash160::Hash);
-        /// SegWit version of a public key hash.
-        pub struct WPubkeyHash(hash160::Hash);
-        /// SegWit version of a Bitcoin Script bytecode hash.
-        pub struct WScriptHash(sha256::Hash);
-
         /// A hash of the Merkle tree branch or root for transactions
         pub struct TxMerkleNode(sha256d::Hash);
         /// A hash corresponding to the Merkle tree root for witness data
@@ -102,6 +93,4 @@ mod newtypes {
 
     impl_hashencode!(FilterHash);
     impl_hashencode!(FilterHeader);
-
-    impl_asref_push_bytes!(PubkeyHash, ScriptHash, WPubkeyHash, WScriptHash);
 }
diff --git a/bitcoin/src/lib.rs b/bitcoin/src/lib.rs
index 0665662a..38a2bf36 100644
--- a/bitcoin/src/lib.rs
+++ b/bitcoin/src/lib.rs
@@ -135,17 +135,15 @@
 pub use crate::blockdata::locktime::{self, absolute, relative};
 pub use crate::blockdata::script::witness_program::{self, WitnessProgram};
 pub use crate::blockdata::script::witness_version::{self, WitnessVersion};
-pub use crate::blockdata::script::{self, Script, ScriptBuf};
+pub use crate::blockdata::script::{self, Script, ScriptBuf, ScriptHash, WScriptHash};
 pub use crate::blockdata::transaction::{self, OutPoint, Sequence, Transaction, TxIn, TxOut};
 pub use crate::blockdata::weight::Weight;
 pub use crate::blockdata::witness::{self, Witness};
 pub use crate::blockdata::{constants, opcodes};
 pub use crate::consensus::encode::VarInt;
-pub use crate::crypto::key::{self, PrivateKey, PublicKey};
+pub use crate::crypto::key::{self, PrivateKey, PubkeyHash, PublicKey, WPubkeyHash};
 pub use crate::crypto::{ecdsa, sighash};
-pub use crate::hash_types::{
-    BlockHash, PubkeyHash, ScriptHash, Txid, WPubkeyHash, WScriptHash, Wtxid,
-};
+pub use crate::hash_types::{BlockHash, Txid, Wtxid};
 pub use crate::merkle_tree::MerkleBlock;
 pub use crate::network::Network;
 pub use crate::pow::{CompactTarget, Target, Work};


Test patch:
diff --git a/bitcoin/src/blockdata/script/tests.rs b/bitcoin/src/blockdata/script/tests.rs
index c1bf41d3..4b494a46 100644
--- a/bitcoin/src/blockdata/script/tests.rs
+++ b/bitcoin/src/blockdata/script/tests.rs
@@ -8,8 +8,7 @@
 use super::*;
 use crate::blockdata::opcodes;
 use crate::consensus::encode::{deserialize, serialize};
-use crate::crypto::key::{PublicKey, XOnlyPublicKey};
-use crate::hash_types::{PubkeyHash, WPubkeyHash};
+use crate::crypto::key::{PubkeyHash, PublicKey, WPubkeyHash, XOnlyPublicKey};
 
 #[test]
 #[rustfmt::skip]