From 3635d432681847313c098f9827483372a840e70f Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 09:54:30 -0300 Subject: [PATCH 01/10] test: rpc_createmultisig, remove manual wallet initialization There is no need to manually initialize the wallets within the test case. The test framework already initializes them when `_requires_wallet` is true. --- test/functional/rpc_createmultisig.py | 2 +- test/functional/test_framework/test_framework.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 65d7b4c4223..a540e7730be 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -32,6 +32,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.setup_clean_chain = True self.num_nodes = 3 self.supports_cli = False + self.enable_wallet_if_possible() def get_keys(self): self.pub = [] @@ -51,7 +52,6 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.wallet = MiniWallet(test_node=node0) if self.is_bdb_compiled(): - self.import_deterministic_coinbase_privkeys() self.check_addmultisigaddress_errors() self.log.info('Generating blocks ...') diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index a2f767cc989..71551ffd7cb 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -444,6 +444,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors, load_on_startup=True) n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase', rescan=True) + # Only enables wallet support when the module is available + def enable_wallet_if_possible(self): + self._requires_wallet = self.is_wallet_compiled() + def run_test(self): """Tests must override this method to define test logic""" raise NotImplementedError From b5a328943362cfac6e90fd4e1b167c357d53b7d4 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 16:47:54 -0300 Subject: [PATCH 02/10] test: refactor, multiple cleanups in rpc_createmultisig.py Cleaning up the test in the following ways: * Generate priv-pub key pairs used for testing only once (instead of doing it 4 times). * Simplifies 'wmulti' wallet creation, load and unload process. * Removes confusing class members initialized and updated inside a nested for-loop. * Simplifies do_multisig() outpoint detection: The outpoint index information is already contained in MiniWallet's `send_to` return value dictionary as "sent_vout". Co-authored-by: Sebastian Falbesoner --- test/functional/rpc_createmultisig.py | 90 ++++++++++++--------------- 1 file changed, 40 insertions(+), 50 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index a540e7730be..4e6683182c3 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -10,9 +10,9 @@ import os from test_framework.address import address_to_scriptpubkey from test_framework.blocktools import COINBASE_MATURITY -from test_framework.authproxy import JSONRPCException from test_framework.descriptors import descsum_create, drop_origins from test_framework.key import ECPubKey +from test_framework.messages import COIN from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_raises_rpc_error, @@ -34,19 +34,22 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.supports_cli = False self.enable_wallet_if_possible() - def get_keys(self): + def create_keys(self, num_keys): self.pub = [] self.priv = [] - node0, node1, node2 = self.nodes - for _ in range(self.nkeys): + for _ in range(num_keys): privkey, pubkey = generate_keypair(wif=True) self.pub.append(pubkey.hex()) self.priv.append(privkey) if self.is_bdb_compiled(): - self.final = node2.getnewaddress() + self.final = self.nodes[2].getnewaddress() else: self.final = getnewdestination('bech32')[2] + def create_wallet(self, node, wallet_name): + node.createwallet(wallet_name=wallet_name, disable_private_keys=True) + return node.get_wallet_rpc(wallet_name) + def run_test(self): node0, node1, node2 = self.nodes self.wallet = MiniWallet(test_node=node0) @@ -57,12 +60,15 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.log.info('Generating blocks ...') self.generate(self.wallet, 149) + wallet_multi = self.create_wallet(node1, 'wmulti') if self._requires_wallet else None self.moved = 0 - for self.nkeys in [3, 5]: - for self.nsigs in [2, 3]: - for self.output_type in ["bech32", "p2sh-segwit", "legacy"]: - self.get_keys() - self.do_multisig() + self.create_keys(5) + for nkeys in [3, 5]: + for nsigs in [2, 3]: + for output_type in ["bech32", "p2sh-segwit", "legacy"]: + self.do_multisig(nkeys, nsigs, output_type, wallet_multi) + if wallet_multi is not None: + wallet_multi.unloadwallet() if self.is_bdb_compiled(): self.checkbalances() @@ -149,93 +155,77 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): assert bal2 == self.moved assert_equal(bal0 + bal1 + bal2 + balw, total) - def do_multisig(self): + def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes - - if self.is_bdb_compiled(): - if 'wmulti' not in node1.listwallets(): - try: - node1.loadwallet('wmulti') - except JSONRPCException as e: - path = self.nodes[1].wallets_path / "wmulti" - if e.error['code'] == -18 and "Wallet file verification failed. Failed to load database path '{}'. Path does not exist.".format(path) in e.error['message']: - node1.createwallet(wallet_name='wmulti', disable_private_keys=True) - else: - raise - wmulti = node1.get_wallet_rpc('wmulti') + pub_keys = self.pub[0: nkeys] + priv_keys = self.priv[0: nkeys] # Construct the expected descriptor - desc = 'multi({},{})'.format(self.nsigs, ','.join(self.pub)) - if self.output_type == 'legacy': + desc = 'multi({},{})'.format(nsigs, ','.join(pub_keys)) + if output_type == 'legacy': desc = 'sh({})'.format(desc) - elif self.output_type == 'p2sh-segwit': + elif output_type == 'p2sh-segwit': desc = 'sh(wsh({}))'.format(desc) - elif self.output_type == 'bech32': + elif output_type == 'bech32': desc = 'wsh({})'.format(desc) desc = descsum_create(desc) - msig = node2.createmultisig(self.nsigs, self.pub, self.output_type) + msig = node2.createmultisig(nsigs, pub_keys, output_type) assert 'warnings' not in msig madd = msig["address"] mredeem = msig["redeemScript"] assert_equal(desc, msig['descriptor']) - if self.output_type == 'bech32': + if output_type == 'bech32': assert madd[0:4] == "bcrt" # actually a bech32 address - if self.is_bdb_compiled(): + if wallet_multi is not None: # compare against addmultisigaddress - msigw = wmulti.addmultisigaddress(self.nsigs, self.pub, None, self.output_type) + msigw = wallet_multi.addmultisigaddress(nsigs, pub_keys, None, output_type) maddw = msigw["address"] mredeemw = msigw["redeemScript"] assert_equal(desc, drop_origins(msigw['descriptor'])) # addmultisigiaddress and createmultisig work the same assert maddw == madd assert mredeemw == mredeem - wmulti.unloadwallet() spk = address_to_scriptpubkey(madd) - txid = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=1300)["txid"] - tx = node0.getrawtransaction(txid, True) - vout = [v["n"] for v in tx["vout"] if madd == v["scriptPubKey"]["address"]] - assert len(vout) == 1 - vout = vout[0] - scriptPubKey = tx["vout"][vout]["scriptPubKey"]["hex"] - value = tx["vout"][vout]["value"] - prevtxs = [{"txid": txid, "vout": vout, "scriptPubKey": scriptPubKey, "redeemScript": mredeem, "amount": value}] + value = decimal.Decimal("0.00001300") + tx = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=int(value * COIN)) + prevtxs = [{"txid": tx["txid"], "vout": tx["sent_vout"], "scriptPubKey": spk.hex(), "redeemScript": mredeem, "amount": value}] self.generate(node0, 1) outval = value - decimal.Decimal("0.00001000") - rawtx = node2.createrawtransaction([{"txid": txid, "vout": vout}], [{self.final: outval}]) + rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{self.final: outval}]) prevtx_err = dict(prevtxs[0]) del prevtx_err["redeemScript"] - assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "Missing redeemScript/witnessScript", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # if witnessScript specified, all ok prevtx_err["witnessScript"] = prevtxs[0]["redeemScript"] - node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # both specified, also ok prevtx_err["redeemScript"] = prevtxs[0]["redeemScript"] - node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # redeemScript mismatch to witnessScript prevtx_err["redeemScript"] = "6a" # OP_RETURN - assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "redeemScript does not correspond to witnessScript", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # redeemScript does not match scriptPubKey del prevtx_err["witnessScript"] - assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) # witnessScript does not match scriptPubKey prevtx_err["witnessScript"] = prevtx_err["redeemScript"] del prevtx_err["redeemScript"] - assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, self.priv[0:self.nsigs-1], [prevtx_err]) + assert_raises_rpc_error(-8, "redeemScript/witnessScript does not match scriptPubKey", node2.signrawtransactionwithkey, rawtx, priv_keys[0:nsigs-1], [prevtx_err]) - rawtx2 = node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs - 1], prevtxs) - rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [self.priv[-1]], prevtxs) + rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs) + rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs) self.moved += outval tx = node0.sendrawtransaction(rawtx3["hex"], 0) @@ -243,7 +233,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): assert tx in node0.getblock(blk)["tx"] txinfo = node0.getrawtransaction(tx, True, blk) - self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (self.nsigs, self.nkeys, self.output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) + self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (nsigs, nkeys, output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) if __name__ == '__main__': From 25a81705d376e8c96dad45436ae3fca975b3daf5 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 18:51:06 -0300 Subject: [PATCH 03/10] test: rpc_createmultisig, remove unnecessary checkbalances() The function exists merely to check that the node2's wallet received the transactions created during all the 'do_multisig()' calls. It was created as a standalone function because 'getbalance()' only returns something when transactions are confirmed. So, the rationale on that time was to have a method mining blocks to confirm the recently created transactions to be able to check the incoming balance. This is why we have the "moved" class field. This change removes all the hardcoded amounts and verifies node2 balance reception directly inside 'do_multisig()'. --- test/functional/rpc_createmultisig.py | 36 +++++++-------------------- 1 file changed, 9 insertions(+), 27 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 4e6683182c3..c2163556b2a 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -9,7 +9,6 @@ import json import os from test_framework.address import address_to_scriptpubkey -from test_framework.blocktools import COINBASE_MATURITY from test_framework.descriptors import descsum_create, drop_origins from test_framework.key import ECPubKey from test_framework.messages import COIN @@ -41,10 +40,6 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): privkey, pubkey = generate_keypair(wif=True) self.pub.append(pubkey.hex()) self.priv.append(privkey) - if self.is_bdb_compiled(): - self.final = self.nodes[2].getnewaddress() - else: - self.final = getnewdestination('bech32')[2] def create_wallet(self, node, wallet_name): node.createwallet(wallet_name=wallet_name, disable_private_keys=True) @@ -54,14 +49,13 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): node0, node1, node2 = self.nodes self.wallet = MiniWallet(test_node=node0) - if self.is_bdb_compiled(): + if self.is_wallet_compiled(): self.check_addmultisigaddress_errors() self.log.info('Generating blocks ...') self.generate(self.wallet, 149) wallet_multi = self.create_wallet(node1, 'wmulti') if self._requires_wallet else None - self.moved = 0 self.create_keys(5) for nkeys in [3, 5]: for nsigs in [2, 3]: @@ -69,8 +63,6 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.do_multisig(nkeys, nsigs, output_type, wallet_multi) if wallet_multi is not None: wallet_multi.unloadwallet() - if self.is_bdb_compiled(): - self.checkbalances() # Test mixed compressed and uncompressed pubkeys self.log.info('Mixed compressed and uncompressed multisigs are not allowed') @@ -139,22 +131,6 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses] assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m") - def checkbalances(self): - node0, node1, node2 = self.nodes - self.generate(node0, COINBASE_MATURITY) - - bal0 = node0.getbalance() - bal1 = node1.getbalance() - bal2 = node2.getbalance() - balw = self.wallet.get_balance() - - height = node0.getblockchaininfo()["blocks"] - assert 150 < height < 350 - total = 149 * 50 + (height - 149 - 100) * 25 - assert bal1 == 0 - assert bal2 == self.moved - assert_equal(bal0 + bal1 + bal2 + balw, total) - def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes pub_keys = self.pub[0: nkeys] @@ -196,7 +172,10 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.generate(node0, 1) outval = value - decimal.Decimal("0.00001000") - rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{self.final: outval}]) + # send coins to node2 when wallet is enabled + node2_balance = node2.getbalances()['mine']['trusted'] if self.is_wallet_compiled() else 0 + out_addr = node2.getnewaddress() if self.is_wallet_compiled() else getnewdestination('bech32')[2] + rawtx = node2.createrawtransaction([{"txid": tx["txid"], "vout": tx["sent_vout"]}], [{out_addr: outval}]) prevtx_err = dict(prevtxs[0]) del prevtx_err["redeemScript"] @@ -227,11 +206,14 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs) rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs) - self.moved += outval tx = node0.sendrawtransaction(rawtx3["hex"], 0) blk = self.generate(node0, 1)[0] assert tx in node0.getblock(blk)["tx"] + # When the wallet is enabled, assert node2 sees the incoming amount + if self.is_wallet_compiled(): + assert_equal(node2.getbalances()['mine']['trusted'], node2_balance + outval) + txinfo = node0.getrawtransaction(tx, True, blk) self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (nsigs, nkeys, output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) From 4f33dbd8f8c0e29f37b04e6af6d2c7905ecceaf6 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 19:18:00 -0300 Subject: [PATCH 04/10] test: rpc_createmultisig, decouple 'test_mixing_uncompressed_and_compressed_keys' And also, simplified the test a bit by re-using the already existing 'wallet_multi' (instead of creating a new one). Plus, removed the 'is_bdb_compiled()' calls which were there basically to check if the test has the wallet compiled or not. --- test/functional/rpc_createmultisig.py | 74 +++++++++++++-------------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index c2163556b2a..2414dfd116b 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -61,45 +61,8 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): for nsigs in [2, 3]: for output_type in ["bech32", "p2sh-segwit", "legacy"]: self.do_multisig(nkeys, nsigs, output_type, wallet_multi) - if wallet_multi is not None: - wallet_multi.unloadwallet() - # Test mixed compressed and uncompressed pubkeys - self.log.info('Mixed compressed and uncompressed multisigs are not allowed') - pk0, pk1, pk2 = [getnewdestination('bech32')[0].hex() for _ in range(3)] - - # decompress pk2 - pk_obj = ECPubKey() - pk_obj.set(bytes.fromhex(pk2)) - pk_obj.compressed = False - pk2 = pk_obj.get_bytes().hex() - - if self.is_bdb_compiled(): - node0.createwallet(wallet_name='wmulti0', disable_private_keys=True) - wmulti0 = node0.get_wallet_rpc('wmulti0') - - # Check all permutations of keys because order matters apparently - for keys in itertools.permutations([pk0, pk1, pk2]): - # Results should be the same as this legacy one - legacy_addr = node0.createmultisig(2, keys, 'legacy')['address'] - - if self.is_bdb_compiled(): - result = wmulti0.addmultisigaddress(2, keys, '', 'legacy') - assert_equal(legacy_addr, result['address']) - assert 'warnings' not in result - - # Generate addresses with the segwit types. These should all make legacy addresses - err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."] - - for addr_type in ['bech32', 'p2sh-segwit']: - result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type) - assert_equal(legacy_addr, result['address']) - assert_equal(result['warnings'], err_msg) - - if self.is_bdb_compiled(): - result = wmulti0.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type) - assert_equal(legacy_addr, result['address']) - assert_equal(result['warnings'], err_msg) + self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors') with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: @@ -217,6 +180,41 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): txinfo = node0.getrawtransaction(tx, True, blk) self.log.info("n/m=%d/%d %s size=%d vsize=%d weight=%d" % (nsigs, nkeys, output_type, txinfo["size"], txinfo["vsize"], txinfo["weight"])) + def test_mixing_uncompressed_and_compressed_keys(self, node, wallet_multi): + self.log.info('Mixed compressed and uncompressed multisigs are not allowed') + pk0, pk1, pk2 = [getnewdestination('bech32')[0].hex() for _ in range(3)] + + # decompress pk2 + pk_obj = ECPubKey() + pk_obj.set(bytes.fromhex(pk2)) + pk_obj.compressed = False + pk2 = pk_obj.get_bytes().hex() + + # Check all permutations of keys because order matters apparently + for keys in itertools.permutations([pk0, pk1, pk2]): + # Results should be the same as this legacy one + legacy_addr = node.createmultisig(2, keys, 'legacy')['address'] + + if wallet_multi is not None: + # 'addmultisigaddress' should return the same address + result = wallet_multi.addmultisigaddress(2, keys, '', 'legacy') + assert_equal(legacy_addr, result['address']) + assert 'warnings' not in result + + # Generate addresses with the segwit types. These should all make legacy addresses + err_msg = ["Unable to make chosen address type, please ensure no uncompressed public keys are present."] + + for addr_type in ['bech32', 'p2sh-segwit']: + result = self.nodes[0].createmultisig(nrequired=2, keys=keys, address_type=addr_type) + assert_equal(legacy_addr, result['address']) + assert_equal(result['warnings'], err_msg) + + if wallet_multi is not None: + result = wallet_multi.addmultisigaddress(nrequired=2, keys=keys, address_type=addr_type) + assert_equal(legacy_addr, result['address']) + assert_equal(result['warnings'], err_msg) + + if __name__ == '__main__': RpcCreateMultiSigTest().main() From f7a173b5785cda460470df9a74a0e0f94d7f9a18 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 19 Aug 2023 19:20:04 -0300 Subject: [PATCH 05/10] test: rpc_createmultisig, decouple 'test_sortedmulti_descriptors_bip67' Move-only commit. No behavior change. --- test/functional/rpc_createmultisig.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 2414dfd116b..983aeafc1c5 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -63,18 +63,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.do_multisig(nkeys, nsigs, output_type, wallet_multi) self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) - - self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors') - with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: - vectors = json.load(f) - - for t in vectors: - key_str = ','.join(t['keys']) - desc = descsum_create('sh(sortedmulti(2,{}))'.format(key_str)) - assert_equal(self.nodes[0].deriveaddresses(desc)[0], t['address']) - sorted_key_str = ','.join(t['sorted_keys']) - sorted_key_desc = descsum_create('sh(multi(2,{}))'.format(sorted_key_str)) - assert_equal(self.nodes[0].deriveaddresses(sorted_key_desc)[0], t['address']) + self.test_sortedmulti_descriptors_bip67() # 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") @@ -214,6 +203,18 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): assert_equal(legacy_addr, result['address']) assert_equal(result['warnings'], err_msg) + def test_sortedmulti_descriptors_bip67(self): + self.log.info('Testing sortedmulti descriptors with BIP 67 test vectors') + with open(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'data/rpc_bip67.json'), encoding='utf-8') as f: + vectors = json.load(f) + + for t in vectors: + key_str = ','.join(t['keys']) + desc = descsum_create('sh(sortedmulti(2,{}))'.format(key_str)) + assert_equal(self.nodes[0].deriveaddresses(desc)[0], t['address']) + sorted_key_str = ','.join(t['sorted_keys']) + sorted_key_desc = descsum_create('sh(multi(2,{}))'.format(sorted_key_str)) + assert_equal(self.nodes[0].deriveaddresses(sorted_key_desc)[0], t['address']) if __name__ == '__main__': From 0c9fedfc45fa7cbd6801ca5fd756863ec9a6911c Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 17:45:34 -0300 Subject: [PATCH 06/10] fix incorrect multisig redeem script size limit for segwit The multisig script generation process currently fails when the user exceeds 15 keys, even when it shouldn't. The maximum number of keys allowed for segwit redeem scripts (p2sh-segwit and bech32) is 20 keys. This is because the redeem script placed in the witness is not restricted by the item size limit. The reason behind this issue is the utilization of the legacy p2sh redeem script restrictions on segwit ones. Redeem scripts longer than 520 bytes are blocked from being inserted into the keystore, which causes the signing process and the descriptor inference process to fail. This occurs because the multisig generation flow uses the same keystore as the legacy spkm (FillableSigningProvider), which contains the 520-byte limit. --- src/outputtype.cpp | 8 ++++---- src/outputtype.h | 2 +- src/rpc/output_script.cpp | 3 +-- src/rpc/util.cpp | 2 +- src/rpc/util.h | 2 +- src/wallet/rpc/addresses.cpp | 12 ++++++++++-- 6 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/outputtype.cpp b/src/outputtype.cpp index c72d9deacb0..8c2b76494bd 100644 --- a/src/outputtype.cpp +++ b/src/outputtype.cpp @@ -81,11 +81,11 @@ std::vector GetAllDestinationsForKey(const CPubKey& key) } } -CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType type) +CTxDestination AddAndGetDestinationForScript(FlatSigningProvider& keystore, const CScript& script, OutputType type) { // Add script to keystore - keystore.AddCScript(script); - // Note that scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are not yet supported. + keystore.scripts.emplace(CScriptID(script), script); + switch (type) { case OutputType::LEGACY: return ScriptHash(script); @@ -94,7 +94,7 @@ CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, CTxDestination witdest = WitnessV0ScriptHash(script); CScript witprog = GetScriptForDestination(witdest); // Add the redeemscript, so that P2WSH and P2SH-P2WSH outputs are recognized as ours. - keystore.AddCScript(witprog); + keystore.scripts.emplace(CScriptID(witprog), witprog); if (type == OutputType::BECH32) { return witdest; } else { diff --git a/src/outputtype.h b/src/outputtype.h index a2d59423208..feef7991a60 100644 --- a/src/outputtype.h +++ b/src/outputtype.h @@ -46,7 +46,7 @@ std::vector GetAllDestinationsForKey(const CPubKey& key); * This function will automatically add the script (and any other * necessary scripts) to the keystore. */ -CTxDestination AddAndGetDestinationForScript(FillableSigningProvider& keystore, const CScript& script, OutputType); +CTxDestination AddAndGetDestinationForScript(FlatSigningProvider& keystore, const CScript& script, OutputType); /** Get the OutputType for a CTxDestination */ std::optional OutputTypeFromDestination(const CTxDestination& dest); diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index f9343f48a89..91d4f283f09 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -143,8 +143,7 @@ static RPCHelpMan createmultisig() output_type = parsed.value(); } - // Construct using pay-to-script-hash: - FillableSigningProvider keystore; + FlatSigningProvider keystore; CScript inner; const CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, keystore, inner); diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index f6838780549..435801b45b9 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -224,7 +224,7 @@ CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& } // Creates a multisig address from a given list of public keys, number of signatures required, and the address type -CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out) +CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FlatSigningProvider& keystore, CScript& script_out) { // Gather public keys if (required < 1) { diff --git a/src/rpc/util.h b/src/rpc/util.h index 177af90c05e..6bfe4146887 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -117,7 +117,7 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList& CPubKey HexToPubKey(const std::string& hex_in); CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& addr_in); -CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FillableSigningProvider& keystore, CScript& script_out); +CTxDestination AddAndGetMultisigDestination(const int required, const std::vector& pubkeys, OutputType type, FlatSigningProvider& keystore, CScript& script_out); UniValue DescribeAddress(const CTxDestination& dest); diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index bed9ec029aa..bcc39b05b86 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -289,9 +289,17 @@ RPCHelpMan addmultisigaddress() output_type = parsed.value(); } - // Construct using pay-to-script-hash: + // Construct multisig scripts + FlatSigningProvider provider; CScript inner; - CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner); + CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, provider, inner); + + // Import scripts into the wallet + for (const auto& [id, script] : provider.scripts) { + spk_man.AddCScript(script); + } + + // Store destination in the addressbook pwallet->SetAddressBook(dest, label, AddressPurpose::SEND); // Make the descriptor From 9d9a91c4ea6b3bb32ef4131bca86f1d6683fc901 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 17:50:09 -0300 Subject: [PATCH 07/10] rpc: bugfix, incorrect segwit redeem script size used in signrawtransactionwithkey The process currently fails to sign redeem scripts that are longer than 520 bytes. Even when it shouldn't. The 520 bytes redeem scripts limit is a legacy p2sh rule, and not a segwit limitation. Segwit redeem scripts are not restricted by the script item size limit. The reason why this occurs, is the usage of the same keystore used by the legacy spkm. Which contains blockage for any redeem scripts longer than the script item size limit. --- src/rpc/rawtransaction.cpp | 8 ++++++-- src/rpc/rawtransaction_util.cpp | 6 +++--- src/rpc/rawtransaction_util.h | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 634be2f7fb9..ce71c052abc 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -785,7 +785,7 @@ static RPCHelpMan signrawtransactionwithkey() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input."); } - FillableSigningProvider keystore; + FlatSigningProvider keystore; const UniValue& keys = request.params[1].get_array(); for (unsigned int idx = 0; idx < keys.size(); ++idx) { UniValue k = keys[idx]; @@ -793,7 +793,11 @@ static RPCHelpMan signrawtransactionwithkey() if (!key.IsValid()) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key"); } - keystore.AddKey(key); + + CPubKey pubkey = key.GetPubKey(); + CKeyID key_id = pubkey.GetID(); + keystore.pubkeys.emplace(key_id, pubkey); + keystore.keys.emplace(key_id, key); } // Fetch previous transactions (inputs): diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp index a9e11622a7a..a27e1be5446 100644 --- a/src/rpc/rawtransaction_util.cpp +++ b/src/rpc/rawtransaction_util.cpp @@ -181,7 +181,7 @@ static void TxInErrorToJSON(const CTxIn& txin, UniValue& vErrorsRet, const std:: vErrorsRet.push_back(entry); } -void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map& coins) +void ParsePrevouts(const UniValue& prevTxsUnival, FlatSigningProvider* keystore, std::map& coins) { if (!prevTxsUnival.isNull()) { const UniValue& prevTxs = prevTxsUnival.get_array(); @@ -247,11 +247,11 @@ void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keyst // work from witnessScript when possible std::vector scriptData(!ws.isNull() ? ParseHexV(ws, "witnessScript") : ParseHexV(rs, "redeemScript")); CScript script(scriptData.begin(), scriptData.end()); - keystore->AddCScript(script); + keystore->scripts.emplace(CScriptID(script), script); // Automatically also add the P2WSH wrapped version of the script (to deal with P2SH-P2WSH). // This is done for redeemScript only for compatibility, it is encouraged to use the explicit witnessScript field instead. CScript witness_output_script{GetScriptForDestination(WitnessV0ScriptHash(script))}; - keystore->AddCScript(witness_output_script); + keystore->scripts.emplace(CScriptID(witness_output_script), witness_output_script); if (!ws.isNull() && !rs.isNull()) { // if both witnessScript and redeemScript are provided, diff --git a/src/rpc/rawtransaction_util.h b/src/rpc/rawtransaction_util.h index 964d0b095b1..40d6bbba873 100644 --- a/src/rpc/rawtransaction_util.h +++ b/src/rpc/rawtransaction_util.h @@ -12,7 +12,7 @@ #include struct bilingual_str; -class FillableSigningProvider; +struct FlatSigningProvider; class UniValue; struct CMutableTransaction; class Coin; @@ -38,7 +38,7 @@ void SignTransactionResultToJSON(CMutableTransaction& mtx, bool complete, const * @param keystore A pointer to the temporary keystore if there is one * @param coins Map of unspent outputs - coins in mempool and current chain UTXO set, may be extended by previous txns outputs after call */ -void ParsePrevouts(const UniValue& prevTxsUnival, FillableSigningProvider* keystore, std::map& coins); +void ParsePrevouts(const UniValue& prevTxsUnival, FlatSigningProvider* keystore, std::map& coins); /** Normalize univalue-represented inputs and add them to the transaction */ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, bool rbf); From 9be6065cc03f2408f290a332b203eef9c9cebf24 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 17:58:46 -0300 Subject: [PATCH 08/10] test: coverage for 16-20 segwit multisig scripts This exercises the bug fixed by previous commits, where we were unable to generate and sign for segwit redeem scripts (in this case multisig redeem scripts) longer than 520 bytes. and also, this adds coverage for legacy 15-15 multisig script generation and signing. --- test/functional/rpc_createmultisig.py | 31 +++++++++++++++++++++------ 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 983aeafc1c5..e14dea70e61 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -56,12 +56,13 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): self.generate(self.wallet, 149) wallet_multi = self.create_wallet(node1, 'wmulti') if self._requires_wallet else None - self.create_keys(5) - for nkeys in [3, 5]: - for nsigs in [2, 3]: - for output_type in ["bech32", "p2sh-segwit", "legacy"]: - self.do_multisig(nkeys, nsigs, output_type, wallet_multi) + self.create_keys(21) # max number of allowed keys + 1 + m_of_n = [(2, 3), (3, 3), (2, 5), (3, 5), (10, 15), (15, 15)] + for (sigs, keys) in m_of_n: + for output_type in ["bech32", "p2sh-segwit", "legacy"]: + self.do_multisig(keys, sigs, output_type, wallet_multi) + self.test_multisig_script_limit() self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) self.test_sortedmulti_descriptors_bip67() @@ -83,6 +84,21 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses] assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m") + def test_multisig_script_limit(self): + node1 = self.nodes[1] + pubkeys = self.pub[0:20] + + self.log.info('Test legacy redeem script max size limit') + assert_raises_rpc_error(-8, "redeemScript exceeds size limit: 684 > 520", node1.createmultisig, 16, pubkeys, 'legacy') + + self.log.info('Test valid 16-20 multisig p2sh-legacy and bech32 (no wallet)') + self.do_multisig(nkeys=20, nsigs=16, output_type="p2sh-segwit", wallet_multi=None) + self.do_multisig(nkeys=20, nsigs=16, output_type="bech32", wallet_multi=None) + + self.log.info('Test invalid 16-21 multisig p2sh-legacy and bech32 (no wallet)') + assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'p2sh-segwit') + assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'bech32') + def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes pub_keys = self.pub[0: nkeys] @@ -117,13 +133,13 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): assert mredeemw == mredeem spk = address_to_scriptpubkey(madd) - value = decimal.Decimal("0.00001300") + value = decimal.Decimal("0.00004000") tx = self.wallet.send_to(from_node=self.nodes[0], scriptPubKey=spk, amount=int(value * COIN)) prevtxs = [{"txid": tx["txid"], "vout": tx["sent_vout"], "scriptPubKey": spk.hex(), "redeemScript": mredeem, "amount": value}] self.generate(node0, 1) - outval = value - decimal.Decimal("0.00001000") + outval = value - decimal.Decimal("0.00002000") # deduce fee (must be higher than the min relay fee) # send coins to node2 when wallet is enabled node2_balance = node2.getbalances()['mine']['trusted'] if self.is_wallet_compiled() else 0 out_addr = node2.getnewaddress() if self.is_wallet_compiled() else getnewdestination('bech32')[2] @@ -157,6 +173,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): rawtx2 = node2.signrawtransactionwithkey(rawtx, priv_keys[0:nsigs - 1], prevtxs) rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [priv_keys[-1]], prevtxs) + assert rawtx3['complete'] tx = node0.sendrawtransaction(rawtx3["hex"], 0) blk = self.generate(node0, 1)[0] From 53302a09817e5b799d345dfea432546a55a9d727 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 18:16:41 -0300 Subject: [PATCH 09/10] bugfix: addmultisigaddress, add unsupported operation for redeem scripts over 520 bytes Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed on 'p2sh-segwit' and 'bech32' redeem scripts. Although redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are technically valid for segwit output types, we don't want to enable this feature in legacy wallets for the following reasons: 1) It introduces a compatibility-breaking change requiring downgrade protection; older wallets would be unable to interact with these "new" legacy wallets. 2) Considering the ongoing deprecation of the legacy spkm, this issue adds another good reason to transition towards descriptors. --- src/wallet/rpc/addresses.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index bcc39b05b86..62188249dae 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -296,7 +296,20 @@ RPCHelpMan addmultisigaddress() // Import scripts into the wallet for (const auto& [id, script] : provider.scripts) { - spk_man.AddCScript(script); + // Due to a bug in the legacy wallet, the p2sh maximum script size limit is also imposed on 'p2sh-segwit' and 'bech32' redeem scripts. + // Even when redeem scripts over MAX_SCRIPT_ELEMENT_SIZE bytes are valid for segwit output types, we don't want to + // enable it because: + // 1) It introduces a compatibility-breaking change requiring downgrade protection; older wallets would be unable to interact with these "new" legacy wallets. + // 2) Considering the ongoing deprecation of the legacy spkm, this issue adds another good reason to transition towards descriptors. + if (script.size() > MAX_SCRIPT_ELEMENT_SIZE) throw JSONRPCError(RPC_WALLET_ERROR, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts"); + + if (!spk_man.AddCScript(script)) { + if (CScript inner_script; spk_man.GetCScript(CScriptID(script), inner_script)) { + CHECK_NONFATAL(inner_script == script); // Nothing to add, script already contained by the wallet + continue; + } + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Error importing script into the wallet")); + } } // Store destination in the addressbook From 2451a217dd2c21b6d2f2b2699ceddd0bf9073019 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 21 Aug 2023 18:18:00 -0300 Subject: [PATCH 10/10] test: addmultisigaddress, coverage for script size limits --- test/functional/rpc_createmultisig.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index e14dea70e61..fdac3623d3c 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -62,7 +62,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): for output_type in ["bech32", "p2sh-segwit", "legacy"]: self.do_multisig(keys, sigs, output_type, wallet_multi) - self.test_multisig_script_limit() + self.test_multisig_script_limit(wallet_multi) self.test_mixing_uncompressed_and_compressed_keys(node0, wallet_multi) self.test_sortedmulti_descriptors_bip67() @@ -84,7 +84,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses] assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m") - def test_multisig_script_limit(self): + def test_multisig_script_limit(self, wallet_multi): node1 = self.nodes[1] pubkeys = self.pub[0:20] @@ -99,6 +99,17 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'p2sh-segwit') assert_raises_rpc_error(-8, "Number of keys involved in the multisignature address creation > 20", node1.createmultisig, 16, self.pub, 'bech32') + # Check legacy wallet related command + self.log.info('Test legacy redeem script max size limit (with wallet)') + if wallet_multi is not None and not self.options.descriptors: + assert_raises_rpc_error(-8, "redeemScript exceeds size limit: 684 > 520", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'legacy') + + self.log.info('Test legacy wallet unsupported operation. 16-20 multisig p2sh-legacy and bech32 generation') + # Due an internal limitation on legacy wallets, the redeem script limit also applies to p2sh-segwit and bech32 (even when the scripts are valid) + # We take this as a "good thing" to tell users to upgrade to descriptors. + assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'p2sh-segwit') + assert_raises_rpc_error(-4, "Unsupported multisig script size for legacy wallet. Upgrade to descriptors to overcome this limitation for p2sh-segwit or bech32 scripts", wallet_multi.addmultisigaddress, 16, pubkeys, '', 'bech32') + def do_multisig(self, nkeys, nsigs, output_type, wallet_multi): node0, node1, node2 = self.nodes pub_keys = self.pub[0: nkeys]