Do not create default wallet

achow101 · achow101 · commit 1bee1e6269b7 · 2020-09-08T21:02:53.000-04:00
No longer create a default wallet. The default wallet will still be
loaded if it exists and not other wallets were specified (anywhere,
including settings.json, bitcoin.conf, and command line).

Tests are updated to be started with -wallet= if they need the default
wallet.

Added test to wallet_startup.py testing that no default wallet is
created and that it is loaded if it exists and no other wallets were
specified.
diff --git a/doc/release-notes-15454.md b/doc/release-notes-15454.md
@@ -0,0 +1,6 @@
+Wallet
+------
+
+Bitcoin Core will no longer create an unnamed `""` wallet by default when no wallet is specified on the command line or in the configuration files.
+For backwards compatibility, if an unnamed `""` wallet already exists and would have been loaded previously, then it will still be loaded.
+Users without an unnamed `""` wallet and without any other wallets to be loaded on startup  will be prompted to either choose a wallet to load, or to create a new wallet.
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
@@ -488,8 +488,7 @@ class WalletImpl : public Wallet
 class WalletClientImpl : public WalletClient
 {
 public:
-    WalletClientImpl(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
-        : m_wallet_filenames(std::move(wallet_filenames))
+    WalletClientImpl(Chain& chain, ArgsManager& args)
     {
         m_context.chain = &chain;
         m_context.args = &args;
@@ -506,8 +505,8 @@ class WalletClientImpl : public WalletClient
             m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));
         }
     }
-    bool verify() override { return VerifyWallets(*m_context.chain, m_wallet_filenames); }
-    bool load() override { return LoadWallets(*m_context.chain, m_wallet_filenames); }
+    bool verify() override { return VerifyWallets(*m_context.chain); }
+    bool load() override { return LoadWallets(*m_context.chain); }
     void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }
     void flush() override { return FlushWallets(); }
     void stop() override { return StopWallets(); }
@@ -566,9 +565,9 @@ class WalletClientImpl : public WalletClient
 
 std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? MakeUnique<WalletImpl>(wallet) : nullptr; }
 
-std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)
+std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args)
 {
-    return MakeUnique<WalletClientImpl>(chain, args, std::move(wallet_filenames));
+    return MakeUnique<WalletClientImpl>(chain, args);
 }
 
 } // namespace interfaces
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
@@ -411,7 +411,7 @@ std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet);
 
 //! Return implementation of ChainClient interface for a wallet client. This
 //! function will be undefined in builds where ENABLE_WALLET is false.
-std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames);
+std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args);
 
 } // namespace interfaces
 
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
@@ -107,16 +107,7 @@ void WalletInit::Construct(NodeContext& node) const
         LogPrintf("Wallet disabled!\n");
         return;
     }
-    // If there's no -wallet setting with a list of wallets to load, set it to
-    // load the default "" wallet.
-    if (!args.IsArgSet("wallet")) {
-        args.LockSettings([&](util::Settings& settings) {
-            util::SettingsValue wallets(util::SettingsValue::VARR);
-            wallets.push_back(""); // Default wallet name is ""
-            settings.rw_settings["wallet"] = wallets;
-        });
-    }
-    auto wallet_client = interfaces::MakeWalletClient(*node.chain, args, args.GetArgs("-wallet"));
+    auto wallet_client = interfaces::MakeWalletClient(*node.chain, args);
     node.wallet_client = wallet_client.get();
     node.chain_clients.emplace_back(std::move(wallet_client));
 }
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
@@ -16,7 +16,7 @@
 
 #include <univalue.h>
 
-bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
+bool VerifyWallets(interfaces::Chain& chain)
 {
     if (gArgs.IsArgSet("-walletdir")) {
         fs::path wallet_dir = gArgs.GetArg("-walletdir", "");
@@ -41,10 +41,27 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
 
     chain.initMessage(_("Verifying wallet(s)...").translated);
 
+    // For backwards compatibility if an unnamed top level wallet exists in the
+    // wallets directory, include it in the default list of wallets to load.
+    if (!gArgs.IsArgSet("wallet")) {
+        DatabaseOptions options;
+        DatabaseStatus status;
+        bilingual_str error_string;
+        options.require_existing = true;
+        options.verify = false;
+        if (MakeWalletDatabase("", options, status, error_string)) {
+            gArgs.LockSettings([&](util::Settings& settings) {
+                util::SettingsValue wallets(util::SettingsValue::VARR);
+                wallets.push_back(""); // Default wallet name is ""
+                settings.rw_settings["wallet"] = wallets;
+            });
+        }
+    }
+
     // Keep track of each wallet absolute path to detect duplicates.
     std::set<fs::path> wallet_paths;
 
-    for (const auto& wallet_file : wallet_files) {
+    for (const auto& wallet_file : gArgs.GetArgs("-wallet")) {
         const fs::path path = fs::absolute(wallet_file, GetWalletDir());
 
         if (!wallet_paths.insert(path).second) {
@@ -65,10 +82,10 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
     return true;
 }
 
-bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files)
+bool LoadWallets(interfaces::Chain& chain)
 {
     try {
-        for (const std::string& name : wallet_files) {
+        for (const std::string& name : gArgs.GetArgs("-wallet")) {
             DatabaseOptions options;
             DatabaseStatus status;
             options.verify = false; // No need to verify, assuming verified earlier in VerifyWallets()
diff --git a/src/wallet/load.h b/src/wallet/load.h
@@ -17,10 +17,10 @@ class Chain;
 } // namespace interfaces
 
 //! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
-bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files);
+bool VerifyWallets(interfaces::Chain& chain);
 
 //! Load wallet databases.
-bool LoadWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files);
+bool LoadWallets(interfaces::Chain& chain);
 
 //! Complete startup of wallets.
 void StartWallets(CScheduler& scheduler, const ArgsManager& args);
diff --git a/src/wallet/test/init_test_fixture.cpp b/src/wallet/test/init_test_fixture.cpp
@@ -10,7 +10,7 @@
 
 InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
 {
-    m_wallet_client = MakeWalletClient(*m_chain, *Assert(m_node.args), {});
+    m_wallet_client = MakeWalletClient(*m_chain, *Assert(m_node.args));
 
     std::string sep;
     sep += fs::path::preferred_separator;
diff --git a/src/wallet/test/wallet_test_fixture.h b/src/wallet/test/wallet_test_fixture.h
@@ -21,7 +21,7 @@ struct WalletTestingSetup : public TestingSetup {
     explicit WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
 
     std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain(m_node);
-    std::unique_ptr<interfaces::WalletClient> m_wallet_client = interfaces::MakeWalletClient(*m_chain, *Assert(m_node.args), {});
+    std::unique_ptr<interfaces::WalletClient> m_wallet_client = interfaces::MakeWalletClient(*m_chain, *Assert(m_node.args));
     CWallet m_wallet;
     std::unique_ptr<interfaces::Handler> m_chain_notifications_handler;
 };
diff --git a/test/functional/feature_backwards_compatibility.py b/test/functional/feature_backwards_compatibility.py
@@ -36,12 +36,12 @@ def set_test_params(self):
         self.num_nodes = 6
         # Add new version after each release:
         self.extra_args = [
-            ["-addresstype=bech32"], # Pre-release: use to mine blocks
+            ["-addresstype=bech32", "-wallet="], # 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"], # v0.16.3
+            ["-nowallet", "-walletrbf=1", "-addresstype=bech32", "-wallet=wallet.dat"], # v0.16.3
         ]
 
     def skip_test_if_missing_module(self):
diff --git a/test/functional/feature_fee_estimation.py b/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", "-whitelist=noban@127.0.0.1"],
-            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=68000"],
-            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=32000"],
+            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-wallet="],
+            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=68000", "-wallet="],
+            ["-acceptnonstdtxn", "-whitelist=noban@127.0.0.1", "-blockmaxweight=32000", "-wallet="],
         ]
 
     def skip_test_if_missing_module(self):
diff --git a/test/functional/feature_filelock.py b/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([])
+        self.nodes[0].start(['-wallet='])
         self.nodes[0].wait_for_rpc_connection()
 
     def run_test(self):
@@ -30,7 +30,7 @@ def run_test(self):
             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), '-noserver'], expected_msg=expected_msg, match=ErrorMatch.PARTIAL_REGEX)
+            self.nodes[1].assert_start_raises_init_error(extra_args=['-walletdir={}'.format(wallet_dir), '-wallet=', '-noserver'], expected_msg=expected_msg, match=ErrorMatch.PARTIAL_REGEX)
 
 if __name__ == '__main__':
     FilelockTest().main()
diff --git a/test/functional/mempool_compatibility.py b/test/functional/mempool_compatibility.py
@@ -31,7 +31,7 @@ def setup_network(self):
             150200, # oldest version supported by the test framework
             None,
         ])
