From c456fbd8dfcc748e5ec9feaa57ec0f2900f99cde Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 24 Oct 2018 16:08:54 -0400 Subject: [PATCH 1/3] 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. --- 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 74787eb5d2..b2404e2a85 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -90,13 +90,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(); } } @@ -463,7 +463,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); @@ -508,7 +508,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; @@ -563,12 +563,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(); } } } @@ -586,7 +587,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 8f96483a18..e9f89ac6cb 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; @@ -115,6 +117,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(); @@ -122,6 +126,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) { @@ -161,6 +172,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; From 15c93f075a881deb3ad7b1dd8a4516a9b06e5e11 Mon Sep 17 00:00:00 2001 From: Chun Kuan Lee Date: Tue, 23 Oct 2018 13:26:27 +0800 Subject: [PATCH 2/3] wallet: Add trailing wallet.dat when detecting duplicate wallet if it's a directory. --- 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 b2404e2a85..bdd7b41b9d 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 unused 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 e9f89ac6cb..9832094ccb 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -97,6 +97,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 2ea9f45462..0061ac2294 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3848,11 +3848,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 4f663c82c7..8ab569a3c3 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -223,6 +223,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') From 591203149f1700f594f781862e88cbbfe83d8d37 Mon Sep 17 00:00:00 2001 From: Chun Kuan Lee Date: Thu, 8 Nov 2018 11:41:56 +0800 Subject: [PATCH 3/3] wallet: Create IsDatabaseLoaded function --- 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 bdd7b41b9d..d75e30d336 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 9832094ccb..e453d441d7 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; } /**