Skip to content

Commit 907d636

Browse files
laanwjlaanwj
committed
Merge bitcoin/bitcoin#21353: interfaces: Stop exposing wallet destdata to gui
f5ba424 wallet: Add IsAddressUsed / SetAddressUsed methods (Russell Yanofsky) 62252c9 interfaces: Stop exposing wallet destdata to gui (Russell Yanofsky) 985430d test: Add gui test for wallet receive requests (Russell Yanofsky) Pull request description: Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information. This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage. There are no changes in behavior. ACKs for top commit: jarolrod: tACK f5ba424 laanwj: Code review ACK f5ba424 Tree-SHA512: 5423df4786e537a59013cb5bfb9e1bc29a7ca4b8835360c00cc2165a59f925fdc355907a4ceb8bca0285bb4946ba235bffa7645537a951ad03fd3b4cee17b6b0
2 parents 8837f1e + f5ba424 commit 907d636

File tree

9 files changed

+80
-81
lines changed

9 files changed

+80
-81
lines changed

‎src/interfaces/wallet.h

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

115-
//! Add dest data.
116-
virtual bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) = 0;
115+
//! Get receive requests.
116+
virtual std::vector<std::string> getAddressReceiveRequests() = 0;
117117

118-
//! Erase dest data.
119-
virtual bool eraseDestData(const CTxDestination& dest, const std::string& key) = 0;
120-
121-
//! Get dest values with prefix.
122-
virtual std::vector<std::string> getDestValues(const std::string& prefix) = 0;
118+
//! Save or remove receive request.
119+
virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0;
123120

124121
//! Lock coin.
125122
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,7 +10,10 @@
1010
#include <qt/walletmodel.h>
1111

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

1518
#include <utility>
1619

@@ -21,10 +24,9 @@ RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
2124
QAbstractTableModel(parent), walletModel(parent)
2225
{
2326
// Load entries from wallet
24-
std::vector<std::string> vReceiveRequests;
25-
parent->loadReceiveRequests(vReceiveRequests);
26-
for (const std::string& request : vReceiveRequests)
27+
for (const std::string& request : parent->wallet().getAddressReceiveRequests()) {
2728
addNewRequest(request);
29+
}
2830

2931
/* These columns must match the indices in the ColumnIndex enumeration */
3032
columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();
@@ -150,7 +152,7 @@ bool RecentRequestsTableModel::removeRows(int row, int count, const QModelIndex
150152
for (int i = 0; i < count; ++i)
151153
{
152154
const RecentRequestEntry* rec = &list[row+i];
153-
if (!walletModel->saveReceiveRequest(rec->recipient.address.toStdString(), rec->id, ""))
155+
if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(rec->recipient.address.toStdString()), ToString(rec->id), ""))
154156
return false;
155157
}
156158

@@ -179,7 +181,7 @@ void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient
179181
CDataStream ss(SER_DISK, CLIENT_VERSION);
180182
ss << newEntry;
181183

182-
if (!walletModel->saveReceiveRequest(recipient.address.toStdString(), newEntry.id, ss.str()))
184+
if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(recipient.address.toStdString()), ToString(newEntry.id), ss.str()))
183185
return;
184186

185187
addNewRequest(newEntry);

‎src/qt/test/wallettests.cpp

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,7 @@ void TestGUI(interfaces::Node& node)
224224
int initialRowCount = requestTableModel->rowCount({});
225225
QPushButton* requestPaymentButton = receiveCoinsDialog.findChild<QPushButton*>("receiveButton");
226226
requestPaymentButton->click();
227+
QString address;
227228
for (QWidget* widget : QApplication::topLevelWidgets()) {
228229
if (widget->inherits("ReceiveRequestDialog")) {
229230
ReceiveRequestDialog* receiveRequestDialog = qobject_cast<ReceiveRequestDialog*>(widget);
@@ -232,6 +233,9 @@ void TestGUI(interfaces::Node& node)
232233
QString uri = receiveRequestDialog->QObject::findChild<QLabel*>("uri_content")->text();
233234
QCOMPARE(uri.count("bitcoin:"), 2);
234235
QCOMPARE(receiveRequestDialog->QObject::findChild<QLabel*>("address_tag")->text(), QString("Address:"));
236+
QVERIFY(address.isEmpty());
237+
address = receiveRequestDialog->QObject::findChild<QLabel*>("address_content")->text();
238+
QVERIFY(!address.isEmpty());
235239

236240
QCOMPARE(uri.count("amount=0.00000001"), 2);
237241
QCOMPARE(receiveRequestDialog->QObject::findChild<QLabel*>("amount_tag")->text(), QString("Amount:"));
@@ -258,12 +262,30 @@ void TestGUI(interfaces::Node& node)
258262
int currentRowCount = requestTableModel->rowCount({});
259263
QCOMPARE(currentRowCount, initialRowCount+1);
260264

265+
// Check addition to wallet
266+
std::vector<std::string> requests = walletModel.wallet().getAddressReceiveRequests();
267+
QCOMPARE(requests.size(), size_t{1});
268+
RecentRequestEntry entry;
269+
CDataStream{MakeUCharSpan(requests[0]), SER_DISK, CLIENT_VERSION} >> entry;
270+
QCOMPARE(entry.nVersion, int{1});
271+
QCOMPARE(entry.id, int64_t{1});
272+
QVERIFY(entry.date.isValid());
273+
QCOMPARE(entry.recipient.address, address);
274+
QCOMPARE(entry.recipient.label, QString{"TEST_LABEL_1"});
275+
QCOMPARE(entry.recipient.amount, CAmount{1});
276+
QCOMPARE(entry.recipient.message, QString{"TEST_MESSAGE_1"});
277+
QCOMPARE(entry.recipient.sPaymentRequest, std::string{});
278+
QCOMPARE(entry.recipient.authenticatedMerchant, QString{});
279+
261280
// Check Remove button
262281
QTableView* table = receiveCoinsDialog.findChild<QTableView*>("recentRequestsView");
263282
table->selectRow(currentRowCount-1);
264283
QPushButton* removeRequestButton = receiveCoinsDialog.findChild<QPushButton*>("removeRequestButton");
265284
removeRequestButton->click();
266285
QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1);
286+
287+
// Check removal from wallet
288+
QCOMPARE(walletModel.wallet().getAddressReceiveRequests().size(), size_t{0});
267289
}
268290

