Merge #11466: Specify custom wallet directory with -walletdir param

c1e5d40 Make debugging test crash easier (MeshCollider)
8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider)
9587a9c Default walletdir is wallets/ if it exists (MeshCollider)
d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider)
80c5cbc Add test for -walletdir (MeshCollider)
0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider)

Pull request description:

  Closes #11348

  Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists.

  Includes tests and release notes. Things which might need to be considered more:
  - there is no 'lock' on the wallets directory, which might be needed?
  - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir)
  - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in #11687
  - doc/files.md needs updating (will do soon)

  I also considered including  a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review.

Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d

doc/release-notes.md

@@ -20,7 +20,7 @@ How to Upgrade
 ==============
 
 If you are running an older version, shut it down. Wait until it has completely
-shut down (which might take a few minutes for older versions), then run the 
+shut down (which might take a few minutes for older versions), then run the
 installer (on Windows) or just copy over `/Applications/Bitcoin-Qt` (on Mac)
 or `bitcoind`/`bitcoin-qt` (on Linux).
 
@@ -62,6 +62,20 @@ Due to a backward-incompatible change in the wallet database, wallets created
 with version 0.16.0 will be rejected by previous versions. Also, version 0.16.0
 will only create hierarchical deterministic (HD) wallets.
 
+Custom wallet directories
+---------------------
+The ability to specify a directory other than the default data directory in which to store
+wallets has been added. An existing directory can be specified using the `-walletdir=<dir>`
+argument. Wallets loaded via `-wallet` arguments must be in this wallet directory. Care should be taken
+when choosing a wallet directory location, as if  it becomes unavailable during operation,
+funds may be lost.
+
+Default wallet directory change
+--------------------------
+On new installations (if the data directory doesn't exist), wallets will now be stored in a
+new `wallets/` subdirectory inside the data directory. If this `wallets/` subdirectory
+doesn't exist (i.e. on existing nodes), the current datadir root is used instead, as it was.
+
 Low-level RPC changes
 ----------------------
 - The deprecated RPC `getinfo` was removed. It is recommended that the more specific RPCs are used:

src/Makefile.am

@@ -168,6 +168,7 @@ BITCOIN_CORE_H = \
   wallet/rpcwallet.h \
   wallet/wallet.h \
   wallet/walletdb.h \
+  wallet/walletutil.h \
   warnings.h \
   zmq/zmqabstractnotifier.h \
   zmq/zmqconfig.h\
@@ -249,6 +250,7 @@ libbitcoin_wallet_a_SOURCES = \
   wallet/rpcwallet.cpp \
   wallet/wallet.cpp \
   wallet/walletdb.cpp \
+  wallet/walletutil.cpp \
   $(BITCOIN_CORE_H)
 
 # crypto primitives library

src/qt/intro.cpp

@@ -214,7 +214,10 @@ bool Intro::pickDataDirectory()
             }
             dataDir = intro.getDataDirectory();
             try {
-                TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir));
+                if (TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir))) {
+                    // If a new data directory has been created, make wallets subdirectory too
+                    TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir) / "wallets");
+                }
                 break;
             } catch (const fs::filesystem_error&) {
                 QMessageBox::critical(0, tr(PACKAGE_NAME),

src/util.cpp

@@ -574,7 +574,10 @@ const fs::path &GetDataDir(bool fNetSpecific)
     if (fNetSpecific)
         path /= BaseParams().DataDir();
 
-    fs::create_directories(path);
+    if (fs::create_directories(path)) {
+        // This is the first run, create wallets subdirectory too
+        fs::create_directories(path / "wallets");
+    }
 
     return path;
 }

src/wallet/db.cpp

@@ -11,6 +11,7 @@
 #include <protocol.h>
 #include <util.h>
 #include <utilstrencodings.h>
+#include <wallet/walletutil.h>
 
 #include <stdint.h>
 
@@ -257,23 +258,23 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco
     return fSuccess;
 }
 
-bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr)
+bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walletDir, std::string& errorStr)
 {
     LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0));
     LogPrintf("Using wallet %s\n", walletFile);
 
     // Wallet file must be a plain filename without a directory
     if (walletFile != fs::basename(walletFile) + fs::extension(walletFile))
     {
-        errorStr = strprintf(_("Wallet %s resides outside data directory %s"), walletFile, dataDir.string());
+        errorStr = strprintf(_("Wallet %s resides outside wallet directory %s"), walletFile, walletDir.string());
         return false;
     }
 
-    if (!bitdb.Open(dataDir))
+    if (!bitdb.Open(walletDir))
     {
         // try moving the database env out of the way
-        fs::path pathDatabase = dataDir / "database";
-        fs::path pathDatabaseBak = dataDir / strprintf("database.%d.bak", GetTime());
+        fs::path pathDatabase = walletDir / "database";
+        fs::path pathDatabaseBak = walletDir / strprintf("database.%d.bak", GetTime());
         try {
             fs::rename(pathDatabase, pathDatabaseBak);
             LogPrintf("Moved old %s to %s. Retrying.\n", pathDatabase.string(), pathDatabaseBak.string());
@@ -282,18 +283,18 @@ bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataD
         }
 
         // try again
-        if (!bitdb.Open(dataDir)) {
+        if (!bitdb.Open(walletDir)) {
             // if it still fails, it probably means we can't even create the database env
-            errorStr = strprintf(_("Error initializing wallet database environment %s!"), GetDataDir());
+            errorStr = strprintf(_("Error initializing wallet database environment %s!"), walletDir);
             return false;
         }
     }
     return true;
 }
 
