Repository: rust-bitcoin/rust-bitcoin
Pull request #1995 (2023-08-14 03:59:44)

Improve segwit signature hash API
The word "segwit" refers to segwit v0 and taproot but these functions are version specific. Add `v0` into the function names.

This is similar to #1994, both based on recent post of mine to bitcoin dev mailing list.

apoelstra: concept ACK. Though I wonder if we want to split these into `wpkh` and `wsh` variants rather than a single `v0_segwit` version. (See also #1932)
junderw: > BIP-143

0xcf is 207, the number of bytes in the redeemScript. It is a PUSH_DATA OP code with the encoded length.

Since the input's scriptSig will contain the script `OP_PUSHDATA <redeemscript>` that is why we don't need to add it.
apoelstra: @tcharding `0xcf` is a length prefix and it is automatically added when consensus encoding byte vectors such as scripts.
apoelstra: @tcharding `0xcf` is a length prefix and it is automatically added when consensus encoding byte vectors such as scripts.
tcharding: ah BOOM! I've been bitten by that one before :)
tcharding: Added some more docs.

Patch:
diff --git a/README.md b/README.md
index a7cd5117..57cdc8a4 100644
--- a/README.md
+++ b/README.md
@@ -82,8 +82,6 @@ cargo update -p proc-macro2 --precise 1.0.63
 cargo update -p serde_test --precise 1.0.175
 ```
 
-The above commands are sourced from `./contrib/test.sh`.
-
 ## External dependencies
 
 We integrate with a few external libraries, most notably `serde`. These
diff --git a/bitcoin/examples/sighash.rs b/bitcoin/examples/sighash.rs
index 9bb3a497..0230f379 100644
--- a/bitcoin/examples/sighash.rs
+++ b/bitcoin/examples/sighash.rs
@@ -38,11 +38,10 @@ fn compute_sighash_p2wpkh(raw_tx: &[u8], inp_idx: usize, value: u64) {
     let wpkh = pk.wpubkey_hash().expect("compressed key");
     println!("Script pubkey hash: {:x}", wpkh);
     let spk = ScriptBuf::new_v0_p2wpkh(&wpkh);
-    let script_code = spk.p2wpkh_script_code().expect("spk is known to be p2wpkh");
 
     let mut cache = sighash::SighashCache::new(&tx);
     let sighash = cache
-        .segwit_signature_hash(inp_idx, &script_code, Amount::from_sat(value), sig.hash_ty)
+        .p2wpkh_signature_hash(inp_idx, &spk, Amount::from_sat(value), sig.hash_ty)
         .expect("failed to compute sighash");
     println!("Segwit p2wpkh sighash: {:x}", sighash);
     let msg = secp256k1::Message::from(sighash);
@@ -124,7 +123,7 @@ fn compute_sighash_p2wsh(raw_tx: &[u8], inp_idx: usize, value: u64) {
         assert!((70..=72).contains(&sig_len), "signature length {} out of bounds", sig_len);
         //here we assume that all sighash_flags are the same. Can they be different?
         let sighash = cache
-            .segwit_signature_hash(inp_idx, witness_script, Amount::from_sat(value), sig.hash_ty)
+            .p2wsh_signature_hash(inp_idx, witness_script, Amount::from_sat(value), sig.hash_ty)
             .expect("failed to compute sighash");
         println!("Segwit p2wsh sighash: {:x} ({})", sighash, sig.hash_ty);
     }
diff --git a/bitcoin/src/crypto/sighash.rs b/bitcoin/src/crypto/sighash.rs
index 751410a9..ddc827c4 100644
--- a/bitcoin/src/crypto/sighash.rs
+++ b/bitcoin/src/crypto/sighash.rs
@@ -239,6 +239,9 @@ pub enum Error {
 
     /// Invalid Sighash type.
     InvalidSighashType(u32),
+
+    /// Script is not a witness program for a p2wpkh output.
+    NotP2wpkhScript,
 }
 
 impl fmt::Display for Error {
@@ -254,6 +257,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
             PrevoutKind => write!(f, "a single prevout has been provided but all prevouts are needed without `ANYONECANPAY`"),
             WrongAnnex => write!(f, "annex must be at least one byte long and the first bytes must be `0x50`"),
             InvalidSighashType(hash_ty) => write!(f, "Invalid taproot signature hash type : {} ", hash_ty),
+            NotP2wpkhScript => write!(f, "script is not a script pubkey for a p2wpkh output"),
         }
     }
 }
@@ -271,7 +275,8 @@ fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
             | PrevoutIndex
             | PrevoutKind
             | WrongAnnex
-            | InvalidSighashType(_) => None,
+            | InvalidSighashType(_)
+            | NotP2wpkhScript => None,
         }
     }
 }
@@ -761,7 +766,25 @@ pub fn taproot_script_spend_signature_hash<S: Into<TapLeafHash>, T: Borrow<TxOut
 
     /// Encodes the BIP143 signing data for any flag type into a given object implementing a
     /// [`std::io::Write`] trait.
+    #[deprecated(since = "0.31.0", note = "use segwit_v0_encode_signing_data_to instead")]
     pub fn segwit_encode_signing_data_to<Write: io::Write>(
+        &mut self,
+        writer: Write,
+        input_index: usize,
+        script_code: &Script,
+        value: Amount,
+        sighash_type: EcdsaSighashType,
+    ) -> Result<(), Error> {
+        self.segwit_v0_encode_signing_data_to(writer, input_index, script_code, value, sighash_type)
+    }
+
+    /// Encodes the BIP143 signing data for any flag type into a given object implementing a
+    /// [`std::io::Write`] trait.
+    ///
+    /// `script_code` is dependent on the type of the spend transaction. For p2wpkh use
+    /// [`Script::p2wpkh_script_code`], for p2wsh just pass in the witness script. (Also see
+    /// [`Self::p2wpkh_signature_hash`] and [`SighashCache::p2wsh_signature_hash`].)
+    pub fn segwit_v0_encode_signing_data_to<Write: io::Write>(
         &mut self,
         mut writer: Write,
         input_index: usize,
@@ -821,6 +844,7 @@ pub fn segwit_encode_signing_data_to<Write: io::Write>(
     }
 
     /// Computes the BIP143 sighash for any flag type.
+    #[deprecated(since = "0.31.0", note = "use segwit_v0_signature_hash instead")]
     pub fn segwit_signature_hash(
         &mut self,
         input_index: usize,
@@ -829,7 +853,7 @@ pub fn segwit_signature_hash(
         sighash_type: EcdsaSighashType,
     ) -> Result<SegwitV0Sighash, Error> {
         let mut enc = SegwitV0Sighash::engine();
-        self.segwit_encode_signing_data_to(
+        self.segwit_v0_encode_signing_data_to(
             &mut enc,
             input_index,
             script_code,
@@ -839,6 +863,49 @@ pub fn segwit_signature_hash(
         Ok(SegwitV0Sighash::from_engine(enc))
     }
 
+    /// Computes the BIP143 sighash to spend a p2wpkh transaction for any flag type.
+    ///
+    /// `script_pubkey` is the `scriptPubkey` (native segwit) of the spend transaction
+    /// ([`TxOut::script_pubkey`]) or the `redeemScript` (wrapped segwit).
+    pub fn p2wpkh_signature_hash(
+        &mut self,
+        input_index: usize,
+        script_pubkey: &Script,
+        value: Amount,
+        sighash_type: EcdsaSighashType,
+    ) -> Result<SegwitV0Sighash, Error> {
+        let script_code = script_pubkey.p2wpkh_script_code().ok_or(Error::NotP2wpkhScript)?;
+
+        let mut enc = SegwitV0Sighash::engine();
+        self.segwit_v0_encode_signing_data_to(
+            &mut enc,
+            input_index,
+            &script_code,
+            value,
+            sighash_type,
+        )?;
+        Ok(SegwitV0Sighash::from_engine(enc))
+    }
+
+    /// Computes the BIP143 sighash to spend a p2wsh transaction for any flag type.
+    pub fn p2wsh_signature_hash(
+        &mut self,
+        input_index: usize,
+        witness_script: &Script,
+        value: Amount,
+        sighash_type: EcdsaSighashType,
+    ) -> Result<SegwitV0Sighash, Error> {
+        let mut enc = SegwitV0Sighash::engine();
+        self.segwit_v0_encode_signing_data_to(
+            &mut enc,
+            input_index,
+            witness_script,
+            value,
+            sighash_type,
+        )?;
+        Ok(SegwitV0Sighash::from_engine(enc))
+    }
+
     /// Encodes the legacy signing data from which a signature hash for a given input index with a
     /// given sighash flag can be computed.
     ///
@@ -1194,13 +1261,10 @@ mod tests {
     use hex::FromHex;
 
     use super::*;
-    use crate::address::Address;
     use crate::blockdata::locktime::absolute;
     use crate::consensus::deserialize;
-    use crate::crypto::key::PublicKey;
     use crate::crypto::sighash::{LegacySighash, TapSighash};
     use crate::internal_macros::hex;
-    use crate::network::Network;
     use crate::taproot::TapLeafHash;
 
     extern crate serde_json;
@@ -1739,11 +1803,6 @@ fn sighashtype_fromstr_display() {
         }
     }
 
-    fn p2pkh_hex(pk: &str) -> ScriptBuf {
-        let pk: PublicKey = PublicKey::from_str(pk).unwrap();
-        Address::p2pkh(&pk, Network::Bitcoin).script_pubkey()
-    }
-
     #[test]
     fn bip143_p2wpkh() {
         let tx = deserialize::<Transaction>(
@@ -1755,13 +1814,12 @@ fn bip143_p2wpkh() {
             ),
         ).unwrap();
 
-        let witness_script =
-            p2pkh_hex("025476c2e83188368da1ff3e292e7acafcdb3566bb0ad253f62fc70f07aeee6357");
+        let spk = ScriptBuf::from_hex("00141d0f172a0ecb48aee1be1f2687d2963ae33f71a1").unwrap();
         let value = Amount::from_sat(600_000_000);
 
         let mut cache = SighashCache::new(&tx);
         assert_eq!(
-            cache.segwit_signature_hash(1, &witness_script, value, EcdsaSighashType::All).unwrap(),
+            cache.p2wpkh_signature_hash(1, &spk, value, EcdsaSighashType::All).unwrap(),
             "c37af31116d1b27caf68aae9e3ac82f1477929014d5b917657d0eb49478cb670"
                 .parse::<SegwitV0Sighash>()
                 .unwrap(),
@@ -1796,13 +1854,13 @@ fn bip143_p2wpkh_nested_in_p2sh() {
             ),
         ).unwrap();
 
-        let witness_script =
-            p2pkh_hex("03ad1d8e89212f0b92c74d23bb710c00662ad1470198ac48c43f7d6f93a2a26873");
+        let redeem_script =
+            ScriptBuf::from_hex("001479091972186c449eb1ded22b78e40d009bdf0089").unwrap();
         let value = Amount::from_sat(1_000_000_000);
 
         let mut cache = SighashCache::new(&tx);
         assert_eq!(
-            cache.segwit_signature_hash(0, &witness_script, value, EcdsaSighashType::All).unwrap(),
+            cache.p2wpkh_signature_hash(0, &redeem_script, value, EcdsaSighashType::All).unwrap(),
             "64f3b0f4dd2bb3aa1ce8566d220cc74dda9df97d8490cc81d89d735c92e59fb6"
                 .parse::<SegwitV0Sighash>()
                 .unwrap(),
@@ -1827,8 +1885,10 @@ fn bip143_p2wpkh_nested_in_p2sh() {
         );
     }
 
-    #[test]
-    fn bip143_p2wsh_nested_in_p2sh() {
+    // Note, if you are looking at the test vectors in BIP-143 and wondering why there is a `cf`
+    // prepended to all the script_code hex it is the length byte, it gets added when we consensus
+    // encode a script.
+    fn bip143_p2wsh_nested_in_p2sh_data() -> (Transaction, ScriptBuf, Amount) {
         let tx = deserialize::<Transaction>(&hex!(
             "010000000136641869ca081e70f394c6948e8af409e18b619df2ed74aa106c1ca29787b96e0100000000\
              ffffffff0200e9a435000000001976a914389ffce9cd9ae88dcc0631e88a821ffdbe9bfe2688acc0832f\
@@ -1845,16 +1905,27 @@ fn bip143_p2wsh_nested_in_p2sh() {
              56ae",
         )
         .unwrap();
+
         let value = Amount::from_sat(987_654_321);
+        (tx, witness_script, value)
+    }
 
+    #[test]
+    fn bip143_p2wsh_nested_in_p2sh_sighash_type_all() {
+        let (tx, witness_script, value) = bip143_p2wsh_nested_in_p2sh_data();
         let mut cache = SighashCache::new(&tx);
         assert_eq!(
-            cache.segwit_signature_hash(0, &witness_script, value, EcdsaSighashType::All).unwrap(),
+            cache.p2wsh_signature_hash(0, &witness_script, value, EcdsaSighashType::All).unwrap(),
             "185c0be5263dce5b4bb50a047973c1b6272bfbd0103a89444597dc40b248ee7c"
                 .parse::<SegwitV0Sighash>()
                 .unwrap(),
         );
 
+        // We only test the cache intermediate values for `EcdsaSighashType::All` because they are
+        // not the same as the BIP test vectors for all the rest of the sighash types. These fields
+        // are private so it does not effect sighash cache usage, we do test against the produced
+        // sighash for all sighash types.
+
         let cache = cache.segwit_cache();
         // Parse hex into Vec because BIP143 test vector displays forwards but our sha256d::Hash displays backwards.
         assert_eq!(
@@ -1873,4 +1944,34 @@ fn bip143_p2wsh_nested_in_p2sh() {
                 .unwrap()[..],
         );
     }
+
+    macro_rules! check_bip143_p2wsh_nested_in_p2sh {
+        ($($test_name:ident, $sighash_type:ident, $sighash:literal);* $(;)?) => {
+            $(
+                #[test]
+                fn $test_name() {
+                    use EcdsaSighashType::*;
+
+                    let (tx, witness_script, value) = bip143_p2wsh_nested_in_p2sh_data();
+                    let mut cache = SighashCache::new(&tx);
+                    assert_eq!(
+                        cache
+                            .p2wsh_signature_hash(0, &witness_script, value, $sighash_type)
+                            .unwrap(),
+                        $sighash
+                            .parse::<SegwitV0Sighash>()
+                            .unwrap(),
+                    );
+                }
+            )*
+        }
+    }
+    check_bip143_p2wsh_nested_in_p2sh! {
+        // EcdsaSighashType::All tested above.
+        bip143_p2wsh_nested_in_p2sh_sighash_none, None, "e9733bc60ea13c95c6527066bb975a2ff29a925e80aa14c213f686cbae5d2f36";
+        bip143_p2wsh_nested_in_p2sh_sighash_single, Single, "1e1f1c303dc025bd664acb72e583e933fae4cff9148bf78c157d1e8f78530aea";
+        bip143_p2wsh_nested_in_p2sh_sighash_all_plus_anyonecanpay, AllPlusAnyoneCanPay, "2a67f03e63a6a422125878b40b82da593be8d4efaafe88ee528af6e5a9955c6e";
+        bip143_p2wsh_nested_in_p2sh_sighash_none_plus_anyonecanpay, NonePlusAnyoneCanPay, "781ba15f3779d5542ce8ecb5c18716733a5ee42a6f51488ec96154934e2c890a";
+        bip143_p2wsh_nested_in_p2sh_sighash_single_plus_anyonecanpay, SinglePlusAnyoneCanPay, "511e8e52ed574121fc1b654970395502128263f62662e076dc6baf05c2e6a99b";
+    }
 }
diff --git a/bitcoin/src/lib.rs b/bitcoin/src/lib.rs
index a4a74a50..57482e41 100644
--- a/bitcoin/src/lib.rs
+++ b/bitcoin/src/lib.rs
@@ -140,16 +140,13 @@
 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, PubkeyHash, PublicKey, WPubkeyHash, XOnlyPublicKey,
-};
+pub use crate::crypto::key::{self, PrivateKey, PubkeyHash, PublicKey, WPubkeyHash};
 pub use crate::crypto::{ecdsa, sighash};
 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};
 pub use crate::psbt::Psbt;
-pub use crate::sighash::{EcdsaSighashType, TapSighashType};
 
 #[cfg(not(feature = "std"))]
 mod io_extras {
diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs
index df9f6c90..70143433 100644
--- a/bitcoin/src/psbt/mod.rs
+++ b/bitcoin/src/psbt/mod.rs
@@ -335,27 +335,20 @@ pub fn sighash_ecdsa<T: Borrow<Transaction>>(
                 Ok((Message::from(sighash), hash_ty))
             }
             Wpkh => {
-                let script_code = spk.p2wpkh_script_code().ok_or(SignError::NotWpkh)?;
-                let sighash =
-                    cache.segwit_signature_hash(input_index, &script_code, utxo.value, hash_ty)?;
+                let sighash = cache.p2wpkh_signature_hash(input_index, spk, utxo.value, hash_ty)?;
                 Ok((Message::from(sighash), hash_ty))
             }
             ShWpkh => {
-                let script_code = input
-                    .redeem_script
-                    .as_ref()
-                    .expect("checked above")
-                    .p2wpkh_script_code()
-                    .ok_or(SignError::NotWpkh)?;
+                let redeem_script = input.redeem_script.as_ref().expect("checked above");
                 let sighash =
-                    cache.segwit_signature_hash(input_index, &script_code, utxo.value, hash_ty)?;
+                    cache.p2wpkh_signature_hash(input_index, redeem_script, utxo.value, hash_ty)?;
                 Ok((Message::from(sighash), hash_ty))
             }
             Wsh | ShWsh => {
-                let script_code =
+                let witness_script =
                     input.witness_script.as_ref().ok_or(SignError::MissingWitnessScript)?;
                 let sighash =
-                    cache.segwit_signature_hash(input_index, script_code, utxo.value, hash_ty)?;
+                    cache.p2wsh_signature_hash(input_index, witness_script, utxo.value, hash_ty)?;
                 Ok((Message::from(sighash), hash_ty))
             }
             Tr => {


Test patch:
diff --git a/contrib/test.sh b/contrib/test.sh
index 74f8ffb3..adee9fdf 100755
--- a/contrib/test.sh
+++ b/contrib/test.sh
@@ -22,25 +22,25 @@ fi
 
 for dep in $DEPS
 do
-    cp "Cargo-$dep.lock" Cargo.lock
-    for crate in ${CRATES}
-    do
-        (
-            cd "$crate"
-            ./contrib/test.sh
-        )
-    done
-    if [ "$dep" = recent ];
-    then
-        # We always test committed dependencies but we want to warn if they could've been updated
-        cargo update
-        if diff Cargo-recent.lock Cargo.lock;
-        then
-            echo Dependencies are up to date
-        else
-            echo "::warning file=Cargo-recent.lock::Dependencies could be updated"
-        fi
-    fi
+	cp "Cargo-$dep.lock" Cargo.lock
+	for crate in ${CRATES}
+	do
+	    (
+		cd "$crate"
+		./contrib/test.sh
+	    )
+	done
+	if [ "$dep" = recent ];
+	then
+		# We always test committed dependencies but we want to warn if they could've been updated
+		cargo update
+		if diff Cargo-recent.lock Cargo.lock;
+		then
+			echo Dependencies are up to date
+		else
+			echo "::warning file=Cargo-recent.lock::Dependencies could be updated"
+		fi
+	fi
 done
 
 exit 0