Merge bitcoin/bitcoin#28312: test: fix keys_to_multisig_script (P2MS) helper for n/k > 16

5cf0a1f230 test: add `createmultisig` P2MS encoding test for all n (1..20) (Sebastian Falbesoner)
0570d2c204 test: add unit test for `keys_to_multisig_script` (Sebastian Falbesoner)
0c41fc3fa5 test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16 (Sebastian Falbesoner)

Pull request description:

  While reviewing #28307, I noticed that the test framework's `key_to_multisig_script` helper (introduced in #23305) is broken for pubkey count (n) and threshold (k) values larger than 16. This is due to the implementation currently enforcing a direct single-byte data push (using `CScriptOp.encode_op_n`), which obviously fails for values 17+. Fix that by passing the numbers directly to the CScript list, where it's automatically converted to minimally-encoded pushes (see class method `CScript.__coerce_instance`, branch `isinstance(other, int)`).

  The second commit adds a unit test to ensure that the encoding  is correct.

ACKs for top commit:
  achow101:
    ACK 5cf0a1f230
  tdb3:
    ACK 5cf0a1f230
  rkrux:
    reACK [5cf0a1f](5cf0a1f230)

Tree-SHA512: 4168a165c3f483ec8e37a27dba1628a7ea0063545a2b7e74d9e20d753fddd7e33d37e1a190434fa6dca39adf9eef5d0211f7a0c1c7b44979f0a3bb350e267562

test/functional/feature_framework_unit_tests.py

@@ -27,6 +27,7 @@ TEST_FRAMEWORK_MODULES = [
     "crypto.ripemd160",
     "crypto.secp256k1",
     "script",
+    "script_util",
     "segwit_addr",
     "wallet_util",
 ]

test/functional/rpc_createmultisig.py

@@ -12,6 +12,7 @@ from test_framework.address import address_to_scriptpubkey
 from test_framework.descriptors import descsum_create, drop_origins
 from test_framework.key import ECPubKey
 from test_framework.messages import COIN
+from test_framework.script_util import keys_to_multisig_script
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_raises_rpc_error,
@@ -69,6 +70,16 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
         # Check that bech32m is currently not allowed
         assert_raises_rpc_error(-5, "createmultisig cannot create bech32m multisig addresses", self.nodes[0].createmultisig, 2, self.pub, "bech32m")
 
+        self.log.info('Check correct encoding of multisig script for all n (1..20)')
+        for nkeys in range(1, 20+1):
+            keys = [self.pub[0]]*nkeys
+            expected_ms_script = keys_to_multisig_script(keys, k=nkeys)  # simply use n-of-n
+            # note that the 'legacy' address type fails for n values larger than 15
+            # due to exceeding the P2SH size limit (520 bytes), so we use 'bech32' instead
+            # (for the purpose of this encoding test, we don't care about the resulting address)
+            res = self.nodes[0].createmultisig(nrequired=nkeys, keys=keys, address_type='bech32')
+            assert_equal(res['redeemScript'], expected_ms_script.hex())
+
     def check_addmultisigaddress_errors(self):
         if self.options.descriptors:
             return

test/functional/test_framework/script_util.py

@@ -3,10 +3,13 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Useful Script constants and utils."""
+import unittest
+
 from test_framework.script import (
     CScript,
-    CScriptOp,
     OP_0,
+    OP_15,
+    OP_16,
     OP_CHECKMULTISIG,
     OP_CHECKSIG,
     OP_DUP,
@@ -49,10 +52,8 @@ def keys_to_multisig_script(keys, *, k=None):
     if k is None:  # n-of-n multisig by default
         k = n
     assert k <= n
-    op_k = CScriptOp.encode_op_n(k)
-    op_n = CScriptOp.encode_op_n(n)
     checked_keys = [check_key(key) for key in keys]
-    return CScript([op_k] + checked_keys + [op_n, OP_CHECKMULTISIG])
+    return CScript([k] + checked_keys + [n, OP_CHECKMULTISIG])
 
 
 def keyhash_to_p2pkh_script(hash):
@@ -125,3 +126,19 @@ def check_script(script):
     if isinstance(script, bytes) or isinstance(script, CScript):
         return script
     assert False
+
+
+class TestFrameworkScriptUtil(unittest.TestCase):
+    def test_multisig(self):
+        fake_pubkey = bytes([0]*33)
+        # check correct encoding of P2MS script with n,k <= 16
+        normal_ms_script = keys_to_multisig_script([fake_pubkey]*16, k=15)
+        self.assertEqual(len(normal_ms_script), 1 + 16*34 + 1 + 1)
+        self.assertTrue(normal_ms_script.startswith(bytes([OP_15])))
+        self.assertTrue(normal_ms_script.endswith(bytes([OP_16, OP_CHECKMULTISIG])))
+
+        # check correct encoding of P2MS script with n,k > 16
+        max_ms_script = keys_to_multisig_script([fake_pubkey]*20, k=19)
+        self.assertEqual(len(max_ms_script), 2 + 20*34 + 2 + 1)
+        self.assertTrue(max_ms_script.startswith(bytes([1, 19])))  # using OP_PUSH1
+        self.assertTrue(max_ms_script.endswith(bytes([1, 20, OP_CHECKMULTISIG])))