db: Change DatabaseCursor::Next to return status enum

achow101 · achow101 · commit 4aebd832a405 · 2022-12-16T12:35:54.000-05:00
Next()'s result is a tri-state - failed, more to go, complete. Replace
the way that this is returned with an enum with values FAIL, MORE, and
DONE rather than with two booleans.
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
@@ -481,11 +481,10 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
                         while (fSuccess) {
                             CDataStream ssKey(SER_DISK, CLIENT_VERSION);
                             CDataStream ssValue(SER_DISK, CLIENT_VERSION);
-                            bool complete;
-                            bool ret1 = cursor->Next(ssKey, ssValue, complete);
-                            if (complete) {
+                            DatabaseCursor::Status ret1 = cursor->Next(ssKey, ssValue);
+                            if (ret1 == DatabaseCursor::Status::DONE) {
                                 break;
-                            } else if (!ret1) {
+                            } else if (ret1 == DatabaseCursor::Status::FAIL) {
                                 fSuccess = false;
                                 break;
                             }
@@ -668,21 +667,19 @@ BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database)
     }
 }
 
-bool BerkeleyCursor::Next(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
+DatabaseCursor::Status BerkeleyCursor::Next(CDataStream& ssKey, CDataStream& ssValue)
 {
-    complete = false;
-    if (m_cursor == nullptr) return false;
+    if (m_cursor == nullptr) return Status::FAIL;
     // Read at cursor
     SafeDbt datKey;
     SafeDbt datValue;
     int ret = m_cursor->get(datKey, datValue, DB_NEXT);
     if (ret == DB_NOTFOUND) {
-        complete = true;
+        return Status::DONE;
+    }
+    if (ret != 0 || datKey.get_data() == nullptr || datValue.get_data() == nullptr) {
+        return Status::FAIL;
     }
-    if (ret != 0)
-        return false;
-    else if (datKey.get_data() == nullptr || datValue.get_data() == nullptr)
-        return false;
 
     // Convert to streams
     ssKey.SetType(SER_DISK);
@@ -691,7 +688,7 @@ bool BerkeleyCursor::Next(CDataStream& ssKey, CDataStream& ssValue, bool& comple
     ssValue.SetType(SER_DISK);
     ssValue.clear();
     ssValue.write({AsBytePtr(datValue.get_data()), datValue.get_size()});
-    return true;
+    return Status::MORE;
 }
 
 BerkeleyCursor::~BerkeleyCursor()
diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h
@@ -194,7 +194,7 @@ class BerkeleyCursor : public DatabaseCursor
     explicit BerkeleyCursor(BerkeleyDatabase& database);
     ~BerkeleyCursor() override;
 
-    bool Next(CDataStream& key, CDataStream& value, bool& complete) override;
+    Status Next(CDataStream& key, CDataStream& value) override;
 };
 
 /** RAII class that provides access to a Berkeley database */
diff --git a/src/wallet/db.h b/src/wallet/db.h
@@ -31,7 +31,14 @@ class DatabaseCursor
     DatabaseCursor(const DatabaseCursor&) = delete;
     DatabaseCursor& operator=(const DatabaseCursor&) = delete;
 
-    virtual bool Next(CDataStream& key, CDataStream& value, bool& complete) { return false; }
+    enum class Status
+    {
+        FAIL,
+        MORE,
+        DONE,
+    };
+
+    virtual Status Next(CDataStream& key, CDataStream& value) { return Status::FAIL; }
 };
 
 /** RAII class that provides access to a WalletDatabase */
@@ -168,7 +175,7 @@ class WalletDatabase
 
 class DummyCursor : public DatabaseCursor
 {
-    bool Next(CDataStream& key, CDataStream& value, bool& complete) override { return false; }
+    Status Next(CDataStream& key, CDataStream& value) override { return Status::FAIL; }
 };
 
 /** RAII class that provides access to a DummyDatabase. Never fails. */
diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp
@@ -69,13 +69,13 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
         while (true) {
             CDataStream ss_key(SER_DISK, CLIENT_VERSION);
             CDataStream ss_value(SER_DISK, CLIENT_VERSION);
-            bool complete;
-            ret = cursor->Next(ss_key, ss_value, complete);
-            if (complete) {
+            DatabaseCursor::Status status = cursor->Next(ss_key, ss_value);
+            if (status == DatabaseCursor::Status::DONE) {
                 ret = true;
                 break;
-            } else if (!ret) {
+            } else if (status == DatabaseCursor::Status::FAIL) {
                 error = _("Error reading next record from wallet database");
+                ret = false;
                 break;
             }
             std::string key_str = HexStr(ss_key);
diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp
@@ -470,18 +470,15 @@ bool SQLiteBatch::HasKey(CDataStream&& key)
     return res == SQLITE_ROW;
 }
 
-bool SQLiteCursor::Next(CDataStream& key, CDataStream& value, bool& complete)
+DatabaseCursor::Status SQLiteCursor::Next(CDataStream& key, CDataStream& value)
 {
-    complete = false;
-
     int res = sqlite3_step(m_cursor_stmt);
     if (res == SQLITE_DONE) {
-        complete = true;
-        return true;
+        return Status::DONE;
     }
     if (res != SQLITE_ROW) {
         LogPrintf("%s: Unable to execute cursor step: %s\n", __func__, sqlite3_errstr(res));
-        return false;
+        return Status::FAIL;
     }
 
     // Leftmost column in result is index 0
@@ -491,7 +488,7 @@ bool SQLiteCursor::Next(CDataStream& key, CDataStream& value, bool& complete)
     const std::byte* value_data{AsBytePtr(sqlite3_column_blob(m_cursor_stmt, 1))};
     size_t value_data_size(sqlite3_column_bytes(m_cursor_stmt, 1));
     value.write({value_data, value_data_size});
-    return true;
+    return Status::MORE;
 }
 
 SQLiteCursor::~SQLiteCursor()
diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h
@@ -22,7 +22,7 @@ class SQLiteCursor : public DatabaseCursor
     explicit SQLiteCursor() {}
     ~SQLiteCursor() override;
 
-    bool Next(CDataStream& key, CDataStream& value, bool& complete) override;
+    Status Next(CDataStream& key, CDataStream& value) override;
 };
 
 /** RAII class that provides access to a WalletDatabase */
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
@@ -59,9 +59,9 @@ std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database,
     while (true) {
         CDataStream key(SER_DISK, CLIENT_VERSION);
         CDataStream value(SER_DISK, CLIENT_VERSION);
-        bool complete;
-        cursor->Next(key, value, complete);
-        if (complete) break;
+        DatabaseCursor::Status status = cursor->Next(key, value);
+        assert(status != DatabaseCursor::Status::FAIL);
+        if (status == DatabaseCursor::Status::DONE) break;
         new_batch->Write(key, value);
     }
 
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -870,7 +870,7 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)
 class FailCursor : public DatabaseCursor
 {
 public:
-    bool Next(CDataStream& key, CDataStream& value, bool& complete) override { return false; }
+    Status Next(CDataStream& key, CDataStream& value) override { return Status::FAIL; }
 };
 
 /** RAII class that provides access to a FailDatabase. Which fails if needed. */
diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
@@ -60,9 +60,9 @@ bool HasAnyRecordOfType(WalletDatabase& db, const std::string& key)
     while (true) {
         CDataStream ssKey(SER_DISK, CLIENT_VERSION);
         CDataStream ssValue(SER_DISK, CLIENT_VERSION);
-        bool complete;
-        BOOST_CHECK(cursor->Next(ssKey, ssValue, complete));
-        if (complete) break;
+        DatabaseCursor::Status status = cursor->Next(ssKey, ssValue);
+        assert(status != DatabaseCursor::Status::FAIL);
+        if (status == DatabaseCursor::Status::DONE) break;
         std::string type;
         ssKey >> type;
         if (type == key) return true;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -3776,12 +3776,12 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
         error = _("Error: Unable to begin reading all records in the database");
         return false;
     }
