From 27b27663849932971eb5deadb1f19234b9cd97ea Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 15 Jun 2020 17:59:24 -0400 Subject: [PATCH 1/4] walletdb: Move BerkeleyDatabase::Flush(true) to Close() Instead of having Flush optionally shutdown the database and environment, add a Close() function that does that. --- src/wallet/bdb.cpp | 32 ++++++++++++++++++-------------- src/wallet/bdb.h | 15 +++++++-------- src/wallet/load.cpp | 4 ++-- src/wallet/wallet.cpp | 9 +++++++-- src/wallet/wallet.h | 5 ++++- src/wallet/wallettool.cpp | 6 +++--- 6 files changed, 41 insertions(+), 30 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 44d1bafaf61..0989dc21c32 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -324,6 +324,15 @@ void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile) dbenv->lsn_reset(strFile.c_str(), 0); } +BerkeleyDatabase::~BerkeleyDatabase() +{ + if (env) { + LOCK(cs_db); + size_t erased = env->m_databases.erase(strFile); + assert(erased == 1); + env->m_fileids.erase(strFile); + } +} BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr) { @@ -685,22 +694,17 @@ bool BerkeleyDatabase::Backup(const std::string& strDest) const } } -void BerkeleyDatabase::Flush(bool shutdown) +void BerkeleyDatabase::Flush() { if (!IsDummy()) { - env->Flush(shutdown); - if (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); - } + env->Flush(false); + } +} + +void BerkeleyDatabase::Close() +{ + if (!IsDummy()) { + env->Flush(true); } } diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index e54776fc0d2..52cd9d92dfd 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -115,12 +115,7 @@ public: assert(inserted.second); } - ~BerkeleyDatabase() { - if (env) { - size_t erased = env->m_databases.erase(strFile); - assert(erased == 1); - } - } + ~BerkeleyDatabase(); /** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero */ @@ -130,9 +125,13 @@ public: */ bool Backup(const std::string& strDest) const; - /** Make sure all changes are flushed to disk. + /** Make sure all changes are flushed to database file. */ - void Flush(bool shutdown); + void Flush(); + /** Flush to the database file and close the database. + * Also close the environment if no other databases are open in it. + */ + void Close(); /* flush the wallet passively (TRY_LOCK) ideal to be called periodically */ bool PeriodicFlush(); diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index c2818a41e70..2a81d30133d 100644 --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -99,14 +99,14 @@ void StartWallets(CScheduler& scheduler, const ArgsManager& args) void FlushWallets() { for (const std::shared_ptr& pwallet : GetWallets()) { - pwallet->Flush(false); + pwallet->Flush(); } } void StopWallets() { for (const std::shared_ptr& pwallet : GetWallets()) { - pwallet->Flush(true); + pwallet->Close(); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8eec00993fe..cee2f2214c7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -439,9 +439,14 @@ bool CWallet::HasWalletSpend(const uint256& txid) const return (iter != mapTxSpends.end() && iter->first.hash == txid); } -void CWallet::Flush(bool shutdown) +void CWallet::Flush() { - database->Flush(shutdown); + database->Flush(); +} + +void CWallet::Close() +{ + database->Close(); } void CWallet::SyncMetaData(std::pair range) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8cb2a64484d..a761caf38c6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1087,7 +1087,10 @@ public: bool HasWalletSpend(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Flush wallet (bitdb flush) - void Flush(bool shutdown=false); + void Flush(); + + //! Close wallet database + void Close(); /** Wallet is about to be unloaded */ boost::signals2::signal NotifyUnload; diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 8a45d81456f..9f25b1ae7dd 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -17,7 +17,7 @@ namespace WalletTool { static void WalletToolReleaseWallet(CWallet* wallet) { wallet->WalletLogPrintf("Releasing wallet\n"); - wallet->Flush(true); + wallet->Close(); delete wallet; } @@ -133,7 +133,7 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name) std::shared_ptr wallet_instance = CreateWallet(name, path); if (wallet_instance) { WalletShowInfo(wallet_instance.get()); - wallet_instance->Flush(true); + wallet_instance->Close(); } } else if (command == "info" || command == "salvage") { if (!fs::exists(path)) { @@ -145,7 +145,7 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name) std::shared_ptr wallet_instance = LoadWallet(name, path); if (!wallet_instance) return false; WalletShowInfo(wallet_instance.get()); - wallet_instance->Flush(true); + wallet_instance->Close(); } else if (command == "salvage") { return SalvageWallet(path); } From 71d28e7cdca1c8553531bb3a4725d7916363ec5c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 19 Jun 2020 20:51:07 -0400 Subject: [PATCH 2/4] walletdb: Introduce AddRef and RemoveRef functions Refactor mapFileUseCount increment and decrement to separate functions AddRef and RemoveRef --- src/wallet/bdb.cpp | 25 ++++++++++++++++++------- src/wallet/bdb.h | 6 ++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 0989dc21c32..8ca4991f04d 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -334,7 +334,7 @@ BerkeleyDatabase::~BerkeleyDatabase() } } -BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr) +BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database) { fReadOnly = (!strchr(pszMode, '+') && !strchr(pszMode, 'w')); fFlushOnClose = fFlushOnCloseIn; @@ -408,7 +408,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo fReadOnly = fTmp; } } - ++env->mapFileUseCount[strFilename]; + database.AddRef(); strFile = strFilename; } } @@ -446,11 +446,7 @@ void BerkeleyBatch::Close() if (fFlushOnClose) Flush(); - { - LOCK(cs_db); - --env->mapFileUseCount[strFile]; - } - env->m_db_in_use.notify_all(); + m_database.RemoveRef(); } void BerkeleyEnvironment::CloseDb(const std::string& strFile) @@ -846,6 +842,21 @@ bool BerkeleyBatch::HasKey(CDataStream&& key) return ret == 0; } +void BerkeleyDatabase::AddRef() +{ + LOCK(cs_db); + ++env->mapFileUseCount[strFile]; +} + +void BerkeleyDatabase::RemoveRef() +{ + { + LOCK(cs_db); + --env->mapFileUseCount[strFile]; + } + env->m_db_in_use.notify_all(); +} + std::unique_ptr BerkeleyDatabase::MakeBatch(const char* mode, bool flush_on_close) { return MakeUnique(*this, mode, flush_on_close); diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 52cd9d92dfd..9c4d6afa87d 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -121,6 +121,11 @@ public: */ bool Rewrite(const char* pszSkip=nullptr); + /** Indicate the a new database user has began using the database. */ + void AddRef(); + /** Indicate that database user has stopped using the database and that it could be flushed or closed. */ + void RemoveRef(); + /** Back up the entire database to a file. */ bool Backup(const std::string& strDest) const; @@ -212,6 +217,7 @@ protected: bool fReadOnly; bool fFlushOnClose; BerkeleyEnvironment *env; + BerkeleyDatabase& m_database; public: explicit BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode = "r+", bool fFlushOnCloseIn=true); From 2179dbcbcd0b9bef7ad9c907b85294b9a1bccf0f Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 19 Jun 2020 20:55:07 -0400 Subject: [PATCH 3/4] walletdb: Add BerkeleyDatabase::Open dummy function Adds an Open function for the class abstraction that does nothing for now. --- src/wallet/bdb.cpp | 5 +++++ src/wallet/bdb.h | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 8ca4991f04d..06ee9ae5eed 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -413,6 +413,11 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo } } +void BerkeleyDatabase::Open(const char* mode) +{ + throw std::logic_error("BerkeleyDatabase does not implement Open. This function should not be called."); +} + void BerkeleyBatch::Flush() { if (activeTxn) diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 9c4d6afa87d..bbaeefa4140 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -117,6 +117,10 @@ public: ~BerkeleyDatabase(); + /** Open the database if it is not already opened. + * Dummy function, doesn't do anything right now, but is needed for class abstraction */ + void Open(const char* mode); + /** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero */ bool Rewrite(const char* pszSkip=nullptr); From d416ae560e46a4846a3fd5990b7d390d2ef30ec8 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 15 Jun 2020 16:24:00 -0400 Subject: [PATCH 4/4] walletdb: Introduce WalletDatabase abstract class Make WalletDatabase actually an abstract class and not just a typedef for BerkeleyDatabase. Have BerkeleyDatabase inherit this class. --- src/qt/rpcconsole.cpp | 1 - src/wallet/bdb.cpp | 2 +- src/wallet/bdb.h | 37 +++++++++++-------------- src/wallet/db.h | 60 +++++++++++++++++++++++++++++++++++++++++ src/wallet/walletdb.cpp | 6 ++--- src/wallet/walletdb.h | 11 +++----- 6 files changed, 84 insertions(+), 33 deletions(-) diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 71094f7112c..85b09d8baca 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -24,7 +24,6 @@ #include #ifdef ENABLE_WALLET -#include #include #include #endif diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 06ee9ae5eed..7fed9601b0b 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -862,7 +862,7 @@ void BerkeleyDatabase::RemoveRef() env->m_db_in_use.notify_all(); } -std::unique_ptr BerkeleyDatabase::MakeBatch(const char* mode, bool flush_on_close) +std::unique_ptr BerkeleyDatabase::MakeBatch(const char* mode, bool flush_on_close) { return MakeUnique(*this, mode, flush_on_close); } diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index bbaeefa4140..b8b585631ac 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -98,64 +98,59 @@ class BerkeleyBatch; /** An instance of this class represents one database. * For BerkeleyDB this is just a (env, strFile) tuple. **/ -class BerkeleyDatabase +class BerkeleyDatabase : public WalletDatabase { friend class BerkeleyBatch; public: /** Create dummy DB handle */ - BerkeleyDatabase() : nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(nullptr) + BerkeleyDatabase() : WalletDatabase(), env(nullptr) { } /** Create DB handle to real database */ BerkeleyDatabase(std::shared_ptr env, std::string filename) : - nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(std::move(env)), strFile(std::move(filename)) + WalletDatabase(), env(std::move(env)), strFile(std::move(filename)) { auto inserted = this->env->m_databases.emplace(strFile, std::ref(*this)); assert(inserted.second); } - ~BerkeleyDatabase(); + ~BerkeleyDatabase() override; /** Open the database if it is not already opened. * Dummy function, doesn't do anything right now, but is needed for class abstraction */ - void Open(const char* mode); + void Open(const char* mode) override; /** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero */ - bool Rewrite(const char* pszSkip=nullptr); + bool Rewrite(const char* pszSkip=nullptr) override; /** Indicate the a new database user has began using the database. */ - void AddRef(); + void AddRef() override; /** Indicate that database user has stopped using the database and that it could be flushed or closed. */ - void RemoveRef(); + void RemoveRef() override; /** Back up the entire database to a file. */ - bool Backup(const std::string& strDest) const; + bool Backup(const std::string& strDest) const override; /** Make sure all changes are flushed to database file. */ - void Flush(); + void Flush() override; /** Flush to the database file and close the database. * Also close the environment if no other databases are open in it. */ - void Close(); + void Close() override; /* flush the wallet passively (TRY_LOCK) ideal to be called periodically */ - bool PeriodicFlush(); + bool PeriodicFlush() override; - void IncrementUpdateCounter(); + void IncrementUpdateCounter() override; - void ReloadDbEnv(); - - std::atomic nUpdateCounter; - unsigned int nLastSeen; - unsigned int nLastFlushed; - int64_t nLastWalletUpdate; + void ReloadDbEnv() override; /** Verifies the environment and database file */ - bool Verify(bilingual_str& error); + bool Verify(bilingual_str& error) override; /** * Pointer to shared database environment. @@ -172,7 +167,7 @@ public: std::unique_ptr m_db; /** Make a BerkeleyBatch connected to this database */ - std::unique_ptr MakeBatch(const char* mode, bool flush_on_close); + std::unique_ptr MakeBatch(const char* mode = "r+", bool flush_on_close = true) override; private: std::string strFile; diff --git a/src/wallet/db.h b/src/wallet/db.h index 76668f8dc28..12dc1cc96bd 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -10,8 +10,12 @@ #include #include +#include +#include #include +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); @@ -94,4 +98,60 @@ public: virtual bool TxnAbort() = 0; }; +/** An instance of this class represents one database. + **/ +class WalletDatabase +{ +public: + /** Create dummy DB handle */ + WalletDatabase() : nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0) {} + virtual ~WalletDatabase() {}; + + /** Open the database if it is not already opened. */ + virtual void Open(const char* mode) = 0; + + //! Counts the number of active database users to be sure that the database is not closed while someone is using it + std::atomic m_refcount{0}; + /** Indicate the a new database user has began using the database. Increments m_refcount */ + virtual void AddRef() = 0; + /** Indicate that database user has stopped using the database and that it could be flushed or closed. Decrement m_refcount */ + virtual void RemoveRef() = 0; + + /** Rewrite the entire database on disk, with the exception of key pszSkip if non-zero + */ + virtual bool Rewrite(const char* pszSkip=nullptr) = 0; + + /** Back up the entire database to a file. + */ + virtual bool Backup(const std::string& strDest) const = 0; + + /** Make sure all changes are flushed to database file. + */ + virtual void Flush() = 0; + /** Flush to the database file and close the database. + * Also close the environment if no other databases are open in it. + */ + virtual void Close() = 0; + /* flush the wallet passively (TRY_LOCK) + ideal to be called periodically */ + virtual bool PeriodicFlush() = 0; + + virtual void IncrementUpdateCounter() = 0; + + virtual void ReloadDbEnv() = 0; + + std::atomic nUpdateCounter; + unsigned int nLastSeen; + 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 */ + virtual std::unique_ptr MakeBatch(const char* mode = "r+", bool flush_on_close = true) = 0; +}; + #endif // BITCOIN_WALLET_DB_H diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 1478687bf94..8c409b40cd0 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1012,20 +1012,20 @@ bool IsWalletLoaded(const fs::path& wallet_path) } /** Return object for accessing database at specified path. */ -std::unique_ptr CreateWalletDatabase(const fs::path& 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() +std::unique_ptr CreateDummyWalletDatabase() { return MakeUnique(); } /** Return object for accessing temporary in-memory database. */ -std::unique_ptr CreateMockWalletDatabase() +std::unique_ptr CreateMockWalletDatabase() { return MakeUnique(std::make_shared(), ""); } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 6b55361c07d..7c5bf7652b7 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -40,9 +40,6 @@ class CWalletTx; class uint160; class uint256; -/** Backend-agnostic database type. */ -using WalletDatabase = BerkeleyDatabase; - /** Error statuses for the wallet database */ enum class DBErrors { @@ -280,7 +277,7 @@ public: //! Abort current transaction bool TxnAbort(); private: - std::unique_ptr m_batch; + std::unique_ptr m_batch; WalletDatabase& m_database; }; @@ -294,12 +291,12 @@ bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, st bool IsWalletLoaded(const fs::path& wallet_path); /** Return object for accessing database at specified path. */ -std::unique_ptr CreateWalletDatabase(const fs::path& path); +std::unique_ptr CreateWalletDatabase(const fs::path& path); /** Return object for accessing dummy database with no read/write capabilities. */ -std::unique_ptr CreateDummyWalletDatabase(); +std::unique_ptr CreateDummyWalletDatabase(); /** Return object for accessing temporary in-memory database. */ -std::unique_ptr CreateMockWalletDatabase(); +std::unique_ptr CreateMockWalletDatabase(); #endif // BITCOIN_WALLET_WALLETDB_H