-bool CDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& dataDir, std::string& warningStr, std::string& errorStr, CDBEnv::recoverFunc_type recoverFunc)
+bool CDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& walletDir, std::string& warningStr, std::string& errorStr, CDBEnv::recoverFunc_type recoverFunc)
 {
-    if (fs::exists(dataDir / walletFile))
+    if (fs::exists(walletDir / walletFile))
     {
         std::string backup_filename;
         CDBEnv::VerifyResult r = bitdb.Verify(walletFile, recoverFunc, backup_filename);
@@ -303,7 +304,7 @@ bool CDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& data
                                      " Original %s saved as %s in %s; if"
                                      " your balance or transactions are incorrect you should"
                                      " restore from a backup."),
-                                   walletFile, backup_filename, dataDir);
+                                   walletFile, backup_filename, walletDir);
         }
         if (r == CDBEnv::RECOVER_FAIL)
         {
@@ -407,7 +408,7 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
 
     {
         LOCK(env->cs_db);
-        if (!env->Open(GetDataDir()))
+        if (!env->Open(GetWalletDir()))
             throw std::runtime_error("CDB: Failed to open database environment.");
 
         pdb = env->mapDb[strFilename];
@@ -695,7 +696,7 @@ bool CWalletDBWrapper::Backup(const std::string& strDest)
                 env->mapFileUseCount.erase(strFile);
 
                 // Copy wallet file
-                fs::path pathSrc = GetDataDir() / strFile;
+                fs::path pathSrc = GetWalletDir() / strFile;
                 fs::path pathDest(strDest);
                 if (fs::is_directory(pathDest))
                     pathDest /= strFile;

src/wallet/db.h

@@ -167,9 +167,9 @@ class CDB
        ideal to be called periodically */
     static bool PeriodicFlush(CWalletDBWrapper& dbw);
     /* verifies the database environment */
-    static bool VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr);
+    static bool VerifyEnvironment(const std::string& walletFile, const fs::path& walletDir, std::string& errorStr);
     /* verifies the database file */
-    static bool VerifyDatabaseFile(const std::string& walletFile, const fs::path& dataDir, std::string& warningStr, std::string& errorStr, CDBEnv::recoverFunc_type recoverFunc);
+    static bool VerifyDatabaseFile(const std::string& walletFile, const fs::path& walletDir, std::string& warningStr, std::string& errorStr, CDBEnv::recoverFunc_type recoverFunc);
 
 public:
     template <typename K, typename T>

src/wallet/init.cpp

@@ -9,8 +9,9 @@
 #include <util.h>
 #include <utilmoneystr.h>
 #include <validation.h>
-#include <wallet/wallet.h>
 #include <wallet/rpcwallet.h>
+#include <wallet/wallet.h>
+#include <wallet/walletutil.h>
 
 std::string GetWalletHelpString(bool showDebug)
 {
@@ -34,6 +35,7 @@ std::string GetWalletHelpString(bool showDebug)
     strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format on startup"));
     strUsage += HelpMessageOpt("-wallet=<file>", _("Specify wallet file (within data directory)") + " " + strprintf(_("(default: %s)"), DEFAULT_WALLET_DAT));
     strUsage += HelpMessageOpt("-walletbroadcast", _("Make the wallet broadcast transactions") + " " + strprintf(_("(default: %u)"), DEFAULT_WALLETBROADCAST));
+    strUsage += HelpMessageOpt("-walletdir=<dir>", _("Specify directory to hold wallets (default: <datadir>/wallets if it exists, otherwise <datadir>)"));
     strUsage += HelpMessageOpt("-walletnotify=<cmd>", _("Execute command when a wallet transaction changes (%s in cmd is replaced by TxID)"));
     strUsage += HelpMessageOpt("-zapwallettxes=<mode>", _("Delete all wallet transactions and only recover those parts of the blockchain through -rescan on startup") +
                                " " + _("(1 = keep tx meta data e.g. account owner and payment request information, 2 = drop tx meta data)"));
@@ -191,6 +193,12 @@ bool VerifyWallets()
         return true;
     }
 
+    if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir())) {
+        return InitError(strprintf(_("Error: Specified wallet directory \"%s\" does not exist."), gArgs.GetArg("-walletdir", "").c_str()));
+    }
+
+    LogPrintf("Using wallet directory %s\n", GetWalletDir().string());
+
     uiInterface.InitMessage(_("Verifying wallet(s)..."));
 
     // Keep track of each wallet absolute path to detect duplicates.
@@ -205,7 +213,7 @@ bool VerifyWallets()
             return InitError(strprintf(_("Error loading wallet %s. Invalid characters in -wallet filename."), walletFile));
         }
 
-        fs::path wallet_path = fs::absolute(walletFile, GetDataDir());
+        fs::path wallet_path = fs::absolute(walletFile, GetWalletDir());
 
         if (fs::exists(wallet_path) && (!fs::is_regular_file(wallet_path) || fs::is_symlink(wallet_path))) {
             return InitError(strprintf(_("Error loading wallet %s. -wallet filename must be a regular file."), walletFile));
@@ -216,7 +224,7 @@ bool VerifyWallets()
         }
 
         std::string strError;
-        if (!CWalletDB::VerifyEnvironment(walletFile, GetDataDir().string(), strError)) {
+        if (!CWalletDB::VerifyEnvironment(walletFile, GetWalletDir().string(), strError)) {
             return InitError(strError);
         }
 
@@ -230,7 +238,7 @@ bool VerifyWallets()
         }
 
         std::string strWarning;
-        bool dbV = CWalletDB::VerifyDatabaseFile(walletFile, GetDataDir().string(), strWarning, strError);
+        bool dbV = CWalletDB::VerifyDatabaseFile(walletFile, GetWalletDir().string(), strWarning, strError);
         if (!strWarning.empty()) {
             InitWarning(strWarning);
         }

src/wallet/rpcwallet.cpp

@@ -26,6 +26,7 @@
 #include <wallet/feebumper.h>
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
+#include <wallet/walletutil.h>
 
 #include <init.h>  // For StartShutdown
 

