Merge #20034: test: Get rid of default wallet hacks

c1585bc test: Get rid of default wallet hacks (Russell Yanofsky)
ed3acda test, refactor: add default_wallet_name and wallet_data_filename variables (Russell Yanofsky)

Pull request description:

  Changes:

  - Get rid of setup_nodes (`-wallet`, `-nowallet`, `-disablewallet`) argument rewriting
  - Get rid of hardcoded wallet `""` names and `-wallet=""` args

  Motivation:

  - Simplify test framework behavior so it's easier to write new tests without having arguments mangled by the framework
  - Make tests more readable, replacing unexplained `""` string literals with `default_wallet_name` references
  - Make it trivial to update default wallet name and wallet data filename for sqlite #19077 testing
  - Stop relying on `-wallet` arguments to create wallets, so it is easy to change `-wallet` option in the future to only load existing wallets not create new ones (to avoid accidental wallet creation, and encourage use of wallet encryption and descriptor features)

ACKs for top commit:
  MarcoFalke:
    ACK c1585bc, only effective change is adding documentation 🎵

Tree-SHA512: f62dec7cbdacb5f330aa0e1eec89ab4d065540d91495bbedcb375eda1c080b45ce9edb310ce253c44c4839f1b4cc2c7df9816c58402d5d43f94a437e301ea8bc

Author: MarcoFalke

test/functional/feature_backwards_compatibility.py

@@ -36,13 +36,14 @@ def set_test_params(self):
         self.num_nodes = 6
         # Add new version after each release:
         self.extra_args = [
-            ["-addresstype=bech32", "-wallet="], # Pre-release: use to mine blocks
+            ["-addresstype=bech32"], # Pre-release: use to mine blocks
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32"], # Pre-release: use to receive coins, swap wallets, etc
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32"], # v0.19.1
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32"], # v0.18.1
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32"], # v0.17.2
             ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-wallet=wallet.dat"], # v0.16.3
         ]
+        self.wallet_names = [self.default_wallet_name]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -59,6 +60,7 @@ def setup_nodes(self):
         ])
 
         self.start_nodes()
+        self.import_deterministic_coinbase_privkeys()
 
     def run_test(self):
         self.nodes[0].generatetoaddress(101, self.nodes[0].getnewaddress())

test/functional/feature_config_args.py

@@ -14,6 +14,7 @@ def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
         self.supports_cli = False
+        self.wallet_names = []
 
     def test_config_file_parser(self):
         # Assume node is stopped

test/functional/feature_dbcrash.py

@@ -56,7 +56,7 @@ def set_test_params(self):
         # Set -maxmempool=0 to turn off mempool memory sharing with dbcache
         # Set -rpcservertimeout=900 to reduce socket disconnects in this
         # long-running test
-        self.base_args = ["-limitdescendantsize=0", "-maxmempool=0", "-rpcservertimeout=900", "-dbbatchsize=200000", "-wallet="]
+        self.base_args = ["-limitdescendantsize=0", "-maxmempool=0", "-rpcservertimeout=900", "-dbbatchsize=200000"]
 
         # Set different crash ratios and cache sizes.  Note that not all of
         # -dbcache goes to the in-memory coins cache.
@@ -66,7 +66,7 @@ def set_test_params(self):
 
         # Node3 is a normal node with default args, except will mine full blocks
         # and non-standard txs (e.g. txs with "dust" outputs)
-        self.node3_args = ["-blockmaxweight=4000000", "-acceptnonstdtxn", "-wallet="]
+        self.node3_args = ["-blockmaxweight=4000000", "-acceptnonstdtxn"]
         self.extra_args = [self.node0_args, self.node1_args, self.node2_args, self.node3_args]
 
     def skip_test_if_missing_module(self):

test/functional/feature_fee_estimation.py

