wallet: Introduce DatabaseCursor RAII class for managing cursor

Instead of having DatabaseBatch deal with opening and closing database
cursors, have a separate RAII class that deals with those.

For now, DatabaseBatch manages DatabaseCursor, but this will change
later.

src/wallet/bdb.cpp

@@ -8,6 +8,7 @@
 #include <wallet/bdb.h>
 #include <wallet/db.h>
 
+#include <util/check.h>
 #include <util/strencodings.h>
 #include <util/translation.h>
 
@@ -307,7 +308,7 @@ BerkeleyDatabase::~BerkeleyDatabase()
     }
 }
 
-BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)
+BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_database(database)
 {
     database.AddRef();
     database.Open();
@@ -656,16 +657,18 @@ void BerkeleyDatabase::ReloadDbEnv()
     env->ReloadDbEnv();
 }
 
-bool BerkeleyBatch::StartCursor()
+BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database)
 {
-    assert(!m_cursor);
+    if (!database.m_db.get()) {
-    if (!pdb)
+        throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist"));
-        return false;
+    }
-    int ret = pdb->cursor(nullptr, &m_cursor, 0);
+    int ret = database.m_db->cursor(nullptr, &m_cursor, 0);
-    return ret == 0;
+    if (ret != 0) {
+        throw std::runtime_error(STR_INTERNAL_BUG(strprintf("BDB Cursor could not be created. Returned %d", ret).c_str()));
+    }
 }
 
-bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
+bool BerkeleyCursor::Next(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
 {
     complete = false;
     if (m_cursor == nullptr) return false;
@@ -691,13 +694,19 @@ bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool&
     return true;
 }
 
-void BerkeleyBatch::CloseCursor()
+BerkeleyCursor::~BerkeleyCursor()
 {
     if (!m_cursor) return;
     m_cursor->close();
     m_cursor = nullptr;
 }
 
+std::unique_ptr<DatabaseCursor> BerkeleyBatch::GetNewCursor()
+{
+    if (!pdb) return nullptr;
+    return std::make_unique<BerkeleyCursor>(m_database);
+}
+
 bool BerkeleyBatch::TxnBegin()
 {
     if (!pdb || activeTxn)

src/wallet/bdb.h

@@ -185,6 +185,18 @@ public:
     operator Dbt*();
 };
 
+class BerkeleyCursor : public DatabaseCursor
+{
+private:
+    Dbc* m_cursor;
+
+public:
+    explicit BerkeleyCursor(BerkeleyDatabase& database);
+    ~BerkeleyCursor() override;
+
+    bool Next(CDataStream& key, CDataStream& value, bool& complete) override;
+};
+
 /** RAII class that provides access to a Berkeley database */
 class BerkeleyBatch : public DatabaseBatch
 {
@@ -198,7 +210,6 @@ protected:
     Db* pdb;
     std::string strFile;
     DbTxn* activeTxn;
-    Dbc* m_cursor;
     bool fReadOnly;
     bool fFlushOnClose;
     BerkeleyEnvironment *env;
@@ -214,9 +225,7 @@ public:
     void Flush() override;
     void Close() override;
 
-    bool StartCursor() override;
+    std::unique_ptr<DatabaseCursor> GetNewCursor() override;
-    bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override;
-    void CloseCursor() override;
     bool TxnBegin() override;
     bool TxnCommit() override;
     bool TxnAbort() override;

src/wallet/db.h

@@ -22,10 +22,24 @@ struct bilingual_str;
 namespace wallet {
 void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename);
 
+class DatabaseCursor
+{
+public:
+    explicit DatabaseCursor() {}
+    virtual ~DatabaseCursor() {}
+
+    DatabaseCursor(const DatabaseCursor&) = delete;
+    DatabaseCursor& operator=(const DatabaseCursor&) = delete;
+
+    virtual bool Next(CDataStream& key, CDataStream& value, bool& complete) { return false; }
+};
+
 /** RAII class that provides access to a WalletDatabase */
 class DatabaseBatch
 {
 private:
+    std::unique_ptr<DatabaseCursor> m_cursor;
+
     virtual bool ReadKey(CDataStream&& key, CDataStream& value) = 0;
     virtual bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite=true) = 0;
     virtual bool EraseKey(CDataStream&& key) = 0;
@@ -92,9 +106,21 @@ public:
         return HasKey(std::move(ssKey));
     }
 
-    virtual bool StartCursor() = 0;
+    virtual std::unique_ptr<DatabaseCursor> GetNewCursor() = 0;
-    virtual bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) = 0;
+    bool StartCursor()
-    virtual void CloseCursor() = 0;
+    {
+        m_cursor = GetNewCursor();
+        return m_cursor != nullptr;
+    }
+    bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
+    {
+        if (!m_cursor) return false;
+        return m_cursor->Next(ssKey, ssValue, complete);
+    }
+    void CloseCursor()
+    {
+        m_cursor.reset();
+    }
     virtual bool TxnBegin() = 0;
     virtual bool TxnCommit() = 0;
     virtual bool TxnAbort() = 0;
@@ -156,6 +182,11 @@ public:
     virtual std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) = 0;
 };
 
+class DummyCursor : public DatabaseCursor
+{
+    bool Next(CDataStream& key, CDataStream& value, bool& complete) override { return false; }
+};
+
 /** RAII class that provides access to a DummyDatabase. Never fails. */
 class DummyBatch : public DatabaseBatch
 {
@@ -169,9 +200,7 @@ public:
     void Flush() override {}
     void Close() override {}
 
-    bool StartCursor() override { return true; }
+    std::unique_ptr<DatabaseCursor> GetNewCursor() override { return std::make_unique<DummyCursor>(); }
-    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; }