-    bool complete = false;
+    DatabaseCursor::Status status = DatabaseCursor::Status::FAIL;
     while (true) {
         CDataStream ss_key(SER_DISK, CLIENT_VERSION);
         CDataStream ss_value(SER_DISK, CLIENT_VERSION);
-        bool ret = cursor->Next(ss_key, ss_value, complete);
-        if (complete || !ret) {
+        status = cursor->Next(ss_key, ss_value);
+        if (status != DatabaseCursor::Status::MORE) {
             break;
         }
         SerializeData key(ss_key.begin(), ss_key.end());
@@ -3790,7 +3790,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
     }
     cursor.reset();
     batch.reset();
-    if (!complete) {
+    if (status != DatabaseCursor::Status::DONE) {
         error = _("Error: Unable to read all records in the database");
         return false;
     }
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
@@ -824,13 +824,10 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
             // Read next record
             CDataStream ssKey(SER_DISK, CLIENT_VERSION);
             CDataStream ssValue(SER_DISK, CLIENT_VERSION);
-            bool complete;
-            bool ret = cursor->Next(ssKey, ssValue, complete);
-            if (complete) {
+            DatabaseCursor::Status status = cursor->Next(ssKey, ssValue);
+            if (status == DatabaseCursor::Status::DONE) {
                 break;
-            }
-            else if (!ret)
-            {
+            } else if (status == DatabaseCursor::Status::FAIL) {
                 cursor.reset();
                 pwallet->WalletLogPrintf("Error reading next record from wallet database\n");
                 return DBErrors::CORRUPT;
@@ -998,11 +995,10 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWal
             // Read next record
             CDataStream ssKey(SER_DISK, CLIENT_VERSION);
             CDataStream ssValue(SER_DISK, CLIENT_VERSION);
-            bool complete;
-            bool ret = cursor->Next(ssKey, ssValue, complete);
-            if (complete) {
+            DatabaseCursor::Status status = cursor->Next(ssKey, ssValue);
+            if (status == DatabaseCursor::Status::DONE) {
                 break;
-            } else if (!ret) {
+            } else if (status == DatabaseCursor::Status::FAIL) {
                 LogPrintf("Error reading next record from wallet database\n");
                 return DBErrors::CORRUPT;
             }
@@ -1125,13 +1121,10 @@ bool WalletBatch::EraseRecords(const std::unordered_set<std::string>& types)
         // Read next record
         CDataStream key(SER_DISK, CLIENT_VERSION);
         CDataStream value(SER_DISK, CLIENT_VERSION);
-        bool complete;
-        bool ret = cursor->Next(key, value, complete);
-        if (complete) {
+        DatabaseCursor::Status status = cursor->Next(key, value);
+        if (status == DatabaseCursor::Status::DONE) {
             break;
-        }
-        else if (!ret)
-        {
+        } else if (status == DatabaseCursor::Status::FAIL) {
             return false;
         }
 

Original file line number	Diff line number	Diff line change
`@@ -470,18 +470,15 @@ bool SQLiteBatch::HasKey(CDataStream&& key)`
`470`	`470`	`return res == SQLITE_ROW;`
`471`	`471`	`}`
`472`	`472`
`473`		`-bool SQLiteCursor::Next(CDataStream& key, CDataStream& value, bool& complete)`
	`473`	`+DatabaseCursor::Status SQLiteCursor::Next(CDataStream& key, CDataStream& value)`
`474`	`474`	`{`
`475`		`- complete = false;`
`476`		`-`
`477`	`475`	`int res = sqlite3_step(m_cursor_stmt);`
`478`	`476`	`if (res == SQLITE_DONE) {`
`479`		`- complete = true;`
`480`		`- return true;`
	`477`	`+ return Status::DONE;`
`481`	`478`	`}`
`482`	`479`	`if (res != SQLITE_ROW) {`
`483`	`480`	`LogPrintf("%s: Unable to execute cursor step: %s\n", __func__, sqlite3_errstr(res));`
`484`		`- return false;`
	`481`	`+ return Status::FAIL;`
`485`	`482`	`}`
`486`	`483`
`487`	`484`	`// Leftmost column in result is index 0`
`@@ -491,7 +488,7 @@ bool SQLiteCursor::Next(CDataStream& key, CDataStream& value, bool& complete)`
`491`	`488`	`const std::byte* value_data{AsBytePtr(sqlite3_column_blob(m_cursor_stmt, 1))};`
`492`	`489`	`size_t value_data_size(sqlite3_column_bytes(m_cursor_stmt, 1));`
`493`	`490`	`value.write({value_data, value_data_size});`
`494`		`- return true;`
	`491`	`+ return Status::MORE;`
`495`	`492`	`}`
`496`	`493`
`497`	`494`	`SQLiteCursor::~SQLiteCursor()`
Original file line number	Diff line number	Diff line change
`@@ -870,7 +870,7 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup)`
`870`	`870`	`class FailCursor : public DatabaseCursor`
`871`	`871`	`{`
`872`	`872`	`public:`
`873`		`- bool Next(CDataStream& key, CDataStream& value, bool& complete) override { return false; }`
	`873`	`+ Status Next(CDataStream& key, CDataStream& value) override { return Status::FAIL; }`
`874`	`874`	`};`
`875`	`875`
`876`	`876`	`/** RAII class that provides access to a FailDatabase. Which fails if needed. */`
Original file line number	Diff line number	Diff line change
`@@ -3776,12 +3776,12 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)`
`3776`	`3776`	`error = _("Error: Unable to begin reading all records in the database");`
`3777`	`3777`	`return false;`
`3778`	`3778`	`}`
`3779`		`- bool complete = false;`
	`3779`	`+ DatabaseCursor::Status status = DatabaseCursor::Status::FAIL;`
`3780`	`3780`	`while (true) {`
`3781`	`3781`	`CDataStream ss_key(SER_DISK, CLIENT_VERSION);`
`3782`	`3782`	`CDataStream ss_value(SER_DISK, CLIENT_VERSION);`
`3783`		`- bool ret = cursor->Next(ss_key, ss_value, complete);`
`3784`		`- if (complete \|\| !ret) {`
	`3783`	`+ status = cursor->Next(ss_key, ss_value);`
	`3784`	`+ if (status != DatabaseCursor::Status::MORE) {`
`3785`	`3785`	`break;`
`3786`	`3786`	`}`
`3787`	`3787`	`SerializeData key(ss_key.begin(), ss_key.end());`
`@@ -3790,7 +3790,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)`
`3790`	`3790`	`}`
`3791`	`3791`	`cursor.reset();`
`3792`	`3792`	`batch.reset();`
`3793`		`- if (!complete) {`
	`3793`	`+ if (status != DatabaseCursor::Status::DONE) {`
`3794`	`3794`	`error = _("Error: Unable to read all records in the database");`
`3795`	`3795`	`return false;`
`3796`	`3796`	`}`