269291
} // namespace

‎src/qt/walletmodel.cpp

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

469-
void WalletModel::loadReceiveRequests(std::vector<std::string>& vReceiveRequests)
470-
{
471-
vReceiveRequests = m_wallet->getDestValues("rr"); // receive request
472-
}
473-
474-
bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest)
475-
{
476-
CTxDestination dest = DecodeDestination(sAddress);
477-
478-
std::stringstream ss;
479-
ss << nId;
480-
std::string key = "rr" + ss.str(); // "rr" prefix = "receive request" in destdata
481-
482-
if (sRequest.empty())
483-
return m_wallet->eraseDestData(dest, key);
484-
else
485-
return m_wallet->addDestData(dest, key, sRequest);
486-
}
487-
488469
bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
489470
{
490471
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/interfaces.cpp

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -197,22 +197,14 @@ class WalletImpl : public Wallet
197197
}
198198
return result;
199199
}
200-
bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override
201-
{
200+
std::vector<std::string> getAddressReceiveRequests() override {
202201
LOCK(m_wallet->cs_wallet);
203-
WalletBatch batch{m_wallet->GetDatabase()};
204-
return m_wallet->AddDestData(batch, dest, key, value);
202+
return m_wallet->GetAddressReceiveRequests();
205203
}
206-
bool eraseDestData(const CTxDestination& dest, const std::string& key) override
207-
{
204+
bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) override {
208205
LOCK(m_wallet->cs_wallet);
209206
WalletBatch batch{m_wallet->GetDatabase()};
210-
return m_wallet->EraseDestData(batch, dest, key);
211-
}
212-
std::vector<std::string> getDestValues(const std::string& prefix) override
213-
{
214-
LOCK(m_wallet->cs_wallet);
215-
return m_wallet->GetDestValues(prefix);
207+
return m_wallet->SetAddressReceiveRequest(batch, dest, id, value);
216208
}
217209
void lockCoin(const COutPoint& output) override
218210
{

‎src/wallet/test/wallet_tests.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -385,11 +385,11 @@ BOOST_AUTO_TEST_CASE(LoadReceiveRequests)
385385
CTxDestination dest = PKHash();
386386
LOCK(m_wallet.cs_wallet);
387387
WalletBatch batch{m_wallet.GetDatabase()};
388-
m_wallet.AddDestData(batch, dest, "misc", "val_misc");
389-
m_wallet.AddDestData(batch, dest, "rr0", "val_rr0");
390-
m_wallet.AddDestData(batch, dest, "rr1", "val_rr1");
388+
m_wallet.SetAddressUsed(batch, dest, true);
389+
m_wallet.SetAddressReceiveRequest(batch, dest, "0", "val_rr0");
390+
m_wallet.SetAddressReceiveRequest(batch, dest, "1", "val_rr1");
391391

392-
auto values = m_wallet.GetDestValues("rr");
392+
auto values = m_wallet.GetAddressReceiveRequests();
393393
BOOST_CHECK_EQUAL(values.size(), 2U);
394394
BOOST_CHECK_EQUAL(values[0], "val_rr0");
395395
BOOST_CHECK_EQUAL(values[1], "val_rr1");

‎src/wallet/wallet.cpp

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -814,12 +814,11 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned
814814
CTxDestination dst;
815815
if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) {
816816
if (IsMine(dst)) {
817-
if (used && !GetDestData(dst, "used", nullptr)) {
818-
if (AddDestData(batch, dst, "used", "p")) { // p for "present", opposite of absent (null)
817+
if (used != IsAddressUsed(dst)) {
818+
if (used) {
819819
tx_destinations.insert(dst);
820820
}
821-
} else if (!used && GetDestData(dst, "used", nullptr)) {
822-
EraseDestData(batch, dst, "used");
821+
SetAddressUsed(batch, dst, used);
823822
}
824823
}
825824
}
@@ -835,23 +834,23 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
835834
if (!ExtractDestination(srctx->tx->vout[n].scriptPubKey, dest)) {
836835
return false;
837836
}
838-
if (GetDestData(dest, "used", nullptr)) {
837+
if (IsAddressUsed(dest)) {
839838
return true;
840839
}
841840
if (IsLegacy()) {
842841
LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan();
843842
assert(spk_man != nullptr);
844843
for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) {
845844
WitnessV0KeyHash wpkh_dest(keyid);
846-
if (GetDestData(wpkh_dest, "used", nullptr)) {
845+
if (IsAddressUsed(wpkh_dest)) {
847846
return true;
848847
}
849848
ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest));
850-
if (GetDestData(sh_wpkh_dest, "used", nullptr)) {
849+
if (IsAddressUsed(sh_wpkh_dest)) {
851850
return true;
852851
}
853852
PKHash pkh_dest(keyid);
854-
if (GetDestData(pkh_dest, "used", nullptr)) {
853+
if (IsAddressUsed(pkh_dest)) {
855854
return true;
856855
}
857856
}
@@ -2387,45 +2386,45 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
23872386
return nTimeSmart;
23882387
}
23892388

2390-
bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key, const std::string &value)
2389+
bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used)
23912390
{
2391+
const std::string key{"used"};
23922392
if (std::get_if<CNoDestination>(&dest))
23932393
return false;
23942394

2395+
if (!used) {
2396+
if (auto* data = util::FindKey(m_address_book, dest)) data->destdata.erase(key);
2397+
return batch.EraseDestData(EncodeDestination(dest), key);
2398+
}
2399+
2400+
const std::string value{"1"};
23952401
m_address_book[dest].destdata.insert(std::make_pair(key, value));
23962402
return batch.WriteDestData(EncodeDestination(dest), key, value);
23972403
}
23982404

2399-
bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key)
2400-
{
2401-
if (!m_address_book[dest].destdata.erase(key))
2402-
return false;
2403-
return batch.EraseDestData(EncodeDestination(dest), key);
2404-
}
2405-
24062405
void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
24072406
{
24082407
m_address_book[dest].destdata.insert(std::make_pair(key, value));
24092408
}
24102409

2411-
bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const
2410+
bool CWallet::IsAddressUsed(const CTxDestination& dest) const
24122411
{
2412+
const std::string key{"used"};
24132413
std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dest);
24142414
if(i != m_address_book.end())
24152415
{
24162416
CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key);
24172417
if(j != i->second.destdata.end())
24182418
{
2419-
if(value)
2420-
*value = j->second;
24212419
return true;
24222420
}
24232421
}
24242422
return false;
24252423
}
24262424