@@ -145,9 +145,9 @@ def set_test_params(self):
         # mine non-standard txs (e.g. txs with "dust" outputs)
         # Force fSendTrickle to true (via whitelist.noban)
         self.extra_args = [
-            ["-acceptnonstdtxn", "[email protected]", "-wallet="],
-            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=68000", "-wallet="],
-            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=32000", "-wallet="],
+            ["-acceptnonstdtxn", "[email protected]"],
+            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=68000"],
+            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=32000"],
         ]
 
     def skip_test_if_missing_module(self):

test/functional/feature_filelock.py

@@ -15,7 +15,7 @@ def set_test_params(self):
 
     def setup_network(self):
         self.add_nodes(self.num_nodes, extra_args=None)
-        self.nodes[0].start(['-wallet='])
+        self.nodes[0].start()
         self.nodes[0].wait_for_rpc_connection()
 
     def run_test(self):
@@ -27,10 +27,11 @@ def run_test(self):
         self.nodes[1].assert_start_raises_init_error(extra_args=['-datadir={}'.format(self.nodes[0].datadir), '-noserver'], expected_msg=expected_msg)
 
         if self.is_wallet_compiled():
+            self.nodes[0].createwallet(self.default_wallet_name)
             wallet_dir = os.path.join(datadir, 'wallets')
             self.log.info("Check that we can't start a second bitcoind instance using the same wallet")
             expected_msg = "Error: Error initializing wallet database environment"
-            self.nodes[1].assert_start_raises_init_error(extra_args=['-walletdir={}'.format(wallet_dir), '-wallet=', '-noserver'], expected_msg=expected_msg, match=ErrorMatch.PARTIAL_REGEX)
+            self.nodes[1].assert_start_raises_init_error(extra_args=['-walletdir={}'.format(wallet_dir), '-wallet=' + self.default_wallet_name, '-noserver'], expected_msg=expected_msg, match=ErrorMatch.PARTIAL_REGEX)
 
 if __name__ == '__main__':
     FilelockTest().main()

test/functional/feature_notifications.py

@@ -45,8 +45,8 @@ def setup_network(self):
                             "-blocknotify=echo > {}".format(os.path.join(self.blocknotify_dir, '%s'))],
                            ["-blockversion=211",
                             "-rescan",
-                            "-wallet={}".format(self.wallet),
                             "-walletnotify=echo > {}".format(os.path.join(self.walletnotify_dir, notify_outputname('%w', '%s')))]]
+        self.wallet_names = [self.default_wallet_name, self.wallet]
         super().setup_network()
 
     def run_test(self):

test/functional/feature_pruning.py

@@ -81,16 +81,16 @@ def set_test_params(self):
 
         # Create nodes 0 and 1 to mine.
         # Create node 2 to test pruning.
-        self.full_node_default_args = ["-maxreceivebuffer=20000", "-checkblocks=5", "-wallet="]
+        self.full_node_default_args = ["-maxreceivebuffer=20000", "-checkblocks=5"]
         # Create nodes 3 and 4 to test manual pruning (they will be re-started with manual pruning later)
         # Create nodes 5 to test wallet in prune mode, but do not connect
         self.extra_args = [
             self.full_node_default_args,
             self.full_node_default_args,
-            ["-wallet=", "-maxreceivebuffer=20000", "-prune=550"],
-            ["-wallet=", "-maxreceivebuffer=20000"],
-            ["-wallet=", "-maxreceivebuffer=20000"],
-            ["-wallet=", "-prune=550"],
+            ["-maxreceivebuffer=20000", "-prune=550"],
+            ["-maxreceivebuffer=20000"],
+            ["-maxreceivebuffer=20000"],
+            ["-prune=550"],
         ]
         self.rpc_timeout = 120
 
@@ -112,8 +112,7 @@ def setup_network(self):
     def setup_nodes(self):
         self.add_nodes(self.num_nodes, self.extra_args)
         self.start_nodes()
