Skip to content

Commit a62231b

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#26690: wallet: Refactor database cursor into its own object with proper return codes
4aebd83 db: Change DatabaseCursor::Next to return status enum (Andrew Chow) d79e8dc wallet: Have cursor users use DatabaseCursor directly (Andrew Chow) 7a198bb wallet: Introduce DatabaseCursor RAII class for managing cursor (Andrew Chow) 69efbc0 Move SafeDbt out of BerkeleyBatch (Andrew Chow) Pull request description: Instead of having database cursors be tied to a particular `DatabaseBatch` object and requiring its setup and teardown be separate functions in that batch, we can have cursors be separate RAII classes. This makes it easier to create and destroy cursors as well as having cursors that have slightly different behaviors. Additionally, since reading data from a cursor is a tri-state, this PR changes the return value of the `Next` function (formerly `ReadAtCursor`) to return an Enum rather than the current system of 2 booleans. This greatly simplifies and unifies the code that deals with cursors as now there is no confusion as to what the function returns when there are no records left to be read. Extracted from #24914 ACKs for top commit: furszy: diff ACK 4aebd83 theStack: Code-review ACK 4aebd83 Tree-SHA512: 5d0be56a18de5b08c777dd5a73ba5a6ef1e696fdb07d1dca952a88ded07887b7c5c04342f9a76feb2f6fe24a45dc31f094f1f5d9500e6bdf4a44f4edb66dcaa1
2 parents 5271c77 + 4aebd83 commit a62231b

File tree

11 files changed

+178
-134
lines changed

11 files changed

+178
-134
lines changed

‎src/wallet/bdb.cpp

Lines changed: 36 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <wallet/bdb.h>
99
#include <wallet/db.h>
1010

11+
#include <util/check.h>
1112
#include <util/strencodings.h>
1213
#include <util/translation.h>
1314

@@ -220,17 +221,17 @@ BerkeleyEnvironment::BerkeleyEnvironment() : m_use_shared_memory(false)
220221
fMockDb = true;
221222
}
222223

223-
BerkeleyBatch::SafeDbt::SafeDbt()
224+
SafeDbt::SafeDbt()
224225
{
225226
m_dbt.set_flags(DB_DBT_MALLOC);
226227
}
227228

228-
BerkeleyBatch::SafeDbt::SafeDbt(void* data, size_t size)
229+
SafeDbt::SafeDbt(void* data, size_t size)
229230
: m_dbt(data, size)
230231
{
231232
}
232233

233-
BerkeleyBatch::SafeDbt::~SafeDbt()
234+
SafeDbt::~SafeDbt()
234235
{
235236
if (m_dbt.get_data() != nullptr) {
236237
// Clear memory, e.g. in case it was a private key
@@ -244,17 +245,17 @@ BerkeleyBatch::SafeDbt::~SafeDbt()
244245
}
245246
}
246247

247-
const void* BerkeleyBatch::SafeDbt::get_data() const
248+
const void* SafeDbt::get_data() const
248249
{
249250
return m_dbt.get_data();
250251
}
251252

252-
uint32_t BerkeleyBatch::SafeDbt::get_size() const
253+
uint32_t SafeDbt::get_size() const
253254
{
254255
return m_dbt.get_size();
255256
}
256257

257-
BerkeleyBatch::SafeDbt::operator Dbt*()
258+
SafeDbt::operator Dbt*()
258259
{
259260
return &m_dbt;
260261
}
@@ -307,7 +308,7 @@ BerkeleyDatabase::~BerkeleyDatabase()
307308
}
308309
}
309310

310-
BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database)
311+
BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_database(database)
311312
{
312313
database.AddRef();
313314
database.Open();
@@ -398,7 +399,6 @@ void BerkeleyBatch::Close()
398399
activeTxn->abort();
399400
activeTxn = nullptr;
400401
pdb = nullptr;
401-
CloseCursor();
402402

403403
if (fFlushOnClose)
404404
Flush();
@@ -476,15 +476,15 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
476476
fSuccess = false;
477477
}
478478

