wallet: Make -wallet setting not create wallets

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

- Fixes settings.json corner cases reported by sjors & promag:
  bitcoin-core/gui#95,
  bitcoin/bitcoin#19754 (comment),
  bitcoin/bitcoin#19754 (comment)

- 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.

It is a bug fix and simplifies behavior of the [[bitcoin/bitcoin#15937 | core#15937]] / [[bitcoin/bitcoin#19754 | core#19754]] / [[bitcoin/bitcoin#15454 | core#15454]] features

This is a backport of [[bitcoin/bitcoin#20186 | core#20186]]

Depends on D15143 and D14142

Test Plan:
`ninja all check-all`

With bitcoin-qt, create and load a wallet, close the program. In the file explorer rename the wallet directory. Restart bitcoin-qt and check that now the old wallet name is not recreated (in addition to the wallet with the new name) but an error message warns about it not being found. The new wallet name is now the only wallet avaialble.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D15144

doc/release-notes.md

@@ -4,4 +4,12 @@ Bitcoin ABC version 0.28.8 is now available from:
 
   <https://download.bitcoinabc.org/0.28.8/>
 
-This is a maintenance release with no user-visible change.
+This release includes the following features and fixes:
+ - Bitcoin ABC will no longer automatically create new wallets on startup. It will
+   load existing wallets specified by `-wallet` options on the command line or in
+   `bitcoin.conf` or `settings.json` files. And by default it will also load a
+   top-level unnamed ("") wallet. However, if specified wallets don't exist,
+   Bitcoin ABC will now just log warnings instead of creating new wallets with
+   new keys and addresses like previous releases did.
+   New wallets can be created through the GUI , through the `bitcoin-cli createwallet`
+   or `bitcoin-wallet create` commands, or the `createwallet` RPC.

src/wallet/init.cpp

@@ -109,12 +109,12 @@ void WalletInit::AddWalletOptions(ArgsManager &argsman) const {
         ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     argsman.AddArg(
         "-wallet=<path>",
-        "Specify wallet database path. Can be specified multiple "
-        "times to load multiple wallets. Path is interpreted relative "
-        "to <walletdir> if it is not absolute, and will be created if "
-        "it does not exist (as a directory containing a wallet.dat "
-        "file and log files). For backwards compatibility this will "
-        "also accept names of existing data files in <walletdir>.)",
+        "Specify wallet path to load at startup. Can be used multiple times to "
+        "load multiple wallets. Path is to a directory containing wallet data "
+        "and log files. If the path is not absolute, it is interpreted "
+        "relative to <walletdir>. This only loads existing wallets and does "
+        "not create new ones. For backwards compatibility this also accepts "
+        "names of existing top-level data files in <walletdir>.",
         ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY,
         OptionsCategory::WALLET);
     argsman.AddArg(

src/wallet/load.cpp

@@ -84,11 +84,18 @@ bool VerifyWallets(interfaces::Chain &chain) {
 
         DatabaseOptions options;
         DatabaseStatus status;
+        options.require_existing = true;
         options.verify = true;
         bilingual_str error_string;
         if (!MakeWalletDatabase(wallet_file, options, status, error_string)) {
-            chain.initError(error_string);
-            return false;
+            if (status == DatabaseStatus::FAILED_NOT_FOUND) {
+                chain.initWarning(Untranslated(
+                    strprintf("Skipping -wallet path that doesn't exist. %s\n",
+                              error_string.original)));
+            } else {
+                chain.initError(error_string);
+                return false;
+            }
         }
     }
 
@@ -104,12 +111,16 @@ bool LoadWallets(interfaces::Chain &chain) {
             }
             DatabaseOptions options;
             DatabaseStatus status;
+            options.require_existing = true;
             // No need to verify, assuming verified earlier in VerifyWallets()
             options.verify = false;
             bilingual_str error;
             std::vector<bilingual_str> warnings;
             std::unique_ptr<WalletDatabase> database =
                 MakeWalletDatabase(name, options, status, error);
+            if (!database && status == DatabaseStatus::FAILED_NOT_FOUND) {
+                continue;
+            }
             std::shared_ptr<CWallet> pwallet =
                 database
                     ? CWallet::Create(chain, name, std::move(database),

test/functional/feature_config_args.py

@@ -356,25 +356,15 @@ def run_test(self):
 
         # Create the directory and ensure the config file now works
         os.mkdir(new_data_dir)
-        self.start_node(0, [f"-conf={conf_file}", "-wallet=w1"])
+        self.start_node(0, [f"-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, [f"-datadir={new_data_dir_2}", f"-conf={conf_file}", "-wallet=w2"]
-        )
+        self.start_node(0, [f"-datadir={new_data_dir_2}", f"-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__":

test/functional/rpc_signrawtransaction.py

@@ -299,6 +299,8 @@ def multiwallet_signing_test(self):
         outputs = {"mpLQjfK79b7CCV4VMJWEWAj5Mpx8Up5zxB": 100000}
 
         multiwallet_node = self.nodes[0]
+        multiwallet_node.createwallet("w1")
+        multiwallet_node.createwallet("w2")
 
         rawTx = multiwallet_node.createrawtransaction(inputs, outputs)
 
@@ -337,10 +339,6 @@ def run_test(self):
         self.test_sighashes()
         self.test_with_lock_outputs()
         self.test_fully_signed_tx()
-
-        # The multiwalet require the node to use different flags, so we run it
-        # last.
-        self.restart_node(0, ["-wallet=w1", "-wallet=w2"])
         self.multiwallet_signing_test()
 
 

test/functional/test_framework/test_framework.py

@@ -126,6 +126,7 @@ def __init__(self):
         # addrman will not result in automatic connections to them.
         self.disable_autoconnect = True
         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
         # optionally, increase timeout by a factor
@@ -528,13 +529,19 @@ def setup_nodes(self):
                 assert_equal(chain_info["initialblockdownload"], False)
 
     def import_deterministic_coinbase_privkeys(self):
-        wallet_names = (
-            [self.default_wallet_name] * len(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
+            else self.wallet_names[i]
+            if i < len(self.wallet_names)
+            else False
         )
-        assert len(wallet_names) <= len(self.nodes)
-        for wallet_name, n in zip(wallet_names, self.nodes):
+        if wallet_name is not False:
+            n = self.nodes[i]
             if wallet_name is not None:
                 n.createwallet(
                     wallet_name=wallet_name,

test/functional/tool_wallet.py

@@ -259,7 +259,8 @@ 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")

test/functional/wallet_backup.py

@@ -87,10 +87,10 @@ def do_one_round(self):
         self.generate(self.nodes[3], 1)
 
     # 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)
@@ -156,6 +156,11 @@ def restore_wallet_existent_name(self):
             wallet_file,
         )
 
+    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.generate(self.nodes[0], 1)
@@ -231,7 +236,8 @@ def run_test(self):
         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)

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", "-wallet=dump"]]
+        self.extra_args = [["-keypool=90"]]
         self.rpc_timeout = 120
 
     def skip_test_if_missing_module(self):
@@ -106,6 +106,8 @@ def setup_network(self):
         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"
         )
@@ -229,7 +231,8 @@ def run_test(self):
         )
 
         # 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)

test/functional/wallet_multiwallet.py

@@ -45,6 +45,7 @@ def set_test_params(self):
         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()
@@ -98,7 +99,9 @@ def wallet_file(name):
         # 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")
@@ -133,8 +136,11 @@ def wallet_file(name):
         ]
         if os.name == "nt":
             wallet_names.remove("w7_symlink")
-        extra_args = ["-nowallet"] + [f"-wallet={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(w["name"] for w in self.nodes[0].listwalletdir()["wallets"]),
             [
@@ -152,19 +158,20 @@ def wallet_file(name):
 
         assert_equal(set(node.listwallets()), set(wallet_names))
 
+        # should raise rpc error if wallet path can't be created
+        assert_raises_rpc_error(
+            -1,
+            "filesystem error:" if sys.platform != "win32" else "create_directories:",
+            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 = (
-            "filesystem error:" if sys.platform != "win32" else "create_directories:"
-        )
-        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',
@@ -219,7 +226,9 @@ def wallet_file(name):
         # 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")
         self.generatetoaddress(
@@ -229,17 +238,18 @@ def wallet_file(name):
         # 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", f"-walletdir={data_dir()}"]
-        )
+        self.restart_node(0, ["-nowallet", f"-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()
         assert_equal(w5_info["immature_balance"], 50000000)
 
         competing_wallet_dir = os.path.join(self.options.tmpdir, "competing_walletdir")
         os.mkdir(competing_wallet_dir)
-        self.restart_node(0, [f"-walletdir={competing_wallet_dir}"])
+        self.restart_node(0, ["-nowallet", f"-walletdir={competing_wallet_dir}"])
+        self.nodes[0].createwallet(self.default_wallet_name, descriptors=False)
         exp_stderr = (
             r"Error: Error initializing wallet database environment"
             r" \"\S+competing_walletdir\S*\"!"
@@ -250,7 +260,9 @@ def wallet_file(name):
             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(w["name"] for w in self.nodes[0].listwalletdir()["wallets"]),