From f455979eb1b65c9822b414aa9e6b04b5c43322a0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Feb 2018 15:28:42 -0500 Subject: Add function to close all Db's and reload the databae environment Adds a ReloadDbEnv function to BerkeleyEnvironment in order to close all Db instances, closes the environment, resets it, and then reopens the BerkeleyEnvironment. Also adds a ReloadDbEnv function to BerkeleyDatabase that calls BerkeleyEnvironment's ReloadDbEnv. Github-Pull: #12493 Rebased-From: 5d296ac --- src/wallet/db.cpp | 34 ++++++++++++++++++++++++++++++++++ src/wallet/db.h | 4 ++++ 2 files changed, 38 insertions(+) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index d0fe51801e..679cb200b7 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -556,6 +556,7 @@ void BerkeleyBatch::Close() LOCK(cs_db); --env->mapFileUseCount[strFile]; } + env->m_db_in_use.notify_all(); } void BerkeleyEnvironment::CloseDb(const std::string& strFile) @@ -572,6 +573,32 @@ void BerkeleyEnvironment::CloseDb(const std::string& strFile) } } +void BerkeleyEnvironment::ReloadDbEnv() +{ + // Make sure that no Db's are in use + AssertLockNotHeld(cs_db); + std::unique_lock lock(cs_db); + m_db_in_use.wait(lock, [this](){ + for (auto& count : mapFileUseCount) { + if (count.second > 0) return false; + } + return true; + }); + + std::vector filenames; + for (auto it : mapDb) { + filenames.push_back(it.first); + } + // Close the individual Db's + for (const std::string& filename : filenames) { + CloseDb(filename); + } + // Reset the environment + Flush(true); // This will flush and close the environment + Reset(); + Open(true); +} + bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip) { if (database.IsDummy()) { @@ -799,3 +826,10 @@ void BerkeleyDatabase::Flush(bool shutdown) if (shutdown) env = nullptr; } } + +void BerkeleyDatabase::ReloadDbEnv() +{ + if (!IsDummy()) { + env->ReloadDbEnv(); + } +} diff --git a/src/wallet/db.h b/src/wallet/db.h index b078edab7b..467ed13b45 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -38,6 +38,7 @@ public: std::unique_ptr dbenv; std::map mapFileUseCount; std::map mapDb; + std::condition_variable_any m_db_in_use; BerkeleyEnvironment(const fs::path& env_directory); ~BerkeleyEnvironment(); @@ -75,6 +76,7 @@ public: void CheckpointLSN(const std::string& strFile); void CloseDb(const std::string& strFile); + void ReloadDbEnv(); DbTxn* TxnBegin(int flags = DB_TXN_WRITE_NOSYNC) { @@ -145,6 +147,8 @@ public: void IncrementUpdateCounter(); + void ReloadDbEnv(); + std::atomic nUpdateCounter; unsigned int nLastSeen; unsigned int nLastFlushed; -- cgit v1.2.3 From 048fda2a66df405cd98706612c87b59c2912c441 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Feb 2018 16:08:36 -0500 Subject: After encrypting the wallet, reload the database environment Calls ReloadDbEnv after encrypting the wallet so that the database environment is flushed, closed, and reopened to prevent unencrypted keys from being saved on disk. Github-Pull: #12493 Rebased-From: d7637c5 --- src/wallet/wallet.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e86aa04d18..4bf2ec1e4e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -760,6 +760,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) // bits of the unencrypted private key in slack space in the database file. database->Rewrite(); + // BDB seems to have a bad habit of writing old data into + // slack space in .dat files; that is bad if the old data is + // unencrypted private keys. So: + database->ReloadDbEnv(); + } NotifyStatusChanged(this); -- cgit v1.2.3 From 435df68c62562e30a6d11b0bfc2cf56434dbc4a0 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 13 Jul 2018 19:15:30 -0700 Subject: Move BerkeleyEnvironment deletion from internal method to callsite Instead of having the object destroy itself, having the caller destroy it. Github-Pull: #12493 Rebased-From: a769461 --- src/wallet/db.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 679cb200b7..2d57c92ccc 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -724,7 +724,6 @@ void BerkeleyEnvironment::Flush(bool fShutdown) if (!fMockDb) { fs::remove_all(fs::path(strPath) / "database"); } - g_dbenvs.erase(strPath); } } } @@ -823,7 +822,11 @@ void BerkeleyDatabase::Flush(bool shutdown) { if (!IsDummy()) { env->Flush(shutdown); - if (shutdown) env = nullptr; + if (shutdown) { + LOCK(cs_db); + g_dbenvs.erase(env->Directory().string()); + env = nullptr; + } } } -- cgit v1.2.3 From 1c98a758d0f43f12d600731373758303cefe7cd7 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Feb 2018 16:09:51 -0500 Subject: No longer shutdown after encrypting the wallet Since the database environment is flushed, closed, and reopened during EncryptWallet, there is no need to shut down the software anymore. Github-Pull: #12493 Rebased-From: c1dde3a --- src/qt/askpassphrasedialog.cpp | 5 ++--- src/wallet/rpcwallet.cpp | 7 +------ test/functional/rpc_fundrawtransaction.py | 6 ++---- test/functional/test_framework/test_node.py | 8 -------- test/functional/wallet_bumpfee.py | 3 +-- test/functional/wallet_dump.py | 3 +-- test/functional/wallet_encryption.py | 3 +-- test/functional/wallet_keypool.py | 4 +--- 8 files changed, 9 insertions(+), 30 deletions(-) diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp index 812d2251e1..612331523e 100644 --- a/src/qt/askpassphrasedialog.cpp +++ b/src/qt/askpassphrasedialog.cpp @@ -123,16 +123,15 @@ void AskPassphraseDialog::accept() { QMessageBox::warning(this, tr("Wallet encrypted"), "" + - tr("%1 will close now to finish the encryption process. " + tr("Your wallet is now encrypted. " "Remember that encrypting your wallet cannot fully protect " - "your bitcoins from being stolen by malware infecting your computer.").arg(tr(PACKAGE_NAME)) + + "your bitcoins from being stolen by malware infecting your computer.") + "

