Merge pull request #2996 from chromatic/add-backupdir-option

Add configurable backup directory for wallet dumps and backups

qa/rpc-tests/wallet-dump.py

@@ -4,7 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import (start_nodes, start_node, assert_equal, bitcoind_processes)
+from test_framework.util import (start_nodes, start_node, assert_equal, bitcoind_processes, JSONRPCException)
 
 
 def read_dump(file_name, addrs, hd_master_addr_old):
@@ -69,6 +69,7 @@ class WalletDumpTest(BitcoinTestFramework):
 
     def run_test (self):
         tmpdir = self.options.tmpdir
+        backupsdir = tmpdir + "/node0/regtest/backups/"
 
         # generate 20 addresses to compare against the dump
         test_addr_count = 20
@@ -81,10 +82,12 @@ class WalletDumpTest(BitcoinTestFramework):
         self.nodes[0].keypoolrefill()
 
         # dump unencrypted wallet
-        self.nodes[0].dumpwallet(tmpdir + "/node0/wallet.unencrypted.dump")
+        # note that the RPC extracts only the filename
+        # thus writing to the backups/ default directory
+        self.nodes[0].dumpwallet("/node0/wallet.unencrypted.dump")
 
         found_addr, found_addr_chg, found_addr_rsv, hd_master_addr_unenc = \
-            read_dump(tmpdir + "/node0/wallet.unencrypted.dump", addrs, None)
+            read_dump(backupsdir + "wallet.unencrypted.dump", addrs, None)
         assert_equal(found_addr, test_addr_count)  # all keys must be in the dump
         assert_equal(found_addr_chg, 30)  # 30 blocks where mined
         assert_equal(found_addr_rsv, 90 + 1)  # keypool size (TODO: fix off-by-one)
@@ -99,10 +102,16 @@ class WalletDumpTest(BitcoinTestFramework):
         self.nodes[0].dumpwallet(tmpdir + "/node0/wallet.encrypted.dump")
 
         found_addr, found_addr_chg, found_addr_rsv, hd_master_addr_enc = \
-            read_dump(tmpdir + "/node0/wallet.encrypted.dump", addrs, hd_master_addr_unenc)
+            read_dump(backupsdir + "wallet.encrypted.dump", addrs, hd_master_addr_unenc)
         assert_equal(found_addr, test_addr_count)
         assert_equal(found_addr_chg, 90 + 1 + 30)  # old reserve keys are marked as change now
         assert_equal(found_addr_rsv, 90 + 1)  # keypool size (TODO: fix off-by-one)
 
+        try:
+            self.nodes[0].dumpwallet(tmpdir + "/node0/wallet.encrypted.dump")
+            raise AssertionError("dumpwallet should throw exception instead of overwriting existing file")
+        except JSONRPCException as e:
+            assert("Wallet dump file already exists; not overwriting" in e.error["message"])
+
 if __name__ == '__main__':
     WalletDumpTest().main ()

qa/rpc-tests/wallet-hd.py

@@ -23,6 +23,7 @@ class WalletHDTest(BitcoinTestFramework):
         self.node_args = [['-usehd=0'], ['-usehd=1', '-keypool=0']]
 
     def setup_network(self):
+        self.node_args[1].append('-backupdir=' + self.options.tmpdir)
         self.nodes = start_nodes(self.num_nodes, self.options.tmpdir, self.node_args)
         self.is_network_split = False
         connect_nodes_bi(self.nodes, 0, 1)
@@ -39,8 +40,8 @@ class WalletHDTest(BitcoinTestFramework):
         self.nodes[1].importprivkey(self.nodes[0].dumpprivkey(non_hd_add))
 
         # This should be enough to keep the master key and the non-HD key
-        self.nodes[1].backupwallet(tmpdir + "/hd.bak")
-        #self.nodes[1].dumpwallet(tmpdir + "/hd.dump")
+        self.nodes[1].backupwallet("hd.bak")
+        #self.nodes[1].dumpwallet("hd.dump")
 
         # Derive some HD addresses and remember the last
         # Also send funds to each add
