From c0e27fc825cb9537f41b0914b177a3c32fe12da7 Mon Sep 17 00:00:00 2001 From: chromatic Date: Mon, 30 May 2022 19:16:16 -0700 Subject: [PATCH] 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 | 17 +++++++++++++---- qa/rpc-tests/wallet-hd.py | 11 ++++++----- qa/rpc-tests/walletbackup.py | 30 +++++++++++++++++++++--------- src/util.cpp | 27 ++++++++++++++++++--------- src/wallet/rpcdump.cpp | 23 +++++++++++++++++++++-- src/wallet/rpcwallet.cpp | 21 +++++++++++++++++---- src/wallet/test/wallet_tests.cpp | 2 +- 7 files changed, 97 insertions(+), 34 deletions(-) diff --git a/qa/rpc-tests/wallet-dump.py b/qa/rpc-tests/wallet-dump.py index 7fb261358..2184481c7 100755 --- a/qa/rpc-tests/wallet-dump.py +++ b/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 () diff --git a/qa/rpc-tests/wallet-hd.py b/qa/rpc-tests/wallet-hd.py index 0c96a5ad0..bcd773d39 100755 --- a/qa/rpc-tests/wallet-hd.py +++ b/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) diff --git a/qa/rpc-tests/walletbackup.py b/qa/rpc-tests/walletbackup.py index 7997e68cc..4f132c744 100755 --- a/qa/rpc-tests/walletbackup.py +++ b/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() diff --git a/src/util.cpp b/src/util.cpp index 61935c749..a0e7b4684 100644 --- a/src/util.cpp +++ b/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; } diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index ee6481f9d..b961ae52b 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -21,6 +21,8 @@ #include #include +#include +#include #include @@ -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 mapKeyBirth; std::set setKeyPool; pwalletMain->GetKeyBirthTimes(mapKeyBirth); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 436192021..b08dfbac0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/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; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b79c33798..ce0813920 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/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);