Merge #19334: wallet: Introduce WalletDatabase abstract class

d416ae560e walletdb: Introduce WalletDatabase abstract class (Andrew Chow)
2179dbcbcd walletdb: Add BerkeleyDatabase::Open dummy function (Andrew Chow)
71d28e7cdc walletdb: Introduce AddRef and RemoveRef functions (Andrew Chow)
27b2766384 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 d416ae560e. 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 d416ae560e
  meshcollider:
    Code review & test run ACK d416ae560e

Tree-SHA512: 98d05ec093d7446c4488e2b0914584222a331e9a2f4d5be6af98e3f6d78fdd8e75526c12f91a8a52d4820c25bce02aa02aabe92d38bee7eb2fce07d0691b7b0d

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

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);
 }

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(bool shutdown);
+    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() 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<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);

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

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();
     }
 }
 

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)

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;

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>(), "");
 }

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

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);
         }