Skip to content

Commit 6847561

Browse files
ryanofskyryanofsky
committed
refactor: Remove CAddressBookData::destdata
This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - 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 eef90c1 commit 6847561

File tree

11 files changed

+126
-149
lines changed

11 files changed

+126
-149
lines changed

‎src/interfaces/wallet.cpp‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -177,22 +177,20 @@ class WalletImpl : public Wallet
177177
}
178178
return result;
179179
}
180-
bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override
181-
{
180+
std::vector<std::string> getReceiveRequests() override {
182181
LOCK(m_wallet->cs_wallet);
183-
WalletBatch batch{m_wallet->GetDatabase()};
184-
return m_wallet->AddDestData(batch, dest, key, value);
182+
std::vector<std::string> requests;
183+
for (const auto& dest : m_wallet->m_address_book) {
184+
for (const auto& request : dest.second.GetReceiveRequests()) {
185+
requests.emplace_back(request.second);
186+
}
187+
}
188+
return requests;
185189
}
186-
bool eraseDestData(const CTxDestination& dest, const std::string& key) override
187-
{
190+
bool saveReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) override {
188191
LOCK(m_wallet->cs_wallet);
189192
WalletBatch batch{m_wallet->GetDatabase()};
190-
return m_wallet->EraseDestData(batch, dest, key);
191-
}
192-
std::vector<std::string> getDestValues(const std::string& prefix) override
193-
{
194-
LOCK(m_wallet->cs_wallet);
195-
return m_wallet->GetDestValues(prefix);
193+
return m_wallet->m_address_book[dest].SetReceiveRequest(id, value) && batch.WriteReceiveRequest(dest, id, value);
196194
}
197195
void lockCoin(const COutPoint& output) override
198196
{

‎src/interfaces/wallet.h‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -109,14 +109,11 @@ class Wallet
109109
//! Get wallet address list.
110110
virtual std::vector<WalletAddress> getAddresses() = 0;
111111

112-
//! Add dest data.
113-
virtual bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) = 0;
112+
//! Get receive requests.
113+
virtual std::vector<std::string> getReceiveRequests() = 0;
114114

115-
//! Erase dest data.
116-
virtual bool eraseDestData(const CTxDestination& dest, const std::string& key) = 0;
117-
118-
//! Get dest values with prefix.
119-
virtual std::vector<std::string> getDestValues(const std::string& prefix) = 0;
115+
//! Save receive request.
116+
virtual bool saveReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0;
120117

121118
//! Lock coin.
122119
virtual void lockCoin(const COutPoint& output) = 0;

‎src/qt/recentrequeststablemodel.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,18 +10,20 @@
1010
#include <qt/walletmodel.h>
1111

1212
#include <clientversion.h>
13+
#include <key_io.h>
14+
#include <interfaces/wallet.h>
1315
#include <streams.h>
16+
#include <util/string.h>
1417

1518
#include <utility>
1619

1720
RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
1821
QAbstractTableModel(parent), walletModel(parent)
1922
{
2023
// Load entries from wallet
21-
std::vector<std::string> vReceiveRequests;
22-
parent->loadReceiveRequests(vReceiveRequests);
23-
for (const std::string& request : vReceiveRequests)
24+
for (const std::string& request : parent->wallet().getReceiveRequests()) {
2425
addNewRequest(request);
26+
}
2527

2628
/* These columns must match the indices in the ColumnIndex enumeration */
2729
columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();
@@ -141,7 +143,7 @@ bool RecentRequestsTableModel::removeRows(int row, int count, const QModelIndex
141143
for (int i = 0; i < count; ++i)
142144
{
143145
const RecentRequestEntry* rec = &list[row+i];
144-
if (!walletModel->saveReceiveRequest(rec->recipient.address.toStdString(), rec->id, ""))
146+
if (!walletModel->wallet().saveReceiveRequest(DecodeDestination(rec->recipient.address.toStdString()), ToString(rec->id), ""))
145147
return false;
146148
}
147149

@@ -170,7 +172,7 @@ void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient
170172
CDataStream ss(SER_DISK, CLIENT_VERSION);
171173
ss << newEntry;
172174

173-
if (!walletModel->saveReceiveRequest(recipient.address.toStdString(), newEntry.id, ss.str()))
175+
if (!walletModel->wallet().saveReceiveRequest(DecodeDestination(recipient.address.toStdString()), ToString(newEntry.id), ss.str()))
174176
return;
175177

176178
addNewRequest(newEntry);

‎src/qt/walletmodel.cpp‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -459,25 +459,6 @@ void WalletModel::UnlockContext::CopyFrom(UnlockContext&& rhs)
459459
rhs.relock = false;
460460
}
461461

462-
void WalletModel::loadReceiveRequests(std::vector<std::string>& vReceiveRequests)
463-
{
464-
vReceiveRequests = m_wallet->getDestValues("rr"); // receive request
465-
}
466-
467-
bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest)
468-
{
469-
CTxDestination dest = DecodeDestination(sAddress);
470-
471-
std::stringstream ss;
472-
ss << nId;
473-
std::string key = "rr" + ss.str(); // "rr" prefix = "receive request" in destdata
474-
475-
if (sRequest.empty())
476-
return m_wallet->eraseDestData(dest, key);
477-
else
478-
return m_wallet->addDestData(dest, key, sRequest);
479-
}
480-
481462
bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
482463
{
483464
CCoinControl coin_control;

‎src/qt/walletmodel.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -135,9 +135,6 @@ class WalletModel : public QObject
135135

136136
UnlockContext requestUnlock();
137137

138-
void loadReceiveRequests(std::vector<std::string>& vReceiveRequests);
139-
bool saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest);
140-
141138
bool bumpFee(uint256 hash, uint256& new_hash);
142139

143140
static bool isWalletEnabled();

‎src/wallet/db.h‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ class BerkeleyBatch
341341
if (!pdb)
342342
return nullptr;
343343
Dbc* pcursor = nullptr;
344-
int ret = pdb->cursor(nullptr, &pcursor, 0);
344+
int ret = pdb->cursor(activeTxn, &pcursor, 0);
345345
if (ret != 0)
346346
return nullptr;
347347
return pcursor;
@@ -368,6 +368,22 @@ class BerkeleyBatch
368368
return 0;
369369
}
370370