@@ -62,9 +63,9 @@ class WalletHDTest(BitcoinTestFramework):
 
         print("Restore backup ...")
         self.stop_node(1)
-        os.remove(self.options.tmpdir + "/node1/regtest/wallet.dat")
+        os.remove(tmpdir + "/node1/regtest/wallet.dat")
         shutil.copyfile(tmpdir + "/hd.bak", tmpdir + "/node1/regtest/wallet.dat")
-        self.nodes[1] = start_node(1, self.options.tmpdir, self.node_args[1])
+        self.nodes[1] = start_node(1, tmpdir, self.node_args[1])
         #connect_nodes_bi(self.nodes, 0, 1)
 
         # Assert that derivation is deterministic
@@ -78,7 +79,7 @@ class WalletHDTest(BitcoinTestFramework):
 
         # Needs rescan
         self.stop_node(1)
-        self.nodes[1] = start_node(1, self.options.tmpdir, self.node_args[1] + ['-rescan'])
+        self.nodes[1] = start_node(1, tmpdir, self.node_args[1] + ['-rescan'])
         #connect_nodes_bi(self.nodes, 0, 1)
         assert_equal(self.nodes[1].getbalance(), num_hd_adds + 1)
 

qa/rpc-tests/walletbackup.py

@@ -1,5 +1,6 @@
 #!/usr/bin/env python3
 # Copyright (c) 2014-2016 The Bitcoin Core developers
+# Copyright (c) 2022 The Dogecoin Core developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -124,11 +125,11 @@ class WalletBackupTest(BitcoinTestFramework):
 
         logging.info("Backing up")
         tmpdir = self.options.tmpdir
-        self.nodes[0].backupwallet(tmpdir + "/node0/wallet.bak")
+        self.nodes[0].backupwallet(tmpdir + "/node0/wallet0.bak")
         self.nodes[0].dumpwallet(tmpdir + "/node0/wallet.dump")
-        self.nodes[1].backupwallet(tmpdir + "/node1/wallet.bak")
+        self.nodes[1].backupwallet(tmpdir + "/node1/wallet1.bak")
         self.nodes[1].dumpwallet(tmpdir + "/node1/wallet.dump")
-        self.nodes[2].backupwallet(tmpdir + "/node2/wallet.bak")
+        self.nodes[2].backupwallet(tmpdir + "/node2/wallet2.bak")
         self.nodes[2].dumpwallet(tmpdir + "/node2/wallet.dump")
 
         logging.info("More transactions")
@@ -161,9 +162,9 @@ class WalletBackupTest(BitcoinTestFramework):
         shutil.rmtree(self.options.tmpdir + "/node2/regtest/chainstate")
 
         # Restore wallets from backup
-        shutil.copyfile(tmpdir + "/node0/wallet.bak", tmpdir + "/node0/regtest/wallet.dat")
-        shutil.copyfile(tmpdir + "/node1/wallet.bak", tmpdir + "/node1/regtest/wallet.dat")
-        shutil.copyfile(tmpdir + "/node2/wallet.bak", tmpdir + "/node2/regtest/wallet.dat")
+        shutil.copyfile(tmpdir + "/node0/regtest/backups/wallet0.bak", tmpdir + "/node0/regtest/wallet.dat")
+        shutil.copyfile(tmpdir + "/node1/regtest/backups/wallet1.bak", tmpdir + "/node1/regtest/wallet.dat")
+        shutil.copyfile(tmpdir + "/node2/regtest/backups/wallet2.bak", tmpdir + "/node2/regtest/wallet.dat")
 
         logging.info("Re-starting nodes")
         self.start_three()
@@ -187,9 +188,9 @@ class WalletBackupTest(BitcoinTestFramework):
         assert_equal(self.nodes[1].getbalance(), 0)
         assert_equal(self.nodes[2].getbalance(), 0)
 
