Skip to content

Commit 2a44c4e

Browse files
ryanofskyryanofsky
committed
refactor: Remove CAddressBookData::destdata
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Simplifies CWallet code, deals with used address and received request serialization in walletdb.cpp - Adds test coverage - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs
1 parent d4ebdce commit 2a44c4e

File tree

10 files changed

+147
-85
lines changed

10 files changed

+147
-85
lines changed

‎src/wallet/bdb.cpp‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -656,12 +656,12 @@ void BerkeleyDatabase::ReloadDbEnv()
656656
env->ReloadDbEnv();
657657
}
658658

659-
BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database)
659+
BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database, BerkeleyBatch* batch)
660660
{
661661
if (!database.m_db.get()) {
662662
throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist"));
663663
}
664-
int ret = database.m_db->cursor(nullptr, &m_cursor, 0);
664+
int ret = database.m_db->cursor(batch ? batch->txn() : nullptr, &m_cursor, 0);
665665
if (ret != 0) {
666666
throw std::runtime_error(STR_INTERNAL_BUG(strprintf("BDB Cursor could not be created. Returned %d", ret)));
667667
}
@@ -808,6 +808,22 @@ bool BerkeleyBatch::HasKey(DataStream&& key)
808808
return ret == 0;
809809
}
810810

811+
bool BerkeleyBatch::ErasePrefix(Span<std::byte> prefix)
812+
{
813+
if (!TxnBegin()) return false;
814+
auto cursor = std::make_unique<BerkeleyCursor>(m_database, this);
815+
Dbt prefix_key(prefix.data(), prefix.size()), prefix_value;
816+
int ret = cursor->dbc()->get(&prefix_key, &prefix_value, DB_SET_RANGE);
817+
for (int flag = DB_CURRENT; ret == 0; flag = DB_NEXT) {
818+
SafeDbt key, value;
819+
ret = cursor->dbc()->get(key, value, flag);
820+
if (ret != 0 || key.get_size() < prefix.size() || memcmp(key.get_data(), prefix.data(), prefix.size()) != 0) break;
821+
ret = cursor->dbc()->del(0);
822+
}
823+
cursor.reset();
824+
return TxnCommit() && (ret == 0 || ret == DB_NOTFOUND);
825+
}
826+
811827
void BerkeleyDatabase::AddRef()
812828
{
813829
LOCK(cs_db);

‎src/wallet/bdb.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,11 @@ class BerkeleyCursor : public DatabaseCursor
191191
Dbc* m_cursor;
192192

193193
public:
194-
explicit BerkeleyCursor(BerkeleyDatabase& database);
194+
explicit BerkeleyCursor(BerkeleyDatabase& database, BerkeleyBatch* batch=nullptr);
195195
~BerkeleyCursor() override;
196196

197197
Status Next(DataStream& key, DataStream& value) override;
198+
Dbc* dbc() const { return m_cursor; }
198199
};
199200

200201
/** RAII class that provides access to a Berkeley database */
@@ -205,6 +206,7 @@ class BerkeleyBatch : public DatabaseBatch
205206
bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite = true) override;
206207
bool EraseKey(DataStream&& key) override;
207208
bool HasKey(DataStream&& key) override;
209+
bool ErasePrefix(Span<std::byte> prefix) override;
208210

209211
protected:
210212
Db* pdb{nullptr};
@@ -229,6 +231,7 @@ class BerkeleyBatch : public DatabaseBatch
229231
bool TxnBegin() override;
230232
bool TxnCommit() override;
231233
bool TxnAbort() override;
234+
DbTxn* txn() const { return activeTxn; }
232235
};
233236

234237
std::string BerkeleyDatabaseVersion();

‎src/wallet/db.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ class DatabaseBatch
110110

111111
return HasKey(std::move(ssKey));
112112
}
113+
virtual bool ErasePrefix(Span<std::byte> prefix) = 0;
113114

114115
virtual std::unique_ptr<DatabaseCursor> GetNewCursor() = 0;
115116
virtual bool TxnBegin() = 0;
@@ -186,6 +187,7 @@ class DummyBatch : public DatabaseBatch
186187
bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite = true) override { return true; }
187188
bool EraseKey(DataStream&& key) override { return true; }
188189
bool HasKey(DataStream&& key) override { return true; }
190+
bool ErasePrefix(Span<std::byte> prefix) override { return true; }
189191

