From dcb43eaf61b41c60e577a6e85ee13e3bad4e61ee Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 15 Jun 2021 02:13:35 -0300 Subject: [PATCH] Create walletdir if datadir doesn't exist and correct tests Adaptation of btc@8263f6a5ac3f3af102a2819b7e179b00db7e0437 And cleanups for walletdir PR Adaptation of btc@b67342906ced2353d378f4369c8d8a979d525fee --- doc/release-notes.md | 2 +- src/init.cpp | 2 -- src/pivxd.cpp | 8 -------- src/qt/intro.cpp | 5 ++++- src/qt/pivx.cpp | 5 ----- src/util/system.cpp | 5 ++++- src/wallet/init.cpp | 9 +++++++++ src/wallet/walletdb.cpp | 3 ++- test/functional/wallet_backup.py | 20 ++++++++++---------- test/functional/wallet_hd.py | 6 +++--- test/functional/wallet_keypool_topup.py | 4 ++-- test/functional/wallet_multiwallet.py | 25 +++++++++++++------------ 12 files changed, 48 insertions(+), 46 deletions(-) diff --git a/doc/release-notes.md b/doc/release-notes.md index f5eee78edd01e..62d81590fc4bb 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -304,7 +304,7 @@ 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=` 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, +when choosing a wallet directory location, as if it becomes unavailable during operation, funds may be lost. Default wallet directory change diff --git a/src/init.cpp b/src/init.cpp index 6191de8a1d634..9a80c481892be 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -64,7 +64,6 @@ #include "wallet/init.h" #include "wallet/wallet.h" #include "wallet/rpcwallet.h" -#include "wallet/walletutil.h" #endif #include @@ -1269,7 +1268,6 @@ bool AppInitMain() LogPrintf("Startup time: %s\n", FormatISO8601DateTime(GetTime())); LogPrintf("Default data directory %s\n", GetDefaultDataDir().string()); LogPrintf("Using data directory %s\n", GetDataDir().string()); - LogPrintf("Using wallet directory %s\n", GetWalletDir().string()); LogPrintf("Using config file %s\n", GetConfigFile(gArgs.GetArg("-conf", PIVX_CONF_FILENAME)).string()); LogPrintf("Using at most %i connections (%i file descriptors available)\n", nMaxConnections, nFD); std::ostringstream strErrors; diff --git a/src/pivxd.cpp b/src/pivxd.cpp index 3407a07ec10d8..be30d3aae6c61 100644 --- a/src/pivxd.cpp +++ b/src/pivxd.cpp @@ -13,10 +13,6 @@ #include "noui.h" #include "util/system.h" -#ifdef ENABLE_WALLET -#include "wallet/walletutil.h" -#endif - #include /* Introduction text for doxygen: */ @@ -84,10 +80,6 @@ bool AppInit(int argc, char* argv[]) fprintf(stderr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "").c_str()); return false; } - if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir())) { - fprintf(stderr, "Error: Specified wallet directory \"%s\" does not exist.\n", gArgs.GetArg("-walletdir", "").c_str()); - return false; - } try { gArgs.ReadConfigFile(gArgs.GetArg("-conf", PIVX_CONF_FILENAME)); } catch (const std::exception& e) { diff --git a/src/qt/intro.cpp b/src/qt/intro.cpp index 28f8a1e99fdef..c8485cdb14ea2 100644 --- a/src/qt/intro.cpp +++ b/src/qt/intro.cpp @@ -193,7 +193,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& e) { QMessageBox::critical(0, tr("PIVX Core"), diff --git a/src/qt/pivx.cpp b/src/qt/pivx.cpp index 218f6573e5dde..ccd933a1d4e55 100644 --- a/src/qt/pivx.cpp +++ b/src/qt/pivx.cpp @@ -608,11 +608,6 @@ int main(int argc, char* argv[]) QObject::tr("Error: Specified data directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-datadir", "")))); return 1; } - if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir())) { - QMessageBox::critical(0, QObject::tr(PACKAGE_NAME), - QObject::tr("Error: Specified wallet directory \"%1\" does not exist.").arg(QString::fromStdString(gArgs.GetArg("-walletdir", "")))); - return EXIT_FAILURE; - } try { gArgs.ReadConfigFile(gArgs.GetArg("-conf", PIVX_CONF_FILENAME)); } catch (const std::exception& e) { diff --git a/src/util/system.cpp b/src/util/system.cpp index a262036a9e30e..d3792e57d6a46 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -697,7 +697,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; } diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index d2caf86fab8e4..0f7e0fd065167 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -148,6 +148,15 @@ bool WalletVerify() return true; } + if (gArgs.IsArgSet("-walletdir") && !fs::is_directory(GetWalletDir())) { + if (fs::exists(fs::system_complete(gArgs.GetArg("-walletdir", "")))) { + return UIError(strprintf(_("Specified -walletdir \"%s\" is not a directory"), gArgs.GetArg("-walletdir", "").c_str())); + } + return UIError(strprintf(_("Specified -walletdir \"%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. diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index fece219f49e35..2daef700a0068 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -18,6 +18,7 @@ #include "util/system.h" #include "utiltime.h" #include "wallet/wallet.h" +#include "wallet/walletutil.h" #include #include @@ -974,7 +975,7 @@ bool BackupWallet(const CWallet& wallet, const fs::path& strDest) // Copy wallet file fs::path pathDest(strDest); - fs::path pathSrc = GetDataDir() / strFile; + fs::path pathSrc = GetWalletDir() / strFile; if (is_directory(pathDest)) { if(!exists(pathDest)) create_directory(pathDest); pathDest /= strFile; diff --git a/test/functional/wallet_backup.py b/test/functional/wallet_backup.py index dbe8f91a8c524..4c839e8e329cc 100755 --- a/test/functional/wallet_backup.py +++ b/test/functional/wallet_backup.py @@ -98,9 +98,9 @@ def stop_three(self): self.stop_node(2) def erase_three(self): - os.remove(self.options.tmpdir + "/node0/regtest/wallet.dat") - os.remove(self.options.tmpdir + "/node1/regtest/wallet.dat") - os.remove(self.options.tmpdir + "/node2/regtest/wallet.dat") + os.remove(self.options.tmpdir + "/node0/regtest/wallets/wallet.dat") + os.remove(self.options.tmpdir + "/node1/regtest/wallets/wallet.dat") + os.remove(self.options.tmpdir + "/node2/regtest/wallets/wallet.dat") def run_test(self): self.log.info("Generating initial blockchain") @@ -162,9 +162,9 @@ def run_test(self): 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/wallet.bak", tmpdir + "/node0/regtest/wallets/wallet.dat") + shutil.copyfile(tmpdir + "/node1/wallet.bak", tmpdir + "/node1/regtest/wallets/wallet.dat") + shutil.copyfile(tmpdir + "/node2/wallet.bak", tmpdir + "/node2/regtest/wallets/wallet.dat") self.log.info("Re-starting nodes") self.start_three() @@ -200,10 +200,10 @@ def run_test(self): # Backup to source wallet file must fail sourcePaths = [ - tmpdir + "/node0/regtest/wallet.dat", - tmpdir + "/node0/./regtest/wallet.dat", - tmpdir + "/node0/regtest/", - tmpdir + "/node0/regtest"] + tmpdir + "/node0/regtest/wallets/wallet.dat", + tmpdir + "/node0/./regtest/wallets/wallet.dat", + tmpdir + "/node0/regtest/wallets/", + tmpdir + "/node0/regtest/wallets"] for sourcePath in sourcePaths: assert_raises_rpc_error(-4, "backup failed", self.nodes[0].backupwallet, sourcePath) diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py index d52d8d9963728..e0d295b951c08 100755 --- a/test/functional/wallet_hd.py +++ b/test/functional/wallet_hd.py @@ -131,7 +131,7 @@ def run_test(self): # otherwise node1 would auto-recover all funds in flag the keypool keys as used shutil.rmtree(os.path.join(self.nodes[1].datadir, "regtest", "blocks")) shutil.rmtree(os.path.join(self.nodes[1].datadir, "regtest", "chainstate")) - shutil.copyfile(os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, "regtest", "wallet.dat")) + shutil.copyfile(os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, "regtest", "wallets", "wallet.dat")) self.start_node(1) # Assert that derivation is deterministic @@ -154,7 +154,7 @@ def run_test(self): self.stop_node(1) shutil.rmtree(os.path.join(self.nodes[1].datadir, "regtest", "blocks")) shutil.rmtree(os.path.join(self.nodes[1].datadir, "regtest", "chainstate")) - shutil.copyfile(os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, "regtest", "wallet.dat")) + shutil.copyfile(os.path.join(self.nodes[1].datadir, "hd.bak"), os.path.join(self.nodes[1].datadir, "regtest", "wallets", "wallet.dat")) self.start_and_connect_node1() # Wallet automatically scans blocks older than key on startup (but shielded addresses need to be regenerated) assert_equal(self.nodes[1].getbalance(), NUM_HD_ADDS + 1) @@ -213,7 +213,7 @@ def run_test(self): # Delete wallet and recover from first seed self.stop_node(1) - os.remove(os.path.join(self.nodes[1].datadir, "regtest", "wallet.dat")) + os.remove(os.path.join(self.nodes[1].datadir, "regtest", "wallets", "wallet.dat")) self.start_node(1) # Regenerate old shield addresses self.log.info("Restore from seed ...") diff --git a/test/functional/wallet_keypool_topup.py b/test/functional/wallet_keypool_topup.py index c2d301d01496d..080799a0367a4 100755 --- a/test/functional/wallet_keypool_topup.py +++ b/test/functional/wallet_keypool_topup.py @@ -36,7 +36,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(self.nodes[0], 1) @@ -59,7 +59,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") diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index a97cacdec28aa..ee3594bd15149 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -25,9 +25,8 @@ def set_test_params(self): def run_test(self): node = self.nodes[0] - # !TODO: backport bitcoin#12220 - #data_dir = lambda *p: os.path.join(node.datadir, 'regtest', *p) - #wallet_dir = lambda *p: data_dir('wallets', *p) + data_dir = lambda *p: os.path.join(node.datadir, 'regtest', *p) + wallet_dir = lambda *p: data_dir('wallets', *p) wallet = lambda name: node.get_wallet_rpc(name) assert_equal(set(node.listwallets()), {"w1", "w2", "w3", "w"}) @@ -35,7 +34,7 @@ def run_test(self): self.stop_nodes() # !TODO: backport bitcoin#12220 - #self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist') + self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist') #self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=data_dir()) #self.assert_start_raises_init_error(0, ['-walletdir=debug.log'], 'Error: Specified -walletdir "debug.log" is not a directory', cwd=data_dir()) @@ -43,22 +42,24 @@ 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')) - #os.mkdir(wallet_dir('w11')) + os.mkdir(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(wallet_dir('w2'), wallet_dir('w22')) # !TODO: backport bitcoin#11970 - shutil.copyfile(os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w2'), - os.path.join(self.options.tmpdir, 'node0', 'regtest', 'w22')) + shutil.copyfile(wallet_dir('w2'), wallet_dir('w22')) # !TODO: backport bitcoin#11970 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(wallet_dir('w1'), wallet_dir('w12')) # !TODO: backport bitcoin#11970 + os.symlink(wallet_dir('w1'), wallet_dir('w12')) # !TODO: backport bitcoin#11970 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 -walletdir "bad" does not exist') + # should not initialize if the specified walletdir is not a directory + not_a_dir = wallet_dir('notadir') + open(not_a_dir, 'a', encoding="utf8").close() + self.assert_start_raises_init_error(0, ['-walletdir='+not_a_dir], 'Error: Specified -walletdir "' + not_a_dir + '" is not a directory') + self.log.info("Do not allow -zapwallettxes with multiwallet") self.assert_start_raises_init_error(0, ['-zapwallettxes', '-wallet=w1', '-wallet=w2'], "Error: -zapwallettxes is only allowed with a single wallet file") self.assert_start_raises_init_error(0, ['-zapwallettxes=1', '-wallet=w1', '-wallet=w2'], "Error: -zapwallettxes is only allowed with a single wallet file")