-        self.nodes[0].importwallet(tmpdir + "/node0/wallet.dump")
-        self.nodes[1].importwallet(tmpdir + "/node1/wallet.dump")
-        self.nodes[2].importwallet(tmpdir + "/node2/wallet.dump")
+        self.nodes[0].importwallet(tmpdir + "/node0/regtest/backups/wallet.dump")
+        self.nodes[1].importwallet(tmpdir + "/node1/regtest/backups/wallet.dump")
+        self.nodes[2].importwallet(tmpdir + "/node2/regtest/backups/wallet.dump")
 
         sync_blocks(self.nodes)
 
@@ -197,6 +198,17 @@ class WalletBackupTest(BitcoinTestFramework):
         assert_equal(self.nodes[1].getbalance(), balance1)
         assert_equal(self.nodes[2].getbalance(), balance2)
 
+        # start a node that writes backups with a -backupdir
+        initialize_datadir(tmpdir, 4)
+        backupdir = tmpdir + "/onebackup"
+        os.makedirs(backupdir)
+        backupdirnode = start_node(4, tmpdir, extra_args=["-backupdir=" + backupdir])
+        backupdirnode.dumpwallet('backupwallet.dump')
+        backupdirnode.importwallet(tmpdir + "/onebackup/backupwallet.dump")
+        stop_node(backupdirnode, 4)
+        assert(os.path.exists(tmpdir + "/onebackup/backupwallet.dump"))
+
+
 
 if __name__ == '__main__':
     WalletBackupTest().main()

src/Makefile.am

@@ -162,6 +162,7 @@ BITCOIN_CORE_H = \
   wallet/coincontrol.h \
   wallet/crypter.h \
   wallet/db.h \
+  wallet/rpcutil.h \
   wallet/rpcwallet.h \
   wallet/wallet.h \
   wallet/walletdb.h \
@@ -237,6 +238,7 @@ libdogecoin_wallet_a_SOURCES = \
   wallet/crypter.cpp \
   wallet/db.cpp \
   wallet/rpcdump.cpp \
+  wallet/rpcutil.cpp \
   wallet/rpcwallet.cpp \
   wallet/wallet.cpp \
   wallet/walletdb.cpp \

src/bitcoin-cli.cpp

@@ -35,6 +35,7 @@ std::string HelpMessageCli()
     strUsage += HelpMessageOpt("-?", _("This help message"));
     strUsage += HelpMessageOpt("-conf=<file>", strprintf(_("Specify configuration file (default: %s)"), BITCOIN_CONF_FILENAME));
     strUsage += HelpMessageOpt("-datadir=<dir>", _("Specify data directory"));
+    strUsage += HelpMessageOpt("-backupdir=<dir>", _("Specify directory where to write backups and data dumps (default datadir/backups)"));
     AppendParamsHelpMessages(strUsage);
     strUsage += HelpMessageOpt("-named", strprintf(_("Pass named instead of positional arguments (default: %s)"), DEFAULT_NAMED));
     strUsage += HelpMessageOpt("-rpcconnect=<ip>", strprintf(_("Send commands to node running on <ip> (default: %s)"), DEFAULT_RPCCONNECT));

src/init.cpp

@@ -330,6 +330,7 @@ std::string HelpMessage(HelpMessageMode mode)
     if (showDebug)
         strUsage += HelpMessageOpt("-blocksonly", strprintf(_("Whether to operate in a blocks only mode (default: %u)"), DEFAULT_BLOCKSONLY));
     strUsage +=HelpMessageOpt("-assumevalid=<hex>", strprintf(_("If this block is in the chain assume that it and its ancestors are valid and potentially skip their script verification (0 to verify all, default: %s, testnet: %s)"), Params(CBaseChainParams::MAIN).GetConsensus(0).defaultAssumeValid.GetHex(), Params(CBaseChainParams::TESTNET).GetConsensus(0).defaultAssumeValid.GetHex()));