371+
bool ErasePrefix(const char* data, size_t size)
372+
{
373+
TxnBegin();
374+
Dbc* pcursor = GetCursor();
375+
Dbt prefix((void*)data, size), prefix_value;
376+
int ret = pcursor->get(&prefix, &prefix_value, DB_SET_RANGE);
377+
for (int flag = DB_CURRENT; ret == 0; flag = DB_NEXT) {
378+
SafeDbt key, value;
379+
if ((ret = pcursor->get(key, value, flag)) != 0 || key.get_size() < size || memcmp(key.get_data(), data, size) != 0) break;
380+
pcursor->del(0);
381+
}
382+
pcursor->close();
383+
TxnCommit();
384+
return ret == 0 || ret == DB_NOTFOUND;
385+
}
386+
371387
bool TxnBegin()
372388
{
373389
if (!pdb || activeTxn)

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -358,19 +358,47 @@ BOOST_AUTO_TEST_CASE(ComputeTimeSmart)
358358
SetMockTime(0);
359359
}
360360

361-
BOOST_AUTO_TEST_CASE(LoadReceiveRequests)
361+
template<typename F>
362+
void TestLoadWallet(std::shared_ptr<BerkeleyEnvironment> env, F&& f)
362363
{
363-
CTxDestination dest = PKHash();
364-
LOCK(m_wallet.cs_wallet);
365-
WalletBatch batch{m_wallet.GetDatabase()};
366-
m_wallet.AddDestData(batch, dest, "misc", "val_misc");
367-
m_wallet.AddDestData(batch, dest, "rr0", "val_rr0");
368-
m_wallet.AddDestData(batch, dest, "rr1", "val_rr1");
369-
370-
auto values = m_wallet.GetDestValues("rr");
371-
BOOST_CHECK_EQUAL(values.size(), 2U);
372-
BOOST_CHECK_EQUAL(values[0], "val_rr0");
373-
BOOST_CHECK_EQUAL(values[1], "val_rr1");
364+
NodeContext node;
365+
auto chain = interfaces::MakeChain(node);
366+
auto wallet = std::make_shared<CWallet>(chain.get(), WalletLocation(), MakeUnique<WalletDatabase>(env, ""));
367+
bool first_run;
368+
wallet->LoadWallet(first_run);
369+
WITH_LOCK(wallet->cs_wallet, f(wallet));
370+
}
371+
372+
BOOST_FIXTURE_TEST_CASE(LoadReceiveRequests, TestingSetup)
373+
{
374+
auto env = std::make_shared<BerkeleyEnvironment>();
375+
TestLoadWallet(env, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
376+
BOOST_CHECK(!wallet->m_address_book[PKHash()].IsUsed());
377+
BOOST_CHECK(WalletBatch(wallet->GetDatabase()).WriteUsed(PKHash(), true));
378+
BOOST_CHECK(WalletBatch(wallet->GetDatabase()).WriteUsed(ScriptHash(), true));
379+
BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "0", "val_rr00"));
380+
BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "0", ""));
381+
BOOST_CHECK(!interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "0", ""));
382+
BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "1", "val_rr10"));
383+
BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(PKHash(), "1", "val_rr11"));
384+
BOOST_CHECK(interfaces::MakeWallet(wallet)->saveReceiveRequest(ScriptHash(), "2", "val_rr20"));
385+
});
386+
TestLoadWallet(env, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
387+
BOOST_CHECK(wallet->m_address_book[PKHash()].IsUsed());
388+
BOOST_CHECK(wallet->m_address_book[ScriptHash()].IsUsed());
389+
auto requests = interfaces::MakeWallet(wallet)->getReceiveRequests();
390+
auto erequests = {"val_rr11", "val_rr20"};
391+
BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
392+
BOOST_CHECK(WalletBatch(wallet->GetDatabase()).WriteUsed(PKHash(), false));
393+
BOOST_CHECK(WalletBatch(wallet->GetDatabase()).EraseDestData(ScriptHash()));
394+
});
395+
TestLoadWallet(env, [](std::shared_ptr<CWallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet) {
396+
BOOST_CHECK(!wallet->m_address_book[PKHash()].IsUsed());
397+
BOOST_CHECK(!wallet->m_address_book[ScriptHash()].IsUsed());
398+
auto requests = interfaces::MakeWallet(wallet)->getReceiveRequests();
399+
auto erequests = {"val_rr11"};
400+
BOOST_CHECK_EQUAL_COLLECTIONS(requests.begin(), requests.end(), std::begin(erequests), std::end(erequests));
401+
});
374402
}
375403