src/wallet/walletdb.cpp

@@ -814,14 +814,14 @@ bool CWalletDB::RecoverKeysOnlyFilter(void *callbackData, CDataStream ssKey, CDa
     return true;
 }
 
-bool CWalletDB::VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr)
+bool CWalletDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walletDir, std::string& errorStr)
 {
-    return CDB::VerifyEnvironment(walletFile, dataDir, errorStr);
+    return CDB::VerifyEnvironment(walletFile, walletDir, errorStr);
 }
 
-bool CWalletDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& dataDir, std::string& warningStr, std::string& errorStr)
+bool CWalletDB::VerifyDatabaseFile(const std::string& walletFile, const fs::path& walletDir, std::string& warningStr, std::string& errorStr)
 {
-    return CDB::VerifyDatabaseFile(walletFile, dataDir, warningStr, errorStr, CWalletDB::Recover);
+    return CDB::VerifyDatabaseFile(walletFile, walletDir, warningStr, errorStr, CWalletDB::Recover);
 }
 
 bool CWalletDB::WriteDestData(const std::string &address, const std::string &key, const std::string &value)

src/wallet/walletdb.h

@@ -226,9 +226,9 @@ class CWalletDB
     /* Function to determine if a certain KV/key-type is a key (cryptographical key) type */
     static bool IsKeyType(const std::string& strType);
     /* verifies the database environment */
-    static bool VerifyEnvironment(const std::string& walletFile, const fs::path& dataDir, std::string& errorStr);
+    static bool VerifyEnvironment(const std::string& walletFile, const fs::path& walletDir, std::string& errorStr);
     /* verifies the database file */
-    static bool VerifyDatabaseFile(const std::string& walletFile, const fs::path& dataDir, std::string& warningStr, std::string& errorStr);
+    static bool VerifyDatabaseFile(const std::string& walletFile, const fs::path& walletDir, std::string& warningStr, std::string& errorStr);
 
     //! write the hdchain model (external chain child index counter)
     bool WriteHDChain(const CHDChain& chain);

src/wallet/walletutil.cpp

@@ -0,0 +1,27 @@
+// Copyright (c) 2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "wallet/walletutil.h"
+
+fs::path GetWalletDir()
+{
+    fs::path path;
+
+    if (gArgs.IsArgSet("-walletdir")) {
+        path = fs::system_complete(gArgs.GetArg("-walletdir", ""));
+        if (!fs::is_directory(path)) {
+            // If the path specified doesn't exist, we return the deliberately
+            // invalid empty string.
+            path = "";
+        }
+    } else {
+        path = GetDataDir();
+        // If a wallets directory exists, use that, otherwise default to GetDataDir
+        if (fs::is_directory(path / "wallets")) {
+            path /= "wallets";
+        }
+    }
+
+    return path;
+}

src/wallet/walletutil.h

@@ -0,0 +1,13 @@
+// Copyright (c) 2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_WALLET_UTIL_H
+#define BITCOIN_WALLET_UTIL_H
+
+#include "util.h"
+
+//! Get the path of the wallet directory.
+fs::path GetWalletDir();
+
+#endif // BITCOIN_WALLET_UTIL_H

test/functional/keypool-topup.py

@@ -33,7 +33,7 @@ def run_test(self):
 
         self.stop_node(1)
 
-        shutil.copyfile(self.tmpdir + "/node1/regtest/wallet.dat", self.tmpdir + "/wallet.bak")
+        shutil.copyfile(self.tmpdir + "/node1/regtest/wallets/wallet.dat", self.tmpdir + "/wallet.bak")
         self.start_node(1, self.extra_args[1])
         connect_nodes_bi(self.nodes, 0, 1)
 
@@ -56,7 +56,7 @@ def run_test(self):
 
         self.stop_node(1)
 