+    strUsage += HelpMessageOpt("-backupdir=<dir>", _("Specify directory where to write backups and data dumps (default datadir/backups)"));
     strUsage += HelpMessageOpt("-conf=<file>", strprintf(_("Specify configuration file (default: %s)"), BITCOIN_CONF_FILENAME));
     if (mode == HMM_BITCOIND)
     {
@@ -1198,6 +1199,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
         LogPrintf("Startup time: %s\n", DateTimeStrFormat("%Y-%m-%d %H:%M:%S", GetTime()));
     LogPrintf("Default data directory %s\n", GetDefaultDataDir().string());
     LogPrintf("Using data directory %s\n", GetDataDir().string());
+    LogPrintf("Using backup directory %s\n", GetBackupDir().string());
     LogPrintf("Using config file %s\n", GetConfigFile(GetArg("-conf", BITCOIN_CONF_FILENAME)).string());
     LogPrintf("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD);
 

src/util.cpp

@@ -548,6 +548,44 @@ void ClearDatadirCache()
     pathCachedNetSpecific = boost::filesystem::path();
 }
 
+static boost::filesystem::path backupPathCached;
+static CCriticalSection csBackupPathCached;
+
+const boost::filesystem::path &GetBackupDir()
+{
+    namespace fs = boost::filesystem;
+    LOCK(csBackupPathCached);
+
+    fs::path &path = backupPathCached;
+
+    // ensure that any cached path still exists (not an unmounted filesystem, for example)
+    if (!path.empty() && fs::is_directory(path))
+        return path;
+
+    // start with a default, and overwrite if provided a valid path
+    path = GetDataDir() / "backups";
+
+    if (IsArgSet("-backupdir")) {
+        const std::string backupDir = GetArg("-backupdir", "");
+        fs::path backupDirPath = fs::system_complete(backupDir);
+
+        if (fs::create_directories(backupDirPath) || fs::is_directory(backupDirPath)) {
+            path = backupDirPath;
+        } else {
+            LogPrintf("Backupdir %s is not a directory, so using default path\n", backupDirPath);
+        }
+    }
+
+    // ensure the path exists or fall back to a path that does exist
+    if (!fs::is_directory(path) && !fs::create_directories(path)) {
+        LogPrintf("Failed to create directory %s, so using default path %s\n", path, GetDataDir());
+        path = GetDataDir();
+    }
+
+    LogPrintf("Set backupdir %s\n", path);
+    return path;
+}
+
 boost::filesystem::path GetConfigFile(const std::string& confPath)
 {
     boost::filesystem::path pathConfigFile(confPath);
@@ -853,4 +891,4 @@ std::string CopyrightHolders(const std::string& strPrefix)
         strCopyrightHolders += "\n" + strPrefix + "The Bitcoin Core developers";
     }
     return strCopyrightHolders;
-}
+}

src/util.h

@@ -100,6 +100,7 @@ bool RenameOver(boost::filesystem::path src, boost::filesystem::path dest);
 bool TryCreateDirectory(const boost::filesystem::path& p);
 boost::filesystem::path GetDefaultDataDir();
 const boost::filesystem::path &GetDataDir(bool fNetSpecific = true);
+const boost::filesystem::path &GetBackupDir();
 void ClearDatadirCache();
 boost::filesystem::path GetConfigFile(const std::string& confPath);
 #ifndef WIN32

src/wallet/rpcdump.cpp

@@ -13,6 +13,7 @@
 #include "util.h"
 #include "utiltime.h"
 #include "wallet.h"
+#include "wallet/rpcutil.h"
 #include "merkleblock.h"
 #include "core_io.h"
 
@@ -21,6 +22,8 @@
 
 #include <boost/algorithm/string.hpp>
 #include <boost/date_time/posix_time/posix_time.hpp>
+#include <boost/filesystem.hpp>
+#include <boost/filesystem/path.hpp>
 
 #include <univalue.h>
 