-        for n in self.nodes:
-            n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase', rescan=False)
+        self.import_deterministic_coinbase_privkeys()
 
     def create_big_chain(self):
         # Start by creating some coinbases we can spend later

test/functional/feature_settings.py

@@ -17,6 +17,7 @@ class SettingsTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
+        self.wallet_names = []
 
     def run_test(self):
         node, = self.nodes

test/functional/interface_bitcoin_cli.py

@@ -95,7 +95,7 @@ def run_test(self):
             assert_equal(self.nodes[0].cli.getwalletinfo(), wallet_info)
 
             # Setup to test -getinfo, -generate, and -rpcwallet= with multiple wallets.
-            wallets = ['', 'Encrypted', 'secret']
+            wallets = [self.default_wallet_name, 'Encrypted', 'secret']
             amounts = [BALANCE + Decimal('9.999928'), Decimal(9), Decimal(31)]
             self.nodes[0].createwallet(wallet_name=wallets[1])
             self.nodes[0].createwallet(wallet_name=wallets[2])

test/functional/mempool_compatibility.py

@@ -21,6 +21,7 @@
 class MempoolCompatibilityTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
+        self.wallet_names = [None, self.default_wallet_name]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -31,7 +32,7 @@ def setup_network(self):
             150200, # oldest version supported by the test framework
             None,
         ])
-        self.start_nodes([[], ["-wallet="]])
+        self.start_nodes()
         self.import_deterministic_coinbase_privkeys()
 
     def run_test(self):

test/functional/rpc_deprecated.py

@@ -29,7 +29,7 @@ def run_test(self):
             self.nodes[0].generate(101)
             self.nodes[0].createwallet(wallet_name='nopriv', disable_private_keys=True)
             noprivs0 = self.nodes[0].get_wallet_rpc('nopriv')
-            w0 = self.nodes[0].get_wallet_rpc('')
+            w0 = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
             self.nodes[1].createwallet(wallet_name='nopriv', disable_private_keys=True)
             noprivs1 = self.nodes[1].get_wallet_rpc('nopriv')
 

test/functional/rpc_getdescriptorinfo.py

@@ -17,6 +17,7 @@ class DescriptorTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.extra_args = [["-disablewallet"]]
+        self.wallet_names = []
 
     def test_desc(self, desc, isrange, issolvable, hasprivatekeys):
         info = self.nodes[0].getdescriptorinfo(desc)

test/functional/test_framework/test_framework.py

@@ -102,8 +102,16 @@ def __init__(self):
         self.rpc_timeout = 60  # Wait for up to 60 seconds for the RPC server to respond
         self.supports_cli = True
         self.bind_to_localhost_only = True
-        self.set_test_params()
         self.parse_args()
+        self.default_wallet_name = ""
+        self.wallet_data_filename = "wallet.dat"
+        # Optional list of wallet names that can be set in set_test_params to
+        # create and import keys to. If unset, default is len(nodes) *
+        # [default_wallet_name]. If wallet names are None, wallet creation is
+        # skipped. If list is truncated, wallet creation is skipped and keys
+        # are not imported.
+        self.wallet_names = None
+        self.set_test_params()
         if self.options.timeout_factor == 0 :
             self.options.timeout_factor = 99999
         self.rpc_timeout = int(self.rpc_timeout * self.options.timeout_factor) # optionally, increase timeout by a factor
@@ -362,23 +370,12 @@ def setup_network(self):
     def setup_nodes(self):
         """Override this method to customize test node setup"""
         extra_args = [[]] * self.num_nodes
-        wallets = [[]] * self.num_nodes
         if hasattr(self, "extra_args"):
             extra_args = self.extra_args
-            wallets = [[x for x in eargs if x.startswith('-wallet=')] for eargs in extra_args]
-        extra_args = [x + ['-nowallet'] for x in extra_args]
         self.add_nodes(self.num_nodes, extra_args)
         self.start_nodes()