-        self.start_nodes()
+        self.start_nodes([[], ["-wallet="]])
         self.import_deterministic_coinbase_privkeys()
 
     def run_test(self):
diff --git a/test/functional/wallet_backup.py b/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 = [
-            ["-whitelist=noban@127.0.0.1", "-keypool=100"],
-            ["-whitelist=noban@127.0.0.1", "-keypool=100"],
-            ["-whitelist=noban@127.0.0.1", "-keypool=100"],
-            ["-whitelist=noban@127.0.0.1"],
+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
+            ["-whitelist=noban@127.0.0.1", "-keypool=100", "-wallet="],
+            ["-whitelist=noban@127.0.0.1", "-wallet="],
         ]
         self.rpc_timeout = 120
 
diff --git 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"]]
+        self.extra_args = [["-keypool=90", "-addresstype=legacy", "-wallet=dump"]]
         self.rpc_timeout = 120
 
     def skip_test_if_missing_module(self):
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
@@ -151,15 +151,15 @@ def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
     def setup_network(self):
-        self.extra_args = [[] for _ in range(self.num_nodes)]
+        self.extra_args = [["-wallet="] for _ in range(self.num_nodes)]
         for i, import_node in enumerate(IMPORT_NODES, 2):
             if import_node.prune:
                 self.extra_args[i] += ["-prune=1"]
 
         self.add_nodes(self.num_nodes, extra_args=self.extra_args)
 
         # Import keys with pruning disabled
-        self.start_nodes(extra_args=[[]] * self.num_nodes)
+        self.start_nodes(extra_args=[["-wallet="]] * self.num_nodes)
         for n in self.nodes:
             n.importprivkey(privkey=n.get_deterministic_priv_key().key, label='coinbase')
         self.stop_nodes()
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
@@ -43,6 +43,7 @@ def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 2
         self.rpc_timeout = 120
+        self.extra_args = [["-wallet="], ["-wallet="]]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -82,7 +83,7 @@ def wallet_file(name):
         os.rename(wallet_dir("wallet.dat"), wallet_dir("w8"))
 
         # create another dummy wallet for use in testing backups later
-        self.start_node(0, [])
+        self.start_node(0, ["-wallet="])
         self.stop_nodes()
         empty_wallet = os.path.join(self.options.tmpdir, 'empty.dat')
         os.rename(wallet_dir("wallet.dat"), empty_wallet)
@@ -152,7 +153,7 @@ def wallet_file(name):
 
         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, ['-walletdir=' + competing_wallet_dir, '-wallet='])
         exp_stderr = r"Error: Error initializing wallet database environment \"\S+competing_walletdir\"!"
         self.nodes[1].assert_start_raises_init_error(['-walletdir=' + competing_wallet_dir], exp_stderr, match=ErrorMatch.PARTIAL_REGEX)
 
diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py
@@ -26,6 +26,16 @@ def setup_nodes(self):
         self.start_nodes()
 
     def run_test(self):
