Merge #19102: wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase

0fcff54 walletdb: Ensure that having no database handle is a failure (Andrew Chow)
da039d2 Remove BDB dummy databases (Andrew Chow)
0103d64 Introduce DummyDatabase and use it in the tests (Andrew Chow)

Pull request description:

  In the unit tests, we use a dummy `WalletDatabase` which does nothing and always returns true. This is currently implemented by creating a `BerkeleyDatabase` in dummy mode. This PR instead adds a `DummyDatabase` class which does nothing and never fails for use in the tests. `CreateDummyWalletDatabase` is changed to return this `DummyDatabase` and `BerkeleyDatabase` is cleaned up to remove all of the checks for `IsDummy`.

  Based on `WalletDatabase` abstract class introduced in #19334

ACKs for top commit:
  instagibbs:
    utACK bitcoin/bitcoin@0fcff54
  MarcoFalke:
    crACK 0fcff54 🚈

Tree-SHA512: 05fbf32e078753e9a55a05f4c080b6d365b909a2a3a8e571b7e64b59ebbe53da49394f70419cc793192ade79f312f5e0422ca7c261ba81bae5912671c5ff6402

Author: MarcoFalke

src/wallet/bdb.cpp

@@ -337,10 +337,6 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
 
 void BerkeleyDatabase::Open(const char* pszMode)
 {
-    if (IsDummy()){
-        return;
-    }
-
     bool fCreate = strchr(pszMode, 'c') != nullptr;
     unsigned int nFlags = DB_THREAD;
     if (fCreate)
@@ -472,9 +468,6 @@ void BerkeleyEnvironment::ReloadDbEnv()
 
 bool BerkeleyDatabase::Rewrite(const char* pszSkip)
 {
-    if (IsDummy()) {
-        return true;
-    }
     while (true) {
         {
             LOCK(cs_db);
@@ -602,9 +595,6 @@ void BerkeleyEnvironment::Flush(bool fShutdown)
 
 bool BerkeleyDatabase::PeriodicFlush()
 {
-    // There's nothing to do for dummy databases. Return true.
-    if (IsDummy()) return true;
-
     // Don't flush if we can't acquire the lock.
     TRY_LOCK(cs_db, lockDb);
     if (!lockDb) return false;
@@ -632,9 +622,6 @@ bool BerkeleyDatabase::PeriodicFlush()
 
 bool BerkeleyDatabase::Backup(const std::string& strDest) const
 {
-    if (IsDummy()) {
-        return false;
-    }
     while (true)
     {
         {
@@ -672,23 +659,17 @@ bool BerkeleyDatabase::Backup(const std::string& strDest) const
 
 void BerkeleyDatabase::Flush()
 {
-    if (!IsDummy()) {
-        env->Flush(false);
-    }
+    env->Flush(false);
 }
 
 void BerkeleyDatabase::Close()
 {
-    if (!IsDummy()) {
-        env->Flush(true);
-    }
+    env->Flush(true);
 }
 
 void BerkeleyDatabase::ReloadDbEnv()
 {
-    if (!IsDummy()) {
-        env->ReloadDbEnv();
-    }
+    env->ReloadDbEnv();
 }
 
 bool BerkeleyBatch::StartCursor()
@@ -786,7 +767,7 @@ bool BerkeleyBatch::ReadKey(CDataStream&& key, CDataStream& value)
 bool BerkeleyBatch::WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite)
 {
     if (!pdb)
-        return true;
+        return false;
     if (fReadOnly)
         assert(!"Write called on database in read-only mode");
 

src/wallet/bdb.h

@@ -98,10 +98,7 @@ class BerkeleyBatch;
 class BerkeleyDatabase : public WalletDatabase
 {
 public:
-    /** Create dummy DB handle */
-    BerkeleyDatabase() : WalletDatabase(), env(nullptr)
-    {
-    }
+    BerkeleyDatabase() = delete;
 
     /** Create DB handle to real database */
     BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, std::string filename) :
@@ -166,14 +163,6 @@ class BerkeleyDatabase : public WalletDatabase
 
     /** Make a BerkeleyBatch connected to this database */
     std::unique_ptr<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) override;
-
-private:
-
-    /** Return whether this database handle is a dummy for testing.
-     * Only to be used at a low level, application should ideally not care
-     * about this.
-     */
-    bool IsDummy() const { return env == nullptr; }
 };
 
 /** RAII class that provides access to a Berkeley database */

src/wallet/db.h

@@ -9,6 +9,7 @@
 #include <clientversion.h>
 #include <fs.h>
 #include <streams.h>
+#include <util/memory.h>
 
 #include <atomic>
 #include <memory>
@@ -154,4 +155,44 @@ class WalletDatabase
     virtual std::unique_ptr<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) = 0;
 };
 
+/** RAII class that provides access to a DummyDatabase. Never fails. */
+class DummyBatch : public DatabaseBatch
+{
+private:
+    bool ReadKey(CDataStream&& key, CDataStream& value) override { return true; }
+    bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite=true) override { return true; }
+    bool EraseKey(CDataStream&& key) override { return true; }
+    bool HasKey(CDataStream&& key) override { return true; }
+
+public:
+    void Flush() override {}
+    void Close() override {}
+
+    bool StartCursor() override { return true; }
+    bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return true; }
+    void CloseCursor() override {}
+    bool TxnBegin() override { return true; }
+    bool TxnCommit() override { return true; }
+    bool TxnAbort() override { return true; }
+};
+
+/** A dummy WalletDatabase that does nothing and never fails. Only used by unit tests.
+ **/
+class DummyDatabase : public WalletDatabase
+{
+public:
+    void Open(const char* mode) override {};
+    void AddRef() override {}
+    void RemoveRef() override {}
+    bool Rewrite(const char* pszSkip=nullptr) override { return true; }
+    bool Backup(const std::string& strDest) const override { return true; }
+    void Close() override {}
+    void Flush() override {}
+    bool PeriodicFlush() override { return true; }
+    void IncrementUpdateCounter() override { ++nUpdateCounter; }
+    void ReloadDbEnv() override {}
+    bool Verify(bilingual_str& errorStr) override { return true; }
+    std::unique_ptr<DatabaseBatch> MakeBatch(const char* mode = "r+", bool flush_on_close = true) override { return MakeUnique<DummyBatch>(); }
+};
+
 #endif // BITCOIN_WALLET_DB_H

src/wallet/walletdb.cpp

@@ -1021,7 +1021,7 @@ std::unique_ptr<WalletDatabase> CreateWalletDatabase(const fs::path& path)
 /** Return object for accessing dummy database with no read/write capabilities. */
 std::unique_ptr<WalletDatabase> CreateDummyWalletDatabase()
 {
-    return MakeUnique<BerkeleyDatabase>();
+    return MakeUnique<DummyDatabase>();
 }
 
 /** Return object for accessing temporary in-memory database. */