-        for i, n in enumerate(self.nodes):
-            n.extra_args.pop()
-            if '-wallet=0' in n.extra_args or '-nowallet' in n.extra_args or '-disablewallet' in n.extra_args or not self.is_wallet_compiled():
-                continue
-            if '-wallet=' not in wallets[i] and not any([x.startswith('-wallet=') for x in wallets[i]]):
-                wallets[i].append('-wallet=')
-            for w in wallets[i]:
-                wallet_name = w.split('=', 1)[1]
-                n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors)
-        self.import_deterministic_coinbase_privkeys()
+        if self.is_wallet_compiled():
+            self.import_deterministic_coinbase_privkeys()
         if not self.setup_clean_chain:
             for n in self.nodes:
                 assert_equal(n.getblockchaininfo()["blocks"], 199)
@@ -394,13 +391,11 @@ def setup_nodes(self):
                 assert_equal(chain_info["initialblockdownload"], False)
 
     def import_deterministic_coinbase_privkeys(self):
-        for n in self.nodes:
-            try:
-                n.getwalletinfo()
-            except JSONRPCException as e:
-                assert str(e).startswith('Method not found')
-                continue
-
+        wallet_names = [self.default_wallet_name] * len(self.nodes) if self.wallet_names is None else self.wallet_names
+        assert len(wallet_names) <= len(self.nodes)
+        for wallet_name, n in zip(wallet_names, self.nodes):
+            if wallet_name is not None:
+                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')
 
     def run_test(self):

test/functional/tool_wallet.py

@@ -73,7 +73,7 @@ def test_invalid_tool_commands_and_args(self):
         locked_dir = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets")
         self.assert_raises_tool_error(
             'Error initializing wallet database environment "{}"!'.format(locked_dir),
-            '-wallet=wallet.dat',
+            '-wallet=' + self.default_wallet_name,
             'info',
         )
         path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "nonexistent.dat")
@@ -104,7 +104,7 @@ def test_tool_wallet_info(self):
             Transactions: 0
             Address Book: 3
         ''')
-        self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
+        self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
         timestamp_after = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
         self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
@@ -143,7 +143,7 @@ def test_tool_wallet_info_after_transaction(self):
             Transactions: 1
             Address Book: 3
         ''')
-        self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
+        self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
         shasum_after = self.wallet_shasum()
         timestamp_after = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
@@ -181,7 +181,7 @@ def test_tool_wallet_create_on_existing_wallet(self):
 
     def test_getwalletinfo_on_different_wallet(self):
         self.log.info('Starting node with arg -wallet=foo')
-        self.start_node(0, ['-wallet=foo'])
+        self.start_node(0, ['-nowallet', '-wallet=foo'])
 
         self.log.info('Calling getwalletinfo on a different wallet ("foo"), testing output')
         shasum_before = self.wallet_shasum()
@@ -213,7 +213,7 @@ def test_salvage(self):
         self.assert_tool_output('', '-wallet=salvage', 'salvage')
 
     def run_test(self):
-        self.wallet_path = os.path.join(self.nodes[0].datadir, self.chain, 'wallets', 'wallet.dat')
+        self.wallet_path = os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)
         self.test_invalid_tool_commands_and_args()
         # Warning: The following tests are order-dependent.
         self.test_tool_wallet_info()

test/functional/wallet_backup.py

@@ -50,10 +50,10 @@ def set_test_params(self):
         # nodes 1, 2,3 are spenders, let's give them a keypool=100
         # whitelist all peers to speed up tx relay / mempool sync
         self.extra_args = [
-            ["[email protected]", "-keypool=100", "-wallet="],
-            ["[email protected]", "-keypool=100", "-wallet="],
-            ["[email protected]", "-keypool=100", "-wallet="],
-            ["[email protected]", "-wallet="],
+            ["[email protected]", "-keypool=100"],
+            ["[email protected]", "-keypool=100"],
+            ["[email protected]", "-keypool=100"],
+            ["[email protected]"],
         ]
         self.rpc_timeout = 120
 
