Make dumpwallet/backupwallet use backupdir path

This adds more defensiveness around dumping or backing up wallets, so
that the directory and filepaths are always available (even if they were
on transient storage that was removed), and that they never overwrite
other files.

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/util.cpp

@@ -558,22 +558,31 @@ const boost::filesystem::path &GetBackupDir()
 
     fs::path &path = backupPathCached;
 
-    if (!path.empty())
+    // 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")) {
-        LogPrintf("Starting with backupdir arg %s\n", GetArg("-backupdir", ""));
-        path = fs::system_complete(GetArg("-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);
+        }
     }
 
-    if (!fs::is_directory(path)) {
-        LogPrintf( "Backupdir %s is not a directory, apparently\n", path );
-        path = GetDataDir() / "backups";
+    // 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();
     }
 
-    fs::create_directories(path);
-
-    LogPrintf( "Set backupdir %s\n", path );
+    LogPrintf("Set backupdir %s\n", path);
     return path;
 }
 

src/wallet/rpcdump.cpp

@@ -21,6 +21,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 +555,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 +572,27 @@ 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;
+
+    if (userFilename != "") {
+        boost::filesystem::path p(userFilename);
+        boost::filesystem::path filename = p.filename();
+        if (!filename.empty()) {
+            path = GetBackupDir() / filename;
+        }
+    }
+
+    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/rpcwallet.cpp

@@ -1885,9 +1885,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\"")
@@ -1895,8 +1895,21 @@ 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;
+
+    if (userFilename != "") {
+        boost::filesystem::path p(userFilename);
+        boost::filesystem::path filename = p.filename();
+        if (!filename.empty()) {
+            path = GetBackupDir() / filename;
+        }
+    }
+
+    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);