+        self.log.info('Should start without any wallets')
+        assert_equal(self.nodes[0].listwallets(), [])
+        assert_equal(self.nodes[0].listwalletdir(), {'wallets': []})
+
+        self.log.info('New default wallet should load by default when there are no other wallets')
+        self.nodes[0].createwallet(wallet_name='', load_on_startup=False)
+        self.restart_node(0)
+        assert_equal(self.nodes[0].listwallets(), [''])
+
+        self.log.info('Test load on startup behavior')
         self.nodes[0].createwallet(wallet_name='w0', load_on_startup=True)
         self.nodes[0].createwallet(wallet_name='w1', load_on_startup=False)
         self.nodes[0].createwallet(wallet_name='w2', load_on_startup=True)
diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py
@@ -27,7 +27,7 @@ def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
         self.extra_args = [
-            ["-addresstype=bech32"], # current wallet version
+            ["-addresstype=bech32", "-wallet="], # current wallet version
             ["-usehd=1"],            # v0.16.3 wallet
             ["-usehd=0"]             # v0.15.2 wallet
         ]

Original file line number	Diff line number	Diff line change
`@@ -488,8 +488,7 @@ class WalletImpl : public Wallet`
`488`	`488`	`class WalletClientImpl : public WalletClient`
`489`	`489`	`{`
`490`	`490`	`public:`
`491`		`- WalletClientImpl(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)`
`492`		`- : m_wallet_filenames(std::move(wallet_filenames))`
	`491`	`+ WalletClientImpl(Chain& chain, ArgsManager& args)`
`493`	`492`	`{`
`494`	`493`	`m_context.chain = &chain;`
`495`	`494`	`m_context.args = &args;`
`@@ -506,8 +505,8 @@ class WalletClientImpl : public WalletClient`
`506`	`505`	`m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));`
`507`	`506`	`}`
`508`	`507`	`}`
`509`		`- bool verify() override { return VerifyWallets(*m_context.chain, m_wallet_filenames); }`
`510`		`- bool load() override { return LoadWallets(*m_context.chain, m_wallet_filenames); }`
	`508`	`+ bool verify() override { return VerifyWallets(*m_context.chain); }`
	`509`	`+ bool load() override { return LoadWallets(*m_context.chain); }`
`511`	`510`	`void start(CScheduler& scheduler) override { return StartWallets(scheduler, *Assert(m_context.args)); }`
`512`	`511`	`void flush() override { return FlushWallets(); }`
`513`	`512`	`void stop() override { return StopWallets(); }`
`@@ -566,9 +565,9 @@ class WalletClientImpl : public WalletClient`
`566`	`565`
`567`	`566`	`std::unique_ptr<Wallet> MakeWallet(const std::shared_ptr<CWallet>& wallet) { return wallet ? MakeUnique<WalletImpl>(wallet) : nullptr; }`
`568`	`567`
`569`		`-std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args, std::vector<std::string> wallet_filenames)`
	`568`	`+std::unique_ptr<WalletClient> MakeWalletClient(Chain& chain, ArgsManager& args)`
`570`	`569`	`{`
`571`		`- return MakeUnique<WalletClientImpl>(chain, args, std::move(wallet_filenames));`
	`570`	`+ return MakeUnique<WalletClientImpl>(chain, args);`
`572`	`571`	`}`
`573`	`572`
`574`	`573`	`} // namespace interfaces`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@`
`10`	`10`
`11`	`11`	`InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)`
`12`	`12`	`{`
`13`		`- m_wallet_client = MakeWalletClient(m_chain, Assert(m_node.args), {});`
	`13`	`+ m_wallet_client = MakeWalletClient(m_chain, Assert(m_node.args));`
`14`	`14`
`15`	`15`	`std::string sep;`
`16`	`16`	`sep += fs::path::preferred_separator;`
-Original file line number
+Diff line change
         # mine non-standard txs (e.g. txs with "dust" outputs)
         # Force fSendTrickle to true (via whitelist.noban)
         self.extra_args = [
 -            ["-acceptnonstdtxn", "[email protected]"],
 -            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=68000"],
 -            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=32000"],
 +            ["-acceptnonstdtxn", "[email protected]", "-wallet="],
 +            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=68000", "-wallet="],
 +            ["-acceptnonstdtxn", "[email protected]", "-blockmaxweight=32000", "-wallet="],
+        ]
     def skip_test_if_missing_module(self):