479-
if (db.StartCursor()) {
479+
std::unique_ptr<DatabaseCursor> cursor = db.GetNewCursor();
480+
if (cursor) {
480481
while (fSuccess) {
481482
CDataStream ssKey(SER_DISK, CLIENT_VERSION);
482483
CDataStream ssValue(SER_DISK, CLIENT_VERSION);
483-
bool complete;
484-
bool ret1 = db.ReadAtCursor(ssKey, ssValue, complete);
485-
if (complete) {
484+
DatabaseCursor::Status ret1 = cursor->Next(ssKey, ssValue);
485+
if (ret1 == DatabaseCursor::Status::DONE) {
486486
break;
487-
} else if (!ret1) {
487+
} else if (ret1 == DatabaseCursor::Status::FAIL) {
488488
fSuccess = false;
489489
break;
490490
}
@@ -502,7 +502,7 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
502502
if (ret2 > 0)
503503
fSuccess = false;
504504
}
505-
db.CloseCursor();
505+
cursor.reset();
506506
}
507507
if (fSuccess) {
508508
db.Close();
@@ -656,30 +656,30 @@ void BerkeleyDatabase::ReloadDbEnv()
656656
env->ReloadDbEnv();
657657
}
658658

659-
bool BerkeleyBatch::StartCursor()
659+
BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database)
660660
{
661-
assert(!m_cursor);
662-
if (!pdb)
663-
return false;
664-
int ret = pdb->cursor(nullptr, &m_cursor, 0);
665-
return ret == 0;
661+
if (!database.m_db.get()) {
662+
throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist"));
663+
}
664+
int ret = database.m_db->cursor(nullptr, &m_cursor, 0);
665+
if (ret != 0) {
666+
throw std::runtime_error(STR_INTERNAL_BUG(strprintf("BDB Cursor could not be created. Returned %d", ret).c_str()));
667+
}
666668
}
667669

668-
bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
670+
DatabaseCursor::Status BerkeleyCursor::Next(CDataStream& ssKey, CDataStream& ssValue)
669671
{
670-
complete = false;
671-
if (m_cursor == nullptr) return false;
672+
if (m_cursor == nullptr) return Status::FAIL;
672673
// Read at cursor
673674
SafeDbt datKey;
674675
SafeDbt datValue;
675676
int ret = m_cursor->get(datKey, datValue, DB_NEXT);
676677
if (ret == DB_NOTFOUND) {
677-
complete = true;
678+
return Status::DONE;
679+
}
680+
if (ret != 0 || datKey.get_data() == nullptr || datValue.get_data() == nullptr) {
681+
return Status::FAIL;
678682
}
679-
if (ret != 0)
680-
return false;
681-
else if (datKey.get_data() == nullptr || datValue.get_data() == nullptr)
682-
return false;
683683

684684
// Convert to streams
685685
ssKey.SetType(SER_DISK);
@@ -688,16 +688,22 @@ bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool&
688688
ssValue.SetType(SER_DISK);
689689
ssValue.clear();
690690
ssValue.write({AsBytePtr(datValue.get_data()), datValue.get_size()});
691-
return true;
691+
return Status::MORE;
692692
}
693693

694-
void BerkeleyBatch::CloseCursor()
694+
BerkeleyCursor::~BerkeleyCursor()
695695
{
696696
if (!m_cursor) return;
697697
m_cursor->close();
698698
m_cursor = nullptr;
699699
}
700700

701+
std::unique_ptr<DatabaseCursor> BerkeleyBatch::GetNewCursor()
702+
{
703+
if (!pdb) return nullptr;
704+
return std::make_unique<BerkeleyCursor>(m_database);
705+
}
706+
701707
bool BerkeleyBatch::TxnBegin()
702708
{
703709
if (!pdb || activeTxn)

‎src/wallet/bdb.h

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -165,29 +165,41 @@ class BerkeleyDatabase : public WalletDatabase
165165
std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) override;
166166
};
167167

168-
/** RAII class that provides access to a Berkeley database */
169-
class BerkeleyBatch : public DatabaseBatch
168+
/** RAII class that automatically cleanses its data on destruction */
169+
class SafeDbt final
170170
{
171-
/** RAII class that automatically cleanses its data on destruction */
172-
class SafeDbt final
173-
{
174-
Dbt m_dbt;
171+
Dbt m_dbt;
175172

176-
public:
177-
// construct Dbt with internally-managed data
178-
SafeDbt();
179-
// construct Dbt with provided data
180-
SafeDbt(void* data, size_t size);
181-
~SafeDbt();
173+
public:
174+
// construct Dbt with internally-managed data
175+
SafeDbt();
176+
// construct Dbt with provided data
177+
SafeDbt(void* data, size_t size);
178+
~SafeDbt();
179+
180+
// delegate to Dbt
181+
const void* get_data() const;
182+
uint32_t get_size() const;
183+
184+
// conversion operator to access the underlying Dbt
185+
operator Dbt*();
186+
};
182187

183-
// delegate to Dbt
184-
const void* get_data() const;
185-
uint32_t get_size() const;
188+
class BerkeleyCursor : public DatabaseCursor
189+
{
190+
private:
191+
Dbc* m_cursor;
186192

187-
// conversion operator to access the underlying Dbt
188-
operator Dbt*();
189-
};
193+
public:
194+
explicit BerkeleyCursor(BerkeleyDatabase& database);
195+
~BerkeleyCursor() override;
196+
197+
Status Next(CDataStream& key, CDataStream& value) override;
198+
};
190199