2427-
std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
2425+
std::vector<std::string> CWallet::GetAddressReceiveRequests() const
24282426
{
2427+
const std::string prefix{"rr"};
24292428
std::vector<std::string> values;
24302429
for (const auto& address : m_address_book) {
24312430
for (const auto& data : address.second.destdata) {
@@ -2437,6 +2436,20 @@ std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
24372436
return values;
24382437
}
24392438

2439+
bool CWallet::SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value)
2440+
{
2441+
const std::string key{"rr" + id}; // "rr" prefix = "receive request" in destdata
2442+
CAddressBookData& data = m_address_book.at(dest);
2443+
if (value.empty()) {
2444+
if (!batch.EraseDestData(EncodeDestination(dest), key)) return false;
2445+
data.destdata.erase(key);
2446+
} else {
2447+
if (!batch.WriteDestData(EncodeDestination(dest), key, value)) return false;
2448+
data.destdata[key] = value;
2449+
}
2450+
return true;
2451+
}
2452+
24402453
std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string)
24412454
{
24422455
// Do some checking on wallet path. It should be either a:

‎src/wallet/wallet.h

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -479,19 +479,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
479479

480480
bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; return true; }
481481

482-
/**
483-
* Adds a destination data tuple to the store, and saves it to disk
484-
* When adding new fields, take care to consider how DelAddressBook should handle it!
485-
*/
486-
bool AddDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
487-
//! Erases a destination data tuple in the store and on disk
488-
bool EraseDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
489482
//! Adds a destination data tuple to the store, without saving it to disk
490483
void LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
491-
//! Look up a destination data tuple in the store, return true if found false otherwise
492-
bool GetDestData(const CTxDestination& dest, const std::string& key, std::string* value) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
493-
//! Get all destination values matching a prefix.
494-
std::vector<std::string> GetDestValues(const std::string& prefix) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
495484

496485
//! Holds a timestamp at which point the wallet is scheduled (externally) to be relocked. Caller must arrange for actual relocking to occur via Lock().
497486
int64_t nRelockTime GUARDED_BY(cs_wallet){0};
@@ -705,6 +694,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
705694

706695
bool DelAddressBook(const CTxDestination& address);
707696

697+
bool IsAddressUsed(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
698+
bool SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
699+
700+
std::vector<std::string> GetAddressReceiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
701+
bool SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
702+
708703
unsigned int GetKeyPoolSize() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
709704

710705
//! signify that a particular wallet feature is now used.

0 commit comments

Comments
 (0)