Repository: rust-bitcoin/rust-bitcoin
Pull request #2078 (2023-09-18 05:43:45)

Script helper to classify bare multisig
A new `is_multisig` helper method to classify bare multisig output scripts.

The form of a valid multisig script is:
- Pushnum `M`
- &lt;N&gt; pubkeys
- Pushnum `N`
- `OP_CHECKMULTISIG`

`N` must equal the number of pushed pubkeys, and `M` must be less than or equal to `N`.

I've tested this against the RPC output of Core at the block level, checking that the total number of multisig outputs matches.

```
Block 350338, 89 multisig
Block 350340, 29 multisig
Block 350341, 4 multisig
Block 350343, 579 multisig
Block 350344, 48 multisig
Block 350346, 11 multisig
Block 350347, 404 multisig
Block 350350, 127 multisig
Block 350351, 1 multisig
Block 350353, 40 multisig
Block 350356, 13 multisig
Block 350357, 2 multisig
Block 350358, 1 multisig
```

apoelstra: ACK ce9958fe2aac845d9a413fd3e94449d496383a56 except can you rename it to `is_bare_multisig`? Today this is maybe the least-common multisig mode so we should make it clearer that it's something a little weird.

Since other reviewers may flag it: I think it's also fine that we do no length-checking or other form of checking on the "pubkeys". If they're pushes that go into CHECKMULTISIG, and there's the right number of them, then as far as Script is concerned they're public keys.


tcharding: > Since other reviewers may flag it: I think it's also fine that we do no length-checking or other form of checking on the "pubkeys". If they're pushes that go into CHECKMULTISIG, and there's the right number of them, then as far as Script is concerned they're public keys.

Just another opinion; we check `required_sigs > num_sigs`, this implies, at least to me, that we are doing some form of validity checks on the content of the script, which implies that we should check the length of the pubkeys. Its no biggy but its not hard to add 
```rust
                   use secp256k1::constants::{PUBLIC_KEY_SIZE, UNCOMPRESSED_PUBLIC_KEY_SIZE};
...
                    if !(bytes.len() == PUBLIC_KEY_SIZE || bytes.len() == UNCOMPRESSED_PUBLIC_KEY_SIZE) {
                        return false;
                    }
```
tcharding: During review I had to look up what a bare multisig script looked like, perhaps we can add an example in the function rustdocs?
```rust
    /// In a bare multisig pubkey script the keys are not hashed, the script is of the form:
    ///
    ///    2 <pubkey1> <pubkey2> <pubkey3> 3 OP_CHECKMULTISIG
```
tcharding: CI will be fixed with #2077
junderw: > except can you rename it to `is_bare_multisig`? Today this is maybe the least-common multisig mode so we should make it clearer that it's something a little weird.

I disagree. This is a method on `Script` and re don't know if the caller is calling it after parsing the redeemScript or witnessScript, in which case it wouldn't be "bare" because it's not in the scriptPubkey.

> Since other reviewers may flag it: I think it's also fine that we do no length-checking or other form of checking on the "pubkeys". If they're pushes that go into CHECKMULTISIG, and there's the right number of them, then as far as Script is concerned they're public keys.

In that case `OP_16 <8_byte_push> <13_byte_push> OP_12 OP_CHECKMULTISIG` should return true?

It does sort of make sense, but I would say that at least we should check the OP_12 actually has 12 pushes before it, and OP_16 should be less than or equal to OP_12. Since that seems inherent to the OP itself. pubkey length etc. could be changed in a hardfork and CHECKMULTISIG would still work, so it feels like pubkey length is consensus.
tcharding: 
> It does sort of make sense, but I would say that at least we should check the OP_12 actually has 12 pushes before it, and OP_16 should be less than or equal to OP_12. Since that seems inherent to the OP itself.

Both these checks exist in the PR.

> pubkey length etc. could be changed in a hardfork and CHECKMULTISIG would still work, so it feels like pubkey length is consensus and could therefore be ignored if all we care about is whether it fits the shape of OP_CHECKMULTISIG.

This is a fair point and counters my counter to @apoelstra above. I'm now convinced by this argument that we do not need to check the pubkey lengths. Perhaps a code comment or rustdoc about only checking the script shape?
apoelstra: > I disagree. This is a method on `Script` and we don't know if the caller is calling it after parsing the redeemScript or witnessScript, in which case it wouldn't be "bare" because it's not in the scriptPubkey.

Ok, cool, I buy this.

> ...so it feels like pubkey length is consensus...

Yeah. My feeling is that the pubkey length is "CHECKMULTISIG's business" and we're not trying to implement the CHECKMULTISIG opcode, we're just checking a script template. Also, if we were to check length, should we also check the validity of pubkeys? That's significantly more expensive than any other checks. What if validity fails? Do we allow some keys to be invalid as long as threshold-many are valid? Lots of questions here that we don't want to be in the business of answering.
clarkmoody: Rebased onto `master`
tcharding: oooph, my docs suggestion is failing the docs build - sorry about that.
clarkmoody: > oooph, my docs suggestion is failing the docs build - sorry about that.

Fixed and force-pushed.
tcharding: Needs formatting with `cargo +nightly fmt`.