200+
/** RAII class that provides access to a Berkeley database */
201+
class BerkeleyBatch : public DatabaseBatch
202+
{
191203
private:
192204
bool ReadKey(CDataStream&& key, CDataStream& value) override;
193205
bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite = true) override;
@@ -198,7 +210,6 @@ class BerkeleyBatch : public DatabaseBatch
198210
Db* pdb;
199211
std::string strFile;
200212
DbTxn* activeTxn;
201-
Dbc* m_cursor;
202213
bool fReadOnly;
203214
bool fFlushOnClose;
204215
BerkeleyEnvironment *env;
@@ -214,9 +225,7 @@ class BerkeleyBatch : public DatabaseBatch
214225
void Flush() override;
215226
void Close() override;
216227

217-
bool StartCursor() override;
218-
bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override;
219-
void CloseCursor() override;
228+
std::unique_ptr<DatabaseCursor> GetNewCursor() override;
220229
bool TxnBegin() override;
221230
bool TxnCommit() override;
222231
bool TxnAbort() override;

‎src/wallet/db.h

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,25 @@ struct bilingual_str;
2222
namespace wallet {
2323
void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename);
2424

25+
class DatabaseCursor
26+
{
27+
public:
28+
explicit DatabaseCursor() {}
29+
virtual ~DatabaseCursor() {}
30+
31+
DatabaseCursor(const DatabaseCursor&) = delete;
32+
DatabaseCursor& operator=(const DatabaseCursor&) = delete;
33+
34+
enum class Status
35+
{
36+
FAIL,
37+
MORE,
38+
DONE,
39+
};
40+
41+
virtual Status Next(CDataStream& key, CDataStream& value) { return Status::FAIL; }
42+
};
43+
2544
/** RAII class that provides access to a WalletDatabase */
2645
class DatabaseBatch
2746
{
@@ -92,9 +111,7 @@ class DatabaseBatch
92111
return HasKey(std::move(ssKey));
93112
}
94113

95-
virtual bool StartCursor() = 0;
96-
virtual bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) = 0;
97-
virtual void CloseCursor() = 0;
114+
virtual std::unique_ptr<DatabaseCursor> GetNewCursor() = 0;
98115
virtual bool TxnBegin() = 0;
99116
virtual bool TxnCommit() = 0;
100117
virtual bool TxnAbort() = 0;
@@ -156,6 +173,11 @@ class WalletDatabase
156173
virtual std::unique_ptr<DatabaseBatch> MakeBatch(bool flush_on_close = true) = 0;
157174
};
158175

176+
class DummyCursor : public DatabaseCursor
177+
{
178+
Status Next(CDataStream& key, CDataStream& value) override { return Status::FAIL; }
179+
};
180+
159181
/** RAII class that provides access to a DummyDatabase. Never fails. */
160182
class DummyBatch : public DatabaseBatch
161183
{
@@ -169,9 +191,7 @@ class DummyBatch : public DatabaseBatch
169191
void Flush() override {}
170192
void Close() override {}
171193

172-
bool StartCursor() override { return true; }
173-
bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return true; }
174-
void CloseCursor() override {}
194+
std::unique_ptr<DatabaseCursor> GetNewCursor() override { return std::make_unique<DummyCursor>(); }
175195
bool TxnBegin() override { return true; }
176196
bool TxnCommit() override { return true; }
177197
bool TxnAbort() override { return true; }

‎src/wallet/dump.cpp

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
4747
std::unique_ptr<DatabaseBatch> batch = db.MakeBatch();
4848

4949
bool ret = true;
50-
if (!batch->StartCursor()) {
50+
std::unique_ptr<DatabaseCursor> cursor = batch->GetNewCursor();
51+
if (!cursor) {
5152
error = _("Error: Couldn't create cursor into database");
5253
ret = false;
5354
}
@@ -68,13 +69,13 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
6869
while (true) {
6970
CDataStream ss_key(SER_DISK, CLIENT_VERSION);
7071
CDataStream ss_value(SER_DISK, CLIENT_VERSION);
71-
bool complete;
72-
ret = batch->ReadAtCursor(ss_key, ss_value, complete);
73-
if (complete) {
72+
DatabaseCursor::Status status = cursor->Next(ss_key, ss_value);
73+
if (status == DatabaseCursor::Status::DONE) {
7474
ret = true;
7575
break;
76-
} else if (!ret) {
76+
} else if (status == DatabaseCursor::Status::FAIL) {
7777
error = _("Error reading next record from wallet database");
78+
ret = false;
7879
break;
7980
}
8081
std::string key_str = HexStr(ss_key);
@@ -85,7 +86,7 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
8586
}
8687
}
8788

88-
batch->CloseCursor();
89+
cursor.reset();
8990
batch.reset();
9091

9192
// Close the wallet after we're done with it. The caller won't be doing this

0 commit comments

Comments
 (0)