tests: Update more tests to work with descriptor wallets

achow101 commented at 7:57 pm on April 27, 2020: member

I went through all the tests and checked whether they passed with descriptor wallets. This partially informed some changes in #16528. Some tests needed changes to work with descriptor wallets. These were primarily due to import and watchonly behavior. There are some tests and test cases that only test legacy wallet behavior so those tests won’t be run with descriptor wallets.

This PR updates more tests to have to the --descriptors switch in test_runner.py. Additionally a mutually exclusive --legacy-wallet option has been added to force legacy wallets. This does nothing currently but will be useful in the future when descriptor wallets are the default. For the tests that rely on legacy wallet behavior, this option is being set so that we don’t forget in the future. Those tests are feature_segwit.py, wallet_watchonly.py, wallet_implicitsegwit.py, wallet_import_with_label.py, and wallet_import_with_label.py.

If you invert the --descriptors/--legacy-wallet default so that descriptor wallets are the default, all tests (besides the legacy wallet specific ones) will pass.

fanquake added the label Tests on Apr 27, 2020

fanquake added the label Wallet on Apr 27, 2020

DrahtBot commented at 11:03 pm on April 27, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20199 (wallet: ignore (but warn) on duplicate -wallet parameters by jonasschnelli)
#20094 (wallet: Unify wallet directory lock error message by hebasto)
#19502 (Bugfix: Wallet: Soft-fail exceptions within ListWalletDir file checks by luke-jr)
#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#19013 (test: add v0.20.1 to backwards compatibility test by Sjors)

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.

MarcoFalke commented at 12:32 pm on April 28, 2020: member

Concept ACK, but I’d prefer if we got the current tests green first: #18800

DrahtBot added the label Needs rebase on Apr 28, 2020

achow101 force-pushed on Apr 28, 2020

DrahtBot removed the label Needs rebase on Apr 28, 2020

DrahtBot added the label Needs rebase on May 3, 2020

Sjors commented at 9:27 am on May 4, 2020: member

Concept ACK. Needs rebase, now that #18805 landed, which fixed #18800.

achow101 force-pushed on May 4, 2020

DrahtBot removed the label Needs rebase on May 4, 2020

achow101 force-pushed on May 12, 2020

Sjors commented at 7:20 pm on May 15, 2020: member

Tests pass on macOS. The commit messages could be more descriptive than “Fix wallet_importprunedfunds”, if only to aid the lazy reviewer in understanding what problems you’re solving.

MarcoFalke commented at 0:09 am on May 19, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on May 19, 2020

achow101 force-pushed on May 19, 2020

DrahtBot removed the label Needs rebase on May 19, 2020

achow101 commented at 5:04 pm on May 26, 2020: member

I’ve updated the commit messages to be more descriptive.

achow101 force-pushed on May 26, 2020

in test/functional/rpc_fundrawtransaction.py:492 in e3dc50344a outdated

489@@ -480,22 +490,39 @@ def test_spend_2of2(self):
490         oldBalance = self.nodes[1].getbalance()
491         inputs = []
492         outputs = {self.nodes[1].getnewaddress():1.1}
493-        rawtx = self.nodes[2].createrawtransaction(inputs, outputs)

Sjors commented at 2:41 pm on June 1, 2020:

In e3dc50344a57b926373bcc51aa2e213a682051be it seems problematic to replace createrawtransaction and fundrawtransaction with PSBT methods in rpc_fundrawtransaction.py.

achow101 commented at 4:01 pm on June 1, 2020:

I think it’s fine in some instances (like this one) because the funding code is the same and the goal is to check that the funding works.

Sjors commented at 4:22 pm on June 1, 2020:

Do we still sufficiently test the createrawtransaction and fundrawtransaction RPC elsewhere? Those are rather ancient, with all cool kids using PSBT it might be a while before we see a regression.

achow101 commented at 4:30 pm on June 1, 2020:

IMO we do. createrawtransaction and fundrawtransaction are still being tested in rpc_fundrawtransaction.py as well is in other tests. Additionally, fundrawtransaction basically only calls FundTransaction after a bit of options parsing, and walletcreatefundedpsbt calls this function as well, right after transaction creation.

Sjors commented at 2:55 pm on June 1, 2020: member

f57cacf246375ab5f4338a2f169e72bc1cb3c63f looks good, except one issue, see inline.

0472e9f12c4c2efa5a0afaeee55f4311ffbd16f7: the commit message is confusing. Why is this code being ~duplicated?

