From 288b4ffb6b291f0466d513ff3c40af6758ca7c88 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 28 Jul 2020 19:25:14 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 3a71a6ca03..99aafd8dfc 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 e16182b48e..b3b73284d8 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 28839b2ffc..d17110a0f6 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 d6781460a7..35fcb2b0ca 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 98065803e9..d6d2d0e3df 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 5bbc8b91f7..dde29842ec 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 887c5b632b..312b345518 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 934e3d5c86..96fba21339 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 1deedede4c..f38ccba384 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 e416f16044..d5aed99d99 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 4f12079768..f7c1337b0d 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 9241470745..4d6f427618 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 f400e7e944..b3001efd55 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 afe676078c..43dbd48262 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 06069029ea..9c04873c5b 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 9b51461843..abd5652fe7 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 8bac0608a9..d6a8ee9e02 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 a4e4fda8a1..afdcb2e18a 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 { -- cgit v1.2.3 From b5b414151af32e5a07b5757b64482d77519d77c0 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 16:40:31 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 24eb2ee34c..61463aaf5e 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 75546924e8..82ad136649 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 0afaba5fd1..f0f6f03c43 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 962ea66fa0..23c4b69777 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 d6a8ee9e02..e4c72aed98 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; -- cgit v1.2.3 From 0d94e6062547f288a75921d2433458a44a5f2297 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 17:55:13 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index d17110a0f6..d19d0406b6 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 186f5d81a5..b1afbbfd7c 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 828f84ffcc..bee17abf11 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 f0f6f03c43..6a918ec925 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 312b345518..536d11ddd9 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 43dbd48262..4d7c514019 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 9c04873c5b..2e6434f5ca 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 -- cgit v1.2.3 From 3c815cfe54087fd139169161d2fd175e99840e6a Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 19:33:37 -0400 Subject: 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 --- 8 files changed, 12 insertions(+), 57 deletions(-) (limited to 'src') diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 61463aaf5e..8f8652bb0b 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 82ad136649..9f24a2f10b 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 6a918ec925..6e11d7de88 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 dde29842ec..84dc5adf66 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 4d7c514019..b5928f2b7e 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 2e6434f5ca..c9ebb94dc1 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 23c4b69777..29ac52cb3e 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 2548c17508..7e83e3902a 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); -- cgit v1.2.3 From 8b5e7297c02f3100a9cb27bfe206e3fc617ec173 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 20:45:28 -0400 Subject: 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(-) (limited to 'src') diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 8f8652bb0b..fbb3d2cac5 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 9f24a2f10b..fd5a49acc3 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 1eb82a03c7..bd1d114730 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 6e11d7de88..96d1f44d91 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 84dc5adf66..c5d045e9ef 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 b3001efd55..6b98482f98 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 b5928f2b7e..1751cbf5bc 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 c9ebb94dc1..c54480612a 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 -- cgit v1.2.3 From a987438e9d9cad0b5530e218a447928485f3fd93 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 20:58:43 -0400 Subject: 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 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 536d11ddd9..497a4120e5 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()); -- cgit v1.2.3 From 77d5bb72b8722ec7a6c7c33479a532cbd5870ba4 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 21:03:27 -0400 Subject: 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 ------- 2 files changed, 1 insertion(+), 7 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 497a4120e5..891d650ad3 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 1751cbf5bc..0d07904924 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) { -- cgit v1.2.3 From 7bf6dfbb484adfda3b8df26ee3e2ebda239dd263 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 4 Aug 2020 21:16:48 -0400 Subject: 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 -- 5 files changed, 25 insertions(+), 37 deletions(-) (limited to 'src') diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index 96fba21339..225b975067 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 29ac52cb3e..5bf21eb91f 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 7e83e3902a..eda810ed8a 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 abd5652fe7..4452840eb1 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 8ee3355f02..d0b8d6812a 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); -- cgit v1.2.3