Merge rust-bitcoin/rust-bitcoin#2075: Bugfix: Script::count_sigops should not return a Result

026a55809e Fix: Script::count_sigops parsing should not return a Result (junderw)

Pull request description:

  When implementing some tests for the Transaction PR, I noticed that there were coinbase transactions that would pass Bitcoin Core parsing and fail my code.

  It turns out that the Script parsing for sigops calls `break`  to exit the loop and returns the current n value whenever there is an EarlyEndOfScript error.

  See this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/2073#issuecomment-1722403687 for some links to the relevant source.

ACKs for top commit:
  apoelstra:
    ACK 026a55809e
  tcharding:
    ACK 026a55809e

Tree-SHA512: 57c1b88add5e1c9ef9245fcec0e471db55c2f9b1b0b0f8ebd471f1bede0ca5eeb8492d8c75dea1fd43f1343037df44969c9b9fde26a7de9ac68a26dca899e47f

bitcoin/src/blockdata/script/borrowed.rs

@@ -350,11 +350,7 @@ impl Script {
     /// (Note: taproot scripts don't count toward the sigop count of the block,
     /// nor do they have CHECKMULTISIG operations. This function does not count OP_CHECKSIGADD,
     /// so do not use this to try and estimate if a taproot script goes over the sigop budget.)
-    ///
+    pub fn count_sigops(&self) -> usize { self.count_sigops_internal(true) }
-    /// # Errors
-    ///
-    /// If the Script is not able to be parsed to completion.
-    pub fn count_sigops(&self) -> Result<usize, super::Error> { self.count_sigops_internal(true) }
 
     /// Counts the sigops for this Script using legacy counting.
     ///
@@ -368,20 +364,14 @@ impl Script {
     /// (Note: taproot scripts don't count toward the sigop count of the block,
     /// nor do they have CHECKMULTISIG operations. This function does not count OP_CHECKSIGADD,
     /// so do not use this to try and estimate if a taproot script goes over the sigop budget.)
-    ///
+    pub fn count_sigops_legacy(&self) -> usize { self.count_sigops_internal(false) }
-    /// # Errors
-    ///
-    /// If the Script is not able to be parsed to completion.
-    pub fn count_sigops_legacy(&self) -> Result<usize, super::Error> {
-        self.count_sigops_internal(false)
-    }
 
-    fn count_sigops_internal(&self, accurate: bool) -> Result<usize, super::Error> {
+    fn count_sigops_internal(&self, accurate: bool) -> usize {
         let mut n = 0;
         let mut pushnum_cache = None;
         for inst in self.instructions() {
-            match inst? {
+            match inst {
-                Instruction::Op(opcode) => {
+                Ok(Instruction::Op(opcode)) => {
                     match opcode {
                         OP_CHECKSIG | OP_CHECKSIGVERIFY => {
                             n += 1;
@@ -404,13 +394,15 @@ impl Script {
                         }
                     }
                 }
-                Instruction::PushBytes(_) => {
+                Ok(Instruction::PushBytes(_)) => {
                     pushnum_cache = None;
                 }
+                // In Bitcoin Core it does `if (!GetOp(pc, opcode)) break;`
+                Err(_) => break,
             }
         }
 
-        Ok(n)
+        n
     }
 
     /// Iterates over the script instructions.

bitcoin/src/blockdata/script/tests.rs

@@ -625,7 +625,7 @@ fn test_script_get_sigop_count() {
             .push_opcode(OP_EQUAL)
             .into_script()
             .count_sigops(),
-        Ok(0)
+        0
     );
     assert_eq!(
         Builder::new()
@@ -636,7 +636,7 @@ fn test_script_get_sigop_count() {
             .push_opcode(OP_CHECKSIG)
             .into_script()
             .count_sigops(),
-        Ok(1)
+        1
     );
     assert_eq!(
         Builder::new()
@@ -648,7 +648,7 @@ fn test_script_get_sigop_count() {
             .push_opcode(OP_PUSHNUM_1)
             .into_script()
             .count_sigops(),
-        Ok(1)
+        1
     );
     let multi = Builder::new()
         .push_opcode(OP_PUSHNUM_1)
@@ -658,8 +658,8 @@ fn test_script_get_sigop_count() {
         .push_opcode(OP_PUSHNUM_3)
         .push_opcode(OP_CHECKMULTISIG)
         .into_script();
-    assert_eq!(multi.count_sigops(), Ok(3));
+    assert_eq!(multi.count_sigops(), 3);
-    assert_eq!(multi.count_sigops_legacy(), Ok(20));
+    assert_eq!(multi.count_sigops_legacy(), 20);
     let multi_verify = Builder::new()
         .push_opcode(OP_PUSHNUM_1)
         .push_slice([3; 33])
@@ -669,8 +669,8 @@ fn test_script_get_sigop_count() {
         .push_opcode(OP_CHECKMULTISIGVERIFY)
         .push_opcode(OP_PUSHNUM_1)
         .into_script();
-    assert_eq!(multi_verify.count_sigops(), Ok(3));
+    assert_eq!(multi_verify.count_sigops(), 3);
-    assert_eq!(multi_verify.count_sigops_legacy(), Ok(20));
+    assert_eq!(multi_verify.count_sigops_legacy(), 20);
     let multi_nopushnum_pushdata = Builder::new()
         .push_opcode(OP_PUSHNUM_1)
         .push_slice([3; 33])
@@ -678,8 +678,8 @@ fn test_script_get_sigop_count() {
         .push_slice([3; 33])
         .push_opcode(OP_CHECKMULTISIG)
         .into_script();
-    assert_eq!(multi_nopushnum_pushdata.count_sigops(), Ok(20));
+    assert_eq!(multi_nopushnum_pushdata.count_sigops(), 20);
-    assert_eq!(multi_nopushnum_pushdata.count_sigops_legacy(), Ok(20));
+    assert_eq!(multi_nopushnum_pushdata.count_sigops_legacy(), 20);
     let multi_nopushnum_op = Builder::new()
         .push_opcode(OP_PUSHNUM_1)
         .push_slice([3; 33])
@@ -687,8 +687,8 @@ fn test_script_get_sigop_count() {
         .push_opcode(OP_DROP)
         .push_opcode(OP_CHECKMULTISIG)
         .into_script();
-    assert_eq!(multi_nopushnum_op.count_sigops(), Ok(20));
+    assert_eq!(multi_nopushnum_op.count_sigops(), 20);
-    assert_eq!(multi_nopushnum_op.count_sigops_legacy(), Ok(20));
+    assert_eq!(multi_nopushnum_op.count_sigops_legacy(), 20);
 }
 
 #[test]