From dec05b63e9337fe93ab22e68ce544cc5d7db5dcf Mon Sep 17 00:00:00 2001
From: "Tobin C. Harding" <me@tobin.cc>
Date: Fri, 22 Mar 2024 07:07:07 +1100
Subject: [PATCH] Refactor witness_version and is_witness_program

These two functions are related. We cannot, by definition, get the
witness version from a script that is not a witness program but
currently the code is not linking these two things.

Refactor by doing:

- Move the check of the witness program bip rules to `witness_version`
- Call `witness_version().is_some()` in the predicate

Improve the docs while we are at it to include the bip text in the
rustdoc. Note I didn't bother referencing the segwit bip number, this
bip text is pretty well known.
---
 bitcoin/src/blockdata/script/borrowed.rs | 46 ++++++++++++++----------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/bitcoin/src/blockdata/script/borrowed.rs b/bitcoin/src/blockdata/script/borrowed.rs
index 6f817d5d..8e1db39b 100644
--- a/bitcoin/src/blockdata/script/borrowed.rs
+++ b/bitcoin/src/blockdata/script/borrowed.rs
@@ -165,9 +165,35 @@ impl Script {
     }
 
     /// Returns witness version of the script, if any, assuming the script is a `scriptPubkey`.
+    ///
+    /// # Returns
+    ///
+    /// The witness version if this script is found to conform to the SegWit rules:
+    ///
+    /// > A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that consists of a 1-byte
+    /// > push opcode (for 0 to 16) followed by a data push between 2 and 40 bytes gets a new
+    /// > special meaning. The value of the first push is called the "version byte". The following
+    /// > byte vector pushed is called the "witness program".
     #[inline]
     pub fn witness_version(&self) -> Option<WitnessVersion> {
-        self.0.first().and_then(|opcode| WitnessVersion::try_from(Opcode::from(*opcode)).ok())
+        let script_len = self.0.len();
+        if !(4..=42).contains(&script_len) {
+            return None;
+        }
+
+        let ver_opcode = Opcode::from(self.0[0]); // Version 0 or PUSHNUM_1-PUSHNUM_16
+        let push_opbyte = self.0[1]; // Second byte push opcode 2-40 bytes
+
+        if push_opbyte < OP_PUSHBYTES_2.to_u8() || push_opbyte > OP_PUSHBYTES_40.to_u8()
+        {
+            return None;
+        }
+        // Check that the rest of the script has the correct size
+        if script_len - 2 != push_opbyte as usize {
+            return None;
+        }
+
+        WitnessVersion::try_from(ver_opcode).ok()
     }
 
     /// Checks whether a script pubkey is a P2SH output.
@@ -293,23 +319,7 @@ impl Script {
 
     /// Checks whether a script pubkey is a Segregated Witness (segwit) program.
     #[inline]
-    pub fn is_witness_program(&self) -> bool {
-        // A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that consists of a 1-byte
-        // push opcode (for 0 to 16) followed by a data push between 2 and 40 bytes gets a new
-        // special meaning. The value of the first push is called the "version byte". The following
-        // byte vector pushed is called the "witness program".
-        let script_len = self.0.len();
-        if !(4..=42).contains(&script_len) {
-            return false;
-        }
-        let ver_opcode = Opcode::from(self.0[0]); // Version 0 or PUSHNUM_1-PUSHNUM_16
-        let push_opbyte = self.0[1]; // Second byte push opcode 2-40 bytes
-        WitnessVersion::try_from(ver_opcode).is_ok()
-            && push_opbyte >= OP_PUSHBYTES_2.to_u8()
-            && push_opbyte <= OP_PUSHBYTES_40.to_u8()
-            // Check that the rest of the script has the correct size
-            && script_len - 2 == push_opbyte as usize
-    }
+    pub fn is_witness_program(&self) -> bool { self.witness_version().is_some() }
 
     /// Checks whether a script pubkey is a P2WSH output.
     #[inline]