test: use MiniWallet for rpc

ayush933 commented at 5:10 pm on March 16, 2022: contributor

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

in test/functional/rpc_createmultisig.py:24 in aba008256b outdated

17@@ -18,15 +18,23 @@
18     assert_equal,
19 )
20 from test_framework.wallet_util import bytes_to_wif
21+from test_framework.wallet import (
22+    MiniWallet,
23+    address_to_scriptpubkey,
24+    getnewdestination

MarcoFalke commented at 5:15 pm on March 16, 2022:

nit: add trailing comma

0    getnewdestination,

ayush933 commented at 6:11 pm on March 16, 2022:

done

fanquake added the label Tests on Mar 16, 2022

in test/functional/rpc_createmultisig.py:54 in aba008256b outdated

51     def run_test(self):
52         node0, node1, node2 = self.nodes
53+        self.wallet=MiniWallet(test_node=node0)
54 
55-        self.check_addmultisigaddress_errors()
56+        if self.is_wallet_compiled():

MarcoFalke commented at 5:24 pm on March 16, 2022:

I think descriptor wallets don’t support addmultisigaddress, so shouldn’t this check for bdb?

ayush933 commented at 6:14 pm on March 16, 2022:

Changed to check for bdb instead.

in test/functional/rpc_createmultisig.py:37 in aba008256b outdated

33+            self.requires_wallet = True
34 
35     def skip_test_if_missing_module(self):
36-        self.skip_if_no_wallet()
37+        if self.is_wallet_compiled() and not self.options.descriptors:
38+            self.skip_if_no_bdb()

MarcoFalke commented at 5:24 pm on March 16, 2022:

Is this check still needed if you adjust the checks below to check for bdb instead?

ayush933 commented at 6:13 pm on March 16, 2022:

Changed the checks to check for bdb instead. Removed this check.

MarcoFalke approved

MarcoFalke commented at 5:25 pm on March 16, 2022: member

Thanks, Concept ACK. Some questions.

in test/functional/rpc_createmultisig.py:32 in 33560b1d2a outdated

31         self.supports_cli = False
32-
33-    def skip_test_if_missing_module(self):
34-        self.skip_if_no_wallet()
35+        if self.is_bdb_compiled():
36+            self.requires_wallet = True

MarcoFalke commented at 6:36 pm on March 16, 2022:

Why is this needed? IIRC this will only import the coinbase keys, but they aren’t needed?

ayush933 commented at 7:02 pm on March 16, 2022:

This is actually needed to create default wallets. Without this check_addmultisigaddress_errors() fails.

in test/functional/rpc_createmultisig.py:175 in 33560b1d2a outdated

181+            assert maddw == madd
182+            assert mredeemw == mredeem
183+            wmulti.unloadwallet()
184+
185+        if self.output_type == "bech32":
186+            spk = bytes.fromhex(node0.validateaddress(madd)["scriptPubKey"])

MarcoFalke commented at 6:47 pm on March 16, 2022:

I think you can remove the if-else and just unconditionally use validateaddress for simplicity.

in test/functional/rpc_createmultisig.py:135 in 33560b1d2a outdated

138-        height = node0.getblockchaininfo()["blocks"]
139-        assert 150 < height < 350
140-        total = 149 * 50 + (height - 149 - 100) * 25
141-        assert bal1 == 0
142-        assert bal2 == self.moved
143-        assert bal0 + bal1 + bal2 == total

MarcoFalke commented at 6:48 pm on March 16, 2022:

Is there a reason why this check fails? IIUC the multisig addresses are still added to the bdb wallets as watch-only, so the watchonly balance should still match up (considering also the MiniWallet balance)

ayush933 commented at 7:11 pm on March 16, 2022:

Sorry but I have been stuck here for quite some time and not able to figure out why this fails.

MarcoFalke commented at 7:17 pm on March 16, 2022:

Ok, I can take a look later.

MarcoFalke commented at 2:27 pm on March 17, 2022:

This works for me locally:

 0diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py
 1index 4cf49fbcd3..9106fb1366 100755
 2--- a/test/functional/rpc_createmultisig.py
 3+++ b/test/functional/rpc_createmultisig.py
 4@@ -39,7 +39,10 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
 5             k.generate()
 6             self.pub.append(k.get_pubkey().get_bytes().hex())
 7             self.priv.append(bytes_to_wif(k.get_bytes(), k.is_compressed))
 8-        self.final = getnewdestination()[2]
 9+        if self.is_bdb_compiled():
10+         self.final = node2.getnewaddress()
11+        else:
12+         self.final = getnewdestination()[2]
13 
14     def run_test(self):
15         node0, node1, node2 = self.nodes
16@@ -51,11 +54,13 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
17         self.log.info('Generating blocks ...')
18         self.generate(self.wallet, 149)
19 
20+        self.moved=0
21         for self.nkeys in [3, 5]:
22             for self.nsigs in [2, 3]:
23                 for self.output_type in ["bech32", "p2sh-segwit", "legacy"]:
24                     self.get_keys()
25                     self.do_multisig()
26+        self.checkbalances()
27 
28 
29         # Test mixed compressed and uncompressed pubkeys
30@@ -125,6 +130,21 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
31         pubs = [self.nodes[1].getaddressinfo(addr)["pubkey"] for addr in addresses]
32         assert_raises_rpc_error(-5, "Bech32m multisig addresses cannot be created with legacy wallets", self.nodes[0].addmultisigaddress, 2, pubs, "", "bech32m")
33 
34+    def checkbalances(self):
35+        node0, node1, node2 = self.nodes
36+        self.generate(node0, 100)#COINBASE_MATURITY)
37+
38+        bal0 = node0.getbalance()
39+        bal1 = node1.getbalance()
40+        bal2 = node2.getbalance()
41+        balw=self.wallet.get_balance()
42+
43+        height = node0.getblockchaininfo()["blocks"]
44+        assert 150 < height < 350
45+        total = 149 * 50 + (height - 149 - 100) * 25
46+        assert bal1 == 0
47+        assert bal2 == self.moved
48+        assert_equal (bal0 + bal1 + bal2 + balw ,total) 
49 
50     def do_multisig(self):
51         node0, node1, node2 = self.nodes
52@@ -213,6 +233,7 @@ class RpcCreateMultiSigTest(BitcoinTestFramework):
53         rawtx2 = node2.signrawtransactionwithkey(rawtx, self.priv[0:self.nsigs - 1], prevtxs)
54         rawtx3 = node2.signrawtransactionwithkey(rawtx2["hex"], [self.priv[-1]], prevtxs)
55 
56+        self.moved += outval
57         tx = node0.sendrawtransaction(rawtx3["hex"], 0)
58         blk = self.generate(node0, 1)[0]
59         assert tx in node0.getblock(blk)["tx"]
60diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
61index dd41a740ae..979858e2ee 100644
62--- a/test/functional/test_framework/wallet.py
63+++ b/test/functional/test_framework/wallet.py
64@@ -93,6 +93,9 @@ class MiniWallet:
65             self._address, self._internal_key = create_deterministic_address_bcrt1_p2tr_op_true()
66             self._scriptPubKey = bytes.fromhex(self._test_node.validateaddress(self._address)['scriptPubKey'])
67 
68+    def get_balance(self):
69+        return sum(u['value'] for u in self._utxos)
70+
71     def rescan_utxos(self):
72         """Drop all utxos and rescan the utxo set"""
73         self._utxos = []

ayush933 commented at 9:19 pm on March 17, 2022:

Made the necessary changes and squashed commits.

brunoerg commented at 12:31 pm on March 17, 2022: member

Concept ACK.

You should squash the commits, see: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

ayush933 force-pushed on Mar 17, 2022

MarcoFalke approved

theStack commented at 6:51 pm on March 19, 2022: member

Concept ACK

danielabrozzoni approved

danielabrozzoni commented at 8:50 pm on March 19, 2022: contributor

tACK 4ea7c1b410bc6626a14c2cac8841910bc51e2422 - the code looks good, the test works as intended even when the wallet is disabled.

test: use MiniWallet for rpc_createmultisig.py

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

2726b60a3a

ayush933 force-pushed on Mar 22, 2022

in test/functional/test_runner.py:228 in 2726b60a3a

224@@ -225,8 +225,7 @@
225     'feature_rbf.py --descriptors',
226     'mempool_packages.py',
227     'mempool_package_onemore.py',
228-    'rpc_createmultisig.py --legacy-wallet',

michaelfolkson commented at 12:21 pm on March 22, 2022:

Unless I’m mistaken you do want both tests run (–legacy-wallet and –descriptors). We are still supporting the legacy wallet and hence we should still test it. I think you’ve taken it out because of the discussion in #24635 but the legacy wallet is not yet deprecated.

ayush933 commented at 1:51 pm on March 22, 2022:

In this particular test, the wallet, when compiled is used for addmultisigaddress which descriptor wallet doesn’t support, so I don’t think there is any difference in running with or without the flag. The legacy wallet when compiled is actually used even without the flag so there is no need to run the test twice.

michaelfolkson commented at 12:23 pm on March 22, 2022: contributor

Concept ACK

This is one of the tests from #20078 that could use MiniWallet instead of being skipped when the Core wallet is disabled.

ayush933 requested review from MarcoFalke on Mar 23, 2022

MarcoFalke approved

fanquake commented at 9:41 am on March 23, 2022: member

@danielabrozzoni / @theStack want to re-ACK?

michaelfolkson commented at 9:45 am on March 23, 2022: contributor

I’ll test this and ACK it in the next couple of days.

danielabrozzoni commented at 10:46 am on March 23, 2022: contributor

re-ACK 2726b60a3ac098b44f2970bed21147b70e12a1c2

fanquake merged this on Mar 24, 2022

fanquake closed this on Mar 24, 2022

sidhujag referenced this in commit 416803d3a8 on Apr 2, 2022

DrahtBot locked this on Mar 24, 2023

test: use MiniWallet for rpc_createmultisig.py #24587