@@ -553,7 +556,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
 {
     if (!EnsureWalletIsAvailable(request.fHelp))
         return NullUniValue;
-    
+
     if (request.fHelp || request.params.size() != 1)
         throw runtime_error(
             "dumpwallet \"filename\"\n"
@@ -570,10 +573,19 @@ UniValue dumpwallet(const JSONRPCRequest& request)
     EnsureWalletIsUnlocked();
 
     ofstream file;
-    file.open(request.params[0].get_str().c_str());
+
+    string userFilename = request.params[0].get_str();
+    boost::filesystem::path path = GetBackupDirFromInput(userFilename);
+
+    if (boost::filesystem::exists(path))
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Wallet dump file already exists; not overwriting");
+
+    file.open(path.string());
     if (!file.is_open())
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");
 
+    LogPrintf("Dumping wallet to " + path.string() + "\n");
+
     std::map<CTxDestination, int64_t> mapKeyBirth;
     std::set<CKeyID> setKeyPool;
     pwalletMain->GetKeyBirthTimes(mapKeyBirth);

src/wallet/rpcutil.cpp

@@ -0,0 +1,20 @@
+// Copyright (c) 2022 The Dogecoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "wallet/rpcutil.h"
+
+boost::filesystem::path GetBackupDirFromInput(std::string strUserFilename)
+{
+    const boost::filesystem::path backupDir = GetBackupDir();
+
+    if (strUserFilename != "") {
+        boost::filesystem::path p(strUserFilename);
+        boost::filesystem::path filename = p.filename();
+
+        if (!filename.empty())
+            return backupDir / filename;
+    }
+
+    return backupDir;
+}

src/wallet/rpcutil.h

@@ -0,0 +1,15 @@
+// Copyright (c) 2022 The Dogecoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+/**
+ * Utility functions for RPC commands
+ */
+#ifndef DOGECOIN_WALLET_UTIL_H
+#define DOGECOIN_WALLET_UTIL_H
+#include <boost/filesystem/path.hpp>
+#include "util.h"
+
+boost::filesystem::path GetBackupDirFromInput(std::string strUserFilename);
+
+#endif // DOGECOIN_WALLET_UTIL_H

src/wallet/rpcwallet.cpp

@@ -19,6 +19,7 @@
 #include "util.h"
 #include "utilmoneystr.h"
 #include "wallet.h"
+#include "wallet/rpcutil.h"
 #include "walletdb.h"
 
 #include <stdint.h>
@@ -1994,9 +1995,9 @@ UniValue backupwallet(const JSONRPCRequest& request)
     if (request.fHelp || request.params.size() != 1)
         throw runtime_error(
             "backupwallet \"destination\"\n"
-            "\nSafely copies current wallet file to destination, which can be a directory or a path with filename.\n"
+            "\nSafely copies current wallet file to destination file.\n"
             "\nArguments:\n"
-            "1. \"destination\"   (string) The destination directory or file\n"
+            "1. \"destination\"   (string, required) The destination filename\n"
             "\nExamples:\n"
             + HelpExampleCli("backupwallet", "\"backup.dat\"")
             + HelpExampleRpc("backupwallet", "\"backup.dat\"")
@@ -2004,8 +2005,13 @@ UniValue backupwallet(const JSONRPCRequest& request)
 
     LOCK2(cs_main, pwalletMain->cs_wallet);
 
-    string strDest = request.params[0].get_str();
-    if (!pwalletMain->BackupWallet(strDest))
+    string userFilename = request.params[0].get_str();
+    boost::filesystem::path path = GetBackupDirFromInput(userFilename);
+
+    if (boost::filesystem::exists(path))
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Wallet dump file already exists; not overwriting");
+
+    if (!pwalletMain->BackupWallet(path.string()))
         throw JSONRPCError(RPC_WALLET_ERROR, "Error: Wallet backup failed!");
 
     return NullUniValue;

src/wallet/test/wallet_tests.cpp

@@ -476,7 +476,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain240Setup)
 
         JSONRPCRequest request;
         request.params.setArray();
-        request.params.push_back("wallet.backup");
+        request.params.push_back(TestChain240Setup::pathTemp.string() + "/regtest/backups/wallet.backup");
         ::pwalletMain = &wallet;
         ::importwallet(request);