190192
public:
191193
void Flush() override {}

‎src/wallet/sqlite.cpp‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ void SQLiteBatch::SetupSQLStatements()
125125
{&m_insert_stmt, "INSERT INTO main VALUES(?, ?)"},
126126
{&m_overwrite_stmt, "INSERT or REPLACE into main values(?, ?)"},
127127
{&m_delete_stmt, "DELETE FROM main WHERE key = ?"},
128+
{&m_delete_prefix_stmt, "DELETE FROM main WHERE instr(key, ?) = 1"},
128129
};
129130

130131
for (const auto& [stmt_prepared, stmt_text] : statements) {
@@ -375,6 +376,7 @@ void SQLiteBatch::Close()
375376
{&m_insert_stmt, "insert"},
376377
{&m_overwrite_stmt, "overwrite"},
377378
{&m_delete_stmt, "delete"},
379+
{&m_delete_prefix_stmt, "delete prefix"},
378380
};
379381

380382
for (const auto& [stmt_prepared, stmt_description] : statements) {
@@ -441,24 +443,34 @@ bool SQLiteBatch::WriteKey(DataStream&& key, DataStream&& value, bool overwrite)
441443
return res == SQLITE_DONE;
442444
}
443445

444-
bool SQLiteBatch::EraseKey(DataStream&& key)
446+
bool SQLiteBatch::ExecStatement(sqlite3_stmt* stmt, Span<const std::byte> blob)
445447
{
446448
if (!m_database.m_db) return false;
447-
assert(m_delete_stmt);
449+
assert(stmt);
448450

449451
// Bind: leftmost parameter in statement is index 1
450-
if (!BindBlobToStatement(m_delete_stmt, 1, key, "key")) return false;
452+
if (!BindBlobToStatement(stmt, 1, blob, "key")) return false;
451453

452454
// Execute
453-
int res = sqlite3_step(m_delete_stmt);
454-
sqlite3_clear_bindings(m_delete_stmt);
455-
sqlite3_reset(m_delete_stmt);
455+
int res = sqlite3_step(stmt);
456+
sqlite3_clear_bindings(stmt);
457+
sqlite3_reset(stmt);
456458
if (res != SQLITE_DONE) {
457459
LogPrintf("%s: Unable to execute statement: %s\n", __func__, sqlite3_errstr(res));
458460
}
459461
return res == SQLITE_DONE;
460462
}
461463

464+
bool SQLiteBatch::EraseKey(DataStream&& key)
465+
{
466+
return ExecStatement(m_delete_stmt, key);
467+
}
468+
469+
bool SQLiteBatch::ErasePrefix(Span<std::byte> prefix)
470+
{
471+
return ExecStatement(m_delete_prefix_stmt, prefix);
472+
}
473+
462474
bool SQLiteBatch::HasKey(DataStream&& key)
463475
{
464476
if (!m_database.m_db) return false;

‎src/wallet/sqlite.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,16 @@ class SQLiteBatch : public DatabaseBatch
3636
sqlite3_stmt* m_insert_stmt{nullptr};
3737
sqlite3_stmt* m_overwrite_stmt{nullptr};
3838
sqlite3_stmt* m_delete_stmt{nullptr};
39+
sqlite3_stmt* m_delete_prefix_stmt{nullptr};
3940

4041
void SetupSQLStatements();
42+
bool ExecStatement(sqlite3_stmt* stmt, Span<const std::byte> blob);
4143

4244
bool ReadKey(DataStream&& key, DataStream& value) override;
4345
bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite = true) override;
4446
bool EraseKey(DataStream&& key) override;
4547
bool HasKey(DataStream&& key) override;
48+
bool ErasePrefix(Span<std::byte> prefix) override;
4649

4750
public:
4851
explicit SQLiteBatch(SQLiteDatabase& database);

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 57 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -427,19 +427,63 @@ BOOST_AUTO_TEST_CASE(ComputeTimeSmart)
427427
BOOST_CHECK_EQUAL(AddTx(*m_node.chainman, m_wallet, 5, 50, 600), 300);
428428
}
429429

