Merge #20186: wallet: Make -wallet setting not create wallets

01476a88a6095fd3af71cb9bf1eadef920a1197b wallet: Make -wallet setting not create wallets (Russell Yanofsky)

Pull request description:

  This changes `-wallet` setting to only load existing wallets, not create new ones.

  - Fixes settings.json corner cases reported by sjors & promag: https://github.com/bitcoin-core/gui/issues/95, https://github.com/bitcoin/bitcoin/pull/19754#issuecomment-685858578, https://github.com/bitcoin/bitcoin/pull/19754#issuecomment-685858578

  - Prevents accidental creation of wallets reported most recently by jb55 http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-14.html#l-355

  - Simplifies behavior after #15454. #15454 took the big step of disabling creation of the default wallet. This PR extends it to avoid creating other wallets as well. With this change, new wallets just aren't created on startup, instead of sometimes being created, sometimes not. #15454 release notes are updated here and are simpler.

  This change should be targeted for 0.21.0. It's a bug fix and simplifies behavior of the #15937 / #19754 / #15454 features added in 0.21.0.

  ---

  This PR is implementing the simplest, most basic alternative listed in https://github.com/bitcoin-core/gui/issues/95#issuecomment-694236940. Other improvements mentioned there can build on top of this.

ACKs for top commit:
  achow101:
    ACK 01476a88a6095fd3af71cb9bf1eadef920a1197b
  hebasto:
    re-ACK 01476a88a6095fd3af71cb9bf1eadef920a1197b
  MarcoFalke:
    review ACK 01476a88a6095fd3af71cb9bf1eadef920a1197b 🏂

Tree-SHA512: 0d50f4e5dfbd04a2efd9fd66c02085a0ed705807bdec1cf5770d0ae8cb6af07080fb81306349937bf66acdb713d03fb35636f6442b650d0820e66cbae09c2f87

7 files changed, 53 insertions, 29 deletions

diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index 60533e196f..3e28dae4b3 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -179,19 +179,15 @@ class ConfArgsTest(BitcoinTestFramework):
 
         # Create the directory and ensure the config file now works
         os.mkdir(new_data_dir)
-        self.start_node(0, ['-conf='+conf_file, '-wallet=w1'])
+        self.start_node(0, ['-conf='+conf_file])
         self.stop_node(0)
         assert os.path.exists(os.path.join(new_data_dir, self.chain, 'blocks'))
-        if self.is_wallet_compiled():
-            assert os.path.exists(os.path.join(new_data_dir, self.chain, 'wallets', 'w1'))
 
         # Ensure command line argument overrides datadir in conf
         os.mkdir(new_data_dir_2)
         self.nodes[0].datadir = new_data_dir_2
-        self.start_node(0, ['-datadir='+new_data_dir_2, '-conf='+conf_file, '-wallet=w2'])
+        self.start_node(0, ['-datadir='+new_data_dir_2, '-conf='+conf_file])
         assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))
-        if self.is_wallet_compiled():
-            assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'wallets', 'w2'))
 
 
 if __name__ == '__main__':
diff --git a/test/functional/rpc_scantxoutset.py b/test/functional/rpc_scantxoutset.py
index 861b394e70..070f59d314 100755
--- a/test/functional/rpc_scantxoutset.py
+++ b/test/functional/rpc_scantxoutset.py
@@ -55,7 +55,8 @@ class ScantxoutsetTest(BitcoinTestFramework):
         self.log.info("Stop node, remove wallet, mine again some blocks...")
         self.stop_node(0)
         shutil.rmtree(os.path.join(self.nodes[0].datadir, self.chain, 'wallets'))
-        self.start_node(0)
+        self.start_node(0, ['-nowallet'])
+        self.import_deterministic_coinbase_privkeys()
         self.nodes[0].generate(110)
 
         scan = self.nodes[0].scantxoutset("start", [])
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 20f608a9cf..872f612a4d 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -111,6 +111,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         # are not imported.
         self.wallet_names = None
         self.set_test_params()
+        assert self.wallet_names is None or len(self.wallet_names) <= self.num_nodes
         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
@@ -390,9 +391,13 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
                 assert_equal(chain_info["initialblockdownload"], False)
 
     def import_deterministic_coinbase_privkeys(self):
-        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):
+        for i in range(self.num_nodes):
+            self.init_wallet(i)
+
+    def init_wallet(self, i):
+        wallet_name = self.default_wallet_name if self.wallet_names is None else self.wallet_names[i] if i < len(self.wallet_names) else False
+        if wallet_name is not False:
+            n = self.nodes[i]
             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')
diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py
index 958341c691..c7c056a8dc 100755
--- a/test/functional/tool_wallet.py
+++ b/test/functional/tool_wallet.py
@@ -218,7 +218,8 @@ class ToolWalletTest(BitcoinTestFramework):
     def test_salvage(self):
         # TODO: Check salvage actually salvages and doesn't break things. https://github.com/bitcoin/bitcoin/issues/7463
         self.log.info('Check salvage')
-        self.start_node(0, ['-wallet=salvage'])
+        self.start_node(0)
+        self.nodes[0].createwallet("salvage")
         self.stop_node(0)
 
         self.assert_tool_output('', '-wallet=salvage', 'salvage')
diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py
index c7bd2ea02b..f34c1345e0 100755
--- a/test/functional/wallet_backup.py
+++ b/test/functional/wallet_backup.py
@@ -91,10 +91,10 @@ class WalletBackupTest(BitcoinTestFramework):
         self.sync_blocks()
 
     # As above, this mirrors the original bash test.
-    def start_three(self):
-        self.start_node(0)
-        self.start_node(1)
-        self.start_node(2)
+    def start_three(self, args=()):
+        self.start_node(0, self.extra_args[0] + list(args))
+        self.start_node(1, self.extra_args[1] + list(args))
+        self.start_node(2, self.extra_args[2] + list(args))
         self.connect_nodes(0, 3)
         self.connect_nodes(1, 3)
         self.connect_nodes(2, 3)
@@ -110,6 +110,11 @@ class WalletBackupTest(BitcoinTestFramework):
         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 init_three(self):
+        self.init_wallet(0)
+        self.init_wallet(1)
+        self.init_wallet(2)
+
     def run_test(self):
         self.log.info("Generating initial blockchain")
         self.nodes[0].generate(1)
@@ -193,7 +198,8 @@ class WalletBackupTest(BitcoinTestFramework):
             shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'blocks'))
             shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'chainstate'))
 
-            self.start_three()
+            self.start_three(["-nowallet"])
+            self.init_three()
 
             assert_equal(self.nodes[0].getbalance(), 0)
             assert_equal(self.nodes[1].getbalance(), 0)
diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py
index 09581d864b..eb54da99f5 100755
--- a/test/functional/wallet_dump.py
+++ b/test/functional/wallet_dump.py
@@ -95,7 +95,7 @@ def read_dump(file_name, addrs, script_addrs, hd_master_addr_old):
 class WalletDumpTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
-        self.extra_args = [["-keypool=90", "-addresstype=legacy", "-wallet=dump"]]
+        self.extra_args = [["-keypool=90", "-addresstype=legacy"]]
         self.rpc_timeout = 120
 
     def skip_test_if_missing_module(self):
@@ -106,6 +106,8 @@ class WalletDumpTest(BitcoinTestFramework):
         self.start_nodes()
 
     def run_test(self):
+        self.nodes[0].createwallet("dump")
+
         wallet_unenc_dump = os.path.join(self.nodes[0].datadir, "wallet.unencrypted.dump")
         wallet_enc_dump = os.path.join(self.nodes[0].datadir, "wallet.encrypted.dump")
 
@@ -190,7 +192,8 @@ class WalletDumpTest(BitcoinTestFramework):
         assert_raises_rpc_error(-8, "already exists", lambda: self.nodes[0].dumpwallet(wallet_enc_dump))
 
         # Restart node with new wallet, and test importwallet
-        self.restart_node(0, ['-wallet=w2'])
+        self.restart_node(0)
+        self.nodes[0].createwallet("w2")
 
         # Make sure the address is not IsMine before import
         result = self.nodes[0].getaddressinfo(multisig_addr)
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
index 61791a756c..63af9e8720 100755
--- a/test/functional/wallet_multiwallet.py
+++ b/test/functional/wallet_multiwallet.py
@@ -41,6 +41,7 @@ class MultiWalletTest(BitcoinTestFramework):
         self.setup_clean_chain = True
         self.num_nodes = 2
         self.rpc_timeout = 120
+        self.extra_args = [["-nowallet"], []]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -80,7 +81,9 @@ class MultiWalletTest(BitcoinTestFramework):
         # rename wallet.dat to make sure plain wallet file paths (as opposed to
         # directory paths) can be loaded
         # create another dummy wallet for use in testing backups later
-        self.start_node(0, ["-nowallet", "-wallet=empty", "-wallet=plain"])
+        self.start_node(0)
+        node.createwallet("empty", descriptors=False)
+        node.createwallet("plain", descriptors=False)
         node.createwallet("created")
         self.stop_nodes()
         empty_wallet = os.path.join(self.options.tmpdir, 'empty.dat')