Patch:
diff --git a/bitcoin/src/blockdata/script/borrowed.rs b/bitcoin/src/blockdata/script/borrowed.rs
index 3e9c36a3..f5c4a49f 100644
--- a/bitcoin/src/blockdata/script/borrowed.rs
+++ b/bitcoin/src/blockdata/script/borrowed.rs
@@ -218,6 +218,59 @@ pub(in crate::blockdata::script) fn p2pk_pubkey_bytes(&self) -> Option<&[u8]> {
         }
     }
 
+    /// Checks whether a script pubkey is a bare multisig output.
+    ///
+    /// In a bare multisig pubkey script the keys are not hashed, the script
+    /// is of the form:
+    ///
+    ///    `2 <pubkey1> <pubkey2> <pubkey3> 3 OP_CHECKMULTISIG`
+    #[inline]
+    pub fn is_multisig(&self) -> bool {
+        let required_sigs;
+
+        let mut instructions = self.instructions();
+        if let Some(Ok(Instruction::Op(op))) = instructions.next() {
+            if let Some(pushnum) = op.decode_pushnum() {
+                required_sigs = pushnum;
+            } else {
+                return false;
+            }
+        } else {
+            return false;
+        }
+
+        let mut num_pubkeys: u8 = 0;
+        while let Some(Ok(instruction)) = instructions.next() {
+            match instruction {
+                Instruction::PushBytes(_) => {
+                    num_pubkeys += 1;
+                }
+                Instruction::Op(op) => {
+                    if let Some(pushnum) = op.decode_pushnum() {
+                        if pushnum != num_pubkeys {
+                            return false;
+                        }
+                    }
+                    break;
+                }
+            }
+        }
+
+        if required_sigs > num_pubkeys {
+            return false;
+        }
+
+        if let Some(Ok(Instruction::Op(op))) = instructions.next() {
+            if op != OP_CHECKMULTISIG {
+                return false;
+            }
+        } else {
+            return false;
+        }
+
+        instructions.next().is_none()
+    }
+
     /// Checks whether a script pubkey is a Segregated Witness (segwit) program.
     #[inline]
     pub fn is_witness_program(&self) -> bool {


Test patch:
diff --git a/bitcoin/src/blockdata/script/tests.rs b/bitcoin/src/blockdata/script/tests.rs
index 4b494a46..34a67767 100644
--- a/bitcoin/src/blockdata/script/tests.rs
+++ b/bitcoin/src/blockdata/script/tests.rs
@@ -367,6 +367,53 @@ fn op_return_test() {
     assert!(!ScriptBuf::from_hex("").unwrap().is_op_return());
 }
 
+#[test]
+fn multisig() {
+    // First multisig? 1-of-2
+    // In block 164467, txid 60a20bd93aa49ab4b28d514ec10b06e1829ce6818ec06cd3aabd013ebcdc4bb1
+    assert!(
+        ScriptBuf::from_hex("514104cc71eb30d653c0c3163990c47b976f3fb3f37cccdcbedb169a1dfef58bbfbfaff7d8a473e7e2e6d317b87bafe8bde97e3cf8f065dec022b51d11fcdd0d348ac4410461cbdcc5409fb4b4d42b51d33381354d80e550078cb532a34bfa2fcfdeb7d76519aecc62770f5b0e4ef8551946d8a540911abe3e7854a26f39f58b25c15342af52ae")
+            .unwrap()
+            .is_multisig()
+    );
+    // 2-of-2
+    assert!(
+        ScriptBuf::from_hex("5221021c4ac2ecebc398e390e07f045aac5cc421f82f0739c1ce724d3d53964dc6537d21023a2e9155e0b62f76737605504819a2b4e5ce20653f6c397d7a178ae42ba702f452ae")
+            .unwrap()
+            .is_multisig()
+    );
+
+    // Extra opcode after OP_CHECKMULTISIG
+    assert!(
+        !ScriptBuf::from_hex("5221021c4ac2ecebc398e390e07f045aac5cc421f82f0739c1ce724d3d53964dc6537d21023a2e9155e0b62f76737605504819a2b4e5ce20653f6c397d7a178ae42ba702f452ae52")
+            .unwrap()
+            .is_multisig()
+    );
+    // Required sigs > num pubkeys
+    assert!(
+        !ScriptBuf::from_hex("5321021c4ac2ecebc398e390e07f045aac5cc421f82f0739c1ce724d3d53964dc6537d21023a2e9155e0b62f76737605504819a2b4e5ce20653f6c397d7a178ae42ba702f452ae")
+            .unwrap()
+            .is_multisig()
+    );
+    // Num pubkeys != pushnum
+    assert!(
+        !ScriptBuf::from_hex("5221021c4ac2ecebc398e390e07f045aac5cc421f82f0739c1ce724d3d53964dc6537d21023a2e9155e0b62f76737605504819a2b4e5ce20653f6c397d7a178ae42ba702f453ae")
+            .unwrap()
+            .is_multisig()
+    );
+
+    // Taproot hash from another test
+    assert!(!ScriptBuf::from_hex(
+        "20d85a959b0290bf19bb89ed43c916be835475d013da4b362117393e25a48229b8ac"
+    )
+    .unwrap()
+    .is_multisig());
+    // OP_RETURN from another test
+    assert!(!ScriptBuf::from_hex("6aa9149eb21980dc9d413d8eac27314938b9da920ee53e87")
+        .unwrap()
+        .is_multisig());
+}
+
 #[test]
 #[cfg(feature = "serde")]
 fn script_json_serialize() {