achow101 commented at 9:05 pm on June 2, 2020: member

0472e9f: the commit message is confusing. Why is this code being ~duplicated?

I need to get the scriptPubKeys for those things, not just the address. So the functions are partially duplicated to generate the scripts rather than the addresses.

Sjors commented at 12:37 pm on June 5, 2020: member

Is there an easy way to convert scripts to addresses in the Python framework?

Anyway, ACK f57cacf

DrahtBot added the label Needs rebase on Jun 11, 2020

achow101 force-pushed on Jun 12, 2020

DrahtBot removed the label Needs rebase on Jun 12, 2020

Sjors commented at 11:25 am on June 16, 2020: member

re-utACK 02357c7cd965ae5701d129691d4487a89f5d6deb, just some rebase related import changes

MarcoFalke removed the label Wallet on Jun 16, 2020

MarcoFalke renamed this:
~~wallet: tests: Update more tests to work with descriptor wallets~~
tests: Update more tests to work with descriptor wallets
on Jun 16, 2020

DrahtBot added the label Needs rebase on Sep 1, 2020

achow101 force-pushed on Sep 1, 2020

DrahtBot removed the label Needs rebase on Sep 1, 2020

Sjors commented at 12:07 pm on September 1, 2020: member

re-utACK 02357c7cd965ae5701d129691d4487a89f5d6deb

Spurious Travis error reported in #19853.

DrahtBot added the label Needs rebase on Sep 15, 2020

achow101 force-pushed on Sep 15, 2020

DrahtBot removed the label Needs rebase on Sep 15, 2020

fjahr commented at 9:50 pm on September 26, 2020: member

Concept ACK, will take a closer look soon.

Sjors commented at 10:45 am on October 2, 2020: member

re-utACK c45302f6f1d9e7e210b51f7bd580e498a60b2442

DrahtBot added the label Needs rebase on Oct 2, 2020

achow101 force-pushed on Oct 2, 2020

DrahtBot removed the label Needs rebase on Oct 2, 2020

achow101 force-pushed on Oct 7, 2020

DrahtBot added the label Needs rebase on Oct 11, 2020

achow101 force-pushed on Oct 11, 2020

DrahtBot removed the label Needs rebase on Oct 11, 2020

DrahtBot added the label Needs rebase on Oct 15, 2020

laanwj added this to the milestone 0.21.0 on Oct 15, 2020

achow101 force-pushed on Oct 15, 2020

DrahtBot removed the label Needs rebase on Oct 15, 2020

DrahtBot added the label Needs rebase on Oct 21, 2020

achow101 force-pushed on Oct 21, 2020

DrahtBot removed the label Needs rebase on Oct 21, 2020

achow101 force-pushed on Oct 26, 2020

in test/functional/wallet_listtransactions.py:94 in b014b8302b outdated

101-        assert_equal(len(self.nodes[0].listtransactions(dummy="watchonly", include_watchonly=True)), 1)
102-        assert len(self.nodes[0].listtransactions(label="watchonly", count=100, include_watchonly=False)) == 0
103-        assert_array_result(self.nodes[0].listtransactions(label="watchonly", count=100, include_watchonly=True),
104-                            {"category": "receive", "amount": Decimal("0.1")},
105-                            {"txid": txid, "label": "watchonly"})
106+        if not self.options.descriptors:

ryanofsky commented at 10:05 pm on October 26, 2020:

In commit “Make import tests in wallet_listtransactions.py legacy wallet only” (b014b8302bb31d5c8a4c4cbc8998576b3edc5294)

Checks seem to be skipped without explanation here. It’d be good have a comment saying whether it would be good to port the checks to descriptor wallets in the future or if that’s a bad idea or not possible.

achow101 commented at 0:00 am on October 27, 2020:

It’s not necessary because it is testing the include_watchonly parameter in listtransactions which is not present in descriptor wallets.

achow101 commented at 8:07 pm on October 27, 2020:

Added a comment. It is not necessary to test these for descriptor wallets, that’s why they are disabled in the first place. Otherwise there would be a TODO.

in test/functional/wallet_importprunedfunds.py:34 in 35607a2b46 outdated

32@@ -30,8 +33,11 @@ def run_test(self):
33         # pubkey
34         address2 = self.nodes[0].getnewaddress()
35         # privkey
36-        address3 = self.nodes[0].getnewaddress()
37-        address3_privkey = self.nodes[0].dumpprivkey(address3)  # Using privkey