430-
BOOST_AUTO_TEST_CASE(LoadReceiveRequests)
430+
static const DatabaseFormat DATABASE_FORMATS[] = {
431+
#ifdef USE_SQLITE
432+
DatabaseFormat::SQLITE,
433+
#endif
434+
#ifdef USE_BDB
435+
DatabaseFormat::BERKELEY,
436+
#endif
437+
};
438+
439+
void TestLoadWallet(const std::string& name, DatabaseFormat format, std::function<void(std::shared_ptr<CWallet>)> f)
431440
{
432-
CTxDestination dest = PKHash();
433-
LOCK(m_wallet.cs_wallet);
434-
WalletBatch batch{m_wallet.GetDatabase()};
435-
m_wallet.SetAddressUsed(batch, dest, true);
436-
m_wallet.SetAddressReceiveRequest(batch, dest, "0", "val_rr0");
437-
m_wallet.SetAddressReceiveRequest(batch, dest, "1", "val_rr1");
438-
439-
auto values = m_wallet.GetAddressReceiveRequests();
440-
BOOST_CHECK_EQUAL(values.size(), 2U);
441-
BOOST_CHECK_EQUAL(values[0], "val_rr0");
442-
BOOST_CHECK_EQUAL(values[1], "val_rr1");
441+
node::NodeContext node;
442+
auto chain = interfaces::MakeChain(node);
443+
DatabaseOptions options;
444+
options.require_format = format;
445+
DatabaseStatus status;
446+
bilingual_str error;
447+
std::vector<bilingual_str> warnings;
448+
auto database = MakeWalletDatabase(name, options, status, error);
449+
auto wallet = std::make_shared<CWallet>(chain.get(), "", std::move(database));
450+
wallet->LoadWallet();
451+
WITH_LOCK(wallet->cs_wallet, f(wallet));
452+
}
453+
454+
BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup)
455+
{
456+
for (DatabaseFormat format : DATABASE_FORMATS) {
457+
const std::string name = strprintf("receive-requests-%i", format);
458+
TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
459+
BOOST_CHECK(!wallet->m_address_book[PKHash()].IsUsed());
460+
WalletBatch batch{wallet->GetDatabase()};
461+
BOOST_CHECK(batch.WriteUsed(PKHash(), true));
462+
BOOST_CHECK(batch.WriteUsed(ScriptHash(), true));
463+
BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "0", "val_rr00"));
464+
BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "0", ""));
465+
BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "1", "val_rr10"));
466+
BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, PKHash(), "1", "val_rr11"));
467+
BOOST_CHECK(wallet->SetAddressReceiveRequest(batch, ScriptHash(), "2", "val_rr20"));
468+
});
469+
TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
470+
BOOST_CHECK(wallet->m_address_book[PKHash()].IsUsed());
471+
BOOST_CHECK(wallet->m_address_book[ScriptHash()].IsUsed());
472+
auto requests = wallet->GetAddressReceiveRequests();
473+
auto erequests = {"val_rr11", "val_rr20"};
474+
BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
475+
WalletBatch batch{wallet->GetDatabase()};
476+
BOOST_CHECK(batch.WriteUsed(PKHash(), false));
477+
BOOST_CHECK(batch.EraseDestData(ScriptHash()));
478+
});
479+
TestLoadWallet(name, format, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
480+
BOOST_CHECK(!wallet->m_address_book[PKHash()].IsUsed());
481+
BOOST_CHECK(!wallet->m_address_book[ScriptHash()].IsUsed());
482+
auto requests = wallet->GetAddressReceiveRequests();
483+
auto erequests = {"val_rr11"};
484+
BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
485+
});
486+
}
443487
}
444488

445489
// Test some watch-only LegacyScriptPubKeyMan methods by the procedure of loading (LoadWatchOnly),
@@ -922,6 +966,7 @@ class FailBatch : public DatabaseBatch
922966
bool WriteKey(DataStream&& key, DataStream&& value, bool overwrite = true) override { return m_pass; }
923967
bool EraseKey(DataStream&& key) override { return m_pass; }
924968
bool HasKey(DataStream&& key) override { return m_pass; }
969+
bool ErasePrefix(Span<std::byte> prefix) override { return m_pass; }
925970

926971
public:
927972
explicit FailBatch(bool pass) : m_pass(pass) {}

‎src/wallet/wallet.cpp‎

Lines changed: 12 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -2439,12 +2439,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
24392439
return false;
24402440
}
24412441
// Delete destdata tuples associated with address
2442-
std::string strAddress = EncodeDestination(address);
2443-
for (const std::pair<const std::string, std::string> &item : m_address_book[address].destdata)
2444-
{
2445-
batch.EraseDestData(strAddress, item.first);
2446-
}
2447-
m_address_book.erase(address);
2442+
batch.EraseDestData(address);
24482443
is_mine = IsMine(address) != ISMINE_NO;
24492444
}
24502445

