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

theStack · 2023-08-22T01:33:16Z

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.

DrahtBot · 2023-08-22T01:33:18Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3
Concept ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29771 (test: Run framework unit tests in parallel by tdb3)
#28307 (rpc, wallet: fix incorrect segwit redeem script size limit by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

luke-jr · 2023-08-23T17:35:26Z

Maybe add a functional test that would have failed too?

theStack · 2023-08-23T22:14:39Z

Maybe add a functional test that would have failed too?

Yeah, seems like a good idea. Added one in rpc_createmultisig which checks that the multisig script encoding (returned as 'redeemScript') is as expected for all possible n (1..20), only using n-of-n for now to keep it simple.

The helper assumes that the n and k values have to be provided as a single byte push operation, which is only possible for values up to 16. 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)`). In case of 17..20, this means that the data-pushes are done with two bytes using OP_PUSH1 (0x01), e.g. for n=20: 0x01,0x14

rkrux · 2024-03-19T11:12:30Z

test/functional/test_framework/script_util.py

+        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)


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).

rkrux · 2024-03-19T11:19:14Z

test/functional/rpc_createmultisig.py

+        self.log.info('Check correct encoding of multisig script for all n (1..20)')
+        for nkeys in range(1, 20+1):


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

rkrux · 2024-03-19T11:19:32Z

test/functional/rpc_createmultisig.py

+            # 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)


Thanks for adding the comment, helpful!

rkrux

Concept ACK d113709

Testing Methodology

Checked out PR
Built core
Ran make check
Ran test/functional/test_runner.py

rkrux · 2024-03-19T11:30:33Z

test/functional/rpc_createmultisig.py

+        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


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

tdb3

Great work catching the bug and making testing more robust.

ACK for d113709.

One inline recommendation to cover an additional related edge case.

tdb3 · 2024-04-09T01:09:32Z

test/functional/rpc_createmultisig.py

+            # (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())
+


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

DrahtBot · 2024-04-26T20:17:17Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the Tests label Aug 22, 2023

theStack force-pushed the test-fix_keys_to_multisig_for_17+ branch 2 times, most recently from 73c2def to 85c01bd Compare August 22, 2023 09:57

DrahtBot mentioned this pull request Aug 24, 2023

rpc, wallet: fix incorrect segwit redeem script size limit #28307

Open

DrahtBot added CI failed and removed CI failed labels Sep 3, 2023

DrahtBot added CI failed and removed CI failed labels Sep 14, 2023

DrahtBot mentioned this pull request Sep 25, 2023

test: Use test framework utils in functional tests #28528

Closed

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Draft

DrahtBot added the CI failed label Jan 12, 2024

theStack added 3 commits January 22, 2024 13:39

test: add unit test for keys_to_multisig_script

a36ad0c

test: add createmultisig P2MS encoding test for all n (1..20)

d113709

theStack force-pushed the test-fix_keys_to_multisig_for_17+ branch from 16269ef to d113709 Compare January 22, 2024 12:40

DrahtBot removed the CI failed label Jan 22, 2024

DrahtBot added CI failed and removed CI failed labels Mar 8, 2024

rkrux reviewed Mar 19, 2024

View reviewed changes

DrahtBot mentioned this pull request Mar 31, 2024

test: Run framework unit tests in parallel #29771

Merged

tdb3 reviewed Apr 9, 2024

View reviewed changes

DrahtBot requested a review from rkrux April 9, 2024 01:11

bitcoin deleted a comment from Ashflower13 Apr 9, 2024

DrahtBot added CI failed Needs rebase and removed CI failed labels Apr 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theStack commented Aug 22, 2023

DrahtBot commented Aug 22, 2023 •

edited

luke-jr commented Aug 23, 2023

theStack commented Aug 23, 2023

rkrux Mar 19, 2024

rkrux Mar 20, 2024

sipa Mar 20, 2024

rkrux Mar 19, 2024

rkrux Mar 19, 2024

rkrux left a comment •

edited

rkrux Mar 19, 2024

tdb3 left a comment

tdb3 Apr 9, 2024

DrahtBot commented Apr 26, 2024

		self.log.info('Check correct encoding of multisig script for all n (1..20)')
		for nkeys in range(1, 20+1):

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

Are you sure you want to change the base?

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

Conversation

theStack commented Aug 22, 2023

DrahtBot commented Aug 22, 2023 • edited

Code Coverage

Reviews

Conflicts

luke-jr commented Aug 23, 2023

theStack commented Aug 23, 2023

rkrux Mar 19, 2024

Choose a reason for hiding this comment

rkrux Mar 20, 2024

Choose a reason for hiding this comment

sipa Mar 20, 2024

Choose a reason for hiding this comment

rkrux Mar 19, 2024

Choose a reason for hiding this comment

rkrux Mar 19, 2024

Choose a reason for hiding this comment

rkrux left a comment • edited

Choose a reason for hiding this comment

Testing Methodology

rkrux Mar 19, 2024

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

tdb3 Apr 9, 2024

Choose a reason for hiding this comment

DrahtBot commented Apr 26, 2024

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

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

DrahtBot commented Aug 22, 2023 •

edited

rkrux left a comment •

edited