From 16e57594556ac481a32f5d5ed1a988b2772ba804 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Fri, 28 Sep 2018 16:50:18 +0100 Subject: [PATCH] wallet: Refactor to use WalletLocation Github-Pull: #14350 Rebased-From: 65f3672 --- src/bench/coin_selection.cpp | 4 ++-- src/qt/test/addressbooktests.cpp | 2 +- src/qt/test/wallettests.cpp | 2 +- src/wallet/init.cpp | 8 +++---- src/wallet/rpcwallet.cpp | 29 ++++++++++++------------- src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/wallet_test_fixture.cpp | 3 ++- src/wallet/test/wallet_tests.cpp | 16 +++++++------- src/wallet/wallet.cpp | 25 ++++++++++----------- src/wallet/wallet.h | 20 ++++++++--------- test/lint/lint-circular-dependencies.sh | 1 - 11 files changed, 54 insertions(+), 58 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 0a6f5d85e..badc426b3 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -33,7 +33,7 @@ static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector CoinSet; -static const CWallet testWallet("dummy", WalletDatabase::CreateDummy()); +static const CWallet testWallet(WalletLocation(), WalletDatabase::CreateDummy()); std::vector> wtxn; // Copied from src/wallet/test/coinselector_tests.cpp diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 352584604..5aa296eb3 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -57,7 +57,7 @@ void EditAddressAndSubmit( void TestAddAddressesToSendBook() { TestChain100Setup test; - std::shared_ptr wallet = std::make_shared("mock", WalletDatabase::CreateMock()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateMock()); bool firstRun; wallet->LoadWallet(firstRun); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 9598d6484..fdbcb46f5 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -133,7 +133,7 @@ void TestGUI() for (int i = 0; i < 5; ++i) { test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey())); } - std::shared_ptr wallet = std::make_shared("mock", WalletDatabase::CreateMock()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateMock()); bool firstRun; wallet->LoadWallet(firstRun); { diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 7dc6c3064..d226bbc2f 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -203,15 +203,15 @@ bool WalletInit::Verify() const std::set wallet_paths; for (const auto& wallet_file : wallet_files) { - fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); + WalletLocation location(wallet_file); - if (!wallet_paths.insert(wallet_path).second) { + if (!wallet_paths.insert(location.GetPath()).second) { return InitError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), wallet_file)); } std::string error_string; std::string warning_string; - bool verify_success = CWallet::Verify(wallet_file, salvage_wallet, error_string, warning_string); + bool verify_success = CWallet::Verify(location, salvage_wallet, error_string, warning_string); if (!error_string.empty()) InitError(error_string); if (!warning_string.empty()) InitWarning(warning_string); if (!verify_success) return false; @@ -228,7 +228,7 @@ bool WalletInit::Open() const } for (const std::string& walletFile : gArgs.GetArgs("-wallet")) { - std::shared_ptr pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir())); + std::shared_ptr pwallet = CWallet::CreateWalletFromFile(WalletLocation(walletFile)); if (!pwallet) { return false; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 8f92f537a..957e5c6c3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3112,26 +3112,26 @@ static UniValue loadwallet(const JSONRPCRequest& request) + HelpExampleCli("loadwallet", "\"test.dat\"") + HelpExampleRpc("loadwallet", "\"test.dat\"") ); - std::string wallet_file = request.params[0].get_str(); + + WalletLocation location(request.params[0].get_str()); std::string error; - fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); - if (fs::symlink_status(wallet_path).type() == fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + wallet_file + " not found."); - } else if (fs::is_directory(wallet_path)) { + if (!location.Exists()) { + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + location.GetName() + " not found."); + } else if (fs::is_directory(location.GetPath())) { // The given filename is a directory. Check that there's a wallet.dat file. - fs::path wallet_dat_file = wallet_path / "wallet.dat"; + fs::path wallet_dat_file = location.GetPath() / "wallet.dat"; if (fs::symlink_status(wallet_dat_file).type() == fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Directory " + wallet_file + " does not contain a wallet.dat file."); + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Directory " + location.GetName() + " does not contain a wallet.dat file."); } } std::string warning; - if (!CWallet::Verify(wallet_file, false, error, warning)) { + if (!CWallet::Verify(location, false, error, warning)) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error); } - std::shared_ptr const wallet = CWallet::CreateWalletFromFile(wallet_file, fs::absolute(wallet_file, GetWalletDir())); + std::shared_ptr const wallet = CWallet::CreateWalletFromFile(location); if (!wallet) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet loading failed."); } @@ -3165,7 +3165,6 @@ static UniValue createwallet(const JSONRPCRequest& request) + HelpExampleRpc("createwallet", "\"testwallet\"") ); } - std::string wallet_name = request.params[0].get_str(); std::string error; std::string warning; @@ -3174,17 +3173,17 @@ static UniValue createwallet(const JSONRPCRequest& request) disable_privatekeys = request.params[1].get_bool(); } - fs::path wallet_path = fs::absolute(wallet_name, GetWalletDir()); - if (fs::symlink_status(wallet_path).type() != fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_ERROR, "Wallet " + wallet_name + " already exists."); + WalletLocation location(request.params[0].get_str()); + if (location.Exists()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet " + location.GetName() + " already exists."); } // Wallet::Verify will check if we're trying to create a wallet with a duplication name. - if (!CWallet::Verify(wallet_name, false, error, warning)) { + if (!CWallet::Verify(location, false, error, warning)) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error); } - std::shared_ptr const wallet = CWallet::CreateWalletFromFile(wallet_name, fs::absolute(wallet_name, GetWalletDir()), (disable_privatekeys ? (uint64_t)WALLET_FLAG_DISABLE_PRIVATE_KEYS : 0)); + std::shared_ptr const wallet = CWallet::CreateWalletFromFile(location, (disable_privatekeys ? (uint64_t)WALLET_FLAG_DISABLE_PRIVATE_KEYS : 0)); if (!wallet) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed."); } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 0776ffda9..aed5677bd 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -28,7 +28,7 @@ std::vector> wtxn; typedef std::set CoinSet; static std::vector vCoins; -static CWallet testWallet("dummy", WalletDatabase::CreateDummy()); +static CWallet testWallet(WalletLocation(), WalletDatabase::CreateDummy()); static CAmount balance = 0; CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index de59b6034..d42209ab1 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -6,9 +6,10 @@ #include #include +#include WalletTestingSetup::WalletTestingSetup(const std::string& chainName): - TestingSetup(chainName), m_wallet("mock", WalletDatabase::CreateMock()) + TestingSetup(chainName), m_wallet(WalletLocation(), WalletDatabase::CreateMock()) { bool fFirstRun; m_wallet.LoadWallet(fFirstRun); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c474547b7..88eaaf339 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -47,7 +47,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) // Verify ScanForWalletTransactions picks up transactions in both the old // and new block files. { - CWallet wallet("dummy", WalletDatabase::CreateDummy()); + CWallet wallet(WalletLocation(), WalletDatabase::CreateDummy()); AddKey(wallet, coinbaseKey); WalletRescanReserver reserver(&wallet); reserver.reserve(); @@ -62,7 +62,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) // Verify ScanForWalletTransactions only picks transactions in the new block // file. { - CWallet wallet("dummy", WalletDatabase::CreateDummy()); + CWallet wallet(WalletLocation(), WalletDatabase::CreateDummy()); AddKey(wallet, coinbaseKey); WalletRescanReserver reserver(&wallet); reserver.reserve(); @@ -74,7 +74,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) // before the missing block, and success for a key whose creation time is // after. { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); AddWallet(wallet); UniValue keys; keys.setArray(); @@ -135,7 +135,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Import key into wallet and call dumpwallet to create backup file. { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); LOCK(wallet->cs_wallet); wallet->mapKeyMetadata[coinbaseKey.GetPubKey().GetID()].nCreateTime = KEY_TIME; wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); @@ -151,7 +151,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME // were scanned, and no prior blocks were scanned. { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); JSONRPCRequest request; request.params.setArray(); @@ -181,7 +181,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // debit functions. BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) { - CWallet wallet("dummy", WalletDatabase::CreateDummy()); + CWallet wallet(WalletLocation(), WalletDatabase::CreateDummy()); CWalletTx wtx(&wallet, m_coinbase_txns.back()); LOCK2(cs_main, wallet.cs_wallet); wtx.hashBlock = chainActive.Tip()->GetBlockHash(); @@ -274,7 +274,7 @@ public: ListCoinsTestingSetup() { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - wallet = MakeUnique("mock", WalletDatabase::CreateMock()); + wallet = MakeUnique(WalletLocation(), WalletDatabase::CreateMock()); bool firstRun; wallet->LoadWallet(firstRun); AddKey(*wallet, coinbaseKey); @@ -368,7 +368,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); BOOST_CHECK(!wallet->TopUpKeyPool(1000)); CPubKey pubkey; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4bf2ec1e4..fefa1ae02 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include #include @@ -4010,7 +4009,7 @@ void CWallet::MarkPreSplitKeys() } } -bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& error_string, std::string& warning_string) +bool CWallet::Verify(const WalletLocation& location, bool salvage_wallet, std::string& error_string, std::string& warning_string) { // Do some checking on wallet path. It should be either a: // @@ -4019,23 +4018,23 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& // 3. Path to a symlink to a directory. // 4. For backwards compatibility, the name of a data file in -walletdir. LOCK(cs_wallets); - fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); + const fs::path& wallet_path = location.GetPath(); fs::file_type path_type = fs::symlink_status(wallet_path).type(); if (!(path_type == fs::file_not_found || path_type == fs::directory_file || (path_type == fs::symlink_file && fs::is_directory(wallet_path)) || - (path_type == fs::regular_file && fs::path(wallet_file).filename() == wallet_file))) { + (path_type == fs::regular_file && fs::path(location.GetName()).filename() == location.GetName()))) { error_string = strprintf( "Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and " "database/log.?????????? files can be stored, a location where such a directory could be created, " "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)", - wallet_file, GetWalletDir()); + location.GetName(), GetWalletDir()); return false; } // Make sure that the wallet path doesn't clash with an existing wallet path for (auto wallet : GetWallets()) { - if (fs::absolute(wallet->GetName(), GetWalletDir()) == wallet_path) { - error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", wallet_file); + if (wallet->GetLocation().GetPath() == wallet_path) { + error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName()); return false; } } @@ -4045,13 +4044,13 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& return false; } } catch (const fs::filesystem_error& e) { - error_string = strprintf("Error loading wallet %s. %s", wallet_file, e.what()); + error_string = strprintf("Error loading wallet %s. %s", location.GetName(), e.what()); return false; } if (salvage_wallet) { // Recover readable keypairs: - CWallet dummyWallet("dummy", WalletDatabase::CreateDummy()); + CWallet dummyWallet(WalletLocation(), WalletDatabase::CreateDummy()); std::string backup_filename; if (!WalletBatch::Recover(wallet_path, (void *)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) { return false; @@ -4061,9 +4060,9 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& return WalletBatch::VerifyDatabaseFile(wallet_path, warning_string, error_string); } -std::shared_ptr CWallet::CreateWalletFromFile(const std::string& name, const fs::path& path, uint64_t wallet_creation_flags) +std::shared_ptr CWallet::CreateWalletFromFile(const WalletLocation& location, uint64_t wallet_creation_flags) { - const std::string& walletFile = name; + const std::string& walletFile = location.GetName(); // needed to restore wallet transaction meta data after -zapwallettxes std::vector vWtx; @@ -4071,7 +4070,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(const std::string& name, if (gArgs.GetBoolArg("-zapwallettxes", false)) { uiInterface.InitMessage(_("Zapping all transactions from wallet...")); - std::unique_ptr tempWallet = MakeUnique(name, WalletDatabase::Create(path)); + std::unique_ptr tempWallet = MakeUnique(location, WalletDatabase::Create(location.GetPath())); DBErrors nZapWalletRet = tempWallet->ZapWalletTx(vWtx); if (nZapWalletRet != DBErrors::LOAD_OK) { InitError(strprintf(_("Error loading %s: Wallet corrupted"), walletFile)); @@ -4085,7 +4084,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(const std::string& name, bool fFirstRun = true; // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. - std::shared_ptr walletInstance(new CWallet(name, WalletDatabase::Create(path)), ReleaseWallet); + std::shared_ptr walletInstance(new CWallet(location, WalletDatabase::Create(location.GetPath())), ReleaseWallet); DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun); if (nLoadWalletRet != DBErrors::LOAD_OK) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1f2083484..5b7f88350 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -20,7 +20,7 @@ #include #include #include -#include +#include #include #include @@ -755,12 +755,8 @@ private: */ bool AddWatchOnly(const CScript& dest) override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** - * Wallet filename from wallet= command line or config option. - * Used in debug logs and to send RPCs to the right wallet instance when - * more than one wallet is loaded. - */ - std::string m_name; + /** Wallet location which includes wallet name (see WalletLocation). */ + WalletLocation m_location; /** Internal database handle. */ std::unique_ptr database; @@ -800,9 +796,11 @@ public: bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const; + const WalletLocation& GetLocation() const { return m_location; } + /** Get a name for this wallet for logging/debugging purposes. */ - const std::string& GetName() const { return m_name; } + const std::string& GetName() const { return m_location.GetName(); } void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void MarkPreSplitKeys(); @@ -818,7 +816,7 @@ public: unsigned int nMasterKeyMaxID = 0; /** Construct wallet with specified name and database implementation. */ - CWallet(std::string name, std::unique_ptr database) : m_name(std::move(name)), database(std::move(database)) + CWallet(const WalletLocation& location, std::unique_ptr database) : m_location(location), database(std::move(database)) { } @@ -1147,10 +1145,10 @@ public: bool MarkReplaced(const uint256& originalHash, const uint256& newHash); //! Verify wallet naming and perform salvage on the wallet if required - static bool Verify(std::string wallet_file, bool salvage_wallet, std::string& error_string, std::string& warning_string); + static bool Verify(const WalletLocation& location, bool salvage_wallet, std::string& error_string, std::string& warning_string); /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ - static std::shared_ptr CreateWalletFromFile(const std::string& name, const fs::path& path, uint64_t wallet_creation_flags = 0); + static std::shared_ptr CreateWalletFromFile(const WalletLocation& location, uint64_t wallet_creation_flags = 0); /** * Wallet post-init setup diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index b8d105b49..d9a4a131d 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -30,7 +30,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "validation -> validationinterface -> validation" "wallet/coincontrol -> wallet/wallet -> wallet/coincontrol" "wallet/fees -> wallet/wallet -> wallet/fees" - "wallet/rpcwallet -> wallet/wallet -> wallet/rpcwallet" "wallet/wallet -> wallet/walletdb -> wallet/wallet" "policy/fees -> policy/policy -> validation -> policy/fees" "policy/rbf -> txmempool -> validation -> policy/rbf"