@@ -101,21 +104,23 @@ class MultiWalletTest(BitcoinTestFramework):
         #   w8         - to verify existing wallet file is loaded correctly
         #   ''         - to verify default wallet file is created correctly
         wallet_names = ['w1', 'w2', 'w3', 'w', 'sub/w5', os.path.join(self.options.tmpdir, 'extern/w6'), 'w7_symlink', 'w8', self.default_wallet_name]
-        extra_args = ['-nowallet'] + ['-wallet={}'.format(n) for n in wallet_names]
-        self.start_node(0, extra_args)
+        self.start_node(0)
+        for wallet_name in wallet_names[:-2]:
+            self.nodes[0].createwallet(wallet_name, descriptors=False)
+        for wallet_name in wallet_names[-2:]:
+            self.nodes[0].loadwallet(wallet_name)
         assert_equal(sorted(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), [self.default_wallet_name, os.path.join('sub', 'w5'), 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'])
 
         assert_equal(set(node.listwallets()), set(wallet_names))
 
+        # should raise rpc error if wallet path can't be created
+        assert_raises_rpc_error(-1, "boost::filesystem::create_directory:", self.nodes[0].createwallet, "w8/bad", descriptors=False)
+
         # check that all requested wallets were created
         self.stop_node(0)
         for wallet_name in wallet_names:
             assert_equal(os.path.isfile(wallet_file(wallet_name)), True)
 
-        # should not initialize if wallet path can't be created
-        exp_stderr = "boost::filesystem::create_directory:"
-        self.nodes[0].assert_start_raises_init_error(['-wallet=w8/bad'], exp_stderr, match=ErrorMatch.PARTIAL_REGEX)
-
         self.nodes[0].assert_start_raises_init_error(['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist')
         self.nodes[0].assert_start_raises_init_error(['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=data_dir())
         self.nodes[0].assert_start_raises_init_error(['-walletdir=debug.log'], 'Error: Specified -walletdir "debug.log" is not a directory', cwd=data_dir())
@@ -142,14 +147,18 @@ class MultiWalletTest(BitcoinTestFramework):
         # if wallets/ doesn't exist, datadir should be the default wallet dir
         wallet_dir2 = data_dir('walletdir')
         os.rename(wallet_dir(), wallet_dir2)
-        self.start_node(0, ['-nowallet', '-wallet=w4', '-wallet=w5'])
+        self.start_node(0)
+        self.nodes[0].createwallet("w4")
+        self.nodes[0].createwallet("w5")
         assert_equal(set(node.listwallets()), {"w4", "w5"})
         w5 = wallet("w5")
         node.generatetoaddress(nblocks=1, address=w5.getnewaddress())
 
         # now if wallets/ exists again, but the rootdir is specified as the walletdir, w4 and w5 should still be loaded
         os.rename(wallet_dir2, wallet_dir())
-        self.restart_node(0, ['-nowallet', '-wallet=w4', '-wallet=w5', '-walletdir=' + data_dir()])
+        self.restart_node(0, ['-nowallet', '-walletdir=' + data_dir()])
+        self.nodes[0].loadwallet("w4")
+        self.nodes[0].loadwallet("w5")
         assert_equal(set(node.listwallets()), {"w4", "w5"})
         w5 = wallet("w5")
         w5_info = w5.getwalletinfo()
@@ -157,11 +166,14 @@ class MultiWalletTest(BitcoinTestFramework):
 
         competing_wallet_dir = os.path.join(self.options.tmpdir, 'competing_walletdir')
         os.mkdir(competing_wallet_dir)
-        self.restart_node(0, ['-walletdir=' + competing_wallet_dir])
+        self.restart_node(0, ['-nowallet', '-walletdir=' + competing_wallet_dir])
+        self.nodes[0].createwallet(self.default_wallet_name, descriptors=False)
         exp_stderr = r"Error: Error initializing wallet database environment \"\S+competing_walletdir\S*\"!"
         self.nodes[1].assert_start_raises_init_error(['-walletdir=' + competing_wallet_dir], exp_stderr, match=ErrorMatch.PARTIAL_REGEX)
 
-        self.restart_node(0, extra_args)
+        self.restart_node(0)
+        for wallet_name in wallet_names:
+            self.nodes[0].loadwallet(wallet_name)
 
         assert_equal(sorted(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), [self.default_wallet_name, os.path.join('sub', 'w5'), 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8', 'w8_copy'])