ryanofsky commented at 10:11 pm on October 26, 2020:

In commit “Update wallet_importprunedfunds to avoid dumpprivkey” (35607a2b46f4088b1f3ec108baecf8ba221dbc02)

General question that doesn’t necessarily impact the PR, but is there an equivalent to dumpprivkey for descriptor wallets?

achow101 commented at 11:59 pm on October 26, 2020:

Not currently.

in test/functional/rpc_rawtransaction.py:246 in 186642062f outdated

356-        rawTx2 = self.nodes[0].decoderawtransaction(rawTxComb)
357-        self.sync_all()
358-        self.nodes[0].generate(1)
359-        self.sync_all()
360-        assert_equal(self.nodes[0].getbalance(), bal+Decimal('50.00000000')+Decimal('2.19000000')) #block reward + tx
361+        if not self.options.descriptors:

ryanofsky commented at 10:54 pm on October 26, 2020:

In commit “Make raw multisig tests legacy wallet only in rpc_rawtransaction.py” (186642062fc7165d609c6d4f590a18baa4199c77)

Comment from commit description would be helpful to add here: “The traditional multisig workflow doesn’t work with descriptor wallets so make these tests legacy wallet only.” It’d also be good if it said whether it’d make sense in the future to add descriptor multisig tests here or if that wouldn’t be appropriate or possible for some reason

achow101 commented at 8:07 pm on October 27, 2020:

Added a comment. It is not necessary to test these for descriptor wallets, that’s why they are disabled in the first place. Otherwise there would be a TODO.

in test/functional/wallet_address_types.py:246 in bfa84b9881 outdated

251+            self.test_address(1, self.nodes[1].addmultisigaddress(2, [compressed_1, compressed_2])['address'], True, 'p2sh-segwit')
252+            self.test_address(2, self.nodes[2].addmultisigaddress(2, [compressed_1, compressed_2])['address'], True, 'p2sh-segwit')
253+            self.test_address(3, self.nodes[3].addmultisigaddress(2, [compressed_1, compressed_2])['address'], True, 'bech32')
254+
255+        do_multisigs = [False]
256+        if not self.options.descriptors:

ryanofsky commented at 10:57 pm on October 26, 2020:

In commit “Do addmultisigaddress tests in legacy wallet mode in wallet_address_types.py” (bfa84b9881428376804af1586a8a3d6df95d48ba)

Same request here as for other places. Can there be a comment here saying whether it’s good to skip these checks or if it’d be possible or beneficial to add multisig descriptor checks here in the future?

achow101 commented at 8:07 pm on October 27, 2020:

Added a comment. It is not necessary to test these for descriptor wallets, that’s why they are disabled in the first place. Otherwise there would be a TODO.

in test/functional/rpc_signrawtransaction.py:163 in f4f81c98dc outdated

166         self.nodes[0].generate(101)
167         self.nodes[0].sendtoaddress(p2sh_p2wsh_address["address"], 49.999)
168         self.nodes[0].generate(1)
169         self.sync_all()
170-        # Find the UTXO for the transaction node[1] should have received, check witnessScript matches
171-        unspent_output = self.nodes[1].listunspent(0, 999999, [p2sh_p2wsh_address["address"]])[0]

ryanofsky commented at 11:08 pm on October 26, 2020:

In commit “Avoid dumpprivkey and watchonly behavior in rpc_signrawtransaction.py” (f4f81c98dca6da030b84b5b4887cf3ef4b72baec)

Can commit description be updated to mention this change and reason listunspent is not longer used or checked? Change seems reasonable but I don’t get it currently

achow101 commented at 8:08 pm on October 27, 2020:

Updated the commit message. listunspent is not being used and is not the test target for this test. It is tested elsewhere. There is no need to attempt to bring this back for descriptor wallets.

in test/functional/test_framework/script_util.py:81 in a56fc6c8c8 outdated

76+    assert False
77+
78+def check_script(script):
79+    if (type(script) is str):
80+        script = hex_str_to_bytes(script) # Assuming this is hex string
81+    if (type(script) is bytes or type(script) is CScript):

ryanofsky commented at 11:11 pm on October 26, 2020:

In commit “Add script equivalent of functions in address.py” (a56fc6c8c8f58f4a6ade3ac1437875bf8071a844)

Not important, but it’s usually better to say isinstance(obj, obj_type) instead of type(obj) is obj_type to not interfere with subclassing. Applies various places this commit

achow101 commented at 8:08 pm on October 27, 2020:

Done

in test/functional/feature_nulldummy.py:62 in ed97c9b67e outdated

61+        self.address = w0.getnewaddress()
62+        self.pubkey = w0.getaddressinfo(self.address)['pubkey']
63+        self.ms_address = wmulti.addmultisigaddress(1, [self.pubkey])['address']
64+        self.wit_address = w0.getnewaddress(address_type='p2sh-segwit')
65+        self.wit_ms_address = wmulti.addmultisigaddress(1, [self.pubkey], '', 'p2sh-segwit')['address']
66+        if not self.options.descriptors:

ryanofsky commented at 11:20 pm on October 26, 2020:

In commit “Use separate watchonly wallet for multisig in feature_nulldummy.py” (ed97c9b67e715815f653e20f5e478a4dad4e13f8)

Can a comment be added to explain why the non-descriptor version of the test needs these addresses imported to pass, while the descriptor version doesn’t need them? (Assuming that is the case.) I’m missing some understanding here

achow101 commented at 0:04 am on October 27, 2020:

Legacy wallets need these imported for the test to work. Otherwise they won’t watch the multisig. Descriptor wallets will always watch imported things.

achow101 commented at 8:09 pm on October 27, 2020:

Added a comment.

in test/functional/wallet_balance.py:60 in 8ee94451ad outdated

56@@ -57,14 +57,15 @@ def skip_test_if_missing_module(self):
57         self.skip_if_no_wallet()
58 
59     def run_test(self):
60-        self.nodes[0].importaddress(ADDRESS_WATCHONLY)
61-        # Check that nodes don't own any UTXOs
62-        assert_equal(len(self.nodes[0].listunspent()), 0)
63-        assert_equal(len(self.nodes[1].listunspent()), 0)
64+        if not self.options.descriptors:

ryanofsky commented at 11:34 pm on October 26, 2020:

In commit “Move import and watchonly tests to be legacy wallet only in wallet_balance.py” (8ee94451ad55b81a53c38c6b8cee0c13f1b1f9e7)

It would be helpful to add the comment here from the commit description: “Imports and watchonly behavior are legacy wallet only, so make them only run when the test is in legacy wallet mode.” It would also be helpful to say whether it is a good thing to be skipping these checks, or if the test maybe should be port these checks to work with a watchonly descriptor wallet in the future

achow101 commented at 8:09 pm on October 27, 2020:

Added a comment. It is not necessary to test these for descriptor wallets, that’s why they are disabled in the first place. Otherwise there would be a TODO.

in test/functional/wallet_balance.py:167 in 8ee94451ad outdated

161@@ -156,7 +162,12 @@ def test_balances(*, fee_node_1=0):
162             expected_balances_1 = {'mine':      {'immature':          Decimal('0E-8'),
163                                                  'trusted':           Decimal('0E-8'),  # node 1's send had an unsafe input
164                                                  'untrusted_pending': Decimal('30.0') - fee_node_1}}  # Doesn't include output of node 0's send since it was spent
165-            assert_equal(self.nodes[0].getbalances(), expected_balances_0)
166+            if self.options.descriptors:

ryanofsky commented at 11:37 pm on October 26, 2020:

In commit “Move import and watchonly tests to be legacy wallet only in wallet_balance.py” (8ee94451ad55b81a53c38c6b8cee0c13f1b1f9e7)

Would suggest writing this as:

0if self.options.descriptors:
1   del expected_balances_0['watchonly']
2assert_equal(self.nodes[0].getbalances(), expected_balances_0)

to keep checking for new fields as strict and to duplicate less code

achow101 commented at 8:09 pm on October 27, 2020:

Done

in test/functional/wallet_balance.py:249 in 8ee94451ad outdated

256@@ -246,7 +257,7 @@ def test_balances(*, fee_node_1=0):
257         self.nodes[1].sendrawtransaction(hexstring=tx_replace, maxfeerate=0)
258 
259         # Now confirm tx_replace
260-        block_reorg = self.nodes[1].generatetoaddress(1, ADDRESS_WATCHONLY)[0]

ryanofsky commented at 11:44 pm on October 26, 2020:

In commit “Move import and watchonly tests to be legacy wallet only in wallet_balance.py” (8ee94451ad55b81a53c38c6b8cee0c13f1b1f9e7)

I don’t get these changes. Does the address generated to matter for the tests or is just code cleanup? It would be good to mention these changes in the commit description and say what they are

achow101 commented at 8:09 pm on October 27, 2020:

Reverted.

ryanofsky approved

ryanofsky commented at 11:55 pm on October 26, 2020: member

Code review ACK 2cc253dc7f385f774fa057eed5e2340ffbd022f2. All looks very good. My review comments below are just asking for comments & clarifications and aren’t important

achow101 force-pushed on Oct 27, 2020

achow101 commented at 8:09 pm on October 27, 2020: member

Going to also add more changes to wallet_multiwallet.py soon.

achow101 force-pushed on Oct 27, 2020

achow101 commented at 9:36 pm on October 27, 2020: member

Actually I’ll deal with multiwallet separately. It’s going to be involved in a followup to drop the wallet dirs thing.

ryanofsky approved

ryanofsky commented at 9:56 pm on October 27, 2020: member

Code review ACK bf7142f0f0ce5b8d8a65b9732c7ccaccbe718f94. Changes since last review: two new commits, lots of comments and extra commit message information in existing commits, and test runner change to avoid running some legacy tests twice

achow101 force-pushed on Oct 27, 2020

achow101 force-pushed on Oct 29, 2020

achow101 commented at 6:53 pm on October 29, 2020: member

Rebased after #20186 and also fixed wallet_multiwallet.py.

ryanofsky approved

ryanofsky commented at 7:46 pm on October 29, 2020: member

Code review ACK 5d24a64ab8aff27f0df567bd8421f07dbe8c5b2f. Changes since last review: rebase, isinstance fixes, compatibility test fixes, and two new commits adding more –descriptors support. New commit using descriptors more widely in wallet_multiwallet.py makes the test coverage a lot better, and the code clearer too

in test/functional/rpc_fundrawtransaction.py:639 in dc7dda3451 outdated

667 
668         inputs = []
669         outputs = {self.nodes[2].getnewaddress(): self.watchonly_amount}
670         rawtx = self.nodes[3].createrawtransaction(inputs, outputs)
671 
672-        # Backward compatibility test (2nd param is includeWatching).

MarcoFalke commented at 3:17 pm on October 30, 2020:

Looks like this test is removed. Is this tested elsewhere? If not, might be good to skip with descriptor wallets.

achow101 commented at 5:26 pm on October 30, 2020:

Reintroduced this check in the previous watchonly test.

in test/functional/wallet_listsinceblock.py:201 in 9b71e3ba39 outdated

201+        utxo = None
202+        for u in utxos:
203+            if u['address'] == address:
204+                utxo = u
205+                break
206+        assert utxo is not None

MarcoFalke commented at 3:22 pm on October 30, 2020:

Can be written shorter:

0utxo = [u for u in utxos if u['address'] == address][0]

achow101 commented at 5:26 pm on October 30, 2020:

Done

in test/functional/feature_nulldummy.py:63 in 92714b79d0 outdated

62+        self.pubkey = w0.getaddressinfo(self.address)['pubkey']
63+        self.ms_address = wmulti.addmultisigaddress(1, [self.pubkey])['address']
64+        self.wit_address = w0.getnewaddress(address_type='p2sh-segwit')
65+        self.wit_ms_address = wmulti.addmultisigaddress(1, [self.pubkey], '', 'p2sh-segwit')['address']
66+        if not self.options.descriptors:
67+            # Legacy wallets ned to import these so that they are watched by the wallet. This is unnecssary (and does not need to be tested) for descriptor wallets

MarcoFalke commented at 3:30 pm on October 30, 2020:

nit: ned->need

achow101 commented at 5:26 pm on October 30, 2020:

Fixed

in test/functional/feature_backwards_compatibility.py:295 in 6d3a3bd865 outdated

351+            wallet = node_v17.get_wallet_rpc("w2")
352+            info = wallet.getwalletinfo()
353+            assert info['private_keys_enabled'] == False
354+            assert info['keypoolsize'] == 0
355+        else:
356+            # Descriptor wallets appear to corrupted wallets to old software

MarcoFalke commented at 4:02 pm on October 30, 2020:

nit to->as?

achow101 commented at 5:25 pm on October 30, 2020:

Changed to “to be”

in test/functional/feature_backwards_compatibility.py:324 in 6d3a3bd865 outdated

332         # Instead, we stop node and try to launch it with the wallet:
333         self.stop_node(4)
334         node_v17.assert_start_raises_init_error(["-wallet=w3_v18"], "Error: Error loading w3_v18: Wallet requires newer version of Bitcoin Core")
335-        node_v17.assert_start_raises_init_error(["-wallet=w3"], "Error: Error loading w3: Wallet requires newer version of Bitcoin Core")
336+        if self.options.descriptors:
337+            # Descriptor wallets appear to corrupted wallets to old software