-        shutil.copyfile(self.tmpdir + "/wallet.bak", self.tmpdir + "/node1/regtest/wallet.dat")
+        shutil.copyfile(self.tmpdir + "/wallet.bak", self.tmpdir + "/node1/regtest/wallets/wallet.dat")
 
         self.log.info("Verify keypool is restored and balance is correct")
 

test/functional/multiwallet.py

@@ -27,18 +27,40 @@ def run_test(self):
         self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.')
 
         # should not initialize if wallet file is a directory
-        os.mkdir(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w11'))
+        wallet_dir = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'wallets')
+        os.mkdir(os.path.join(wallet_dir, 'w11'))
         self.assert_start_raises_init_error(0, ['-wallet=w11'], 'Error loading wallet w11. -wallet filename must be a regular file.')
 
         # should not initialize if one wallet is a copy of another
-        shutil.copyfile(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w2'),
-                        os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w22'))
+        shutil.copyfile(os.path.join(wallet_dir, 'w2'), os.path.join(wallet_dir, 'w22'))
         self.assert_start_raises_init_error(0, ['-wallet=w2', '-wallet=w22'], 'duplicates fileid')
 
         # should not initialize if wallet file is a symlink
-        os.symlink(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w1'), os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w12'))
+        os.symlink(os.path.join(wallet_dir, 'w1'), os.path.join(wallet_dir, 'w12'))
         self.assert_start_raises_init_error(0, ['-wallet=w12'], 'Error loading wallet w12. -wallet filename must be a regular file.')
 
+        # should not initialize if the specified walletdir does not exist
+        self.assert_start_raises_init_error(0, ['-walletdir=bad'], 'Error: Specified wallet directory "bad" does not exist.')
+
+        # if wallets/ doesn't exist, datadir should be the default wallet dir
+        wallet_dir2 = os.path.join(self.options.tmpdir, 'node0', 'regtest', 'walletdir')
+        os.rename(wallet_dir, wallet_dir2)
+        self.start_node(0, ['-wallet=w4', '-wallet=w5'])
+        assert_equal(set(self.nodes[0].listwallets()), {"w4", "w5"})
+        w5 = self.nodes[0].get_wallet_rpc("w5")
+        w5.generate(1)
+        self.stop_node(0)
+
+        # 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.start_node(0, ['-wallet=w4', '-wallet=w5', '-walletdir=' + os.path.join(self.options.tmpdir, 'node0', 'regtest')])
+        assert_equal(set(self.nodes[0].listwallets()), {"w4", "w5"})
+        w5 = self.nodes[0].get_wallet_rpc("w5")
+        w5_info = w5.getwalletinfo()
+        assert_equal(w5_info['immature_balance'], 50)
+
+        self.stop_node(0)
+
         self.start_node(0, self.extra_args[0])
 
         w1 = self.nodes[0].get_wallet_rpc("w1")

test/functional/test_framework/test_framework.py

@@ -432,7 +432,7 @@ def _initialize_chain(self):
             self.disable_mocktime()
             for i in range(MAX_NODES):
                 os.remove(log_filename(self.options.cachedir, i, "debug.log"))
-                os.remove(log_filename(self.options.cachedir, i, "db.log"))
+                os.remove(log_filename(self.options.cachedir, i, "wallets/db.log"))
                 os.remove(log_filename(self.options.cachedir, i, "peers.dat"))
                 os.remove(log_filename(self.options.cachedir, i, "fee_estimates.dat"))
 

test/functional/test_runner.py

@@ -300,7 +300,11 @@ def run_tests(test_list, src_dir, build_dir, exeext, tmpdir, jobs=1, enable_cove
 
     if len(test_list) > 1 and jobs > 1:
         # Populate cache
-        subprocess.check_output([tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir])
+        try:
+            subprocess.check_output([tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir])
+        except Exception as e:
+            print(e.output)
+            raise e
 
     #Run Tests
     job_queue = TestHandler(jobs, tests_dir, tmpdir, test_list, flags)