" + tr("IMPORTANT: Any previous backups you have made of your wallet file " "should be replaced with the newly generated, encrypted wallet file. " "For security reasons, previous backups of the unencrypted wallet file " "will become useless as soon as you start using the new, encrypted wallet.") + ""); - QApplication::quit(); } else { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 762f5c3012..8f92f537ac 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2740,7 +2740,6 @@ static UniValue encryptwallet(const JSONRPCRequest& request) "will require the passphrase to be set prior the making these calls.\n" "Use the walletpassphrase call for this, and then walletlock call.\n" "If the wallet is already encrypted, use the walletpassphrasechange call.\n" - "Note that this will shutdown the server.\n" "\nArguments:\n" "1. \"passphrase\" (string) The pass phrase to encrypt the wallet with. It must be at least 1 character, but should be long.\n" "\nExamples:\n" @@ -2778,11 +2777,7 @@ static UniValue encryptwallet(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Failed to encrypt the wallet."); } - // BDB seems to have a bad habit of writing old data into - // slack space in .dat files; that is bad if the old data is - // unencrypted private keys. So: - StartShutdown(); - return "wallet encrypted; Bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup."; + return "wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup."; } static UniValue lockunspent(const JSONRPCRequest& request) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index daa890ab15..0c61e9ab62 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -478,10 +478,8 @@ class RawTransactionsTest(BitcoinTestFramework): ############################################################ # locked wallet test - self.stop_node(0) - self.nodes[1].node_encrypt_wallet("test") - self.stop_node(2) - self.stop_node(3) + self.nodes[1].encryptwallet("test") + self.stop_nodes() self.start_nodes() # This test is not meant to test fee estimation and we'd like diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 908dda94c5..b6e3ab56d5 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -305,14 +305,6 @@ class TestNode(): assert_msg = "bitcoind should have exited with expected error " + expected_msg self._raise_assertion_error(assert_msg) - def node_encrypt_wallet(self, passphrase): - """"Encrypts the wallet. - - This causes bitcoind to shutdown, so this method takes - care of cleaning up resources.""" - self.encryptwallet(passphrase) - self.wait_until_stopped() - def add_p2p_connection(self, p2p_conn, *, wait_for_verack=True, **kwargs): """Add a p2p connection to the node. diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index b9fe6c66c5..e478347cd7 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -41,8 +41,7 @@ class BumpFeeTest(BitcoinTestFramework): def run_test(self): # Encrypt wallet for test_locked_wallet_fails test - self.nodes[1].node_encrypt_wallet(WALLET_PASSPHRASE) - self.start_node(1) + self.nodes[1].encryptwallet(WALLET_PASSPHRASE) self.nodes[1].walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT) connect_nodes_bi(self.nodes, 0, 1) diff --git a/test/functional/wallet_dump.py b/test/functional/wallet_dump.py index db731b2a34..a9ff0f6f48 100755 --- a/test/functional/wallet_dump.py +++ b/test/functional/wallet_dump.py @@ -128,8 +128,7 @@ class WalletDumpTest(BitcoinTestFramework): assert_equal(witness_addr_ret, witness_addr) # p2sh-p2wsh address added to the first key #encrypt wallet, restart, unlock and dump - self.nodes[0].node_encrypt_wallet('test') - self.start_node(0) + self.nodes[0].encryptwallet('test') self.nodes[0].walletpassphrase('test', 10) # Should be a no-op: self.nodes[0].keypoolrefill() diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index d8c27b09d9..ab9ebed8d4 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -33,8 +33,7 @@ class WalletEncryptionTest(BitcoinTestFramework): assert_equal(len(privkey), 52) # Encrypt the wallet - self.nodes[0].node_encrypt_wallet(passphrase) - self.start_node(0) + self.nodes[0].encryptwallet(passphrase) # Test that the wallet is encrypted assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].dumpprivkey, address) diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index acc336e4d5..51afa0cb1a 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -25,9 +25,7 @@ class KeyPoolTest(BitcoinTestFramework): assert(addr_before_encrypting_data['hdseedid'] == wallet_info_old['hdseedid']) # Encrypt wallet and wait to terminate - nodes[0].node_encrypt_wallet('test') - # Restart node 0 - self.start_node(0) + nodes[0].encryptwallet('test') # Keep creating keys addr = nodes[0].getnewaddress() addr_data = nodes[0].getaddressinfo(addr) -- cgit v1.2.3 From 21693ff0b743f094e73111a81c1c86e2622d937c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Fri, 28 Sep 2018 12:50:04 +0100 Subject: wallet: Add WalletLocation utility class Github-Pull: #14350 Rebased-From: 01a4c09 --- src/wallet/walletutil.cpp | 11 +++++++++++ src/wallet/walletutil.h | 20 ++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp index 34c76ec4c4..064885ed49 100644 --- a/src/wallet/walletutil.cpp +++ b/src/wallet/walletutil.cpp @@ -25,3 +25,14 @@ fs::path GetWalletDir() return path; } + +WalletLocation::WalletLocation(const std::string& name) + : m_name(name) + , m_path(fs::absolute(name, GetWalletDir())) +{ +} + +bool WalletLocation::Exists() const +{ + return fs::symlink_status(m_path).type() != fs::file_not_found; +} diff --git a/src/wallet/walletutil.h b/src/wallet/walletutil.h index 7c42954616..a8cd7b6180 100644 --- a/src/wallet/walletutil.h +++ b/src/wallet/walletutil.h @@ -11,4 +11,24 @@ //! Get the path of the wallet directory. fs::path GetWalletDir(); +//! 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; +}; + #endif // BITCOIN_WALLET_WALLETUTIL_H -- cgit v1.2.3 From 16e57594556ac481a32f5d5ed1a988b2772ba804 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Fri, 28 Sep 2018 16:50:18 +0100 Subject: wallet: Refactor to use WalletLocation Github-Pull: #14350 Rebased-From: 65f3672 --- src/bench/coin_selection.cpp | 4 ++-- src/qt/test/addressbooktests.cpp | 2 +- src/qt/test/wallettests.cpp | 2 +- src/wallet/init.cpp | 8 ++++---- src/wallet/rpcwallet.cpp | 29 ++++++++++++++--------------- src/wallet/test/coinselector_tests.cpp | 2 +- src/wallet/test/wallet_test_fixture.cpp | 3 ++- src/wallet/test/wallet_tests.cpp | 16 ++++++++-------- src/wallet/wallet.cpp | 25 ++++++++++++------------- src/wallet/wallet.h | 20 +++++++++----------- test/lint/lint-circular-dependencies.sh | 1 - 11 files changed, 54 insertions(+), 58 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 0a6f5d85ea..badc426b32 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -33,7 +33,7 @@ static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector CoinSet; -static const CWallet testWallet("dummy", WalletDatabase::CreateDummy()); +static const CWallet testWallet(WalletLocation(), WalletDatabase::CreateDummy()); std::vector> wtxn; // Copied from src/wallet/test/coinselector_tests.cpp diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 3525846044..5aa296eb3c 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -57,7 +57,7 @@ void EditAddressAndSubmit( void TestAddAddressesToSendBook() { TestChain100Setup test; - std::shared_ptr wallet = std::make_shared("mock", WalletDatabase::CreateMock()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateMock()); bool firstRun; wallet->LoadWallet(firstRun); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 9598d64845..fdbcb46f53 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -133,7 +133,7 @@ void TestGUI() for (int i = 0; i < 5; ++i) { test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey())); } - std::shared_ptr wallet = std::make_shared("mock", WalletDatabase::CreateMock()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateMock()); bool firstRun; wallet->LoadWallet(firstRun); { diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 7dc6c30645..d226bbc2fa 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -203,15 +203,15 @@ bool WalletInit::Verify() const std::set wallet_paths; for (const auto& wallet_file : wallet_files) { - fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); + WalletLocation location(wallet_file); - if (!wallet_paths.insert(wallet_path).second) { + if (!wallet_paths.insert(location.GetPath()).second) { return InitError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), wallet_file)); } std::string error_string; std::string warning_string; - bool verify_success = CWallet::Verify(wallet_file, salvage_wallet, error_string, warning_string); + bool verify_success = CWallet::Verify(location, salvage_wallet, error_string, warning_string); if (!error_string.empty()) InitError(error_string); if (!warning_string.empty()) InitWarning(warning_string); if (!verify_success) return false; @@ -228,7 +228,7 @@ bool WalletInit::Open() const } for (const std::string& walletFile : gArgs.GetArgs("-wallet")) { - std::shared_ptr pwallet = CWallet::CreateWalletFromFile(walletFile, fs::absolute(walletFile, GetWalletDir())); + std::shared_ptr pwallet = CWallet::CreateWalletFromFile(WalletLocation(walletFile)); if (!pwallet) { return false; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 8f92f537ac..957e5c6c3b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3112,26 +3112,26 @@ static UniValue loadwallet(const JSONRPCRequest& request) + HelpExampleCli("loadwallet", "\"test.dat\"") + HelpExampleRpc("loadwallet", "\"test.dat\"") ); - std::string wallet_file = request.params[0].get_str(); + + WalletLocation location(request.params[0].get_str()); std::string error; - fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); - if (fs::symlink_status(wallet_path).type() == fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + wallet_file + " not found."); - } else if (fs::is_directory(wallet_path)) { + if (!location.Exists()) { + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + location.GetName() + " not found."); + } else if (fs::is_directory(location.GetPath())) { // The given filename is a directory. Check that there's a wallet.dat file. - fs::path wallet_dat_file = wallet_path / "wallet.dat"; + fs::path wallet_dat_file = location.GetPath() / "wallet.dat"; if (fs::symlink_status(wallet_dat_file).type() == fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Directory " + wallet_file + " does not contain a wallet.dat file."); + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Directory " + location.GetName() + " does not contain a wallet.dat file."); } } std::string warning; - if (!CWallet::Verify(wallet_file, false, error, warning)) { + if (!CWallet::Verify(location, false, error, warning)) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error); } - std::shared_ptr const wallet = CWallet::CreateWalletFromFile(wallet_file, fs::absolute(wallet_file, GetWalletDir())); + std::shared_ptr const wallet = CWallet::CreateWalletFromFile(location); if (!wallet) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet loading failed."); } @@ -3165,7 +3165,6 @@ static UniValue createwallet(const JSONRPCRequest& request) + HelpExampleRpc("createwallet", "\"testwallet\"") ); } - std::string wallet_name = request.params[0].get_str(); std::string error; std::string warning; @@ -3174,17 +3173,17 @@ static UniValue createwallet(const JSONRPCRequest& request) disable_privatekeys = request.params[1].get_bool(); } - fs::path wallet_path = fs::absolute(wallet_name, GetWalletDir()); - if (fs::symlink_status(wallet_path).type() != fs::file_not_found) { - throw JSONRPCError(RPC_WALLET_ERROR, "Wallet " + wallet_name + " already exists."); + WalletLocation location(request.params[0].get_str()); + if (location.Exists()) { + throw JSONRPCError(RPC_WALLET_ERROR, "Wallet " + location.GetName() + " already exists."); } // Wallet::Verify will check if we're trying to create a wallet with a duplication name. - if (!CWallet::Verify(wallet_name, false, error, warning)) { + if (!CWallet::Verify(location, false, error, warning)) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet file verification failed: " + error); } - std::shared_ptr const wallet = CWallet::CreateWalletFromFile(wallet_name, fs::absolute(wallet_name, GetWalletDir()), (disable_privatekeys ? (uint64_t)WALLET_FLAG_DISABLE_PRIVATE_KEYS : 0)); + std::shared_ptr const wallet = CWallet::CreateWalletFromFile(location, (disable_privatekeys ? (uint64_t)WALLET_FLAG_DISABLE_PRIVATE_KEYS : 0)); if (!wallet) { throw JSONRPCError(RPC_WALLET_ERROR, "Wallet creation failed."); } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 0776ffda9c..aed5677bd2 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -28,7 +28,7 @@ std::vector> wtxn; typedef std::set CoinSet; static std::vector vCoins; -static CWallet testWallet("dummy", WalletDatabase::CreateDummy()); +static CWallet testWallet(WalletLocation(), WalletDatabase::CreateDummy()); static CAmount balance = 0; CoinEligibilityFilter filter_standard(1, 6, 0); diff --git a/src/wallet/test/wallet_test_fixture.cpp b/src/wallet/test/wallet_test_fixture.cpp index de59b60349..d42209ab15 100644 --- a/src/wallet/test/wallet_test_fixture.cpp +++ b/src/wallet/test/wallet_test_fixture.cpp @@ -6,9 +6,10 @@ #include #include +#include WalletTestingSetup::WalletTestingSetup(const std::string& chainName): - TestingSetup(chainName), m_wallet("mock", WalletDatabase::CreateMock()) + TestingSetup(chainName), m_wallet(WalletLocation(), WalletDatabase::CreateMock()) { bool fFirstRun; m_wallet.LoadWallet(fFirstRun); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c474547b7e..88eaaf3398 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -47,7 +47,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) // Verify ScanForWalletTransactions picks up transactions in both the old // and new block files. { - CWallet wallet("dummy", WalletDatabase::CreateDummy()); + CWallet wallet(WalletLocation(), WalletDatabase::CreateDummy()); AddKey(wallet, coinbaseKey); WalletRescanReserver reserver(&wallet); reserver.reserve(); @@ -62,7 +62,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) // Verify ScanForWalletTransactions only picks transactions in the new block // file. { - CWallet wallet("dummy", WalletDatabase::CreateDummy()); + CWallet wallet(WalletLocation(), WalletDatabase::CreateDummy()); AddKey(wallet, coinbaseKey); WalletRescanReserver reserver(&wallet); reserver.reserve(); @@ -74,7 +74,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup) // before the missing block, and success for a key whose creation time is // after. { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); AddWallet(wallet); UniValue keys; keys.setArray(); @@ -135,7 +135,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Import key into wallet and call dumpwallet to create backup file. { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); LOCK(wallet->cs_wallet); wallet->mapKeyMetadata[coinbaseKey.GetPubKey().GetID()].nCreateTime = KEY_TIME; wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey()); @@ -151,7 +151,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME // were scanned, and no prior blocks were scanned. { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); JSONRPCRequest request; request.params.setArray(); @@ -181,7 +181,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // debit functions. BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) { - CWallet wallet("dummy", WalletDatabase::CreateDummy()); + CWallet wallet(WalletLocation(), WalletDatabase::CreateDummy()); CWalletTx wtx(&wallet, m_coinbase_txns.back()); LOCK2(cs_main, wallet.cs_wallet); wtx.hashBlock = chainActive.Tip()->GetBlockHash(); @@ -274,7 +274,7 @@ public: ListCoinsTestingSetup() { CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - wallet = MakeUnique("mock", WalletDatabase::CreateMock()); + wallet = MakeUnique(WalletLocation(), WalletDatabase::CreateMock()); bool firstRun; wallet->LoadWallet(firstRun); AddKey(*wallet, coinbaseKey); @@ -368,7 +368,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { - std::shared_ptr wallet = std::make_shared("dummy", WalletDatabase::CreateDummy()); + std::shared_ptr wallet = std::make_shared(WalletLocation(), WalletDatabase::CreateDummy()); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); BOOST_CHECK(!wallet->TopUpKeyPool(1000)); CPubKey pubkey; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4bf2ec1e4e..fefa1ae02b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #include #include @@ -4010,7 +4009,7 @@ void CWallet::MarkPreSplitKeys() } } -bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& error_string, std::string& warning_string) +bool CWallet::Verify(const WalletLocation& location, bool salvage_wallet, std::string& error_string, std::string& warning_string) { // Do some checking on wallet path. It should be either a: // @@ -4019,23 +4018,23 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& // 3. Path to a symlink to a directory. // 4. For backwards compatibility, the name of a data file in -walletdir. LOCK(cs_wallets); - fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir()); + const fs::path& wallet_path = location.GetPath(); fs::file_type path_type = fs::symlink_status(wallet_path).type(); if (!(path_type == fs::file_not_found || path_type == fs::directory_file || (path_type == fs::symlink_file && fs::is_directory(wallet_path)) || - (path_type == fs::regular_file && fs::path(wallet_file).filename() == wallet_file))) { + (path_type == fs::regular_file && fs::path(location.GetName()).filename() == location.GetName()))) { error_string = strprintf( "Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and " "database/log.?????????? files can be stored, a location where such a directory could be created, " "or (for backwards compatibility) the name of an existing data file in -walletdir (%s)", - wallet_file, GetWalletDir()); + location.GetName(), GetWalletDir()); return false; } // Make sure that the wallet path doesn't clash with an existing wallet path for (auto wallet : GetWallets()) { - if (fs::absolute(wallet->GetName(), GetWalletDir()) == wallet_path) { - error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", wallet_file); + if (wallet->GetLocation().GetPath() == wallet_path) { + error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName()); return false; } } @@ -4045,13 +4044,13 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& return false; } } catch (const fs::filesystem_error& e) { - error_string = strprintf("Error loading wallet %s. %s", wallet_file, e.what()); + error_string = strprintf("Error loading wallet %s. %s", location.GetName(), e.what()); return false; } if (salvage_wallet) { // Recover readable keypairs: - CWallet dummyWallet("dummy", WalletDatabase::CreateDummy()); + CWallet dummyWallet(WalletLocation(), WalletDatabase::CreateDummy()); std::string backup_filename; if (!WalletBatch::Recover(wallet_path, (void *)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) { return false; @@ -4061,9 +4060,9 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string& return WalletBatch::VerifyDatabaseFile(wallet_path, warning_string, error_string); } -std::shared_ptr CWallet::CreateWalletFromFile(const std::string& name, const fs::path& path, uint64_t wallet_creation_flags) +std::shared_ptr CWallet::CreateWalletFromFile(const WalletLocation& location, uint64_t wallet_creation_flags) { - const std::string& walletFile = name; + const std::string& walletFile = location.GetName(); // needed to restore wallet transaction meta data after -zapwallettxes std::vector vWtx; @@ -4071,7 +4070,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(const std::string& name, if (gArgs.GetBoolArg("-zapwallettxes", false)) { uiInterface.InitMessage(_("Zapping all transactions from wallet...")); - std::unique_ptr tempWallet = MakeUnique(name, WalletDatabase::Create(path)); + std::unique_ptr tempWallet = MakeUnique(location, WalletDatabase::Create(location.GetPath())); DBErrors nZapWalletRet = tempWallet->ZapWalletTx(vWtx); if (nZapWalletRet != DBErrors::LOAD_OK) { InitError(strprintf(_("Error loading %s: Wallet corrupted"), walletFile)); @@ -4085,7 +4084,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(const std::string& name, bool fFirstRun = true; // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. - std::shared_ptr walletInstance(new CWallet(name, WalletDatabase::Create(path)), ReleaseWallet); + std::shared_ptr walletInstance(new CWallet(location, WalletDatabase::Create(location.GetPath())), ReleaseWallet); DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun); if (nLoadWalletRet != DBErrors::LOAD_OK) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1f20834843..5b7f88350a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -20,7 +20,7 @@ #include #include #include -#include +#include #include #include @@ -755,12 +755,8 @@ private: */ bool AddWatchOnly(const CScript& dest) override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - /** - * Wallet filename from wallet= command line or config option. - * Used in debug logs and to send RPCs to the right wallet instance when - * more than one wallet is loaded. - */ - std::string m_name; + /** Wallet location which includes wallet name (see WalletLocation). */ + WalletLocation m_location; /** Internal database handle. */ std::unique_ptr database; @@ -800,9 +796,11 @@ public: bool SelectCoins(const std::vector& vAvailableCoins, const CAmount& nTargetValue, std::set& setCoinsRet, CAmount& nValueRet, const CCoinControl& coin_control, CoinSelectionParams& coin_selection_params, bool& bnb_used) const; + const WalletLocation& GetLocation() const { return m_location; } + /** Get a name for this wallet for logging/debugging purposes. */ - const std::string& GetName() const { return m_name; } + const std::string& GetName() const { return m_location.GetName(); } void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void MarkPreSplitKeys(); @@ -818,7 +816,7 @@ public: unsigned int nMasterKeyMaxID = 0; /** Construct wallet with specified name and database implementation. */ - CWallet(std::string name, std::unique_ptr database) : m_name(std::move(name)), database(std::move(database)) + CWallet(const WalletLocation& location, std::unique_ptr database) : m_location(location), database(std::move(database)) { } @@ -1147,10 +1145,10 @@ public: bool MarkReplaced(const uint256& originalHash, const uint256& newHash); //! Verify wallet naming and perform salvage on the wallet if required - static bool Verify(std::string wallet_file, bool salvage_wallet, std::string& error_string, std::string& warning_string); + static bool Verify(const WalletLocation& location, bool salvage_wallet, std::string& error_string, std::string& warning_string); /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */ - static std::shared_ptr CreateWalletFromFile(const std::string& name, const fs::path& path, uint64_t wallet_creation_flags = 0); + static std::shared_ptr CreateWalletFromFile(const WalletLocation& location, uint64_t wallet_creation_flags = 0); /** * Wallet post-init setup diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index b8d105b49b..d9a4a131d6 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -30,7 +30,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "validation -> validationinterface -> validation" "wallet/coincontrol -> wallet/wallet -> wallet/coincontrol" "wallet/fees -> wallet/wallet -> wallet/fees" - "wallet/rpcwallet -> wallet/wallet -> wallet/rpcwallet" "wallet/wallet -> wallet/walletdb -> wallet/wallet" "policy/fees -> policy/policy -> validation -> policy/fees" "policy/rbf -> txmempool -> validation -> policy/rbf" -- cgit v1.2.3 From 8965b6ab4753f3223d90d9c1ab03b190f0320dd8 Mon Sep 17 00:00:00 2001 From: Chun Kuan Lee Date: Tue, 25 Sep 2018 21:56:16 +0800 Subject: wallet: Fix duplicate fileid Github-Pull: #14320 Rebased-From: 2d796fa --- src/wallet/db.cpp | 31 +++++++++++++++++++------------ src/wallet/db.h | 7 +++++++ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 2d57c92ccc..b36a6de2a3 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -20,6 +20,7 @@ #include namespace { + //! Make sure database has a unique fileid within the environment. If it //! doesn't, throw an error. BDB caches do not work properly when more than one //! open database has the same fileid (values written to one database may show @@ -29,25 +30,19 @@ namespace { //! (https://docs.oracle.com/cd/E17275_01/html/programmer_reference/program_copy.html), //! so bitcoin should never create different databases with the same fileid, but //! this error can be triggered if users manually copy database files. -void CheckUniqueFileid(const BerkeleyEnvironment& env, const std::string& filename, Db& db) +void CheckUniqueFileid(const BerkeleyEnvironment& env, const std::string& filename, Db& db, WalletDatabaseFileId& fileid) { if (env.IsMock()) return; - u_int8_t fileid[DB_FILE_ID_LEN]; - int ret = db.get_mpf()->get_fileid(fileid); + int ret = db.get_mpf()->get_fileid(fileid.value); if (ret != 0) { throw std::runtime_error(strprintf("BerkeleyBatch: Can't open database %s (get_fileid failed with %d)", filename, ret)); } - for (const auto& item : env.mapDb) { - u_int8_t item_fileid[DB_FILE_ID_LEN]; - if (item.second && item.second->get_mpf()->get_fileid(item_fileid) == 0 && - memcmp(fileid, item_fileid, sizeof(fileid)) == 0) { - const char* item_filename = nullptr; - item.second->get_dbname(&item_filename, nullptr); + for (const auto& item : env.m_fileids) { + if (fileid == item.second && &fileid != &item.second) { throw std::runtime_error(strprintf("BerkeleyBatch: Can't open database %s (duplicates fileid %s from %s)", filename, - HexStr(std::begin(item_fileid), std::end(item_fileid)), - item_filename ? item_filename : "(unknown database)")); + HexStr(std::begin(item.second.value), std::end(item.second.value)), item.first)); } } } @@ -56,6 +51,11 @@ CCriticalSection cs_db; std::map g_dbenvs GUARDED_BY(cs_db); //!< Map from directory name to open db environment. } // namespace +bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const +{ + return memcmp(value, &rhs.value, sizeof(value)) == 0; +} + BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) { fs::path env_directory; @@ -504,7 +504,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo // versions of BDB have an set_lk_exclusive method for this // purpose, but the older version we use does not.) for (auto& env : g_dbenvs) { - CheckUniqueFileid(env.second, strFilename, *pdb_temp); + CheckUniqueFileid(env.second, strFilename, *pdb_temp, this->env->m_fileids[strFilename]); } pdb = pdb_temp.release(); @@ -826,6 +826,13 @@ void BerkeleyDatabase::Flush(bool shutdown) LOCK(cs_db); g_dbenvs.erase(env->Directory().string()); env = nullptr; + } else { + // TODO: To avoid g_dbenvs.erase erasing the environment prematurely after the + // first database shutdown when multiple databases are open in the same + // environment, should replace raw database `env` pointers with shared or weak + // pointers, or else separate the database and environment shutdowns so + // environments can be shut down after databases. + env->m_fileids.erase(strFile); } } } diff --git a/src/wallet/db.h b/src/wallet/db.h index 467ed13b45..68a59607ae 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -25,6 +26,11 @@ static const unsigned int DEFAULT_WALLET_DBLOGSIZE = 100; static const bool DEFAULT_WALLET_PRIVDB = true; +struct WalletDatabaseFileId { + u_int8_t value[DB_FILE_ID_LEN]; + bool operator==(const WalletDatabaseFileId& rhs) const; +}; + class BerkeleyEnvironment { private: @@ -38,6 +44,7 @@ public: std::unique_ptr dbenv; std::map mapFileUseCount; std::map mapDb; + std::unordered_map m_fileids; std::condition_variable_any m_db_in_use; BerkeleyEnvironment(const fs::path& env_directory); -- cgit v1.2.3 From 34da2b7c76a023459e46e3a2ca4dc3ecc2b9a438 Mon Sep 17 00:00:00 2001 From: Chun Kuan Lee Date: Wed, 24 Oct 2018 12:33:50 +0800 Subject: tests: add test case for loading copied wallet twice Github-Pull: #14320 Rebased-From: 4ea7732 --- test/functional/wallet_multiwallet.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index f53172639c..6e0c109e60 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -220,6 +220,10 @@ class MultiWalletTest(BitcoinTestFramework): # Fail to load if one wallet is a copy of another assert_raises_rpc_error(-1, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy') + # Fail to load if one wallet is a copy of another, test this twice to make sure that we don't re-introduce #14304 + assert_raises_rpc_error(-1, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy') + + # Fail to load if wallet file is a symlink assert_raises_rpc_error(-4, "Wallet file verification failed: Invalid -wallet path 'w8_symlink'", self.nodes[0].loadwallet, 'w8_symlink') -- cgit v1.2.3 From caf1146b1345d70fbe4cc5f662d8393a79ac6068 Mon Sep 17 00:00:00 2001 From: Chun Kuan Lee Date: Tue, 23 Oct 2018 13:26:27 +0800 Subject: wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory. Github-Pull: #14552 Rebased-From: 15c93f0 --- src/wallet/db.cpp | 21 +++++++++++++++++++-- src/wallet/db.h | 3 +++ src/wallet/wallet.cpp | 8 +++----- test/functional/wallet_multiwallet.py | 3 +++ 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index b36a6de2a3..a0f0abd4a6 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -56,9 +56,8 @@ bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const return memcmp(value, &rhs.value, sizeof(value)) == 0; } -BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) +static void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename) { - fs::path env_directory; if (fs::is_regular_file(wallet_path)) { // Special case for backwards compatibility: if wallet path points to an // existing file, treat it as the path to a BDB data file in a parent @@ -71,6 +70,24 @@ BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& data env_directory = wallet_path; database_filename = "wallet.dat"; } +} + +bool IsWalletLoaded(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 db = env->second.m_databases.find(database_filename); + return db != env->second.m_databases.end(); +} + +BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) +{ + fs::path env_directory; + SplitWalletPath(wallet_path, env_directory, database_filename); LOCK(cs_db); // Note: An ununsed temporary BerkeleyEnvironment object may be created inside the // emplace function if the key already exists. This is a little inefficient, diff --git a/src/wallet/db.h b/src/wallet/db.h index 68a59607ae..5ced4c59cf 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -95,6 +95,9 @@ public: } }; +/** Return whether a wallet database is currently loaded. */ +bool IsWalletLoaded(const fs::path& wallet_path); + /** Get BerkeleyEnvironment and database filename given a wallet path. */ BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fefa1ae02b..e9fff5443b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4032,11 +4032,9 @@ bool CWallet::Verify(const WalletLocation& location, bool salvage_wallet, std::s } // Make sure that the wallet path doesn't clash with an existing wallet path - for (auto wallet : GetWallets()) { - if (wallet->GetLocation().GetPath() == wallet_path) { - error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName()); - return false; - } + if (IsWalletLoaded(wallet_path)) { + error_string = strprintf("Error loading wallet %s. Duplicate -wallet filename specified.", location.GetName()); + return false; } try { diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 6e0c109e60..bf33d3c628 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -217,6 +217,9 @@ class MultiWalletTest(BitcoinTestFramework): # 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]) + # 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') + # Fail to load if one wallet is a copy of another assert_raises_rpc_error(-1, "BerkeleyBatch: Can't open database w8_copy (duplicates fileid", self.nodes[0].loadwallet, 'w8_copy') -- cgit v1.2.3 From 7751ea37b65cae2cff766d09b2c95770aa7d71d8 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 24 Oct 2018 16:08:54 -0400 Subject: Refactor: Move m_db pointers into BerkeleyDatabase This is a refactoring change that doesn't affect behavior. The motivation behind the change is give BerkeleyEnvironment objects access to BerkeleyDatabase objects so it will be possible to simplify the duplicate wallet check and more reliably avoid opening the same databases twice. Github-Pull: #14552 Rebased-From: c456fbd --- src/wallet/db.cpp | 27 ++++++++++++++------------- src/wallet/db.h | 16 +++++++++++++++- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index a0f0abd4a6..f7ffd0d6bf 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -107,13 +107,13 @@ void BerkeleyEnvironment::Close() fDbEnvInit = false; - for (auto& db : mapDb) { + for (auto& db : m_databases) { auto count = mapFileUseCount.find(db.first); assert(count == mapFileUseCount.end() || count->second == 0); - if (db.second) { - db.second->close(0); - delete db.second; - db.second = nullptr; + BerkeleyDatabase& database = db.second.get(); + if (database.m_db) { + database.m_db->close(0); + database.m_db.reset(); } } @@ -480,7 +480,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo if (!env->Open(false /* retry */)) throw std::runtime_error("BerkeleyBatch: Failed to open database environment."); - pdb = env->mapDb[strFilename]; + pdb = database.m_db.get(); if (pdb == nullptr) { int ret; std::unique_ptr pdb_temp = MakeUnique(env->dbenv.get(), 0); @@ -525,7 +525,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo } pdb = pdb_temp.release(); - env->mapDb[strFilename] = pdb; + database.m_db.reset(pdb); if (fCreate && !Exists(std::string("version"))) { bool fTmp = fReadOnly; @@ -580,12 +580,13 @@ void BerkeleyEnvironment::CloseDb(const std::string& strFile) { { LOCK(cs_db); - if (mapDb[strFile] != nullptr) { + auto it = m_databases.find(strFile); + assert(it != m_databases.end()); + BerkeleyDatabase& database = it->second.get(); + if (database.m_db) { // Close the database handle - Db* pdb = mapDb[strFile]; - pdb->close(0); - delete pdb; - mapDb[strFile] = nullptr; + database.m_db->close(0); + database.m_db.reset(); } } } @@ -603,7 +604,7 @@ void BerkeleyEnvironment::ReloadDbEnv() }); std::vector filenames; - for (auto it : mapDb) { + for (auto it : m_databases) { filenames.push_back(it.first); } // Close the individual Db's diff --git a/src/wallet/db.h b/src/wallet/db.h index 5ced4c59cf..dd549bc463 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -31,6 +31,8 @@ struct WalletDatabaseFileId { bool operator==(const WalletDatabaseFileId& rhs) const; }; +class BerkeleyDatabase; + class BerkeleyEnvironment { private: @@ -43,7 +45,7 @@ private: public: std::unique_ptr dbenv; std::map mapFileUseCount; - std::map mapDb; + std::map> m_databases; std::unordered_map m_fileids; std::condition_variable_any m_db_in_use; @@ -118,6 +120,8 @@ public: nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0) { env = GetWalletEnv(wallet_path, strFile); + auto inserted = env->m_databases.emplace(strFile, std::ref(*this)); + assert(inserted.second); if (mock) { env->Close(); env->Reset(); @@ -125,6 +129,13 @@ public: } } + ~BerkeleyDatabase() { + if (env) { + size_t erased = env->m_databases.erase(strFile); + assert(erased == 1); + } + } + /** Return object for accessing database at specified path. */ static std::unique_ptr Create(const fs::path& path) { @@ -164,6 +175,9 @@ public: unsigned int nLastFlushed; int64_t nLastWalletUpdate; + /** Database pointer. This is initialized lazily and reset during flushes, so it can be null. */ + std::unique_ptr m_db; + private: /** BerkeleyDB specific */ BerkeleyEnvironment *env; -- cgit v1.2.3 From 0a9af2d4cb093d254a36d094b8d8ed7603fc9404 Mon Sep 17 00:00:00 2001 From: Chun Kuan Lee Date: Thu, 8 Nov 2018 11:41:56 +0800 Subject: wallet: Create IsDatabaseLoaded function Github-Pull: #14552 Rebased-From: 5912031 --- src/wallet/db.cpp | 3 +-- src/wallet/db.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index f7ffd0d6bf..2d841ff8fd 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -80,8 +80,7 @@ bool IsWalletLoaded(const fs::path& wallet_path) LOCK(cs_db); auto env = g_dbenvs.find(env_directory.string()); if (env == g_dbenvs.end()) return false; - auto db = env->second.m_databases.find(database_filename); - return db != env->second.m_databases.end(); + return env->second.IsDatabaseLoaded(database_filename); } BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) diff --git a/src/wallet/db.h b/src/wallet/db.h index dd549bc463..6af37c12cd 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -56,6 +56,7 @@ public: void MakeMock(); 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; } /** -- cgit v1.2.3 From f22d02f5371efcaa48a8d5d1b8cd31c65d8235f3 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 18 May 2018 16:28:50 -0400 Subject: Free BerkeleyEnvironment instances when not in use Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map, use reference counted shared pointers and remove map entries when the last BerkeleyEnvironment reference goes out of scope. This change was requested by Matt Corallo and makes code that sets up mock databases cleaner. The mock database environment will now go out of scope and be reset on destruction so there is no need to call BerkeleyEnvironment::Reset() during wallet construction to clear out prior state. This change does affect bitcoin behavior slightly. On startup, instead of same wallet environments staying open throughout VerifyWallets() and OpenWallets() calls, VerifyWallets() will open and close an environment once for each wallet, and OpenWallets() will create its own environment(s) later. Github-Pull: #11911 Rebased-From: f1f4bb7 --- src/wallet/db.cpp | 39 +++++++++++++++++++++------------------ src/wallet/db.h | 34 +++++++++++++++++++--------------- src/wallet/wallet.cpp | 3 +++ 3 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 2d841ff8fd..624b4c609b 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -48,7 +48,7 @@ void CheckUniqueFileid(const BerkeleyEnvironment& env, const std::string& filena } CCriticalSection cs_db; -std::map g_dbenvs GUARDED_BY(cs_db); //!< Map from directory name to open db environment. +std::map> g_dbenvs GUARDED_BY(cs_db); //!< Map from directory name to db environment. } // namespace bool WalletDatabaseFileId::operator==(const WalletDatabaseFileId& rhs) const @@ -80,19 +80,22 @@ bool IsWalletLoaded(const fs::path& wallet_path) LOCK(cs_db); auto env = g_dbenvs.find(env_directory.string()); if (env == g_dbenvs.end()) return false; - return env->second.IsDatabaseLoaded(database_filename); + auto database = env->second.lock(); + return database && database->IsDatabaseLoaded(database_filename); } -BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) +std::shared_ptr GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) { fs::path env_directory; SplitWalletPath(wallet_path, env_directory, database_filename); LOCK(cs_db); - // Note: An ununsed temporary BerkeleyEnvironment object may be created inside the - // emplace function if the key already exists. This is a little inefficient, - // but not a big concern since the map will be changed in the future to hold - // pointers instead of objects, anyway. - return &g_dbenvs.emplace(std::piecewise_construct, std::forward_as_tuple(env_directory.string()), std::forward_as_tuple(env_directory)).first->second; + auto inserted = g_dbenvs.emplace(env_directory.string(), std::weak_ptr()); + if (inserted.second) { + auto env = std::make_shared(env_directory.string()); + inserted.first->second = env; + return env; + } + return inserted.first->second.lock(); } // @@ -137,6 +140,7 @@ BerkeleyEnvironment::BerkeleyEnvironment(const fs::path& dir_path) : strPath(dir BerkeleyEnvironment::~BerkeleyEnvironment() { + g_dbenvs.erase(strPath); Close(); } @@ -214,10 +218,9 @@ bool BerkeleyEnvironment::Open(bool retry) return true; } -void BerkeleyEnvironment::MakeMock() +BerkeleyEnvironment::BerkeleyEnvironment() { - if (fDbEnvInit) - throw std::runtime_error("BerkeleyEnvironment::MakeMock: Already initialized"); + Reset(); boost::this_thread::interruption_point(); @@ -266,7 +269,7 @@ BerkeleyEnvironment::VerifyResult BerkeleyEnvironment::Verify(const std::string& bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& newFilename) { std::string filename; - BerkeleyEnvironment* env = GetWalletEnv(file_path, filename); + std::shared_ptr env = GetWalletEnv(file_path, filename); // Recovery procedure: // move wallet file to walletfilename.timestamp.bak @@ -335,7 +338,7 @@ bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, boo bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& errorStr) { std::string walletFile; - BerkeleyEnvironment* env = GetWalletEnv(file_path, walletFile); + std::shared_ptr env = GetWalletEnv(file_path, walletFile); fs::path walletDir = env->Directory(); LogPrintf("Using BerkeleyDB version %s\n", DbEnv::version(0, 0, 0)); @@ -359,7 +362,7 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& er bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::string& warningStr, std::string& errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc) { std::string walletFile; - BerkeleyEnvironment* env = GetWalletEnv(file_path, walletFile); + std::shared_ptr env = GetWalletEnv(file_path, walletFile); fs::path walletDir = env->Directory(); if (fs::exists(walletDir / walletFile)) @@ -463,7 +466,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo { fReadOnly = (!strchr(pszMode, '+') && !strchr(pszMode, 'w')); fFlushOnClose = fFlushOnCloseIn; - env = database.env; + env = database.env.get(); if (database.IsDummy()) { return; } @@ -520,7 +523,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo // versions of BDB have an set_lk_exclusive method for this // purpose, but the older version we use does not.) for (auto& env : g_dbenvs) { - CheckUniqueFileid(env.second, strFilename, *pdb_temp, this->env->m_fileids[strFilename]); + CheckUniqueFileid(*env.second.lock().get(), strFilename, *pdb_temp, this->env->m_fileids[strFilename]); } pdb = pdb_temp.release(); @@ -621,7 +624,7 @@ bool BerkeleyBatch::Rewrite(BerkeleyDatabase& database, const char* pszSkip) if (database.IsDummy()) { return true; } - BerkeleyEnvironment *env = database.env; + BerkeleyEnvironment *env = database.env.get(); const std::string& strFile = database.strFile; while (true) { { @@ -752,7 +755,7 @@ bool BerkeleyBatch::PeriodicFlush(BerkeleyDatabase& database) return true; } bool ret = false; - BerkeleyEnvironment *env = database.env; + BerkeleyEnvironment *env = database.env.get(); const std::string& strFile = database.strFile; TRY_LOCK(cs_db, lockDb); if (lockDb) diff --git a/src/wallet/db.h b/src/wallet/db.h index 6af37c12cd..ac70dc55e1 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -50,10 +50,10 @@ public: std::condition_variable_any m_db_in_use; BerkeleyEnvironment(const fs::path& env_directory); + BerkeleyEnvironment(); ~BerkeleyEnvironment(); void Reset(); - void MakeMock(); 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(); } @@ -102,7 +102,7 @@ public: bool IsWalletLoaded(const fs::path& wallet_path); /** Get BerkeleyEnvironment and database filename given a wallet path. */ -BerkeleyEnvironment* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename); +std::shared_ptr GetWalletEnv(const fs::path& wallet_path, std::string& database_filename); /** An instance of this class represents one database. * For BerkeleyDB this is just a (env, strFile) tuple. @@ -117,17 +117,11 @@ public: } /** Create DB handle to real database */ - BerkeleyDatabase(const fs::path& wallet_path, bool mock = false) : - nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0) + BerkeleyDatabase(std::shared_ptr env, std::string filename) : + nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(std::move(env)), strFile(std::move(filename)) { - env = GetWalletEnv(wallet_path, strFile); - auto inserted = env->m_databases.emplace(strFile, std::ref(*this)); + auto inserted = this->env->m_databases.emplace(strFile, std::ref(*this)); assert(inserted.second); - if (mock) { - env->Close(); - env->Reset(); - env->MakeMock(); - } } ~BerkeleyDatabase() { @@ -140,7 +134,8 @@ public: /** Return object for accessing database at specified path. */ static std::unique_ptr Create(const fs::path& path) { - return MakeUnique(path); + std::string filename; + return MakeUnique(GetWalletEnv(path, filename), std::move(filename)); } /** Return object for accessing dummy database with no read/write capabilities. */ @@ -152,7 +147,7 @@ public: /** Return object for accessing temporary in-memory database. */ static std::unique_ptr CreateMock() { - return MakeUnique("", true /* mock */); + return MakeUnique(std::make_shared(), ""); } /** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero @@ -176,12 +171,21 @@ public: unsigned int nLastFlushed; int64_t nLastWalletUpdate; + /** + * Pointer to shared database environment. + * + * Normally there is only one BerkeleyDatabase object per + * BerkeleyEnvivonment, but in the special, backwards compatible case where + * multiple wallet BDB data files are loaded from the same directory, this + * will point to a shared instance that gets freed when the last data file + * is closed. + */ + std::shared_ptr env; + /** Database pointer. This is initialized lazily and reset during flushes, so it can be null. */ std::unique_ptr m_db; private: - /** BerkeleyDB specific */ - BerkeleyEnvironment *env; std::string strFile; /** Return whether this database handle is a dummy for testing. diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e9fff5443b..8d52f7eeca 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4037,6 +4037,9 @@ bool CWallet::Verify(const WalletLocation& location, bool salvage_wallet, std::s return false; } + // Keep same database environment instance across Verify/Recover calls below. + std::unique_ptr database = WalletDatabase::Create(wallet_path); + try { if (!WalletBatch::VerifyEnvironment(wallet_path, error_string)) { return false; -- cgit v1.2.3 From 85c6263ddbde7189bbb52317dd3ad9202b5ebf40 Mon Sep 17 00:00:00 2001 From: Pierre Rochard Date: Fri, 14 Sep 2018 14:13:16 -0400 Subject: Trivial: add doxygen-compatible comments relating to BerkeleyEnvironment Github-Pull: #11911 Rebased-From: 14bc2a1 --- src/wallet/db.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 624b4c609b..408713c0c6 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -84,6 +84,13 @@ bool IsWalletLoaded(const fs::path& wallet_path) 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. + * @return A shared pointer to the BerkeleyEnvironment object for the wallet directory, never empty because ~BerkeleyEnvironment + * erases the weak pointer from the g_dbenvs map. + * @post A new BerkeleyEnvironment weak pointer is inserted into g_dbenvs if the directory path key was not already in the map. + */ std::shared_ptr GetWalletEnv(const fs::path& wallet_path, std::string& database_filename) { fs::path env_directory; @@ -218,6 +225,7 @@ bool BerkeleyEnvironment::Open(bool retry) return true; } +//! Construct an in-memory mock Berkeley environment for testing and as a place-holder for g_dbenvs emplace BerkeleyEnvironment::BerkeleyEnvironment() { Reset(); -- cgit v1.2.3 From f20513bd71d0530ad9285b9558e3a02733250a63 Mon Sep 17 00:00:00 2001 From: Pierre Rochard Date: Fri, 14 Sep 2018 21:10:20 -0400 Subject: Tests: add unit tests for GetWalletEnv Github-Pull: #11911 Rebased-From: 88b1d95 --- src/Makefile.test.include | 1 + src/wallet/test/db_tests.cpp | 72 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) create mode 100644 src/wallet/test/db_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 6f401636f5..1b6827d45a 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -95,6 +95,7 @@ BITCOIN_TESTS =\ if ENABLE_WALLET BITCOIN_TESTS += \ wallet/test/accounting_tests.cpp \ + wallet/test/db_tests.cpp \ wallet/test/psbt_wallet_tests.cpp \ wallet/test/wallet_tests.cpp \ wallet/test/wallet_crypto_tests.cpp \ diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp new file mode 100644 index 0000000000..2a64749379 --- /dev/null +++ b/src/wallet/test/db_tests.cpp @@ -0,0 +1,72 @@ +// Copyright (c) 2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include + +#include +#include +#include + + +BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(getwalletenv_file) +{ + std::string test_name = "test_name.dat"; + fs::path datadir = SetDataDir("tempdir"); + fs::path file_path = datadir / test_name; + std::ofstream f(file_path.BOOST_FILESYSTEM_C_STR); + f.close(); + + std::string filename; + std::shared_ptr env = GetWalletEnv(file_path, filename); + BOOST_CHECK(filename == test_name); + BOOST_CHECK(env->Directory() == datadir); +} + +BOOST_AUTO_TEST_CASE(getwalletenv_directory) +{ + std::string expected_name = "wallet.dat"; + fs::path datadir = SetDataDir("tempdir"); + + std::string filename; + std::shared_ptr env = GetWalletEnv(datadir, filename); + BOOST_CHECK(filename == expected_name); + BOOST_CHECK(env->Directory() == datadir); +} + +BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_multiple) +{ + fs::path datadir = SetDataDir("tempdir"); + fs::path datadir_2 = SetDataDir("tempdir_2"); + std::string filename; + + std::shared_ptr env_1 = GetWalletEnv(datadir, filename); + std::shared_ptr env_2 = GetWalletEnv(datadir, filename); + std::shared_ptr env_3 = GetWalletEnv(datadir_2, filename); + + BOOST_CHECK(env_1 == env_2); + BOOST_CHECK(env_2 != env_3); +} + +BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_free_instance) +{ + fs::path datadir = SetDataDir("tempdir"); + fs::path datadir_2 = SetDataDir("tempdir_2"); + std::string filename; + + std::shared_ptr env_1_a = GetWalletEnv(datadir, filename); + std::shared_ptr env_2_a = GetWalletEnv(datadir_2, filename); + env_1_a.reset(); + + std::shared_ptr env_1_b = GetWalletEnv(datadir, filename); + std::shared_ptr env_2_b = GetWalletEnv(datadir_2, filename); + + BOOST_CHECK(env_1_a != env_1_b); + BOOST_CHECK(env_2_a == env_2_b); +} + +BOOST_AUTO_TEST_SUITE_END() -- cgit v1.2.3 From 22cdb6cf590d61668c85c1c08dcc15b4e95921c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Thu, 31 Jan 2019 00:04:51 +0000 Subject: wallet: Close dbenv error file db.log The error file db.log is opened by BerkeleyEnvironment instance and should be closed after dbenv is closed. Github-Pull: #15297 Rebased-From: 8602a1e --- src/wallet/db.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 408713c0c6..7e7fd24a8c 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -126,11 +126,16 @@ void BerkeleyEnvironment::Close() } } + FILE* error_file = nullptr; + dbenv->get_errfile(&error_file); + int ret = dbenv->close(0); if (ret != 0) LogPrintf("BerkeleyEnvironment::Close: Error %d closing database environment: %s\n", ret, DbEnv::strerror(ret)); if (!fMockDb) DbEnv((u_int32_t)0).remove(strPath.c_str(), 0); + + if (error_file) fclose(error_file); } void BerkeleyEnvironment::Reset() -- cgit v1.2.3 From 2e9e904a5d58e0d288e9abc1cbc602a8674bc1a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Thu, 31 Jan 2019 00:05:18 +0000 Subject: wallet: Close wallet env lock file Close .walletlock file when a BerkeleyEnvironment is deleted. Github-Pull: #15297 Rebased-From: 2f8b8f4 --- src/util.cpp | 6 ++++++ src/util.h | 1 + src/wallet/db.cpp | 2 ++ 3 files changed, 9 insertions(+) diff --git a/src/util.cpp b/src/util.cpp index a391c5e857..b41e61ddf5 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -174,6 +174,12 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b return true; } +void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name) +{ + std::lock_guard lock(cs_dir_locks); + dir_locks.erase((directory / lockfile_name).string()); +} + void ReleaseDirectoryLocks() { std::lock_guard ulock(cs_dir_locks); diff --git a/src/util.h b/src/util.h index 23b2a787e4..9576541249 100644 --- a/src/util.h +++ b/src/util.h @@ -77,6 +77,7 @@ int RaiseFileDescriptorLimit(int nMinFD); void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length); bool RenameOver(fs::path src, fs::path dest); bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false); +void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name); bool DirIsWritable(const fs::path& directory); /** Release all directory locks. This is used for unit testing only, at runtime diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 7e7fd24a8c..89b68495ad 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -136,6 +136,8 @@ void BerkeleyEnvironment::Close() DbEnv((u_int32_t)0).remove(strPath.c_str(), 0); if (error_file) fclose(error_file); + + UnlockDirectory(strPath, ".walletlock"); } void BerkeleyEnvironment::Reset() -- cgit v1.2.3 From fe95f84542f81862e9759503416d9da9f67d191b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Mon, 4 Feb 2019 18:50:21 +0000 Subject: qa: Test .walletlock file is closed Github-Pull: #15297 Rebased-From: d3bf3b9 --- test/functional/wallet_multiwallet.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index bf33d3c628..712a10b731 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -311,6 +311,14 @@ class MultiWalletTest(BitcoinTestFramework): self.nodes[0].loadwallet(wallet_name) assert_equal(rpc.getaddressinfo(addr)['ismine'], True) + # Test .walletlock file is closed + self.start_node(1) + wallet = os.path.join(self.options.tmpdir, 'my_wallet') + self.nodes[0].createwallet(wallet) + assert_raises_rpc_error(-4, "Error initializing wallet database environment", self.nodes[1].loadwallet, wallet) + self.nodes[0].unloadwallet(wallet) + self.nodes[1].loadwallet(wallet) + if __name__ == '__main__': MultiWalletTest().main() -- cgit v1.2.3