diff options
author | Samuel Dobson <dobsonsa68@gmail.com> | 2020-07-23 14:49:17 +1200 |
---|---|---|
committer | Samuel Dobson <dobsonsa68@gmail.com> | 2020-07-23 15:22:25 +1200 |
commit | 9d4b3d86b694ac6e56495e1955f6bf5ff584cbb9 (patch) | |
tree | 49cc2ecdb359158e25b0bb56982a0eb97e62b8f0 | |
parent | ccef10261efc235c8fcc8aad54556615b0cc23be (diff) | |
parent | d416ae560e46a4846a3fd5990b7d390d2ef30ec8 (diff) | |
download | bitcoin-9d4b3d86b694ac6e56495e1955f6bf5ff584cbb9.tar.xz |
Merge #19334: wallet: Introduce WalletDatabase abstract class
d416ae560e46a4846a3fd5990b7d390d2ef30ec8 walletdb: Introduce WalletDatabase abstract class (Andrew Chow)
2179dbcbcd0b9bef7ad9c907b85294b9a1bccf0f walletdb: Add BerkeleyDatabase::Open dummy function (Andrew Chow)
71d28e7cdca1c8553531bb3a4725d7916363ec5c walletdb: Introduce AddRef and RemoveRef functions (Andrew Chow)
27b27663849932971eb5deadb1f19234b9cd97ea walletdb: Move BerkeleyDatabase::Flush(true) to Close() (Andrew Chow)
Pull request description:
A `WalletDatabase` abstract class is created from `BerkeleyDatabase` and is implemented by `BerkeleyDatabase`. First, to get to the point that this is possible, 4 functions need to be added to `BerkeleyDatabase`: `AddRef`, `RemoveRef`, `Open`, and `Close`.
First the increment and decrement of `mapFileUseCount` is refactored into separate functions `AddRef` and `RemoveRef`.
`Open` is introduced as a dummy function. This will raise an exception so that it always fails.
`Close` is refactored from `Flush`. The `shutdown` argument in `Flush` is removed and instead `Flush(true)` is now the `Close` function.
Split from #18971
Requires #19325
ACKs for top commit:
ryanofsky:
Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8. Only changes since last review were rebasing after base PR #19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids
fjahr:
Code review ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8
meshcollider:
Code review & test run ACK d416ae560e46a4846a3fd5990b7d390d2ef30ec8
Tree-SHA512: 98d05ec093d7446c4488e2b0914584222a331e9a2f4d5be6af98e3f6d78fdd8e75526c12f91a8a52d4820c25bce02aa02aabe92d38bee7eb2fce07d0691b7b0d
-rw-r--r-- | src/qt/rpcconsole.cpp | 1 | ||||
-rw-r--r-- | src/wallet/bdb.cpp | 64 | ||||
-rw-r--r-- | src/wallet/bdb.h | 50 | ||||
-rw-r--r-- | src/wallet/db.h | 60 | ||||
-rw-r--r-- | src/wallet/load.cpp | 4 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 9 | ||||
-rw-r--r-- | src/wallet/wallet.h | 5 | ||||
-rw-r--r-- | src/wallet/walletdb.cpp | 6 | ||||
-rw-r--r-- | src/wallet/walletdb.h | 11 | ||||
-rw-r--r-- | src/wallet/wallettool.cpp | 6 |
10 files changed, 152 insertions, 64 deletions
diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 29fd720244..821a337a62 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -24,7 +24,6 @@ #include <univalue.h> #ifdef ENABLE_WALLET -#include <wallet/bdb.h> #include <wallet/db.h> #include <wallet/wallet.h> #endif diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index b8e03e3ec1..1953be2d54 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -312,8 +312,17 @@ 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) +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; @@ -388,11 +397,16 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo fReadOnly = fTmp; } } - ++env->mapFileUseCount[strFilename]; + database.AddRef(); strFile = strFilename; } } +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) @@ -426,11 +440,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) @@ -675,22 +685,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); } } @@ -832,7 +837,22 @@ bool BerkeleyBatch::HasKey(CDataStream&& key) return ret == 0; } -std::unique_ptr<BerkeleyBatch> BerkeleyDatabase::MakeBatch(const char* mode, bool flush_on_close) +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<DatabaseBatch> BerkeleyDatabase::MakeBatch(const char* mode, bool flush_on_close) { return MakeUnique<BerkeleyBatch>(*this, mode, flush_on_close); } diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 1b9b5de9f9..ef3b81d4d6 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -98,56 +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<BerkeleyEnvironment> 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() { - if (env) { - size_t erased = env->m_databases.erase(strFile); - assert(erased == 1); - } - } + ~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) 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() override; + /** Indicate that database user has stopped using the database and that it could be flushed or closed. */ + 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 disk. + /** Make sure all changes are flushed to database file. + */ + 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 Flush(bool shutdown); + void Close() override; /* flush the wallet passively (TRY_LOCK) ideal to be called periodically */ - bool PeriodicFlush(); + bool PeriodicFlush() override; - void IncrementUpdateCounter(); - - void ReloadDbEnv(); + void IncrementUpdateCounter() override; - std::atomic<unsigned int> 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. @@ -164,7 +167,7 @@ public: std::unique_ptr<Db> m_db; /** Make a BerkeleyBatch connected to this database */ - std::unique_ptr<BerkeleyBatch> MakeBatch(const char* mode, bool flush_on_close); + std::unique_ptr<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) override; private: std::string strFile; @@ -213,6 +216,7 @@ protected: bool fReadOnly; bool fFlushOnClose; BerkeleyEnvironment *env; + BerkeleyDatabase& m_database; public: explicit BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode = "r+", bool fFlushOnCloseIn=true); diff --git a/src/wallet/db.h b/src/wallet/db.h index 76668f8dc2..12dc1cc96b 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -10,8 +10,12 @@ #include <fs.h> #include <streams.h> +#include <atomic> +#include <memory> #include <string> +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<int> 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<unsigned int> 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<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) = 0; +}; + #endif // BITCOIN_WALLET_DB_H diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index c2818a41e7..2a81d30133 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<CWallet>& pwallet : GetWallets()) { - pwallet->Flush(false); + pwallet->Flush(); } } void StopWallets() { for (const std::shared_ptr<CWallet>& pwallet : GetWallets()) { - pwallet->Flush(true); + pwallet->Close(); } } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8eec00993f..cee2f2214c 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<TxSpends::iterator, TxSpends::iterator> range) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8cb2a64484..a761caf38c 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<void ()> NotifyUnload; diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 1478687bf9..8c409b40cd 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<BerkeleyDatabase> CreateWalletDatabase(const fs::path& path) +std::unique_ptr<WalletDatabase> CreateWalletDatabase(const fs::path& path) { std::string filename; return MakeUnique<BerkeleyDatabase>(GetWalletEnv(path, filename), std::move(filename)); } /** Return object for accessing dummy database with no read/write capabilities. */ -std::unique_ptr<BerkeleyDatabase> CreateDummyWalletDatabase() +std::unique_ptr<WalletDatabase> CreateDummyWalletDatabase() { return MakeUnique<BerkeleyDatabase>(); } /** Return object for accessing temporary in-memory database. */ -std::unique_ptr<BerkeleyDatabase> CreateMockWalletDatabase() +std::unique_ptr<WalletDatabase> CreateMockWalletDatabase() { return MakeUnique<BerkeleyDatabase>(std::make_shared<BerkeleyEnvironment>(), ""); } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 6b55361c07..7c5bf7652b 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<BerkeleyBatch> m_batch; + std::unique_ptr<DatabaseBatch> 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<BerkeleyDatabase> CreateWalletDatabase(const fs::path& path); +std::unique_ptr<WalletDatabase> CreateWalletDatabase(const fs::path& path); /** Return object for accessing dummy database with no read/write capabilities. */ -std::unique_ptr<BerkeleyDatabase> CreateDummyWalletDatabase(); +std::unique_ptr<WalletDatabase> CreateDummyWalletDatabase(); /** Return object for accessing temporary in-memory database. */ -std::unique_ptr<BerkeleyDatabase> CreateMockWalletDatabase(); +std::unique_ptr<WalletDatabase> CreateMockWalletDatabase(); #endif // BITCOIN_WALLET_WALLETDB_H diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 8a45d81456..9f25b1ae7d 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<CWallet> 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<CWallet> 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); } |