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

Should we add a comment explaining the logic behind the math in these script length assert calls? Or maybe refer an existing doc?

Trying to understand why is there this else block in coerce_instance here that encodes using pushdata. @theStack Do you know the reason by chance? https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/script.py#L451-L452

@rkrux Numbers between -1 and 16 inclusive can use the 1-byte OP_n opcodes. Larger numbers require a script push (which is 2 bytes for numbers up to 127).

nit: In case this file is touched, it might be clearer (as rkrux mentioned) to add a comment explicitly explaining the math, although this is just a nice-to-have since the addition breaks it out such that the reader is provided a hint (typical multisig script). Author's preference.

diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
index 855f3b8cf5..69be490a67 100755
--- a/test/functional/test_framework/script_util.py
+++ b/test/functional/test_framework/script_util.py
@@ -133,7 +133,7 @@ class TestFrameworkScriptUtil(unittest.TestCase):
         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.assertEqual(len(normal_ms_script), 1 + 16*34 + 1 + 1) # OP_k + 16*[OP_PUSHBYTES_33 + 33 byte compressed pubkey] + OP_n + OP_CHECKMULTISIG
         self.assertTrue(normal_ms_script.startswith(bytes([OP_15])))
         self.assertTrue(normal_ms_script.endswith(bytes([OP_16, OP_CHECKMULTISIG])))
 

Nit: Checking for integers between 15 and 17 would also suffice, right?

Thanks for adding the comment, helpful!

Curious: Is it common to test the correctness of a test utility function against the response from the bitcoin node?

Good question. I'm not sure if it's really that common, but there was a wish to also test the utility function via a functional test (see comment #28312 (comment)) and it seemed to be a worthwhile idea to me.

Since > 20 keys are not allowed when creating a multisig address, recommend adding a test case to check this.

Something like:

diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
index 4bbd2a526d..88f4e44a65 100755
--- a/test/functional/rpc_createmultisig.py
+++ b/test/functional/rpc_createmultisig.py
@@ -129,6 +129,9 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
             res = self.nodes[0].createmultisig(nrequired=nkeys, keys=keys, address_type='bech32')
             assert_equal(res['redeemScript'], expected_ms_script.hex())
 
+        self.log.info('Check that number of keys greater than 20 is not allowed')
+        assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", self.nodes[0].createmultisig, 20, [self.pub[0]]*21, "bech32")
+
     def check_addmultisigaddress_errors(self):
         if self.options.descriptors:
             return

Such a test case was already introduced in the recently merged PR #28307 (commit 9be6065cc03f2408f290a332b203eef9c9cebf24). Note that this PR is primarily focused on the keys_to_multisig_script helper in the test framework and only uses the createmultisig RPC to verify it.