MarcoFalke commented at 4:02 pm on October 30, 2020:

nit: to->as

achow101 commented at 5:25 pm on October 30, 2020:

Changed to “to be”

in test/functional/tool_wallet.py:265 in acf24b4a9e outdated

260@@ -261,9 +261,12 @@ def run_test(self):
261         # Warning: The following tests are order-dependent.
262         self.test_tool_wallet_info()
263         self.test_tool_wallet_info_after_transaction()
264-        self.test_tool_wallet_create_on_existing_wallet()
265-        self.test_getwalletinfo_on_different_wallet()
266-        self.test_salvage()
267+        if not self.options.descriptors:
268+            # TODO: Wallet tool needs more create options at which point these can be enabled.

MarcoFalke commented at 4:03 pm on October 30, 2020:

Running the tests on commit 80bb5ca8cf fails.

This commit might need to come first?

achow101 commented at 5:25 pm on October 30, 2020:

Moved it.

MarcoFalke approved

MarcoFalke commented at 4:09 pm on October 30, 2020: member

quick cr ACK 5d24a64ab8

in test/functional/wallet_labels.py:162 in c1f7bfc6bb outdated

155@@ -156,7 +156,7 @@ def run_test(self):
156             ad = BECH32_INVALID[l]
157             assert_raises_rpc_error(
158                 -5,
159-                "Invalid Bitcoin address or script",
160+                "Address is not valid" if self.options.descriptors else "Invalid Bitcoin address or script",
161                 lambda: wallet_watch_only.importaddress(label=l, rescan=False, address=ad),
162             )
163

MarcoFalke commented at 4:11 pm on October 30, 2020:

in commit c1f7bfc6bb1074c26d670976a8f8c0bb95be1858:

Should wallet_labels be run with and without descriptors in the test_runner?

achow101 commented at 5:25 pm on October 30, 2020:

Yes, labels are not effected by descriptor wallets.

achow101 force-pushed on Oct 30, 2020

in test/functional/wallet_createwallet.py:155 in f1ea62d6c6 outdated

152+        if self.options.descriptors:
153+            assert_equal(walletinfo['keypoolsize'], 3)
154+            assert_equal(walletinfo['keypoolsize_hd_internal'], 3)
155+        else:
156+            assert_equal(walletinfo['keypoolsize'], 1)
157+            assert_equal(walletinfo['keypoolsize_hd_internal'], 1)

mjdietzx commented at 6:25 pm on October 30, 2020:

Would this be a little easier to follow? Also, is the comment on l148 still relevant # There should only be 1 key?

0nkeys = 3 if self.options.descriptors else 1
1assert_equal(walletinfo['keypoolsize'], nkeys)
2assert_equal(walletinfo['keypoolsize_hd_internal'], nkeys)

achow101 commented at 6:44 pm on October 30, 2020:

Done

MarcoFalke commented at 8:14 am on October 31, 2020:

I thin the previous version of the code was more correct. See ci failure

achow101 commented at 7:04 pm on October 31, 2020:

Oops, I forgot to change the second assert. Should be fixed now.

in test/functional/test_framework/blocktools.py:186 in f1ea62d6c6 outdated

188+            wrpc = node.get_wallet_rpc(w)
189+            signed_psbt = wrpc.walletprocesspsbt(psbt)
190+            psbt = signed_psbt['psbt']
191+    final_psbt = node.finalizepsbt(psbt)
192+    assert_equal(final_psbt["complete"], True)
193+    return final_psbt['hex']

mjdietzx commented at 6:30 pm on October 30, 2020:

I haven’t been able to wrap my head around this / how this change fits into the greater PR. If you don’t mind could you give me a brief explanation or point me in the right direction?

achow101 commented at 6:46 pm on October 30, 2020:

This change makes this function use the PSBT workflow rather than the old raw transactions workflow. Additionally, it tries to sign with all of the wallets loaded by the node.

ryanofsky commented at 6:50 pm on October 30, 2020:

I haven’t been able to wrap my head around this / how this change fits into the greater PR. If you don’t mind could you give me a brief explanation or point me in the right direction?

