wallet: Have cursor users use DatabaseCursor directly

achow101 · achow101 · commit d79e8dcf2981 · 2022-12-16T12:35:54.000-05:00
Instead of having the DatabaseBatch manage the cursor, having the
consumer handle it directly
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
@@ -399,7 +399,6 @@ void BerkeleyBatch::Close()
         activeTxn->abort();
     activeTxn = nullptr;
     pdb = nullptr;
-    CloseCursor();
 
     if (fFlushOnClose)
         Flush();
@@ -477,12 +476,13 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
                         fSuccess = false;
                     }
 
-                    if (db.StartCursor()) {
+                    std::unique_ptr<DatabaseCursor> cursor = db.GetNewCursor();
+                    if (cursor) {
                         while (fSuccess) {
                             CDataStream ssKey(SER_DISK, CLIENT_VERSION);
                             CDataStream ssValue(SER_DISK, CLIENT_VERSION);
                             bool complete;
-                            bool ret1 = db.ReadAtCursor(ssKey, ssValue, complete);
+                            bool ret1 = cursor->Next(ssKey, ssValue, complete);
                             if (complete) {
                                 break;
                             } else if (!ret1) {
@@ -503,7 +503,7 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
                             if (ret2 > 0)
                                 fSuccess = false;
                         }
-                        db.CloseCursor();
+                        cursor.reset();
                     }
                     if (fSuccess) {
                         db.Close();
diff --git a/src/wallet/db.h b/src/wallet/db.h
@@ -38,8 +38,6 @@ class DatabaseCursor
 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;
@@ -107,20 +105,6 @@ class DatabaseBatch
     }
 
     virtual std::unique_ptr<DatabaseCursor> GetNewCursor() = 0;
-    bool StartCursor()
-    {
-        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;
diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp
@@ -47,7 +47,8 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
     std::unique_ptr<DatabaseBatch> batch = db.MakeBatch();
 
     bool ret = true;
-    if (!batch->StartCursor()) {
+    std::unique_ptr<DatabaseCursor> cursor = batch->GetNewCursor();
+    if (!cursor) {
         error = _("Error: Couldn't create cursor into database");
         ret = false;
     }
@@ -69,7 +70,7 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
             CDataStream ss_key(SER_DISK, CLIENT_VERSION);
             CDataStream ss_value(SER_DISK, CLIENT_VERSION);
             bool complete;
-            ret = batch->ReadAtCursor(ss_key, ss_value, complete);
+            ret = cursor->Next(ss_key, ss_value, complete);
             if (complete) {
                 ret = true;
                 break;
@@ -85,7 +86,7 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
         }
     }
 
-    batch->CloseCursor();
+    cursor.reset();
     batch.reset();
 
     // Close the wallet after we're done with it. The caller won't be doing this
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
@@ -50,7 +50,7 @@ std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database,
 
     // Get a cursor to the original database
     auto batch = database.MakeBatch();
-    batch->StartCursor();
+    std::unique_ptr<wallet::DatabaseCursor> cursor = batch->GetNewCursor();
 
     // Get a batch for the new database
     auto new_batch = new_database->MakeBatch();
@@ -60,7 +60,7 @@ std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database,
         CDataStream key(SER_DISK, CLIENT_VERSION);
         CDataStream value(SER_DISK, CLIENT_VERSION);
         bool complete;
-        batch->ReadAtCursor(key, value, complete);
+        cursor->Next(key, value, complete);
         if (complete) break;
         new_batch->Write(key, value);
     }
diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
@@ -54,12 +54,14 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup)
 bool HasAnyRecordOfType(WalletDatabase& db, const std::string& key)
 {
     std::unique_ptr<DatabaseBatch> batch = db.MakeBatch(false);
-    BOOST_CHECK(batch->StartCursor());
+    BOOST_CHECK(batch);
+    std::unique_ptr<DatabaseCursor> cursor = batch->GetNewCursor();
+    BOOST_CHECK(cursor);
     while (true) {
         CDataStream ssKey(SER_DISK, CLIENT_VERSION);
         CDataStream ssValue(SER_DISK, CLIENT_VERSION);
         bool complete;
-        BOOST_CHECK(batch->ReadAtCursor(ssKey, ssValue, complete));
+        BOOST_CHECK(cursor->Next(ssKey, ssValue, complete));
         if (complete) break;
         std::string type;
         ssKey >> type;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -3770,24 +3770,25 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
 
     // Get all of the records for DB type migration
     std::unique_ptr<DatabaseBatch> batch = m_database->MakeBatch();
+    std::unique_ptr<DatabaseCursor> cursor = batch->GetNewCursor();
     std::vector<std::pair<SerializeData, SerializeData>> records;
-    if (!batch->StartCursor()) {
+    if (!cursor) {
         error = _("Error: Unable to begin reading all records in the database");
         return false;
     }
     bool complete = false;
     while (true) {
         CDataStream ss_key(SER_DISK, CLIENT_VERSION);
         CDataStream ss_value(SER_DISK, CLIENT_VERSION);
-        bool ret = batch->ReadAtCursor(ss_key, ss_value, complete);
-        if (!ret) {
+        bool ret = cursor->Next(ss_key, ss_value, complete);
+        if (complete || !ret) {
             break;
         }
         SerializeData key(ss_key.begin(), ss_key.end());
         SerializeData value(ss_value.begin(), ss_value.end());
         records.emplace_back(key, value);
     }
-    batch->CloseCursor();
+    cursor.reset();
     batch.reset();
     if (!complete) {
         error = _("Error: Unable to read all records in the database");
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
@@ -812,7 +812,8 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
 #endif
 
         // Get cursor
-        if (!m_batch->StartCursor())
+        std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor();
+        if (!cursor)
         {
             pwallet->WalletLogPrintf("Error getting wallet database cursor\n");
             return DBErrors::CORRUPT;
@@ -824,13 +825,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
             CDataStream ssKey(SER_DISK, CLIENT_VERSION);
             CDataStream ssValue(SER_DISK, CLIENT_VERSION);
             bool complete;
-            bool ret = m_batch->ReadAtCursor(ssKey, ssValue, complete);
+            bool ret = cursor->Next(ssKey, ssValue, complete);
             if (complete) {
                 break;
             }
             else if (!ret)
             {
-                m_batch->CloseCursor();
+                cursor.reset();
                 pwallet->WalletLogPrintf("Error reading next record from wallet database\n");
                 return DBErrors::CORRUPT;
             }
@@ -878,7 +879,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
     } catch (...) {
         result = DBErrors::CORRUPT;
     }
-    m_batch->CloseCursor();
 
     // Set the active ScriptPubKeyMans
     for (auto spk_man_pair : wss.m_active_external_spks) {
@@ -986,7 +986,8 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal
         }
 
         // Get cursor
-        if (!m_batch->StartCursor())
+        std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor();
+        if (!cursor)
         {
             LogPrintf("Error getting wallet database cursor\n");
             return DBErrors::CORRUPT;
@@ -998,11 +999,10 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal
             CDataStream ssKey(SER_DISK, CLIENT_VERSION);
             CDataStream ssValue(SER_DISK, CLIENT_VERSION);
             bool complete;
-            bool ret = m_batch->ReadAtCursor(ssKey, ssValue, complete);
+            bool ret = cursor->Next(ssKey, ssValue, complete);
             if (complete) {
                 break;
             } else if (!ret) {
-                m_batch->CloseCursor();
                 LogPrintf("Error reading next record from wallet database\n");
                 return DBErrors::CORRUPT;
             }
@@ -1020,7 +1020,6 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal
     } catch (...) {
         result = DBErrors::CORRUPT;
     }
-    m_batch->CloseCursor();
 
     return result;
 }
@@ -1114,7 +1113,8 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags)
 bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
 {
     // Get cursor
-    if (!m_batch->StartCursor())
+    std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor();
+    if (!cursor)
     {
         return false;
     }
@@ -1126,13 +1126,12 @@ bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
         CDataStream key(SER_DISK, CLIENT_VERSION);
         CDataStream value(SER_DISK, CLIENT_VERSION);
         bool complete;
-        bool ret = m_batch->ReadAtCursor(key, value, complete);
+        bool ret = cursor->Next(key, value, complete);
         if (complete) {
             break;
         }
         else if (!ret)
         {
-            m_batch->CloseCursor();
             return false;
         }
 
@@ -1146,7 +1145,6 @@ bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
             m_batch->Erase(key_data);
         }
     }
-    m_batch->CloseCursor();
     return true;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -812,7 +812,8 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)`
`812`	`812`	`#endif`
`813`	`813`
`814`	`814`	`// Get cursor`
`815`		`- if (!m_batch->StartCursor())`
	`815`	`+ std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor();`
	`816`	`+ if (!cursor)`
`816`	`817`	`{`
`817`	`818`	`pwallet->WalletLogPrintf("Error getting wallet database cursor\n");`
`818`	`819`	`return DBErrors::CORRUPT;`
`@@ -824,13 +825,13 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)`
`824`	`825`	`CDataStream ssKey(SER_DISK, CLIENT_VERSION);`
`825`	`826`	`CDataStream ssValue(SER_DISK, CLIENT_VERSION);`
`826`	`827`	`bool complete;`
`827`		`- bool ret = m_batch->ReadAtCursor(ssKey, ssValue, complete);`
	`828`	`+ bool ret = cursor->Next(ssKey, ssValue, complete);`
`828`	`829`	`if (complete) {`
`829`	`830`	`break;`
`830`	`831`	`}`
`831`	`832`	`else if (!ret)`
`832`	`833`	`{`
`833`		`- m_batch->CloseCursor();`
	`834`	`+ cursor.reset();`
`834`	`835`	`pwallet->WalletLogPrintf("Error reading next record from wallet database\n");`
`835`	`836`	`return DBErrors::CORRUPT;`
`836`	`837`	`}`
`@@ -878,7 +879,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)`
`878`	`879`	`} catch (...) {`
`879`	`880`	`result = DBErrors::CORRUPT;`
`880`	`881`	`}`
`881`		`- m_batch->CloseCursor();`
`882`	`882`
`883`	`883`	`// Set the active ScriptPubKeyMans`
`884`	`884`	`for (auto spk_man_pair : wss.m_active_external_spks) {`
`@@ -986,7 +986,8 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal`
`986`	`986`	`}`
`987`	`987`
`988`	`988`	`// Get cursor`
`989`		`- if (!m_batch->StartCursor())`
	`989`	`+ std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor();`
	`990`	`+ if (!cursor)`
`990`	`991`	`{`
`991`	`992`	`LogPrintf("Error getting wallet database cursor\n");`
`992`	`993`	`return DBErrors::CORRUPT;`
`@@ -998,11 +999,10 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal`
`998`	`999`	`CDataStream ssKey(SER_DISK, CLIENT_VERSION);`
`999`	`1000`	`CDataStream ssValue(SER_DISK, CLIENT_VERSION);`
`1000`	`1001`	`bool complete;`
`1001`		`- bool ret = m_batch->ReadAtCursor(ssKey, ssValue, complete);`
	`1002`	`+ bool ret = cursor->Next(ssKey, ssValue, complete);`
`1002`	`1003`	`if (complete) {`
`1003`	`1004`	`break;`
`1004`	`1005`	`} else if (!ret) {`
`1005`		`- m_batch->CloseCursor();`
`1006`	`1006`	`LogPrintf("Error reading next record from wallet database\n");`
`1007`	`1007`	`return DBErrors::CORRUPT;`
`1008`	`1008`	`}`
`@@ -1020,7 +1020,6 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal`
`1020`	`1020`	`} catch (...) {`
`1021`	`1021`	`result = DBErrors::CORRUPT;`
`1022`	`1022`	`}`
`1023`		`- m_batch->CloseCursor();`
`1024`	`1023`
`1025`	`1024`	`return result;`
`1026`	`1025`	`}`
`@@ -1114,7 +1113,8 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags)`
`1114`	`1113`	`bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)`
`1115`	`1114`	`{`
`1116`	`1115`	`// Get cursor`
`1117`		`- if (!m_batch->StartCursor())`
	`1116`	`+ std::unique_ptr<DatabaseCursor> cursor = m_batch->GetNewCursor();`
	`1117`	`+ if (!cursor)`
`1118`	`1118`	`{`
`1119`	`1119`	`return false;`
`1120`	`1120`	`}`
`@@ -1126,13 +1126,12 @@ bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)`
`1126`	`1126`	`CDataStream key(SER_DISK, CLIENT_VERSION);`
`1127`	`1127`	`CDataStream value(SER_DISK, CLIENT_VERSION);`
`1128`	`1128`	`bool complete;`
`1129`		`- bool ret = m_batch->ReadAtCursor(key, value, complete);`
	`1129`	`+ bool ret = cursor->Next(key, value, complete);`
`1130`	`1130`	`if (complete) {`
`1131`	`1131`	`break;`
`1132`	`1132`	`}`
`1133`	`1133`	`else if (!ret)`
`1134`	`1134`	`{`
`1135`		`- m_batch->CloseCursor();`
`1136`	`1135`	`return false;`
`1137`	`1136`	`}`
`1138`	`1137`
`@@ -1146,7 +1145,6 @@ bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)`
`1146`	`1145`	`m_batch->Erase(key_data);`
`1147`	`1146`	`}`
`1148`	`1147`	`}`
`1149`		`- m_batch->CloseCursor();`
`1150`	`1148`	`return true;`
`1151`	`1149`	`}`
`1152`	`1150`