From 69efbc011bb74fcd8dd9ed2a8a5d31bc9e323c10 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 11 Apr 2022 14:49:46 -0400 Subject: [PATCH 1/4] Move SafeDbt out of BerkeleyBatch --- src/wallet/bdb.cpp | 12 ++++++------ src/wallet/bdb.h | 40 ++++++++++++++++++++-------------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 3a9d277f652..d45e8679ac3 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -220,17 +220,17 @@ BerkeleyEnvironment::BerkeleyEnvironment() : m_use_shared_memory(false) fMockDb = true; } -BerkeleyBatch::SafeDbt::SafeDbt() +SafeDbt::SafeDbt() { m_dbt.set_flags(DB_DBT_MALLOC); } -BerkeleyBatch::SafeDbt::SafeDbt(void* data, size_t size) +SafeDbt::SafeDbt(void* data, size_t size) : m_dbt(data, size) { } -BerkeleyBatch::SafeDbt::~SafeDbt() +SafeDbt::~SafeDbt() { if (m_dbt.get_data() != nullptr) { // Clear memory, e.g. in case it was a private key @@ -244,17 +244,17 @@ BerkeleyBatch::SafeDbt::~SafeDbt() } } -const void* BerkeleyBatch::SafeDbt::get_data() const +const void* SafeDbt::get_data() const { return m_dbt.get_data(); } -uint32_t BerkeleyBatch::SafeDbt::get_size() const +uint32_t SafeDbt::get_size() const { return m_dbt.get_size(); } -BerkeleyBatch::SafeDbt::operator Dbt*() +SafeDbt::operator Dbt*() { return &m_dbt; } diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index ddab85521be..35cc823876b 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -165,29 +165,29 @@ public: std::unique_ptr MakeBatch(bool flush_on_close = true) override; }; +/** RAII class that automatically cleanses its data on destruction */ +class SafeDbt final +{ + Dbt m_dbt; + +public: + // construct Dbt with internally-managed data + SafeDbt(); + // construct Dbt with provided data + SafeDbt(void* data, size_t size); + ~SafeDbt(); + + // delegate to Dbt + const void* get_data() const; + uint32_t get_size() const; + + // conversion operator to access the underlying Dbt + operator Dbt*(); +}; + /** RAII class that provides access to a Berkeley database */ class BerkeleyBatch : public DatabaseBatch { - /** RAII class that automatically cleanses its data on destruction */ - class SafeDbt final - { - Dbt m_dbt; - - public: - // construct Dbt with internally-managed data - SafeDbt(); - // construct Dbt with provided data - SafeDbt(void* data, size_t size); - ~SafeDbt(); - - // delegate to Dbt - const void* get_data() const; - uint32_t get_size() const; - - // conversion operator to access the underlying Dbt - operator Dbt*(); - }; - private: bool ReadKey(CDataStream&& key, CDataStream& value) override; bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite = true) override; From 7a198bba0a1d0a0f0fd4ca947955cb52b84bdd4b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 11 Apr 2022 15:14:24 -0400 Subject: [PATCH 2/4] 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 | 27 ++++++++++++++------- src/wallet/bdb.h | 17 +++++++++---- src/wallet/db.h | 41 +++++++++++++++++++++++++++----- src/wallet/sqlite.cpp | 39 +++++++++++++++++------------- src/wallet/sqlite.h | 18 +++++++++----- src/wallet/test/wallet_tests.cpp | 10 +++++--- 6 files changed, 108 insertions(+), 44 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index d45e8679ac3..edfd443b7d1 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -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 (!pdb) - return false; - int ret = pdb->cursor(nullptr, &m_cursor, 0); - return ret == 0; + if (!database.m_db.get()) { + throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist")); + } + int ret = database.m_db->cursor(nullptr, &m_cursor, 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 BerkeleyBatch::GetNewCursor() +{ + if (!pdb) return nullptr; + return std::make_unique(m_database); +} + bool BerkeleyBatch::TxnBegin() { if (!pdb || activeTxn) diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 35cc823876b..0ee94fa2bc9 100644 --- a/src/wallet/bdb.h +++ b/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; - bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override; - void CloseCursor() override; + std::unique_ptr GetNewCursor() override; bool TxnBegin() override; bool TxnCommit() override; bool TxnAbort() override; diff --git a/src/wallet/db.h b/src/wallet/db.h index f09844c37e4..aa1377ccef3 100644 --- a/src/wallet/db.h +++ b/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 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 bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) = 0; - virtual void CloseCursor() = 0; + virtual std::unique_ptr 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; @@ -156,6 +182,11 @@ public: virtual std::unique_ptr 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; } - bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return true; } - void CloseCursor() override {} + std::unique_ptr GetNewCursor() override { return std::make_unique(); } bool TxnBegin() override { return true; } bool TxnCommit() override { return true; } bool TxnAbort() override { return true; } diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 053fb8f9834..bc1b524adc5 100644 --- a/src/wallet/sqlite.cpp +++ b/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() -{ - 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) +bool SQLiteCursor::Next(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 SQLiteBatch::GetNewCursor() +{ + if (!m_database.m_db) return nullptr; + auto cursor = std::make_unique(); + + 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() diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index 47b7ebb0ecd..286ee1613d2 100644 --- a/src/wallet/sqlite.h +++ b/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; - bool ReadAtCursor(CDataStream& key, CDataStream& value, bool& complete) override; - void CloseCursor() override; + std::unique_ptr GetNewCursor() override; bool TxnBegin() override; bool TxnCommit() override; bool TxnAbort() override; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 0f703b7d00b..ea4dd30dea9 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/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; } - bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return false; } - void CloseCursor() override {} + std::unique_ptr GetNewCursor() override { return std::make_unique(); } bool TxnBegin() override { return false; } bool TxnCommit() override { return false; } bool TxnAbort() override { return false; } From d79e8dcf2981ef1964a2fde8c472b5de1ca1c963 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 11 Apr 2022 16:07:58 -0400 Subject: [PATCH 3/4] wallet: Have cursor users use DatabaseCursor directly Instead of having the DatabaseBatch manage the cursor, having the consumer handle it directly --- src/wallet/bdb.cpp | 8 ++++---- src/wallet/db.h | 16 ---------------- src/wallet/dump.cpp | 7 ++++--- src/wallet/test/util.cpp | 4 ++-- src/wallet/test/walletload_tests.cpp | 6 ++++-- src/wallet/wallet.cpp | 9 +++++---- src/wallet/walletdb.cpp | 22 ++++++++++------------ 7 files changed, 29 insertions(+), 43 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index edfd443b7d1..62b7d7bc16e 100644 --- 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 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 index aa1377ccef3..3373bd1de2d 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -38,8 +38,6 @@ public: class DatabaseBatch { private: - std::unique_ptr 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 @@ public: } virtual std::unique_ptr 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 index 2e46cf54543..ed3b05f1187 100644 --- 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 batch = db.MakeBatch(); bool ret = true; - if (!batch->StartCursor()) { + std::unique_ptr 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 index f6c7ecb598b..da744809eda 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -50,7 +50,7 @@ std::unique_ptr DuplicateMockDatabase(WalletDatabase& database, // Get a cursor to the original database auto batch = database.MakeBatch(); - batch->StartCursor(); + std::unique_ptr cursor = batch->GetNewCursor(); // Get a batch for the new database auto new_batch = new_database->MakeBatch(); @@ -60,7 +60,7 @@ std::unique_ptr 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 index 24d21c2f220..5af5ceb682d 100644 --- 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 batch = db.MakeBatch(false); - BOOST_CHECK(batch->StartCursor()); + BOOST_CHECK(batch); + std::unique_ptr 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 index 2c0ce89929c..bbf1df64c33 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3770,8 +3770,9 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) // Get all of the records for DB type migration std::unique_ptr batch = m_database->MakeBatch(); + std::unique_ptr cursor = batch->GetNewCursor(); std::vector> records; - if (!batch->StartCursor()) { + if (!cursor) { error = _("Error: Unable to begin reading all records in the database"); return false; } @@ -3779,15 +3780,15 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) 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 index 826cecfb6f0..1272b5378af 100644 --- 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 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& vTxHash, std::listStartCursor()) + std::unique_ptr 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& vTxHash, std::listReadAtCursor(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& vTxHash, std::listCloseCursor(); return result; } @@ -1114,7 +1113,8 @@ bool WalletBatch::WriteWalletFlags(const uint64_t flags) bool WalletBatch::EraseRecords(const std::unordered_set& types) { // Get cursor - if (!m_batch->StartCursor()) + std::unique_ptr cursor = m_batch->GetNewCursor(); + if (!cursor) { return false; } @@ -1126,13 +1126,12 @@ bool WalletBatch::EraseRecords(const std::unordered_set& 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& types) m_batch->Erase(key_data); } } - m_batch->CloseCursor(); return true; } From 4aebd832a405090c2608e4b60bb4f34501bcea61 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 29 Nov 2022 22:34:26 -0500 Subject: [PATCH 4/4] db: Change DatabaseCursor::Next to return status enum 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. --- src/wallet/bdb.cpp | 23 ++++++++++------------- src/wallet/bdb.h | 2 +- src/wallet/db.h | 11 +++++++++-- src/wallet/dump.cpp | 8 ++++---- src/wallet/sqlite.cpp | 11 ++++------- src/wallet/sqlite.h | 2 +- src/wallet/test/util.cpp | 6 +++--- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/test/walletload_tests.cpp | 6 +++--- src/wallet/wallet.cpp | 8 ++++---- src/wallet/walletdb.cpp | 25 +++++++++---------------- 11 files changed, 49 insertions(+), 55 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 62b7d7bc16e..c2a96319265 100644 --- 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 index 0ee94fa2bc9..aabd8828bd8 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -194,7 +194,7 @@ public: 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 index 3373bd1de2d..d040af0d146 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -31,7 +31,14 @@ public: 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 @@ public: 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 index ed3b05f1187..26551115329 100644 --- 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 index bc1b524adc5..7ee641a9364 100644 --- 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 index 286ee1613d2..23111cda163 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -22,7 +22,7 @@ public: 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 index da744809eda..1604187120d 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -59,9 +59,9 @@ std::unique_ptr 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 index ea4dd30dea9..600702a5cbb 100644 --- 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 index 5af5ceb682d..f74bf54d9e4 100644 --- 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 index bbf1df64c33..30ba4d2551e 100644 --- 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 index 1272b5378af..79700f06677 100644 --- 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& vTxHash, std::listNext(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& 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; }