376404
// Test some watch-only LegacyScriptPubKeyMan methods by the procedure of loading (LoadWatchOnly),

‎src/wallet/wallet.cpp‎

Lines changed: 15 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -727,6 +727,12 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
727727
return success;
728728
}
729729

730+
static bool IsUsedDest(const std::map<CTxDestination, CAddressBookData>& address_book, const CTxDestination& dest)
731+
{
732+
auto it = address_book.find(dest);
733+
return it != address_book.end() && it->second.IsUsed();
734+
}
735+
730736
void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned int n, bool used, std::set<CTxDestination>& tx_destinations)
731737
{
732738
AssertLockHeld(cs_wallet);
@@ -736,12 +742,12 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned
736742
CTxDestination dst;
737743
if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) {
738744
if (IsMine(dst)) {
739-
if (used && !GetDestData(dst, "used", nullptr)) {
740-
if (AddDestData(batch, dst, "used", "p")) { // p for "present", opposite of absent (null)
745+
if (used != IsUsedDest(m_address_book, dst)) {
746+
if (used) {
741747
tx_destinations.insert(dst);
742748
}
743-
} else if (!used && GetDestData(dst, "used", nullptr)) {
744-
EraseDestData(batch, dst, "used");
749+
m_address_book[dst].SetUsed(used);
750+
batch.WriteUsed(dst, used);
745751
}
746752
}
747753
}
@@ -758,23 +764,23 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
758764
if (!ExtractDestination(srctx->tx->vout[n].scriptPubKey, dest)) {
759765
return false;
760766
}
761-
if (GetDestData(dest, "used", nullptr)) {
767+
if (IsUsedDest(m_address_book, dest)) {
762768
return true;
763769
}
764770
if (IsLegacy()) {
765771
LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
766772
assert(spk_man != nullptr);
767773
for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) {
768774
WitnessV0KeyHash wpkh_dest(keyid);
769-
if (GetDestData(wpkh_dest, "used", nullptr)) {
775+
if (IsUsedDest(m_address_book, wpkh_dest)) {
770776
return true;
771777
}
772778
ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest));
773-
if (GetDestData(sh_wpkh_dest, "used", nullptr)) {
779+
if (IsUsedDest(m_address_book, sh_wpkh_dest)) {
774780
return true;
775781
}
776782
PKHash pkh_dest(keyid);
777-
if (GetDestData(pkh_dest, "used", nullptr)) {
783+
if (IsUsedDest(m_address_book, pkh_dest)) {
778784
return true;
779785
}
780786
}
@@ -3178,12 +3184,7 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
31783184
LOCK(cs_wallet);
31793185

31803186
// Delete destdata tuples associated with address
3181-
std::string strAddress = EncodeDestination(address);
3182-
for (const std::pair<const std::string, std::string> &item : m_address_book[address].destdata)
3183-
{
3184-
WalletBatch(*database).EraseDestData(strAddress, item.first);
3185-
}
3186-
m_address_book.erase(address);
3187+
WalletBatch(*database).EraseDestData(address);
31873188
}
31883189

31893190
NotifyAddressBookChanged(this, address, "", IsMine(address) != ISMINE_NO, "", CT_DELETED);
@@ -3626,56 +3627,6 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
36263627
return nTimeSmart;
36273628
}
36283629

3629-
bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key, const std::string &value)
3630-
{
3631-
if (boost::get<CNoDestination>(&dest))
3632-
return false;
3633-
3634-
m_address_book[dest].destdata.insert(std::make_pair(key, value));
3635-
return batch.WriteDestData(EncodeDestination(dest), key, value);
3636-
}
3637-
3638-
bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key)
3639-
{
3640-
if (!m_address_book[dest].destdata.erase(key))
3641-
return false;
3642-
return batch.EraseDestData(EncodeDestination(dest), key);
3643-
}
3644-
3645-
void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
3646-
{
3647-
m_address_book[dest].destdata.insert(std::make_pair(key, value));
3648-
}
3649-
3650-
bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const
3651-
{
3652-
std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dest);
3653-
if(i != m_address_book.end())
3654-
{
3655-
CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key);
3656-
if(j != i->second.destdata.end())
3657-
{
3658-
if(value)
3659-
*value = j->second;
3660-
return true;
3661-
}
3662-
}
3663-
return false;
3664-
}
3665-
3666-
std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
3667-
{
3668-
std::vector<std::string> values;
3669-
for (const auto& address : m_address_book) {
3670-
for (const auto& data : address.second.destdata) {
3671-
if (!data.first.compare(0, prefix.size(), prefix)) {
3672-
values.emplace_back(data.second);
3673-
}
3674-
}
3675-
}
3676-
return values;
3677-
}
3678-
36793630
bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, bool salvage_wallet, std::string& error_string, std::vector<std::string>& warnings)
36803631
{
36813632
// Do some checking on wallet path. It should be either a:

0 commit comments

Comments
 (0)