My understanding is that legacy wallets support a mix of watch only and spendable scriptpubkeys, while descriptor wallets have to choose between only watch-only or spendable descriptors and refuse to hold both. So while a single wallet worked in many of these tests previously, now two wallets are required to keep all of the same test coverage. In some cases like this the PR creates an extra watchonly wallet to maintain coverage. In other cases it keeps the coverage for legacy wallets but drops it for descriptors to avoid needing the extra wallet.

I think the descriptor watchonly limitation is supposed to be somewhat artificial currently, but in the future if every wallet is required to be fully watchonly or not, then GUI & RPC interfaces may be simplified later (e.g. not have to show two balances).

achow101 force-pushed on Oct 30, 2020

MarcoFalke commented at 8:16 am on October 31, 2020: member

Almost cr ACK d137ec7d8499497fde60e23d0cb5fc7f7387e5e3 🍱

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Almost cr ACK d137ec7d8499497fde60e23d0cb5fc7f7387e5e3 🍱
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUj9cAwAvAxznEyfwZnurWI8ACsC3a6Y0Hj28zdqM99HbnkyRdrwcnXhLpgr++pS
 89sJw03kdVgx6XqYOhet6naCU3AieNexe92TcLaFzTIVxnWVPAsPr97og6w3CpP65
 9SM6HHQdo2bUC1CAum/hq9oj0n9oQ8EAFYBAJhNNfArIEvGKMsO2eai8QlggQU23B
10aiIo+pi+do3GAJ02zjXH9ed7kQJVsobOhO/KCBWQVGqQQjj3sC4t++A9kE41LKu/
11Jv0U7pxpEF7J3ghy9fLPeDLP1QadIsZIQTa/kBuEzLMGdrmr1iTAmPT46vd6VORd
12fUevN58HqkUy+cihFjjT11YxASbeVy9eFrI5ldswZIePN5PUyGj6NVdOCc9LTz8+
13CX1RNw/V7cVSwgk05GA/zQVSRRxjkTDJHpS23i1cxliUiQ17ryQeVimBfWnl0CCt
146fuZa7iMzDt0AyAP231bSuiIuQwG1x9CsaYVZZA9mFa8C7IzaUlR4zO8dwm5s9EG
15B3zeS/DY
16=XRP+
17-----END PGP SIGNATURE-----

Timestamp of file with hash 64884ac60aea3836fd1197bfc420b7916e0c7bc4009dbc1c6a7154bab7e960d9 -

achow101 force-pushed on Oct 31, 2020

Update wallet_importprunedfunds to avoid dumpprivkey

Removes the use of dumpprivkey so that descriptor wallets can pass on
this. Also does a few descriptor wallet specific changes due to
different IsMine semantics.

a357111047

Use a separate watchonly wallet in rpc_fundrawtransaction.py

Import things into a separate watchonly wallet to work with descriptor
wallets.

dc81418fd0

Make import tests in wallet_listtransactions.py legacy wallet only

Existing import* RPCs are disabled for descriptor wallets, so only do
these tests for legacy wallets.

553dbf9af4

Avoid dumpprivkey in wallet_listsinceblock.py

Generate a privkey in the test framework instead of using dumpprivkey so
that descriptor wallets work in this test.

c2711e4230

Use importdescriptors for descriptor wallets in wallet_bumpfee.py

If using descriptor wallets, use importdescriptors instead of
importmulti.

4b871909d6

Move import and watchonly tests to be legacy wallet only in wallet_balance.py

Imports and watchonly behavior are legacy wallet only, so make them only
run when the test is in legacy wallet mode.

a42652ec10

Use separate watchonly wallet for multisig in feature_nulldummy.py

Create and import the multisig into a separate watchonly wallet so that
feature_nulldummy.py works with descriptor wallets.

blocktools.create_raw_transaction is also updated to use multiple nodes
and wallets and to use PSBT so that this test passes.

3457679870

Add descriptor wallet output to tool_wallet.py

Descriptor wallets output slightly different information in the wallet
tool, so check that output when in descriptor wallet mode.

86968882a8

Add script equivalent of functions in address.py 08067aebfd

Avoid dumpprivkey and watchonly behavior in rpc_signrawtransaction.py

dumpprivkey and watchonly behavior don't work with descriptor wallets.

Test for multisigs is modified to not rely on watchonly behavior for
those multisigs. This has a side effect of removing listunspent, but
that's not the target of this test, so that's fine.

0bd1860300

Use importdescriptors when in descriptor wallet mode in wallet_createwallet.py

sethdseed and importmulti are not available for descriptor wallets, so
when doing descriptor wallet tests, use importdescriptors instead.

