From 288b4ffb6b291f0466d513ff3c40af6758ca7c88 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 28 Jul 2020 19:25:14 -0400 Subject: [PATCH 1/8] Remove WalletLocation class This removes a source of complexity and indirection that makes it harder to understand path checking code. Path checks will be simplified in upcoming commits. There is no change in behavior in this commit other than a slightly more descriptive error message in `loadwallet` if the default "" wallet can't be found. (The error message is improved more in upcoming commit "wallet: Remove path checking code from loadwallet RPC".) --- src/bench/coin_selection.cpp | 4 +-- src/bench/wallet_balance.cpp | 2 +- src/interfaces/wallet.cpp | 2 +- src/qt/test/addressbooktests.cpp | 2 +- src/qt/test/wallettests.cpp | 2 +- src/wallet/load.cpp | 9 ++--- src/wallet/rpcwallet.cpp | 15 ++++---- src/wallet/salvage.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 4 +-- src/wallet/test/ismine_tests.cpp | 40 ++++++++++----------- src/wallet/test/scriptpubkeyman_tests.cpp | 2 +- src/wallet/test/wallet_test_fixture.cpp | 2 +- src/wallet/test/wallet_tests.cpp | 22 ++++++------ src/wallet/wallet.cpp | 42 +++++++++++------------ src/wallet/wallet.h | 18 +++++----- src/wallet/wallettool.cpp | 4 +-- src/wallet/walletutil.cpp | 16 --------- src/wallet/walletutil.h | 20 ----------- 18 files changed, 86 insertions(+), 122 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 3a71a6ca0..99aafd8df 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -31,7 +31,7 @@ static void CoinSelection(benchmark::Bench& bench) { NodeContext node; auto chain = interfaces::MakeChain(node); - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); wallet.SetupLegacyScriptPubKeyMan(); std::vector> wtxs; LOCK(wallet.cs_wallet); @@ -65,7 +65,7 @@ static void CoinSelection(benchmark::Bench& bench) typedef std::set CoinSet; static NodeContext testNode; static auto testChain = interfaces::MakeChain(testNode); -static CWallet testWallet(testChain.get(), WalletLocation(), CreateDummyWalletDatabase()); +static CWallet testWallet(testChain.get(), "", CreateDummyWalletDatabase()); std::vector> wtxn; // Copied from src/wallet/test/coinselector_tests.cpp diff --git a/src/bench/wallet_balance.cpp b/src/bench/wallet_balance.cpp index e16182b48..b3b73284d 100644 --- a/src/bench/wallet_balance.cpp +++ b/src/bench/wallet_balance.cpp @@ -26,7 +26,7 @@ static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const b NodeContext node; std::unique_ptr chain = interfaces::MakeChain(node); - CWallet wallet{chain.get(), WalletLocation(), CreateMockWalletDatabase()}; + CWallet wallet{chain.get(), "", CreateMockWalletDatabase()}; { wallet.SetupLegacyScriptPubKeyMan(); bool first_run; diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 28839b2ff..d17110a0f 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -522,7 +522,7 @@ public: } std::unique_ptr loadWallet(const std::string& name, bilingual_str& error, std::vector& warnings) override { - return MakeWallet(LoadWallet(*m_context.chain, WalletLocation(name), true /* load_on_start */, error, warnings)); + return MakeWallet(LoadWallet(*m_context.chain, name, true /* load_on_start */, error, warnings)); } std::string getWalletDir() override { diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index d6781460a..35fcb2b0c 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -61,7 +61,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) { TestChain100Setup test; node.setContext(&test.m_node); - std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), WalletLocation(), CreateMockWalletDatabase()); + std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); bool firstRun; wallet->LoadWallet(firstRun); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 98065803e..d6d2d0e3d 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -139,7 +139,7 @@ void TestGUI(interfaces::Node& node) test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey())); } node.setContext(&test.m_node); - std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), WalletLocation(), CreateMockWalletDatabase()); + std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); bool firstRun; wallet->LoadWallet(firstRun); { diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 5bbc8b91f..dde29842e 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -5,6 +5,7 @@ #include +#include #include #include #include @@ -44,16 +45,16 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector& wal std::set wallet_paths; for (const auto& wallet_file : wallet_files) { - WalletLocation location(wallet_file); + const fs::path path = fs::absolute(wallet_file, GetWalletDir()); - if (!wallet_paths.insert(location.GetPath()).second) { + if (!wallet_paths.insert(path).second) { chain.initError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), wallet_file)); return false; } bilingual_str error_string; std::vector warnings; - bool verify_success = CWallet::Verify(chain, location, error_string, warnings); + bool verify_success = CWallet::Verify(chain, wallet_file, error_string, warnings); if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); if (!verify_success) { chain.initError(error_string); @@ -70,7 +71,7 @@ bool LoadWallets(interfaces::Chain& chain, const std::vector& walle for (const std::string& walletFile : wallet_files) { bilingual_str error; std::vector warnings; - std::shared_ptr pwallet = CWallet::CreateWalletFromFile(chain, WalletLocation(walletFile), error, warnings); + std::shared_ptr pwallet = CWallet::CreateWalletFromFile(chain, walletFile, error, warnings); if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); if (!pwallet) { chain.initError(error); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 887c5b632..312b34551 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2502,22 +2502,23 @@ static UniValue loadwallet(const JSONRPCRequest& request) }.Check(request); WalletContext& context = EnsureWalletContext(request.context); - WalletLocation location(request.params[0].get_str()); + const std::string name(request.params[0].get_str()); + fs::path path(fs::absolute(name, GetWalletDir())); - if (!location.Exists()) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + location.GetName() + " not found."); - } else if (fs::is_directory(location.GetPath())) { + if (fs::symlink_status(path).type() == fs::file_not_found) { + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + name + " not found."); + } else if (fs::is_directory(path)) { // The given filename is a directory. Check that there's a wallet.dat file. - fs::path wallet_dat_file = location.GetPath() / "wallet.dat"; + fs::path wallet_dat_file = path / "wallet.dat"; if (fs::symlink_status(wallet_dat_file).type() == fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Directory " + location.GetName() + " does not contain a wallet.dat file."); + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Directory " + name + " does not contain a wallet.dat file."); } } bilingual_str error; std::vector warnings; Optional load_on_start = request.params[1].isNull() ? nullopt : Optional(request.params[1].get_bool()); - std::shared_ptr const wallet = LoadWallet(*context.chain, location, load_on_start, error, warnings); + std::shared_ptr const wallet = LoadWallet(*context.chain, name, load_on_start, error, warnings); if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original); UniValue obj(UniValue::VOBJ); diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 934e3d5c8..96fba2133 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -123,7 +123,7 @@ bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::v } DbTxn* ptxn = env->TxnBegin(); - CWallet dummyWallet(nullptr, WalletLocation(), CreateDummyWalletDatabase()); + CWallet dummyWallet(nullptr, "", CreateDummyWalletDatabase()); for (KeyValPair& row : salvagedData) { /* Filter for only private key type KV pairs to be added to the salvaged wallet */ diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 1deedede4..f38ccba38 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -29,7 +29,7 @@ typedef std::set CoinSet; static std::vector vCoins; static NodeContext testNode; static auto testChain = interfaces::MakeChain(testNode); -static CWallet testWallet(testChain.get(), WalletLocation(), CreateDummyWalletDatabase()); +static CWallet testWallet(testChain.get(), "", CreateDummyWalletDatabase()); static CAmount balance = 0; CoinEligibilityFilter filter_standard(1, 6, 0); @@ -283,7 +283,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // Make sure that can use BnB when there are preset inputs empty_wallet(); { - std::unique_ptr wallet = MakeUnique(m_chain.get(), WalletLocation(), CreateMockWalletDatabase()); + std::unique_ptr wallet = MakeUnique(m_chain.get(), "", CreateMockWalletDatabase()); bool firstRun; wallet->LoadWallet(firstRun); wallet->SetupLegacyScriptPubKeyMan(); diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp index e416f1604..d5aed99d9 100644 --- a/src/wallet/test/ismine_tests.cpp +++ b/src/wallet/test/ismine_tests.cpp @@ -35,7 +35,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK compressed { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForRawPubKey(pubkeys[0]); @@ -52,7 +52,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PK uncompressed { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForRawPubKey(uncompressedPubkey); @@ -69,7 +69,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH compressed { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForDestination(PKHash(pubkeys[0])); @@ -86,7 +86,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2PKH uncompressed { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); scriptPubKey = GetScriptForDestination(PKHash(uncompressedPubkey)); @@ -103,7 +103,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -127,7 +127,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // (P2PKH inside) P2SH inside P2SH (invalid) { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -145,7 +145,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // (P2PKH inside) P2SH inside P2WSH (invalid) { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -163,7 +163,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2WPKH inside P2WSH (invalid) { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -179,7 +179,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // (P2PKH inside) P2WSH inside P2WSH (invalid) { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -197,7 +197,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2WPKH compressed { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); @@ -212,7 +212,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2WPKH uncompressed { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(uncompressedKey)); @@ -231,7 +231,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // scriptPubKey multisig { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -262,7 +262,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2SH multisig { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(uncompressedKey)); @@ -283,7 +283,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2WSH multisig with compressed keys { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); @@ -309,7 +309,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2WSH multisig with uncompressed key { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(uncompressedKey)); @@ -335,7 +335,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // P2WSH multisig wrapped in P2SH { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); @@ -362,7 +362,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // OP_RETURN { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); @@ -376,7 +376,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // witness unspendable { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); @@ -390,7 +390,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // witness unknown { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); @@ -404,7 +404,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) // Nonstandard { - CWallet keystore(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet keystore(chain.get(), "", CreateDummyWalletDatabase()); keystore.SetupLegacyScriptPubKeyMan(); LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore); BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 4f1207976..f7c1337b0 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -19,7 +19,7 @@ BOOST_AUTO_TEST_CASE(CanProvide) // Set up wallet and keyman variables. NodeContext node; std::unique_ptr chain = interfaces::MakeChain(node); - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); LegacyScriptPubKeyMan& keyman = *wallet.GetOrCreateLegacyScriptPubKeyMan(); // Make a 1 of 2 multisig script diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index 924147074..4d6f42761 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -6,7 +6,7 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName) : TestingSetup(chainName), - m_wallet(m_chain.get(), WalletLocation(), CreateMockWalletDatabase()) + m_wallet(m_chain.get(), "", CreateMockWalletDatabase()) { bool fFirstRun; m_wallet.LoadWallet(fFirstRun); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index f400e7e94..b3001efd5 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -39,7 +39,7 @@ static std::shared_ptr TestLoadWallet(interfaces::Chain& chain) { bilingual_str error; std::vector warnings; - auto wallet = CWallet::CreateWalletFromFile(chain, WalletLocation(""), error, warnings); + auto wallet = CWallet::CreateWalletFromFile(chain, "", error, warnings); wallet->postInitProcess(); return wallet; } @@ -85,7 +85,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions fails to read an unknown start block. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -104,7 +104,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions picks up transactions in both the old // and new block files. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -130,7 +130,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions only picks transactions in the new block // file. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -155,7 +155,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) // Verify ScanForWalletTransactions scans no blocks. { - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); { LOCK(wallet.cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -194,7 +194,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) // before the missing block, and success for a key whose creation time is // after. { - std::shared_ptr wallet = std::make_shared(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(chain.get(), "", CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(newTip->nHeight, newTip->GetBlockHash())); AddWallet(wallet); @@ -259,7 +259,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(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(chain.get(), "", CreateDummyWalletDatabase()); { auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan(); LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); @@ -281,7 +281,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(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(chain.get(), "", CreateDummyWalletDatabase()); LOCK(wallet->cs_wallet); wallet->SetupLegacyScriptPubKeyMan(); @@ -317,7 +317,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) NodeContext node; auto chain = interfaces::MakeChain(node); - CWallet wallet(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + CWallet wallet(chain.get(), "", CreateDummyWalletDatabase()); auto spk_man = wallet.GetOrCreateLegacyScriptPubKeyMan(); CWalletTx wtx(&wallet, m_coinbase_txns.back()); @@ -492,7 +492,7 @@ public: ListCoinsTestingSetup() { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - wallet = MakeUnique(m_chain.get(), WalletLocation(), CreateMockWalletDatabase()); + wallet = MakeUnique(m_chain.get(), "", CreateMockWalletDatabase()); { LOCK2(wallet->cs_wallet, ::cs_main); wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -610,7 +610,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { NodeContext node; auto chain = interfaces::MakeChain(node); - std::shared_ptr wallet = std::make_shared(chain.get(), WalletLocation(), CreateDummyWalletDatabase()); + std::shared_ptr wallet = std::make_shared(chain.get(), "", CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); wallet->SetMinVersion(FEATURE_LATEST); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index afe676078..43dbd4826 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -200,15 +200,15 @@ void UnloadWallet(std::shared_ptr&& wallet) } namespace { -std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const WalletLocation& location, Optional load_on_start, bilingual_str& error, std::vector& warnings) +std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings) { try { - if (!CWallet::Verify(chain, location, error, warnings)) { + if (!CWallet::Verify(chain, name, error, warnings)) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; return nullptr; } - std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, location, error, warnings); + std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings); if (!wallet) { error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error; return nullptr; @@ -217,7 +217,7 @@ std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const Wall wallet->postInitProcess(); // Write the wallet setting - UpdateWalletSetting(chain, location.GetName(), load_on_start, warnings); + UpdateWalletSetting(chain, name, load_on_start, warnings); return wallet; } catch (const std::runtime_error& e) { @@ -227,14 +227,14 @@ std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const Wall } } // namespace -std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, Optional load_on_start, bilingual_str& error, std::vector& warnings) +std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings) { - auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(location.GetName())); + auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(name)); if (!result.second) { error = Untranslated("Wallet already being loading."); return nullptr; } - auto wallet = LoadWalletInternal(chain, location, load_on_start, error, warnings); + auto wallet = LoadWalletInternal(chain, name, load_on_start, error, warnings); WITH_LOCK(g_loading_wallet_mutex, g_loading_wallet_set.erase(result.first)); return wallet; } @@ -250,14 +250,13 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& } // Check the wallet file location - WalletLocation location(name); - if (location.Exists()) { - error = strprintf(Untranslated("Wallet %s already exists."), location.GetName()); + if (fs::symlink_status(fs::absolute(name.empty() ? "wallet.dat" : name, GetWalletDir())).type() != fs::file_not_found) { + error = strprintf(Untranslated("Wallet %s already exists."), name); return WalletCreationStatus::CREATION_FAILED; } // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. - if (!CWallet::Verify(chain, location, error, warnings)) { + if (!CWallet::Verify(chain, name, error, warnings)) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; return WalletCreationStatus::CREATION_FAILED; } @@ -269,7 +268,7 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& } // Make the wallet - std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, location, error, warnings, wallet_creation_flags); + std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings, wallet_creation_flags); if (!wallet) { error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; return WalletCreationStatus::CREATION_FAILED; @@ -3770,7 +3769,7 @@ std::vector CWallet::GetDestValues(const std::string& prefix) const return values; } -bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error_string, std::vector& warnings) +bool CWallet::Verify(interfaces::Chain& chain, const std::string& name, bilingual_str& error_string, std::vector& warnings) { // Do some checking on wallet path. It should be either a: // @@ -3779,22 +3778,22 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b // 3. Path to a symlink to a directory. // 4. For backwards compatibility, the name of a data file in -walletdir. LOCK(cs_wallets); - const fs::path& wallet_path = location.GetPath(); + const fs::path& wallet_path = fs::absolute(name, GetWalletDir()); 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(location.GetName()).filename() == location.GetName()))) { + (path_type == fs::regular_file && fs::path(name).filename() == name))) { error_string = Untranslated(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)", - location.GetName(), GetWalletDir())); + name, GetWalletDir())); return false; } // Make sure that the wallet path doesn't clash with an existing wallet path if (IsWalletLoaded(wallet_path)) { - error_string = Untranslated(strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName())); + error_string = Untranslated(strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", name)); return false; } @@ -3804,14 +3803,15 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b try { return database->Verify(error_string); } catch (const fs::filesystem_error& e) { - error_string = Untranslated(strprintf("Error loading wallet %s. %s", location.GetName(), fsbridge::get_filesystem_error_message(e))); + error_string = Untranslated(strprintf("Error loading wallet %s. %s", name, fsbridge::get_filesystem_error_message(e))); return false; } } -std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags) +std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags) { - const std::string walletFile = WalletDataFilePath(location.GetPath()).string(); + fs::path path = fs::absolute(name, GetWalletDir()); + const std::string walletFile = WalletDataFilePath(path).string(); chain.initMessage(_("Loading wallet...").translated); @@ -3819,7 +3819,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, 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(&chain, location, CreateWalletDatabase(location.GetPath())), ReleaseWallet); + std::shared_ptr walletInstance(new CWallet(&chain, name, CreateWalletDatabase(path)), ReleaseWallet); DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun); if (nLoadWalletRet != DBErrors::LOAD_OK) { if (nLoadWalletRet == DBErrors::CORRUPT) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 06069029e..9c04873c5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -54,7 +54,7 @@ bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on_start); std::vector> GetWallets(); std::shared_ptr GetWallet(const std::string& name); -std::shared_ptr LoadWallet(interfaces::Chain& chain, const WalletLocation& location, Optional load_on_start, bilingual_str& error, std::vector& warnings); +std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings); std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet); enum class WalletCreationStatus { @@ -700,8 +700,8 @@ private: /** Interface for accessing chain state. */ interfaces::Chain* m_chain; - /** Wallet location which includes wallet name (see WalletLocation). */ - WalletLocation m_location; + /** Wallet name: relative directory name or "" for default wallet. */ + std::string m_name; /** Internal database handle. */ std::unique_ptr database; @@ -755,20 +755,18 @@ 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 EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - const WalletLocation& GetLocation() const { return m_location; } - /** Get a name for this wallet for logging/debugging purposes. */ - const std::string& GetName() const { return m_location.GetName(); } + const std::string& GetName() const { return m_name; } typedef std::map MasterKeyMap; MasterKeyMap mapMasterKeys; unsigned int nMasterKeyMaxID = 0; /** Construct wallet with specified name and database implementation. */ - CWallet(interfaces::Chain* chain, const WalletLocation& location, std::unique_ptr database) + CWallet(interfaces::Chain* chain, const std::string& name, std::unique_ptr database) : m_chain(chain), - m_location(location), + m_name(name), database(std::move(database)) { } @@ -1154,10 +1152,10 @@ public: bool MarkReplaced(const uint256& originalHash, const uint256& newHash); //! Verify wallet naming and perform salvage on the wallet if required - static bool Verify(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error_string, std::vector& warnings); + static bool Verify(interfaces::Chain& chain, const std::string& name, bilingual_str& error_string, std::vector& warnings); /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ - static std::shared_ptr CreateWalletFromFile(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags = 0); + static std::shared_ptr CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags = 0); /** * Wallet post-init setup diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 9b5146184..abd5652fe 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -28,7 +28,7 @@ static std::shared_ptr CreateWallet(const std::string& name, const fs:: return nullptr; } // dummy chain interface - std::shared_ptr wallet_instance(new CWallet(nullptr /* chain */, WalletLocation(name), CreateWalletDatabase(path)), WalletToolReleaseWallet); + std::shared_ptr wallet_instance(new CWallet(nullptr /* chain */, name, CreateWalletDatabase(path)), WalletToolReleaseWallet); LOCK(wallet_instance->cs_wallet); bool first_run = true; DBErrors load_wallet_ret = wallet_instance->LoadWallet(first_run); @@ -57,7 +57,7 @@ static std::shared_ptr LoadWallet(const std::string& name, const fs::pa } // dummy chain interface - std::shared_ptr wallet_instance(new CWallet(nullptr /* chain */, WalletLocation(name), CreateWalletDatabase(path)), WalletToolReleaseWallet); + std::shared_ptr wallet_instance(new CWallet(nullptr /* chain */, name, CreateWalletDatabase(path)), WalletToolReleaseWallet); DBErrors load_wallet_ret; try { bool first_run; diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp index 8bac0608a..d6a8ee9e0 100644 --- a/src/wallet/walletutil.cpp +++ b/src/wallet/walletutil.cpp @@ -91,19 +91,3 @@ std::vector ListWalletDir() return paths; } - -WalletLocation::WalletLocation(const std::string& name) - : m_name(name) - , m_path(fs::absolute(name, GetWalletDir())) -{ -} - -bool WalletLocation::Exists() const -{ - fs::path path = m_path; - // For the default wallet, check specifically for the wallet.dat file - if (m_name.empty()) { - path = fs::absolute("wallet.dat", m_path); - } - return fs::symlink_status(path).type() != fs::file_not_found; -} diff --git a/src/wallet/walletutil.h b/src/wallet/walletutil.h index a4e4fda8a..afdcb2e18 100644 --- a/src/wallet/walletutil.h +++ b/src/wallet/walletutil.h @@ -67,26 +67,6 @@ fs::path GetWalletDir(); //! Get wallets in wallet directory. std::vector ListWalletDir(); -//! The WalletLocation class provides wallet information. -class WalletLocation final -{ - std::string m_name; - fs::path m_path; - -public: - explicit WalletLocation() {} - explicit WalletLocation(const std::string& name); - - //! Get wallet name. - const std::string& GetName() const { return m_name; } - - //! Get wallet absolute path. - const fs::path& GetPath() const { return m_path; } - - //! Return whether the wallet exists. - bool Exists() const; -}; - /** Descriptor with some wallet metadata */ class WalletDescriptor { From b5b414151af32e5a07b5757b64482d77519d77c0 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 16:40:31 -0400 Subject: [PATCH 2/8] wallet: Add MakeDatabase function New function is not currently called but will be called in upcoming commits. It moves database path checking, and existence checking, and already-loaded checking, and verification into a single function so this logic does not need to be repeated all over higher level wallet code, and so higher level code does not need to change when SQLite support is added in https://github.com/bitcoin/bitcoin/pull/19077. This also lets higher level wallet code make fewer assumptions about the contents of wallet directories. This commit just adds the new function and does not change behavior in any way. --- src/wallet/bdb.cpp | 32 ++++++++++++++++++++++++++++++++ src/wallet/bdb.h | 9 +++++++++ src/wallet/db.h | 22 ++++++++++++++++++++++ src/wallet/walletdb.cpp | 39 +++++++++++++++++++++++++++++++++++++++ src/wallet/walletutil.cpp | 2 +- 5 files changed, 103 insertions(+), 1 deletion(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 24eb2ee34..61463aaf5 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -824,3 +824,35 @@ std::unique_ptr BerkeleyDatabase::MakeBatch(const char* mode, boo { return MakeUnique(*this, mode, flush_on_close); } + +bool ExistsBerkeleyDatabase(const fs::path& path) +{ + fs::path env_directory; + std::string data_filename; + SplitWalletPath(path, env_directory, data_filename); + return IsBerkeleyBtree(env_directory / data_filename); +} + +std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +{ + std::unique_ptr db; + { + LOCK(cs_db); // Lock env.m_databases until insert in BerkeleyDatabase constructor + std::string data_filename; + std::shared_ptr env = GetWalletEnv(path, data_filename); + if (env->m_databases.count(data_filename)) { + error = Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", (env->Directory() / data_filename).string())); + status = DatabaseStatus::FAILED_ALREADY_LOADED; + return nullptr; + } + db = MakeUnique(std::move(env), std::move(data_filename)); + } + + if (options.verify && !db->Verify(error)) { + status = DatabaseStatus::FAILED_VERIFY; + return nullptr; + } + + status = DatabaseStatus::SUCCESS; + return db; +} diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 75546924e..82ad13664 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -90,6 +90,9 @@ std::shared_ptr GetWalletEnv(const fs::path& wallet_path, s /** Return whether a BDB wallet database is currently loaded. */ bool IsBDBWalletLoaded(const fs::path& wallet_path); +/** Check format of database file */ +bool IsBerkeleyBtree(const fs::path& path); + class BerkeleyBatch; /** An instance of this class represents one database. @@ -224,4 +227,10 @@ public: std::string BerkeleyDatabaseVersion(); +//! Check if Berkeley database exists at specified path. +bool ExistsBerkeleyDatabase(const fs::path& path); + +//! Return object giving access to Berkeley database at specified path. +std::unique_ptr MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); + #endif // BITCOIN_WALLET_BDB_H diff --git a/src/wallet/db.h b/src/wallet/db.h index 0afaba5fd..f0f6f03c4 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -195,4 +195,26 @@ public: std::unique_ptr MakeBatch(const char* mode = "r+", bool flush_on_close = true) override { return MakeUnique(); } }; +enum class DatabaseFormat { + BERKELEY, +}; + +struct DatabaseOptions { + bool require_existing = false; + bool require_create = false; + bool verify = true; +}; + +enum class DatabaseStatus { + SUCCESS, + FAILED_BAD_PATH, + FAILED_BAD_FORMAT, + FAILED_ALREADY_LOADED, + FAILED_ALREADY_EXISTS, + FAILED_NOT_FOUND, + FAILED_VERIFY, +}; + +std::unique_ptr MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); + #endif // BITCOIN_WALLET_DB_H diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 962ea66fa..23c4b6977 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -13,6 +13,8 @@ #include #include #include +#include +#include #include #include @@ -993,6 +995,43 @@ bool WalletBatch::TxnAbort() return m_batch->TxnAbort(); } +std::unique_ptr MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error) +{ + bool exists; + try { + exists = fs::symlink_status(path).type() != fs::file_not_found; + } catch (const fs::filesystem_error& e) { + error = Untranslated(strprintf("Failed to access database path '%s': %s", path.string(), fsbridge::get_filesystem_error_message(e))); + status = DatabaseStatus::FAILED_BAD_PATH; + return nullptr; + } + + Optional format; + if (exists) { + if (ExistsBerkeleyDatabase(path)) { + format = DatabaseFormat::BERKELEY; + } + } else if (options.require_existing) { + error = Untranslated(strprintf("Failed to load database path '%s'. Path does not exist.", path.string())); + status = DatabaseStatus::FAILED_NOT_FOUND; + return nullptr; + } + + if (!format && options.require_existing) { + error = Untranslated(strprintf("Failed to load database path '%s'. Data is not in recognized format.", path.string())); + status = DatabaseStatus::FAILED_BAD_FORMAT; + return nullptr; + } + + if (format && options.require_create) { + error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", path.string())); + status = DatabaseStatus::FAILED_ALREADY_EXISTS; + return nullptr; + } + + return MakeBerkeleyDatabase(path, options, status, error); +} + bool IsWalletLoaded(const fs::path& wallet_path) { return IsBDBWalletLoaded(wallet_path); diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp index d6a8ee9e0..e4c72aed9 100644 --- a/src/wallet/walletutil.cpp +++ b/src/wallet/walletutil.cpp @@ -29,7 +29,7 @@ fs::path GetWalletDir() return path; } -static bool IsBerkeleyBtree(const fs::path& path) +bool IsBerkeleyBtree(const fs::path& path) { if (!fs::exists(path)) return false; From 0d94e6062547f288a75921d2433458a44a5f2297 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 17:55:13 -0400 Subject: [PATCH 3/8] refactor: Use DatabaseStatus and DatabaseOptions types No changes in behavior. Just replaces arguments and return types --- src/interfaces/wallet.cpp | 15 +++++++++++---- src/interfaces/wallet.h | 3 +-- src/qt/walletcontroller.cpp | 5 ++--- src/wallet/db.h | 5 +++++ src/wallet/rpcwallet.cpp | 22 +++++++++++----------- src/wallet/wallet.cpp | 36 +++++++++++++++++++++++------------- src/wallet/wallet.h | 11 ++--------- 7 files changed, 55 insertions(+), 42 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index d17110a0f..d19d0406b 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -514,15 +514,22 @@ public: void setMockTime(int64_t time) override { return SetMockTime(time); } //! WalletClient methods - std::unique_ptr createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, WalletCreationStatus& status, bilingual_str& error, std::vector& warnings) override + std::unique_ptr createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) override { std::shared_ptr wallet; - status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, true /* load_on_start */, error, warnings, wallet); - return MakeWallet(std::move(wallet)); + DatabaseOptions options; + DatabaseStatus status; + options.require_create = true; + options.create_flags = wallet_creation_flags; + options.create_passphrase = passphrase; + return MakeWallet(CreateWallet(*m_context.chain, name, true /* load_on_start */, options, status, error, warnings)); } std::unique_ptr loadWallet(const std::string& name, bilingual_str& error, std::vector& warnings) override { - return MakeWallet(LoadWallet(*m_context.chain, name, true /* load_on_start */, error, warnings)); + DatabaseOptions options; + DatabaseStatus status; + options.require_existing = true; + return MakeWallet(LoadWallet(*m_context.chain, name, true /* load_on_start */, options, status, error, warnings)); } std::string getWalletDir() override { diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 186f5d81a..b1afbbfd7 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -29,7 +29,6 @@ class CWallet; enum class FeeReason; enum class OutputType; enum class TransactionError; -enum class WalletCreationStatus; enum isminetype : unsigned int; struct CRecipient; struct PartiallySignedTransaction; @@ -311,7 +310,7 @@ class WalletClient : public ChainClient { public: //! Create new wallet. - virtual std::unique_ptr createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, WalletCreationStatus& status, bilingual_str& error, std::vector& warnings) = 0; + virtual std::unique_ptr createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) = 0; //! Load existing wallet. virtual std::unique_ptr loadWallet(const std::string& name, bilingual_str& error, std::vector& warnings) = 0; diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index 828f84ffc..bee17abf1 100644 --- a/src/qt/walletcontroller.cpp +++ b/src/qt/walletcontroller.cpp @@ -249,10 +249,9 @@ void CreateWalletActivity::createWallet() } QTimer::singleShot(500, worker(), [this, name, flags] { - WalletCreationStatus status; - std::unique_ptr wallet = node().walletClient().createWallet(name, m_passphrase, flags, status, m_error_message, m_warning_message); + std::unique_ptr wallet = node().walletClient().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message); - if (status == WalletCreationStatus::SUCCESS) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet)); + if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet)); QTimer::singleShot(500, this, &CreateWalletActivity::finish); }); diff --git a/src/wallet/db.h b/src/wallet/db.h index f0f6f03c4..6a918ec92 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -202,6 +203,8 @@ enum class DatabaseFormat { struct DatabaseOptions { bool require_existing = false; bool require_create = false; + uint64_t create_flags = 0; + SecureString create_passphrase; bool verify = true; }; @@ -212,7 +215,9 @@ enum class DatabaseStatus { FAILED_ALREADY_LOADED, FAILED_ALREADY_EXISTS, FAILED_NOT_FOUND, + FAILED_CREATE, FAILED_VERIFY, + FAILED_ENCRYPT, }; std::unique_ptr MakeDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 312b34551..536d11ddd 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2515,10 +2515,12 @@ static UniValue loadwallet(const JSONRPCRequest& request) } } + DatabaseOptions options; + DatabaseStatus status; bilingual_str error; std::vector warnings; Optional load_on_start = request.params[1].isNull() ? nullopt : Optional(request.params[1].get_bool()); - std::shared_ptr const wallet = LoadWallet(*context.chain, name, load_on_start, error, warnings); + std::shared_ptr const wallet = LoadWallet(*context.chain, name, load_on_start, options, status, error, warnings); if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original); UniValue obj(UniValue::VOBJ); @@ -2648,18 +2650,16 @@ static UniValue createwallet(const JSONRPCRequest& request) warnings.emplace_back(Untranslated("Wallet is an experimental descriptor wallet")); } + DatabaseOptions options; + DatabaseStatus status; + options.create_flags = flags; + options.create_passphrase = passphrase; bilingual_str error; - std::shared_ptr wallet; Optional load_on_start = request.params[6].isNull() ? nullopt : Optional(request.params[6].get_bool()); - WalletCreationStatus status = CreateWallet(*context.chain, passphrase, flags, request.params[0].get_str(), load_on_start, error, warnings, wallet); - switch (status) { - case WalletCreationStatus::CREATION_FAILED: - throw JSONRPCError(RPC_WALLET_ERROR, error.original); - case WalletCreationStatus::ENCRYPTION_FAILED: - throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, error.original); - case WalletCreationStatus::SUCCESS: - break; - // no default case, so the compiler can warn about missing cases + std::shared_ptr wallet = CreateWallet(*context.chain, request.params[0].get_str(), load_on_start, options, status, error, warnings); + if (!wallet) { + RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR; + throw JSONRPCError(code, error.original); } UniValue obj(UniValue::VOBJ); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 43dbd4826..4d7c51401 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -200,7 +200,7 @@ void UnloadWallet(std::shared_ptr&& wallet) } namespace { -std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings) +std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) { try { if (!CWallet::Verify(chain, name, error, warnings)) { @@ -227,20 +227,23 @@ std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const std: } } // namespace -std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings) +std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) { auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(name)); if (!result.second) { error = Untranslated("Wallet already being loading."); return nullptr; } - auto wallet = LoadWalletInternal(chain, name, load_on_start, error, warnings); + auto wallet = LoadWalletInternal(chain, name, load_on_start, options, status, error, warnings); WITH_LOCK(g_loading_wallet_mutex, g_loading_wallet_set.erase(result.first)); return wallet; } -WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings, std::shared_ptr& result) +std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) { + uint64_t wallet_creation_flags = options.create_flags; + const SecureString& passphrase = options.create_passphrase; + // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET); @@ -252,39 +255,45 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& // Check the wallet file location if (fs::symlink_status(fs::absolute(name.empty() ? "wallet.dat" : name, GetWalletDir())).type() != fs::file_not_found) { error = strprintf(Untranslated("Wallet %s already exists."), name); - return WalletCreationStatus::CREATION_FAILED; + status = DatabaseStatus::FAILED_CREATE; + return nullptr; } // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. if (!CWallet::Verify(chain, name, error, warnings)) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; - return WalletCreationStatus::CREATION_FAILED; + status = DatabaseStatus::FAILED_VERIFY; + return nullptr; } // Do not allow a passphrase when private keys are disabled if (!passphrase.empty() && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { error = Untranslated("Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled."); - return WalletCreationStatus::CREATION_FAILED; + status = DatabaseStatus::FAILED_CREATE; + return nullptr; } // Make the wallet std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings, wallet_creation_flags); if (!wallet) { error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; - return WalletCreationStatus::CREATION_FAILED; + status = DatabaseStatus::FAILED_CREATE; + return nullptr; } // Encrypt the wallet if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { if (!wallet->EncryptWallet(passphrase)) { error = Untranslated("Error: Wallet created but failed to encrypt."); - return WalletCreationStatus::ENCRYPTION_FAILED; + status = DatabaseStatus::FAILED_ENCRYPT; + return nullptr; } if (!create_blank) { // Unlock the wallet if (!wallet->Unlock(passphrase)) { error = Untranslated("Error: Wallet was encrypted but could not be unlocked"); - return WalletCreationStatus::ENCRYPTION_FAILED; + status = DatabaseStatus::FAILED_ENCRYPT; + return nullptr; } // Set a seed for the wallet @@ -296,7 +305,8 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& for (auto spk_man : wallet->GetActiveScriptPubKeyMans()) { if (!spk_man->SetupGeneration()) { error = Untranslated("Unable to generate initial keys"); - return WalletCreationStatus::CREATION_FAILED; + status = DatabaseStatus::FAILED_CREATE; + return nullptr; } } } @@ -308,12 +318,12 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& } AddWallet(wallet); wallet->postInitProcess(); - result = wallet; // Write the wallet settings UpdateWalletSetting(chain, name, load_on_start, warnings); - return WalletCreationStatus::SUCCESS; + status = DatabaseStatus::SUCCESS; + return wallet; } const uint256 CWalletTx::ABANDON_HASH(UINT256_ONE()); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9c04873c5..2e6434f5c 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -54,17 +54,10 @@ bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on bool RemoveWallet(const std::shared_ptr& wallet, Optional load_on_start); std::vector> GetWallets(); std::shared_ptr GetWallet(const std::string& name); -std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings); +std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); +std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet); -enum class WalletCreationStatus { - SUCCESS, - CREATION_FAILED, - ENCRYPTION_FAILED -}; - -WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, Optional load_on_start, bilingual_str& error, std::vector& warnings, std::shared_ptr& result); - //! -paytxfee default constexpr CAmount DEFAULT_PAY_TX_FEE = 0; //! -fallbackfee default From 3c815cfe54087fd139169161d2fd175e99840e6a Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 19:33:37 -0400 Subject: [PATCH 4/8] wallet: Remove Verify and IsLoaded methods Checks are now consolidated in MakeBerkeleyDatabase function instead of happening in higher level code. This commit does not change behavior except for error messages which now include more complete information. --- src/wallet/bdb.cpp | 12 ------------ src/wallet/bdb.h | 6 +----- src/wallet/db.h | 4 ---- src/wallet/load.cpp | 8 ++++---- src/wallet/wallet.cpp | 27 ++++++--------------------- src/wallet/wallet.h | 4 +--- src/wallet/walletdb.cpp | 5 ----- src/wallet/walletdb.h | 3 --- test/functional/wallet_multiwallet.py | 6 ++++-- 9 files changed, 16 insertions(+), 59 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 61463aaf5..8f8652bb0 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -52,18 +52,6 @@ bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const return memcmp(value, &rhs.value, sizeof(value)) == 0; } -bool IsBDBWalletLoaded(const fs::path& wallet_path) -{ - fs::path env_directory; - std::string database_filename; - SplitWalletPath(wallet_path, env_directory, database_filename); - LOCK(cs_db); - auto env = g_dbenvs.find(env_directory.string()); - if (env == g_dbenvs.end()) return false; - auto database = env->second.lock(); - return database && database->IsDatabaseLoaded(database_filename); -} - /** * @param[in] wallet_path Path to wallet directory. Or (for backwards compatibility only) a path to a berkeley btree data file inside a wallet directory. * @param[out] database_filename Filename of berkeley btree data file inside the wallet directory. diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 82ad13664..9f24a2f10 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -63,7 +63,6 @@ public: bool IsMock() const { return fMockDb; } bool IsInitialized() const { return fDbEnvInit; } - bool IsDatabaseLoaded(const std::string& db_filename) const { return m_databases.find(db_filename) != m_databases.end(); } fs::path Directory() const { return strPath; } bool Open(bilingual_str& error); @@ -87,9 +86,6 @@ public: /** Get BerkeleyEnvironment and database filename given a wallet path. */ std::shared_ptr GetWalletEnv(const fs::path& wallet_path, std::string& database_filename); -/** Return whether a BDB wallet database is currently loaded. */ -bool IsBDBWalletLoaded(const fs::path& wallet_path); - /** Check format of database file */ bool IsBerkeleyBtree(const fs::path& path); @@ -146,7 +142,7 @@ public: void ReloadDbEnv() override; /** Verifies the environment and database file */ - bool Verify(bilingual_str& error) override; + bool Verify(bilingual_str& error); /** * Pointer to shared database environment. diff --git a/src/wallet/db.h b/src/wallet/db.h index 6a918ec92..6e11d7de8 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -147,9 +147,6 @@ public: unsigned int nLastFlushed; int64_t nLastWalletUpdate; - /** Verifies the environment and database file */ - virtual bool Verify(bilingual_str& error) = 0; - std::string m_file_path; /** Make a DatabaseBatch connected to this database */ @@ -192,7 +189,6 @@ public: bool PeriodicFlush() override { return true; } void IncrementUpdateCounter() override { ++nUpdateCounter; } void ReloadDbEnv() override {} - bool Verify(bilingual_str& errorStr) override { return true; } std::unique_ptr MakeBatch(const char* mode = "r+", bool flush_on_close = true) override { return MakeUnique(); } }; diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index dde29842e..84dc5adf6 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -52,11 +52,11 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector& wal return false; } + DatabaseOptions options; + DatabaseStatus status; + options.verify = true; bilingual_str error_string; - std::vector warnings; - bool verify_success = CWallet::Verify(chain, wallet_file, error_string, warnings); - if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); - if (!verify_success) { + if (!MakeWalletDatabase(wallet_file, options, status, error_string)) { chain.initError(error_string); return false; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4d7c51401..b5928f2b7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -203,7 +203,7 @@ namespace { std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) { try { - if (!CWallet::Verify(chain, name, error, warnings)) { + if (!MakeWalletDatabase(name, options, status, error)) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; return nullptr; } @@ -260,7 +260,7 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::strin } // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. - if (!CWallet::Verify(chain, name, error, warnings)) { + if (!MakeWalletDatabase(name, options, status, error)) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_VERIFY; return nullptr; @@ -3779,7 +3779,7 @@ std::vector CWallet::GetDestValues(const std::string& prefix) const return values; } -bool CWallet::Verify(interfaces::Chain& chain, const std::string& name, bilingual_str& error_string, std::vector& warnings) +std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string) { // Do some checking on wallet path. It should be either a: // @@ -3787,7 +3787,6 @@ bool CWallet::Verify(interfaces::Chain& chain, const std::string& name, bilingua // 2. Path to an existing directory. // 3. Path to a symlink to a directory. // 4. For backwards compatibility, the name of a data file in -walletdir. - LOCK(cs_wallets); const fs::path& wallet_path = fs::absolute(name, GetWalletDir()); fs::file_type path_type = fs::symlink_status(wallet_path).type(); if (!(path_type == fs::file_not_found || path_type == fs::directory_file || @@ -3798,24 +3797,10 @@ bool CWallet::Verify(interfaces::Chain& chain, const std::string& name, bilingua "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)", name, GetWalletDir())); - return false; - } - - // Make sure that the wallet path doesn't clash with an existing wallet path - if (IsWalletLoaded(wallet_path)) { - error_string = Untranslated(strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", name)); - return false; - } - - // Keep same database environment instance across Verify/Recover calls below. - std::unique_ptr database = CreateWalletDatabase(wallet_path); - - try { - return database->Verify(error_string); - } catch (const fs::filesystem_error& e) { - error_string = Untranslated(strprintf("Error loading wallet %s. %s", name, fsbridge::get_filesystem_error_message(e))); - return false; + status = DatabaseStatus::FAILED_BAD_PATH; + return nullptr; } + return MakeDatabase(wallet_path, options, status, error_string); } std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 2e6434f5c..c9ebb94dc 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -57,6 +57,7 @@ std::shared_ptr GetWallet(const std::string& name); std::shared_ptr LoadWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::unique_ptr HandleLoadWallet(LoadWalletFn load_wallet); +std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); //! -paytxfee default constexpr CAmount DEFAULT_PAY_TX_FEE = 0; @@ -1144,9 +1145,6 @@ public: /** Mark a transaction as replaced by another transaction (e.g., BIP 125). */ bool MarkReplaced(const uint256& originalHash, const uint256& newHash); - //! Verify wallet naming and perform salvage on the wallet if required - static bool Verify(interfaces::Chain& chain, const std::string& name, bilingual_str& error_string, std::vector& warnings); - /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ static std::shared_ptr CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags = 0); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 23c4b6977..29ac52cb3 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1032,11 +1032,6 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas return MakeBerkeleyDatabase(path, options, status, error); } -bool IsWalletLoaded(const fs::path& wallet_path) -{ - return IsBDBWalletLoaded(wallet_path); -} - /** Return object for accessing database at specified path. */ std::unique_ptr CreateWalletDatabase(const fs::path& path) { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 2548c1750..7e83e3902 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -285,9 +285,6 @@ using KeyFilterFn = std::function; //! Unserialize a given Key-Value pair and load it into the wallet bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn = nullptr); -/** Return whether a wallet database is currently loaded. */ -bool IsWalletLoaded(const fs::path& wallet_path); - /** Return object for accessing database at specified path. */ std::unique_ptr CreateWalletDatabase(const fs::path& path); diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 5c9d7ff62..cc9d0ac4c 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -247,10 +247,12 @@ class MultiWalletTest(BitcoinTestFramework): assert_raises_rpc_error(-18, 'Wallet wallets not found.', self.nodes[0].loadwallet, 'wallets') # Fail to load duplicate wallets - assert_raises_rpc_error(-4, 'Wallet file verification failed. Error loading wallet w1. Duplicate -wallet filename specified.', self.nodes[0].loadwallet, wallet_names[0]) + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "w1", "wallet.dat") + assert_raises_rpc_error(-4, "Wallet file verification failed. Refusing to load database. Data file '{}' is already loaded.".format(path), self.nodes[0].loadwallet, wallet_names[0]) # Fail to load duplicate wallets by different ways (directory and filepath) - assert_raises_rpc_error(-4, "Wallet file verification failed. Error loading wallet wallet.dat. Duplicate -wallet filename specified.", self.nodes[0].loadwallet, 'wallet.dat') + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "wallet.dat") + assert_raises_rpc_error(-4, "Wallet file verification failed. Refusing to load database. Data file '{}' is already loaded.".format(path), self.nodes[0].loadwallet, 'wallet.dat') # Fail to load if one wallet is a copy of another assert_raises_rpc_error(-4, "BerkeleyDatabase: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy') From 8b5e7297c02f3100a9cb27bfe206e3fc617ec173 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 20:45:28 -0400 Subject: [PATCH 5/8] refactor: Pass wallet database into CWallet::Create No changes in behavior --- src/wallet/bdb.cpp | 1 - src/wallet/bdb.h | 3 +++ src/wallet/db.cpp | 8 -------- src/wallet/db.h | 8 ++++---- src/wallet/load.cpp | 8 ++++++-- src/wallet/test/wallet_tests.cpp | 5 ++++- src/wallet/wallet.cpp | 17 +++++++++-------- src/wallet/wallet.h | 2 +- 8 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 8f8652bb0..fbb3d2cac 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -359,7 +359,6 @@ void BerkeleyDatabase::Open(const char* pszMode) if (ret != 0) { throw std::runtime_error(strprintf("BerkeleyDatabase: Error %d, can't open database %s", ret, strFile)); } - m_file_path = (env->Directory() / strFile).string(); // Call CheckUniqueFileid on the containing BDB environment to // avoid BDB data consistency bugs that happen when different data diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 9f24a2f10..fd5a49acc 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -144,6 +144,9 @@ public: /** Verifies the environment and database file */ bool Verify(bilingual_str& error); + /** Return path to main database filename */ + std::string Filename() override { return (env->Directory() / strFile).string(); } + /** * Pointer to shared database environment. * diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 1eb82a03c..bd1d11473 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -23,11 +23,3 @@ void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std:: database_filename = "wallet.dat"; } } - -fs::path WalletDataFilePath(const fs::path& wallet_path) -{ - fs::path env_directory; - std::string database_filename; - SplitWalletPath(wallet_path, env_directory, database_filename); - return env_directory / database_filename; -} diff --git a/src/wallet/db.h b/src/wallet/db.h index 6e11d7de8..96d1f44d9 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -18,8 +18,6 @@ struct bilingual_str; -/** Given a wallet directory path or legacy file path, return path to main data file in the wallet database. */ -fs::path WalletDataFilePath(const fs::path& wallet_path); void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename); /** RAII class that provides access to a WalletDatabase */ @@ -142,13 +140,14 @@ public: virtual void ReloadDbEnv() = 0; + /** Return path to main database file for logs and error messages. */ + virtual std::string Filename() = 0; + std::atomic nUpdateCounter; unsigned int nLastSeen; unsigned int nLastFlushed; int64_t nLastWalletUpdate; - std::string m_file_path; - /** Make a DatabaseBatch connected to this database */ virtual std::unique_ptr MakeBatch(const char* mode = "r+", bool flush_on_close = true) = 0; }; @@ -189,6 +188,7 @@ public: bool PeriodicFlush() override { return true; } void IncrementUpdateCounter() override { ++nUpdateCounter; } void ReloadDbEnv() override {} + std::string Filename() override { return "dummy"; } std::unique_ptr MakeBatch(const char* mode = "r+", bool flush_on_close = true) override { return MakeUnique(); } }; diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 84dc5adf6..c5d045e9e 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -68,10 +68,14 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector& wal bool LoadWallets(interfaces::Chain& chain, const std::vector& wallet_files) { try { - for (const std::string& walletFile : wallet_files) { + for (const std::string& name : wallet_files) { + DatabaseOptions options; + DatabaseStatus status; + options.verify = false; // No need to verify, assuming verified earlier in VerifyWallets() bilingual_str error; std::vector warnings; - std::shared_ptr pwallet = CWallet::CreateWalletFromFile(chain, walletFile, error, warnings); + std::unique_ptr database = MakeWalletDatabase(name, options, status, error); + std::shared_ptr pwallet = database ? CWallet::Create(chain, name, std::move(database), options.create_flags, error, warnings) : nullptr; if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n"))); if (!pwallet) { chain.initError(error); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b3001efd5..6b98482f9 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -37,9 +37,12 @@ BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup) static std::shared_ptr TestLoadWallet(interfaces::Chain& chain) { + DatabaseOptions options; + DatabaseStatus status; bilingual_str error; std::vector warnings; - auto wallet = CWallet::CreateWalletFromFile(chain, "", error, warnings); + auto database = MakeWalletDatabase("", options, status, error); + auto wallet = CWallet::Create(chain, "", std::move(database), options.create_flags, error, warnings); wallet->postInitProcess(); return wallet; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b5928f2b7..1751cbf5b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -203,12 +203,13 @@ namespace { std::shared_ptr LoadWalletInternal(interfaces::Chain& chain, const std::string& name, Optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) { try { - if (!MakeWalletDatabase(name, options, status, error)) { + std::unique_ptr database = MakeWalletDatabase(name, options, status, error); + if (!database) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; return nullptr; } - std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings); + std::shared_ptr wallet = CWallet::Create(chain, name, std::move(database), options.create_flags, error, warnings); if (!wallet) { error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error; return nullptr; @@ -260,7 +261,8 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::strin } // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. - if (!MakeWalletDatabase(name, options, status, error)) { + std::unique_ptr database = MakeWalletDatabase(name, options, status, error); + if (!database) { error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_VERIFY; return nullptr; @@ -274,7 +276,7 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::strin } // Make the wallet - std::shared_ptr wallet = CWallet::CreateWalletFromFile(chain, name, error, warnings, wallet_creation_flags); + std::shared_ptr wallet = CWallet::Create(chain, name, std::move(database), wallet_creation_flags, error, warnings); if (!wallet) { error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_CREATE; @@ -3803,10 +3805,9 @@ std::unique_ptr MakeWalletDatabase(const std::string& name, cons return MakeDatabase(wallet_path, options, status, error_string); } -std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags) +std::shared_ptr CWallet::Create(interfaces::Chain& chain, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) { - fs::path path = fs::absolute(name, GetWalletDir()); - const std::string walletFile = WalletDataFilePath(path).string(); + const std::string& walletFile = database->Filename(); chain.initMessage(_("Loading wallet...").translated); @@ -3814,7 +3815,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, 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(&chain, name, CreateWalletDatabase(path)), ReleaseWallet); + std::shared_ptr walletInstance(new CWallet(&chain, name, std::move(database)), ReleaseWallet); DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun); if (nLoadWalletRet != DBErrors::LOAD_OK) { if (nLoadWalletRet == DBErrors::CORRUPT) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c9ebb94dc..c54480612 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1146,7 +1146,7 @@ public: bool MarkReplaced(const uint256& originalHash, const uint256& newHash); /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ - static std::shared_ptr CreateWalletFromFile(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector& warnings, uint64_t wallet_creation_flags = 0); + static std::shared_ptr Create(interfaces::Chain& chain, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings); /** * Wallet post-init setup From a987438e9d9cad0b5530e218a447928485f3fd93 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 20:58:43 -0400 Subject: [PATCH 6/8] wallet: Remove path checking code from loadwallet RPC This commit does not change behavior except for error messages which now include more complete information. --- src/wallet/rpcwallet.cpp | 19 +++++++------------ test/functional/rpc_createmultisig.py | 3 ++- test/functional/wallet_multiwallet.py | 6 ++++-- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 536d11ddd..497a4120e 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2503,25 +2503,20 @@ static UniValue loadwallet(const JSONRPCRequest& request) WalletContext& context = EnsureWalletContext(request.context); const std::string name(request.params[0].get_str()); - fs::path path(fs::absolute(name, GetWalletDir())); - - if (fs::symlink_status(path).type() == fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + name + " not found."); - } else if (fs::is_directory(path)) { - // The given filename is a directory. Check that there's a wallet.dat file. - fs::path wallet_dat_file = path / "wallet.dat"; - if (fs::symlink_status(wallet_dat_file).type() == fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Directory " + name + " does not contain a wallet.dat file."); - } - } DatabaseOptions options; DatabaseStatus status; + options.require_existing = true; bilingual_str error; std::vector warnings; Optional load_on_start = request.params[1].isNull() ? nullopt : Optional(request.params[1].get_bool()); std::shared_ptr const wallet = LoadWallet(*context.chain, name, load_on_start, options, status, error, warnings); - if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error.original); + if (!wallet) { + // Map bad format to not found, since bad format is returned when the + // wallet directory exists, but doesn't contain a data file. + RPCErrorCode code = status == DatabaseStatus::FAILED_NOT_FOUND || status == DatabaseStatus::FAILED_BAD_FORMAT ? RPC_WALLET_NOT_FOUND : RPC_WALLET_ERROR; + throw JSONRPCError(code, error.original); + } UniValue obj(UniValue::VOBJ); obj.pushKV("name", wallet->GetName()); diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index 3c81a4a4e..f19c60dc3 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -129,7 +129,8 @@ class RpcCreateMultiSigTest(BitcoinTestFramework): try: node1.loadwallet('wmulti') except JSONRPCException as e: - if e.error['code'] == -18 and 'Wallet wmulti not found' in e.error['message']: + path = os.path.join(self.options.tmpdir, "node1", "regtest", "wallets", "wmulti") + if e.error['code'] == -18 and "Wallet file verification failed. Failed to load database path '{}'. Path does not exist.".format(path) in e.error['message']: node1.createwallet(wallet_name='wmulti', disable_private_keys=True) else: raise diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index cc9d0ac4c..168d6be42 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -244,7 +244,8 @@ class MultiWalletTest(BitcoinTestFramework): assert_equal(set(self.nodes[0].listwallets()), set(wallet_names)) # Fail to load if wallet doesn't exist - assert_raises_rpc_error(-18, 'Wallet wallets not found.', self.nodes[0].loadwallet, 'wallets') + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "wallets") + assert_raises_rpc_error(-18, "Wallet file verification failed. Failed to load database path '{}'. Path does not exist.".format(path), self.nodes[0].loadwallet, 'wallets') # Fail to load duplicate wallets path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "w1", "wallet.dat") @@ -266,7 +267,8 @@ class MultiWalletTest(BitcoinTestFramework): # Fail to load if a directory is specified that doesn't contain a wallet os.mkdir(wallet_dir('empty_wallet_dir')) - assert_raises_rpc_error(-18, "Directory empty_wallet_dir does not contain a wallet.dat file", self.nodes[0].loadwallet, 'empty_wallet_dir') + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "empty_wallet_dir") + assert_raises_rpc_error(-18, "Wallet file verification failed. Failed to load database path '{}'. Data is not in recognized format.".format(path), self.nodes[0].loadwallet, 'empty_wallet_dir') self.log.info("Test dynamic wallet creation.") From 77d5bb72b8722ec7a6c7c33479a532cbd5870ba4 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 21:03:27 -0400 Subject: [PATCH 7/8] wallet: Remove path checking code from createwallet RPC This commit does not change behavior except for error messages which now include more complete information. --- src/wallet/rpcwallet.cpp | 1 + src/wallet/wallet.cpp | 7 ------- test/functional/wallet_multiwallet.py | 3 ++- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 497a4120e..891d650ad 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2647,6 +2647,7 @@ static UniValue createwallet(const JSONRPCRequest& request) DatabaseOptions options; DatabaseStatus status; + options.require_create = true; options.create_flags = flags; options.create_passphrase = passphrase; bilingual_str error; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1751cbf5b..0d0790492 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -253,13 +253,6 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, const std::strin wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET; } - // Check the wallet file location - if (fs::symlink_status(fs::absolute(name.empty() ? "wallet.dat" : name, GetWalletDir())).type() != fs::file_not_found) { - error = strprintf(Untranslated("Wallet %s already exists."), name); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; - } - // Wallet::Verify will check if we're trying to create a wallet with a duplicate name. std::unique_ptr database = MakeWalletDatabase(name, options, status, error); if (!database) { diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 168d6be42..d64d3dcb4 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -273,7 +273,8 @@ class MultiWalletTest(BitcoinTestFramework): self.log.info("Test dynamic wallet creation.") # Fail to create a wallet if it already exists. - assert_raises_rpc_error(-4, "Wallet w2 already exists.", self.nodes[0].createwallet, 'w2') + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "w2") + assert_raises_rpc_error(-4, "Failed to create database path '{}'. Database already exists.".format(path), self.nodes[0].createwallet, 'w2') # Successfully create a wallet with a new name loadwallet_name = self.nodes[0].createwallet('w9') From 7bf6dfbb484adfda3b8df26ee3e2ebda239dd263 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 21:16:48 -0400 Subject: [PATCH 8/8] wallet: Remove path checking code from bitcoin-wallet tool This commit does not change behavior except for error messages which now include more complete information. --- src/wallet/salvage.cpp | 7 ++++++ src/wallet/walletdb.cpp | 7 ------ src/wallet/walletdb.h | 3 --- src/wallet/wallettool.cpp | 43 ++++++++++++++-------------------- src/wallet/wallettool.h | 2 -- test/functional/tool_wallet.py | 6 +++-- 6 files changed, 29 insertions(+), 39 deletions(-) diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 96fba2133..225b97506 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -23,6 +23,13 @@ static bool KeyFilter(const std::string& type) bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::vector& warnings) { + DatabaseOptions options; + DatabaseStatus status; + options.require_existing = true; + options.verify = false; + std::unique_ptr database = MakeDatabase(file_path, options, status, error); + if (!database) return false; + std::string filename; std::shared_ptr env = GetWalletEnv(file_path, filename); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 29ac52cb3..5bf21eb91 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1032,13 +1032,6 @@ std::unique_ptr MakeDatabase(const fs::path& path, const Databas return MakeBerkeleyDatabase(path, options, status, error); } -/** Return object for accessing database at specified path. */ -std::unique_ptr CreateWalletDatabase(const fs::path& path) -{ - std::string filename; - return MakeUnique(GetWalletEnv(path, filename), std::move(filename)); -} - /** Return object for accessing dummy database with no read/write capabilities. */ std::unique_ptr CreateDummyWalletDatabase() { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 7e83e3902..eda810ed8 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -285,9 +285,6 @@ using KeyFilterFn = std::function; //! Unserialize a given Key-Value pair and load it into the wallet bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn = nullptr); -/** Return object for accessing database at specified path. */ -std::unique_ptr CreateWalletDatabase(const fs::path& path); - /** Return object for accessing dummy database with no read/write capabilities. */ std::unique_ptr CreateDummyWalletDatabase(); diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index abd5652fe..4452840eb 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -21,21 +21,9 @@ static void WalletToolReleaseWallet(CWallet* wallet) delete wallet; } -static std::shared_ptr CreateWallet(const std::string& name, const fs::path& path) +static void WalletCreate(CWallet* wallet_instance) { - if (fs::exists(path)) { - tfm::format(std::cerr, "Error: File exists already\n"); - return nullptr; - } - // dummy chain interface - std::shared_ptr wallet_instance(new CWallet(nullptr /* chain */, name, CreateWalletDatabase(path)), WalletToolReleaseWallet); LOCK(wallet_instance->cs_wallet); - bool first_run = true; - DBErrors load_wallet_ret = wallet_instance->LoadWallet(first_run); - if (load_wallet_ret != DBErrors::LOAD_OK) { - tfm::format(std::cerr, "Error creating %s", name); - return nullptr; - } wallet_instance->SetMinVersion(FEATURE_HD_SPLIT); @@ -46,18 +34,26 @@ static std::shared_ptr CreateWallet(const std::string& name, const fs:: tfm::format(std::cout, "Topping up keypool...\n"); wallet_instance->TopUpKeyPool(); - return wallet_instance; } -static std::shared_ptr LoadWallet(const std::string& name, const fs::path& path) +static std::shared_ptr MakeWallet(const std::string& name, const fs::path& path, bool create) { - if (!fs::exists(path)) { - tfm::format(std::cerr, "Error: Wallet files does not exist\n"); + DatabaseOptions options; + DatabaseStatus status; + if (create) { + options.require_create = true; + } else { + options.require_existing = true; + } + bilingual_str error; + std::unique_ptr database = MakeDatabase(path, options, status, error); + if (!database) { + tfm::format(std::cerr, "%s\n", error.original); return nullptr; } // dummy chain interface - std::shared_ptr wallet_instance(new CWallet(nullptr /* chain */, name, CreateWalletDatabase(path)), WalletToolReleaseWallet); + std::shared_ptr wallet_instance{new CWallet(nullptr /* chain */, name, std::move(database)), WalletToolReleaseWallet}; DBErrors load_wallet_ret; try { bool first_run; @@ -89,6 +85,8 @@ static std::shared_ptr LoadWallet(const std::string& name, const fs::pa } } + if (create) WalletCreate(wallet_instance.get()); + return wallet_instance; } @@ -109,19 +107,14 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name) fs::path path = fs::absolute(name, GetWalletDir()); if (command == "create") { - std::shared_ptr wallet_instance = CreateWallet(name, path); + std::shared_ptr wallet_instance = MakeWallet(name, path, /* create= */ true); if (wallet_instance) { WalletShowInfo(wallet_instance.get()); wallet_instance->Close(); } } else if (command == "info" || command == "salvage") { - if (!fs::exists(path)) { - tfm::format(std::cerr, "Error: no wallet file at %s\n", name); - return false; - } - if (command == "info") { - std::shared_ptr wallet_instance = LoadWallet(name, path); + std::shared_ptr wallet_instance = MakeWallet(name, path, /* create= */ false); if (!wallet_instance) return false; WalletShowInfo(wallet_instance.get()); wallet_instance->Close(); diff --git a/src/wallet/wallettool.h b/src/wallet/wallettool.h index 8ee3355f0..d0b8d6812 100644 --- a/src/wallet/wallettool.h +++ b/src/wallet/wallettool.h @@ -9,8 +9,6 @@ namespace WalletTool { -std::shared_ptr CreateWallet(const std::string& name, const fs::path& path); -std::shared_ptr LoadWallet(const std::string& name, const fs::path& path); void WalletShowInfo(CWallet* wallet_instance); bool ExecuteWalletToolFunc(const std::string& command, const std::string& file); diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index 18f0beb59..fa5b5c10f 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -70,12 +70,14 @@ class ToolWalletTest(BitcoinTestFramework): self.assert_raises_tool_error('Invalid command: help', 'help') self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create') self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo') + locked_dir = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets") self.assert_raises_tool_error( - 'Error loading wallet.dat. Is wallet being used by another process?', + 'Error initializing wallet database environment "{}"!'.format(locked_dir), '-wallet=wallet.dat', 'info', ) - self.assert_raises_tool_error('Error: no wallet file at nonexistent.dat', '-wallet=nonexistent.dat', 'info') + path = os.path.join(self.options.tmpdir, "node0", "regtest", "wallets", "nonexistent.dat") + self.assert_raises_tool_error("Failed to load database path '{}'. Path does not exist.".format(path), '-wallet=nonexistent.dat', 'info') def test_tool_wallet_info(self): # Stop the node to close the wallet to call the info command.