Fix wallet directory initialization

util/system datadir code should not care about wallets or create any wallet paths
If -walletdir is specified, wallet init code should use that directory
If -walletdir is specified and path is not a directory, return a fatal error
If -walletdir is not specified, use <datadir>/wallets if it is directory.
If -walletdir is not specified and <datadir>/wallets does not exist:
<datadir>/wallets wallet should call ListWalletDir ListDatabases on <datadir>
If list is empty wallet should create <datadir>/wallets as a directory,
otherwise it should create it as a symlink pointing to .
discussion at bitcoin#16220

src/common/init.cpp

@@ -45,21 +45,11 @@ std::optional<ConfigError> InitConfig(ArgsManager& args, SettingsAbortFn setting
         // Create datadir if it does not exist.
         const auto base_path{args.GetDataDirBase()};
         if (!fs::exists(base_path)) {
-            // When creating a *new* datadir, also create a "wallets" subdirectory,
-            // whether or not the wallet is enabled now, so if the wallet is enabled
-            // in the future, it will use the "wallets" subdirectory for creating
-            // and listing wallets, rather than the top-level directory where
-            // wallets could be mixed up with other files. For backwards
-            // compatibility, wallet code will use the "wallets" subdirectory only
-            // if it already exists, but never create it itself. There is discussion
-            // in https://github.com/bitcoin/bitcoin/issues/16220 about ways to
-            // change wallet code so it would no longer be necessary to create
-            // "wallets" subdirectories here.
-            fs::create_directories(base_path / "wallets");
+            fs::create_directories(base_path);
         }
         const auto net_path{args.GetDataDirNet()};
         if (!fs::exists(net_path)) {
-            fs::create_directories(net_path / "wallets");
+            fs::create_directories(net_path);
         }
 
         // Show an error or warning if there is a bitcoin.conf file in the

src/qt/intro.cpp

@@ -240,10 +240,7 @@ bool Intro::showIfNeeded(bool& did_show_intro, int64_t& prune_MiB)
             }
             dataDir = intro.getDataDirectory();
             try {
-                if (TryCreateDirectories(GUIUtil::QStringToPath(dataDir))) {
-                    // If a new data directory has been created, make wallets subdirectory too
-                    TryCreateDirectories(GUIUtil::QStringToPath(dataDir) / "wallets");
-                }
+                TryCreateDirectories(GUIUtil::QStringToPath(dataDir));
                 break;
             } catch (const fs::filesystem_error&) {
                 QMessageBox::critical(nullptr, PACKAGE_NAME,

src/wallet/load.cpp

@@ -48,6 +48,18 @@ bool VerifyWallets(WalletContext& context)
         args.ForceSetArg("-walletdir", fs::PathToString(canonical_wallet_dir));
     }
 
+    // Initialise the wallet directory
+    fs::path wallet_path;
+    if (!args.IsArgSet("-walletdir")) {
+        wallet_path = args.GetDataDirNet();
+        if (!fs::is_directory(wallet_path / "wallets")) {
+            std::vector<fs::path> db_paths = ListDatabases(wallet_path);
+            if (db_paths.empty()) {
+                fs::create_directories(wallet_path / "wallets");
+            }
+        }
+    }
+
     LogPrintf("Using wallet directory %s\n", fs::PathToString(GetWalletDir()));
 
     chain.initMessage(_("Verifying wallet(s)…").translated);

test/functional/feature_posix_fs_permissions.py

@@ -34,7 +34,8 @@ def run_test(self):
         datadir = self.nodes[0].chain_path
         self.check_directory_permissions(datadir)
         walletsdir = self.nodes[0].wallets_path
-        self.check_directory_permissions(walletsdir)
+        if os.path.exists(walletsdir):
+            self.check_directory_permissions(walletsdir)
         debuglog = self.nodes[0].debug_log_path
         self.check_file_permissions(debuglog)
 

test/functional/test_framework/test_framework.py

@@ -841,8 +841,8 @@ def _initialize_chain(self):
 
             def cache_path(*paths):
                 return os.path.join(cache_node_dir, self.chain, *paths)
-
-            os.rmdir(cache_path('wallets'))  # Remove empty wallets dir
+            if os.path.exists(cache_path('wallets')):
+                os.rmdir(cache_path('wallets'))  # Remove empty wallets dir
             for entry in os.listdir(cache_path()):
                 if entry not in ['chainstate', 'blocks', 'indexes']:  # Only indexes, chainstate and blocks folders
                     os.remove(cache_path(entry))

test/functional/test_runner.py

@@ -382,6 +382,7 @@
     'feature_help.py',
     'feature_shutdown.py',
     'wallet_migration.py',
+    'wallet_init.py',
     'p2p_ibd_txrelay.py',
     # Don't append tests at the end to avoid merge conflicts
     # Put them in a random line within the section that fits their approximate run-time

test/functional/wallet_init.py

@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017-2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test wallet load on startup.
+
+Verify that a bitcoind node can initialise wallet directory correctly on startup
+"""
+
+import os
+import shutil
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal,
+)
+
+
+class WalletInitTest(BitcoinTestFramework):
+    def add_options(self, parser):
+        self.add_wallet_options(parser)
+
+    def set_test_params(self):
+        self.num_nodes = 4
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def setup_nodes(self):
+        self.add_nodes(self.num_nodes)
+        self.start_nodes()
+
+    def run_test(self):
+        self.log.info('Test node0 is initialized with a wallets/ dir')
+        node0_data_dir = self.nodes[0].chain_path
+        assert_equal(os.path.isdir(os.path.join(node0_data_dir, "wallets")), True)
+        self.nodes[0].createwallet("w0")
+        assert_equal(self.nodes[0].listwallets(), ['w0'])
+        assert_equal(os.path.isdir(os.path.join(node0_data_dir, "wallets", "w0")), True)
+
+        self.log.info('Test node2 initialized with invalid -walletsdir')
+        node2_data_dir = self.nodes[2].chain_path
+        self.stop_node(2)
+        self.nodes[2].assert_start_raises_init_error(['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist')
+        self.nodes[2].assert_start_raises_init_error(['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=node2_data_dir)
+        self.nodes[2].assert_start_raises_init_error(['-walletdir=debug.log'], 'Error: Specified -walletdir "debug.log" is not a directory', cwd=node2_data_dir)
+
+        self.log.info('Test node3 is initialized with a valid -walletsdir')
+        node3_data_dir = self.nodes[3].chain_path
+        new_walletdir = os.path.join(node3_data_dir, "my_wallets")
+        self.stop_node(3)
+        shutil.rmtree(os.path.join(node3_data_dir, "wallets"))
+        os.makedirs(new_walletdir)
+        self.restart_node(3, ['-walletdir=' + str(new_walletdir)])
+        self.nodes[3].createwallet("w3")
+        assert_equal(self.nodes[3].listwallets(), ["w3"])
+        assert_equal(os.path.isdir(os.path.join(new_walletdir, "w3")), True)
+
+if __name__ == '__main__':
+    WalletInitTest().main()

test/functional/wallet_listtransactions.py

@@ -234,8 +234,8 @@ def run_externally_generated_address_test(self):
         # refill keypool otherwise the second node wouldn't recognize addresses generated on the first nodes
         self.nodes[0].keypoolrefill(1000)
         self.stop_nodes()
-        wallet0 = os.path.join(self.nodes[0].chain_path, self.default_wallet_name, "wallet.dat")
-        wallet2 = os.path.join(self.nodes[2].chain_path, self.default_wallet_name, "wallet.dat")
+        wallet0 = os.path.join(self.nodes[0].chain_path, "wallets", self.default_wallet_name, "wallet.dat")
+        wallet2 = os.path.join(self.nodes[2].chain_path, "wallets", self.default_wallet_name, "wallet.dat")
         shutil.copyfile(wallet0, wallet2)
         self.start_nodes()
         # reconnect nodes

test/functional/wallet_reorgsrestore.py

@@ -88,7 +88,7 @@ def run_test(self):
         # Node0 wallet file is loaded on longest sync'ed node1
         self.stop_node(1)
         self.nodes[0].backupwallet(self.nodes[0].datadir_path / 'wallet.bak')
-        shutil.copyfile(self.nodes[0].datadir_path / 'wallet.bak', self.nodes[1].chain_path / self.default_wallet_name / self.wallet_data_filename)
+        shutil.copyfile(self.nodes[0].datadir_path / 'wallet.bak', self.nodes[1].chain_path / "wallets" / self.default_wallet_name / self.wallet_data_filename)
         self.start_node(1)
         tx_after_reorg = self.nodes[1].gettransaction(txid)
         # Check that normal confirmed tx is confirmed again but with different blockhash