Also changes some output to match what descriptor wallets will return.

25bc5dccbf

Do addmultisigaddress tests in legacy wallet mode in wallet_address_types.py

addmultisigaddress is not available in descriptor wallets, so only run
these when testing legacy wallets

59d3da5bce

Make raw multisig tests legacy wallet only in rpc_rawtransaction.py

The traditional multisig workflow doesn't work with descriptor wallets
so make these tests legacy wallet only.

47d3243160

Disable some tests for tool_wallet when descriptors

Some tests are legacy wallet only (and make legacy wallets) so they
shouldn't be run when doing descriptor wallet tests.

388053e172

tests: Add a --legacy-wallet that is mutually exclusive with --descriptors

Although legacy wallet is still the default, for future use, add a
--legacy-wallet option to the test framework. Additional tests for
descriptor wallets have been enabled with the --descriptors option.
Tests that must be legacy wallet only are being started with
--legacy-wallet. Even though this option does not currently do anything,
this will be helpful in the future when descriptor wallets become the
default.

242aed7cc1

Update wallet_labels.py to not require descriptors=False 9a4c631e1c

Update feature_backwards_compatibility for descriptor wallets 6c9c12bf87

Avoid creating legacy wallets in wallet_importdescriptors.py d4b67ad214

tests: Make only desc wallets for wallet_multwallet.py --descriptors c7b7e0a692

achow101 force-pushed on Nov 1, 2020

MarcoFalke commented at 10:00 am on November 2, 2020: member

review ACK c7b7e0a69265946aecc885be911c7650911ba2e3 🎿

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK c7b7e0a69265946aecc885be911c7650911ba2e3 🎿
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUixwgv+P+5WXXpHmaWiW4IZOO+ZwjR3ywM62Ifs63hMf83bFO0cwDH3NkwzY7hn
 8+qNiNx7qexza6nJUsiy0EcekBo743AGYx2Ttun33F+Dj9yN9I/aGZPJ3sIIc5grU
 9LXWfYgwgeQcb1Kkovkv5viEOo0riA4SksdCySxQkybreWgeCV5hfiz+aS95WJx6w
1090cGwfLqoj0ZOsAhE59Y4cNK0ZGZ/8MuJ+DLTT+uyJBnHAaxrGBFPtWx1MG+Npzq
11AKBHPO8d554nAp5Vw3blf/ZjKB++mCB0iN6gR0Ja3Mtne1YetnYXEVKHQ/Yfvo2F
12po/GIccCibNb1TvFjtnWUeX12bkBCqzXIKg0JAP4V6+THR6o3DwpeVIO4drk/oVf
13dxb1EVW/IugsM5diPhfyicbvYJavodB7eR8e03oVZv7JkBJmvyuuHLweKn+Ziuol
140ZuU6GxJGECuTT2r7VA4gwop/pueJ4l9lCm+RO9yTnfRLgIZ6gCq0lfQQctTlpWq
15d7wv/9hm
16=LD2k
17-----END PGP SIGNATURE-----

Timestamp of file with hash 4756f18231367024c68d5255b8972d1d0c9ad9f91dd41296c63c8c42b777c68b -

laanwj commented at 5:50 pm on November 2, 2020: member

ACK c7b7e0a69265946aecc885be911c7650911ba2e3

laanwj merged this on Nov 2, 2020

laanwj closed this on Nov 2, 2020

sidhujag referenced this in commit 571786b31e on Nov 3, 2020

MarcoFalke referenced this in commit d1e4c56309 on Jul 9, 2021

Fabcien referenced this in commit 579529325a on Dec 10, 2021

Fabcien referenced this in commit 4e56a15772 on Dec 10, 2021

Fabcien referenced this in commit e51202ca79 on Dec 10, 2021

deadalnix referenced this in commit 51cb39c87b on Dec 13, 2021

Fabcien referenced this in commit 4a6f5dba70 on Dec 13, 2021

Fabcien referenced this in commit ba8ac32b3d on Dec 13, 2021

Fabcien referenced this in commit 6cabd8ddde on Dec 15, 2021

Fabcien referenced this in commit b375c53efa on Dec 16, 2021

Fabcien referenced this in commit a2c5e69ed4 on Dec 16, 2021

Fabcien referenced this in commit 5b31979a5c on Dec 16, 2021

DrahtBot locked this on Feb 15, 2022

tests: Update more tests to work with descriptor wallets #18788

Conflicts