@@ -107,9 +107,9 @@ def stop_three(self):
         self.stop_node(2)
 
     def erase_three(self):
-        os.remove(os.path.join(self.nodes[0].datadir, self.chain, 'wallets', 'wallet.dat'))
-        os.remove(os.path.join(self.nodes[1].datadir, self.chain, 'wallets', 'wallet.dat'))
-        os.remove(os.path.join(self.nodes[2].datadir, self.chain, 'wallets', 'wallet.dat'))
+        os.remove(os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
+        os.remove(os.path.join(self.nodes[1].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
+        os.remove(os.path.join(self.nodes[2].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
 
     def run_test(self):
         self.log.info("Generating initial blockchain")
@@ -171,9 +171,9 @@ def run_test(self):
         shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'chainstate'))
 
         # Restore wallets from backup
-        shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[0].datadir, self.chain, 'wallets', 'wallet.dat'))
-        shutil.copyfile(os.path.join(self.nodes[1].datadir, 'wallet.bak'), os.path.join(self.nodes[1].datadir, self.chain, 'wallets', 'wallet.dat'))
-        shutil.copyfile(os.path.join(self.nodes[2].datadir, 'wallet.bak'), os.path.join(self.nodes[2].datadir, self.chain, 'wallets', 'wallet.dat'))
+        shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
+        shutil.copyfile(os.path.join(self.nodes[1].datadir, 'wallet.bak'), os.path.join(self.nodes[1].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
+        shutil.copyfile(os.path.join(self.nodes[2].datadir, 'wallet.bak'), os.path.join(self.nodes[2].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
 
         self.log.info("Re-starting nodes")
         self.start_three()
@@ -209,9 +209,9 @@ def run_test(self):
 
         # Backup to source wallet file must fail
         sourcePaths = [
-            os.path.join(self.nodes[0].datadir, self.chain, 'wallets', 'wallet.dat'),
-            os.path.join(self.nodes[0].datadir, self.chain, '.', 'wallets', 'wallet.dat'),
-            os.path.join(self.nodes[0].datadir, self.chain, 'wallets', ''),
+            os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename),
+            os.path.join(self.nodes[0].datadir, self.chain, '.', 'wallets', self.default_wallet_name, self.wallet_data_filename),
+            os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name),
             os.path.join(self.nodes[0].datadir, self.chain, 'wallets')]
 
         for sourcePath in sourcePaths:

test/functional/wallet_balance.py

@@ -215,10 +215,10 @@ def test_balances(*, fee_node_1=0):
         # dynamically loading the wallet.
         before = self.nodes[1].getbalances()['mine']['untrusted_pending']
         dst = self.nodes[1].getnewaddress()
-        self.nodes[1].unloadwallet('')
+        self.nodes[1].unloadwallet(self.default_wallet_name)
         self.nodes[0].sendtoaddress(dst, 0.1)
         self.sync_all()
-        self.nodes[1].loadwallet('')
+        self.nodes[1].loadwallet(self.default_wallet_name)
         after = self.nodes[1].getbalances()['mine']['untrusted_pending']
         assert_equal(before + Decimal('0.1'), after)
 

test/functional/wallet_descriptor.py

@@ -24,7 +24,7 @@ def run_test(self):
         # Make a descriptor wallet
         self.log.info("Making a descriptor wallet")
         self.nodes[0].createwallet(wallet_name="desc1", descriptors=True)
-        self.nodes[0].unloadwallet("")
+        self.nodes[0].unloadwallet(self.default_wallet_name)
 
         # A descriptor wallet should have 100 addresses * 3 types = 300 keys
         self.log.info("Checking wallet info")

test/functional/wallet_disable.py

@@ -16,6 +16,7 @@ def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
         self.extra_args = [["-disablewallet"]]
+        self.wallet_names = []
 
     def run_test (self):
         # Make sure wallet is really disabled