@@ -2821,66 +2816,31 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old
28212816

28222817
bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used)
28232818
{
2824-
const std::string key{"used"};
2825-
if (std::get_if<CNoDestination>(&dest))
2826-
return false;
2827-
2828-
if (!used) {
2829-
if (auto* data = util::FindKey(m_address_book, dest)) data->destdata.erase(key);
2830-
return batch.EraseDestData(EncodeDestination(dest), key);
2831-
}
2832-
2833-
const std::string value{"1"};
2834-
m_address_book[dest].destdata.insert(std::make_pair(key, value));
2835-
return batch.WriteDestData(EncodeDestination(dest), key, value);
2836-
}
2837-
2838-
void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
2839-
{
2840-
m_address_book[dest].destdata.insert(std::make_pair(key, value));
2819+
m_address_book[dest].SetUsed(used);
2820+
return batch.WriteUsed(dest, used);
28412821
}
28422822

28432823
bool CWallet::IsAddressUsed(const CTxDestination& dest) const
28442824
{
2845-
const std::string key{"used"};
2846-
std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dest);
2847-
if(i != m_address_book.end())
2848-
{
2849-
CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key);
2850-
if(j != i->second.destdata.end())
2851-
{
2852-
return true;
2853-
}
2854-
}
2855-
return false;
2825+
auto it = m_address_book.find(dest);
2826+
return it != m_address_book.end() && it->second.IsUsed();
28562827
}
28572828

28582829
std::vector<std::string> CWallet::GetAddressReceiveRequests() const
28592830
{
2860-
const std::string prefix{"rr"};
2861-
std::vector<std::string> values;
2862-
for (const auto& address : m_address_book) {
2863-
for (const auto& data : address.second.destdata) {
2864-
if (!data.first.compare(0, prefix.size(), prefix)) {
2865-
values.emplace_back(data.second);
2866-
}
2831+
std::vector<std::string> requests;
2832+
for (const auto& dest : m_address_book) {
2833+
for (const auto& request : dest.second.GetReceiveRequests()) {
2834+
requests.emplace_back(request.second);
28672835
}
28682836
}
2869-
return values;
2837+
return requests;
28702838
}
28712839

28722840
bool CWallet::SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value)
28732841
{
2874-
const std::string key{"rr" + id}; // "rr" prefix = "receive request" in destdata
2875-
CAddressBookData& data = m_address_book.at(dest);
2876-
if (value.empty()) {
2877-
if (!batch.EraseDestData(EncodeDestination(dest), key)) return false;
2878-
data.destdata.erase(key);
2879-
} else {
2880-
if (!batch.WriteDestData(EncodeDestination(dest), key, value)) return false;
2881-
data.destdata[key] = value;
2882-
}
2883-
return true;
2842+
m_address_book[dest].SetReceiveRequest(id, value);
2843+
return batch.WriteReceiveRequest(dest, id, value);
28842844
}
28852845

28862846
std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string)

‎src/wallet/wallet.h‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,21 +205,29 @@ class CAddressBookData
205205
{
206206
private:
207207
bool m_change{true};
208+
bool m_used{false};
208209
std::string m_label;
210+
std::map<std::string, std::string> m_receive_requests;
209211
public:
210212
std::string purpose;
211213

212214
CAddressBookData() : purpose("unknown") {}
213215

214-
typedef std::map<std::string, std::string> StringMap;
215-
StringMap destdata;
216-
217216
bool IsChange() const { return m_change; }
217+
bool IsUsed() const { return m_used; }
218+
void SetUsed(bool used) { m_used = used;}
218219
const std::string& GetLabel() const { return m_label; }
219220
void SetLabel(const std::string& label) {
220221
m_change = false;
221222
m_label = label;
222223
}
224+
const std::map<std::string, std::string>& GetReceiveRequests() const { return m_receive_requests; }
225+
bool SetReceiveRequest(const std::string& id, const std::string& value)
226+
{
227+
if (value.empty()) return m_receive_requests.erase(id);
228+
m_receive_requests[id] = value;
229+
return true;
230+
}
223231
};
224232

225233
struct CRecipient

0 commit comments

Comments
 (0)