test: use MiniWallet for rpc

theStack commented at 2:21 am on December 26, 2021: member

This PR enables one more of the non-wallet functional tests (rpc_scantxoutset.py) to be run even with the Bitcoin Core wallet disabled by using the MiniWallet instead, as proposed in #20078 and #23858#issue-1088320649 more recently.

Reviewer’s guide:

[commit 1/3] For replacing the getnewaddress/getaddressinfo RPC calls a helper getnewdestination is introduced which allows to create addresses with the common address format types (’legacy’, ‘p2sh-segwit’ and ‘bech32’), but also additionally returns the corresponding pubkey and output script.
[commit 2/3] In order to send to addresses with MiniWallet, a helper address_to_scriptpubkey is introduced. It only supports legacy addresses (Base58Check) so far, which is sufficient for the scantxoutset test.
[commit 3/3] With those helpers, the use of MiniWallet in the test is quite straight-forward. To avoid repeatedly specifying parameters like from_node to MiniWallet’s send_to method, another test-internal helper sendtodestination is introduced which supports specifying the destination both as outputscript or as address.

fanquake added the label Tests on Dec 26, 2021

theStack force-pushed on Dec 26, 2021

DrahtBot commented at 2:48 am on December 26, 2021: member

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

Conflicts

No conflicts as of last run.

test: introduce `getnewdestination` helper for generating various address types

This serves as a replacement for the getnewaddress RPC if no wallet is
available. In addition to the address, it also returns the corresponding
public key and output script (scriptPubKey).

e704d4d26f

test: introduce `address_to_scriptpubkey` helper

Works only with legacy addresses (Base58Check) right now.

983ca0456c

DrahtBot added the label Needs rebase on Dec 26, 2021

theStack force-pushed on Dec 26, 2021

theStack commented at 11:40 am on December 26, 2021: member

Rebased on master.

w0xlt approved

w0xlt commented at 12:03 pm on December 26, 2021: contributor

tACK a58aea0

in test/functional/rpc_scantxoutset.py:29 in a58aea0ce0 outdated

23@@ -20,47 +24,42 @@ def set_test_params(self):
24         self.num_nodes = 1
25         self.setup_clean_chain = True
26 
27-    def skip_test_if_missing_module(self):
28-        self.skip_if_no_wallet()
29+    def sendtodestination(self, destination, amount):
30+        # interpret strings as addresses, assume scriptPubKey otherwise
31+        if isinstance(destination, str):
32+            destination = address_to_scriptpubkey(destination)

MarcoFalke commented at 12:28 pm on December 26, 2021:

last commit:

An alternative would be self.nodes[0].validateaddress(destination)['scriptPubKey'], but both work fine.

theStack commented at 2:58 pm on December 26, 2021:

Ah nice, for some reason I thought getaddressinfo is the only RPC that can do that. Decided to keep as it is though, to need fewer RPC calls (can be changed if other reviewers prefer to call validateaddress).

in test/functional/rpc_scantxoutset.py:36 in a58aea0ce0 outdated

34 
35     def run_test(self):
36         self.log.info("Mining blocks...")
37-        self.generate(self.nodes[0], 110)
38+        self.wallet = MiniWallet(self.nodes[0])
39+        self.generate(self.wallet, 120)

MarcoFalke commented at 12:28 pm on December 26, 2021:

Could this use the pre-mined chain?

theStack commented at 2:55 pm on December 26, 2021:

Good idea. Ironically to use the pre-mined chain MiniWallet needs the call exactly the RPC that this test checks (scantxoutset), but I don’t think it is a problem. Done.

in test/functional/rpc_scantxoutset.py:63 in a58aea0ce0 outdated

90-        shutil.rmtree(os.path.join(self.nodes[0].datadir, self.chain, 'wallets'))
91-        self.start_node(0, ['-nowallet'])
92-        self.import_deterministic_coinbase_privkeys()
93+        self.log.info("Restart node, mine again some blocks...")
94+        self.restart_node(0)
95         self.generate(self.nodes[0], 110)

MarcoFalke commented at 12:29 pm on December 26, 2021:

Is any of this needed?

theStack commented at 2:59 pm on December 26, 2021:

Hm, I don’t think creating more blocks has any value for the test. Removed it.

in test/functional/rpc_scantxoutset.py:72 in a58aea0ce0 outdated

70@@ -72,12 +71,12 @@ def run_test(self):
71 
72         self.restart_node(0, ['-nowallet'])

MarcoFalke commented at 12:29 pm on December 26, 2021:

is this needed?

theStack commented at 2:58 pm on December 26, 2021:

I don’t think it is, removed it.

MarcoFalke approved

MarcoFalke commented at 12:30 pm on December 26, 2021: member

Nice. Looks very good.

Left some questions on the third commit for unrelated lines.

DrahtBot removed the label Needs rebase on Dec 26, 2021

test: use MiniWallet for rpc_scantxoutset.py

This test can now be run even with the Bitcoin Core wallet disabled.

e844115dea

theStack force-pushed on Dec 26, 2021

theStack commented at 3:01 pm on December 26, 2021: member

Force-pushed with most of the suggestions by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/23866#pullrequestreview-840078708):

use pre-mined chain instead of manually generating new blocks
remove unnecessary node restarts

w0xlt approved

w0xlt commented at 10:27 pm on December 26, 2021: contributor

reACK e844115

MarcoFalke merged this on Dec 27, 2021

MarcoFalke closed this on Dec 27, 2021

theStack deleted the branch on Dec 27, 2021

sidhujag referenced this in commit ff06b44d98 on Dec 28, 2021

Fabcien referenced this in commit 5a9d6ae343 on Dec 2, 2022

DrahtBot locked this on Dec 27, 2022

test: use MiniWallet for rpc_scantxoutset.py #23866

Conflicts