src/wallet/sqlite.cpp

@@ -125,7 +125,6 @@ void SQLiteBatch::SetupSQLStatements()
         {&m_insert_stmt, "INSERT INTO main VALUES(?, ?)"},
         {&m_overwrite_stmt, "INSERT or REPLACE into main values(?, ?)"},
         {&m_delete_stmt, "DELETE FROM main WHERE key = ?"},
-        {&m_cursor_stmt, "SELECT key, value FROM main"},
     };
 
     for (const auto& [stmt_prepared, stmt_text] : statements) {
@@ -374,7 +373,6 @@ void SQLiteBatch::Close()
         {&m_insert_stmt, "insert"},
         {&m_overwrite_stmt, "overwrite"},
         {&m_delete_stmt, "delete"},
-        {&m_cursor_stmt, "cursor"},
     };
 
     for (const auto& [stmt_prepared, stmt_description] : statements) {
@@ -472,27 +470,17 @@ bool SQLiteBatch::HasKey(CDataStream&& key)
     return res == SQLITE_ROW;
 }
 
-bool SQLiteBatch::StartCursor()
+bool SQLiteCursor::Next(CDataStream& key, CDataStream& value, bool& complete)
-{
-    assert(!m_cursor_init);
-    if (!m_database.m_db) return false;
-    m_cursor_init = true;
-    return true;
-}
-
-bool SQLiteBatch::ReadAtCursor(CDataStream& key, CDataStream& value, bool& complete)
 {
     complete = false;
 
-    if (!m_cursor_init) return false;
-
     int res = sqlite3_step(m_cursor_stmt);
     if (res == SQLITE_DONE) {
         complete = true;
         return true;
     }
     if (res != SQLITE_ROW) {
-        LogPrintf("SQLiteBatch::ReadAtCursor: Unable to execute cursor step: %s\n", sqlite3_errstr(res));
+        LogPrintf("%s: Unable to execute cursor step: %s\n", __func__, sqlite3_errstr(res));
         return false;
     }
 
@@ -506,10 +494,29 @@ bool SQLiteBatch::ReadAtCursor(CDataStream& key, CDataStream& value, bool& compl
     return true;
 }
 
-void SQLiteBatch::CloseCursor()
+SQLiteCursor::~SQLiteCursor()
 {
     sqlite3_reset(m_cursor_stmt);
-    m_cursor_init = false;
+    int res = sqlite3_finalize(m_cursor_stmt);
+    if (res != SQLITE_OK) {
+        LogPrintf("%s: cursor closed but could not finalize cursor statement: %s\n",
+                  __func__, sqlite3_errstr(res));
+    }
+}
+
+std::unique_ptr<DatabaseCursor> SQLiteBatch::GetNewCursor()
+{
+    if (!m_database.m_db) return nullptr;
+    auto cursor = std::make_unique<SQLiteCursor>();
+
+    const char* stmt_text = "SELECT key, value FROM main";
+    int res = sqlite3_prepare_v2(m_database.m_db, stmt_text, -1, &cursor->m_cursor_stmt, nullptr);
+    if (res != SQLITE_OK) {
+        throw std::runtime_error(strprintf(
+            "%s: Failed to setup cursor SQL statement: %s\n", __func__, sqlite3_errstr(res)));
+    }
+
+    return cursor;
 }
 
 bool SQLiteBatch::TxnBegin()

src/wallet/sqlite.h

@@ -14,19 +14,27 @@ struct bilingual_str;
 namespace wallet {
 class SQLiteDatabase;
 
+class SQLiteCursor : public DatabaseCursor
+{
+public:
+    sqlite3_stmt* m_cursor_stmt{nullptr};
+
+    explicit SQLiteCursor() {}
+    ~SQLiteCursor() override;
+
+    bool Next(CDataStream& key, CDataStream& value, bool& complete) override;
+};
+
 /** RAII class that provides access to a WalletDatabase */
 class SQLiteBatch : public DatabaseBatch
 {
 private:
     SQLiteDatabase& m_database;
 
-    bool m_cursor_init = false;
-
     sqlite3_stmt* m_read_stmt{nullptr};
     sqlite3_stmt* m_insert_stmt{nullptr};
     sqlite3_stmt* m_overwrite_stmt{nullptr};
     sqlite3_stmt* m_delete_stmt{nullptr};
-    sqlite3_stmt* m_cursor_stmt{nullptr};
 
     void SetupSQLStatements();
 
@@ -44,9 +52,7 @@ public:
 
     void Close() override;
 
-    bool StartCursor() override;
+    std::unique_ptr<DatabaseCursor> GetNewCursor() override;
-    bool ReadAtCursor(CDataStream& key, CDataStream& value, bool& complete) override;
-    void CloseCursor() override;
     bool TxnBegin() override;
     bool TxnCommit() override;
     bool TxnAbort() override;

src/wallet/test/wallet_tests.cpp

@@ -867,6 +867,12 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)
     TestUnloadWallet(std::move(wallet));
 }
 
+class FailCursor : public DatabaseCursor
+{
+public:
+    bool Next(CDataStream& key, CDataStream& value, bool& complete) override { return false; }
+};
+
 /** RAII class that provides access to a FailDatabase. Which fails if needed. */
 class FailBatch : public DatabaseBatch
 {
@@ -882,9 +888,7 @@ public:
     void Flush() override {}
     void Close() override {}
 
-    bool StartCursor() override { return true; }
+    std::unique_ptr<DatabaseCursor> GetNewCursor() override { return std::make_unique<FailCursor>(); }
-    bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return false; }
-    void CloseCursor() override {}
     bool TxnBegin() override { return false; }
     bool TxnCommit() override { return false; }
     bool TxnAbort() override { return false; }