Repository: rust-bitcoin/rust-bitcoin
Pull request #2075 (2023-09-17 18:37:38)

Bugfix: Script::count_sigops should not return a Result
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.



apoelstra: 18df6b637745ff00762db1d4b71341978cbfa549 looks good to me.

I'm a little on-the-fence about copying Core's behavior which seems (to me) to be crazy, but this is consensus code (though I don't think the behavior on unevaluated scripts is consensus-critical) so I guess we should go along with it.

Patch:
diff --git a/bitcoin/src/blockdata/script/borrowed.rs b/bitcoin/src/blockdata/script/borrowed.rs
index 3e9c36a3..bc3c49c9 100644
--- a/bitcoin/src/blockdata/script/borrowed.rs
+++ b/bitcoin/src/blockdata/script/borrowed.rs
@@ -350,11 +350,7 @@ pub fn dust_value(&self) -> crate::Amount {
     /// (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.)
-    ///
-    /// # 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) }
+    pub fn count_sigops(&self) -> usize { self.count_sigops_internal(true) }
 
     /// Counts the sigops for this Script using legacy counting.
     ///
@@ -368,20 +364,14 @@ pub fn count_sigops(&self) -> Result<usize, super::Error> { self.count_sigops_in
     /// (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.)
-    ///
-    /// # 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)
-    }
+    pub fn count_sigops_legacy(&self) -> usize { 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? {
-                Instruction::Op(opcode) => {
+            match inst {
+                Ok(Instruction::Op(opcode)) => {
                     match opcode {
                         OP_CHECKSIG | OP_CHECKSIGVERIFY => {
                             n += 1;
@@ -404,13 +394,15 @@ fn count_sigops_internal(&self, accurate: bool) -> Result<usize, super::Error> {
                         }
                     }
                 }
-                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.


Test patch:
diff --git a/bitcoin/src/blockdata/script/tests.rs b/bitcoin/src/blockdata/script/tests.rs
index 4b494a46..f8e7e930 100644
--- a/bitcoin/src/blockdata/script/tests.rs
+++ b/bitcoin/src/blockdata/script/tests.rs
@@ -616,7 +616,7 @@ fn test_script_get_sigop_count() {
             .push_opcode(OP_EQUAL)
             .into_script()
             .count_sigops(),
-        Ok(0)
+        0
     );
     assert_eq!(
         Builder::new()
@@ -627,7 +627,7 @@ fn test_script_get_sigop_count() {
             .push_opcode(OP_CHECKSIG)
             .into_script()
             .count_sigops(),
-        Ok(1)
+        1
     );
     assert_eq!(
         Builder::new()
@@ -639,7 +639,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)
@@ -649,8 +649,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_legacy(), Ok(20));
+    assert_eq!(multi.count_sigops(), 3);
+    assert_eq!(multi.count_sigops_legacy(), 20);
     let multi_verify = Builder::new()
         .push_opcode(OP_PUSHNUM_1)
         .push_slice([3; 33])
@@ -660,8 +660,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_legacy(), Ok(20));
+    assert_eq!(multi_verify.count_sigops(), 3);
+    assert_eq!(multi_verify.count_sigops_legacy(), 20);
     let multi_nopushnum_pushdata = Builder::new()
         .push_opcode(OP_PUSHNUM_1)
         .push_slice([3; 33])
@@ -669,8 +669,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_legacy(), Ok(20));
+    assert_eq!(multi_nopushnum_pushdata.count_sigops(), 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])
@@ -678,8 +678,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_legacy(), Ok(20));
+    assert_eq!(multi_nopushnum_op.count_sigops(), 20);
+    assert_eq!(multi_nopushnum_op.count_sigops_legacy(), 20);
 }
 
 #[test]