From 26ff28a0289577a13374c07152f8e92bed541f54 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Thu, 3 Jun 2021 15:56:43 +0200 Subject: [PATCH] Merge bitcoin/bitcoin#21353: interfaces: Stop exposing wallet destdata to gui f5ba424cd44619d9b9be88b8593d69a7ba96db26 wallet: Add IsAddressUsed / SetAddressUsed methods (Russell Yanofsky) 62252c95e5aa55f33a5ef22292d5d8161fcb892a interfaces: Stop exposing wallet destdata to gui (Russell Yanofsky) 985430d9b2e183c1f59a34472e413a8d00a7e6da 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 f5ba424cd44619d9b9be88b8593d69a7ba96db26 laanwj: Code review ACK f5ba424cd44619d9b9be88b8593d69a7ba96db26 Tree-SHA512: 5423df4786e537a59013cb5bfb9e1bc29a7ca4b8835360c00cc2165a59f925fdc355907a4ceb8bca0285bb4946ba235bffa7645537a951ad03fd3b4cee17b6b0 --- src/interfaces/wallet.h | 11 +++---- src/qt/recentrequeststablemodel.cpp | 12 ++++--- src/qt/test/wallettests.cpp | 22 +++++++++++++ src/qt/walletmodel.cpp | 19 ----------- src/qt/walletmodel.h | 3 -- src/wallet/interfaces.cpp | 20 ++++-------- src/wallet/test/wallet_tests.cpp | 8 ++--- src/wallet/wallet.cpp | 49 ++++++++++++++++++----------- src/wallet/wallet.h | 17 ++++------ 9 files changed, 80 insertions(+), 81 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 085bb376e6..b798012420 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -128,14 +128,11 @@ public: //! Get wallet address list. virtual std::vector getAddresses() = 0; - //! Add dest data. - virtual bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) = 0; + //! Get receive requests. + virtual std::vector getAddressReceiveRequests() = 0; - //! Erase dest data. - virtual bool eraseDestData(const CTxDestination& dest, const std::string& key) = 0; - - //! Get dest values with prefix. - virtual std::vector getDestValues(const std::string& prefix) = 0; + //! Save or remove receive request. + virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0; //! Lock coin. virtual void lockCoin(const COutPoint& output) = 0; diff --git a/src/qt/recentrequeststablemodel.cpp b/src/qt/recentrequeststablemodel.cpp index 1adcca3413..5c9623f9ff 100644 --- a/src/qt/recentrequeststablemodel.cpp +++ b/src/qt/recentrequeststablemodel.cpp @@ -10,7 +10,10 @@ #include #include +#include +#include #include +#include #include @@ -18,10 +21,9 @@ RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) : QAbstractTableModel(parent), walletModel(parent) { // Load entries from wallet - std::vector vReceiveRequests; - parent->loadReceiveRequests(vReceiveRequests); - for (const std::string& request : vReceiveRequests) + for (const std::string& request : parent->wallet().getAddressReceiveRequests()) { addNewRequest(request); + } /* These columns must match the indices in the ColumnIndex enumeration */ columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle(); @@ -143,7 +145,7 @@ bool RecentRequestsTableModel::removeRows(int row, int count, const QModelIndex for (int i = 0; i < count; ++i) { const RecentRequestEntry* rec = &list[row+i]; - if (!walletModel->saveReceiveRequest(rec->recipient.address.toStdString(), rec->id, "")) + if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(rec->recipient.address.toStdString()), ToString(rec->id), "")) return false; } @@ -172,7 +174,7 @@ void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient CDataStream ss(SER_DISK, CLIENT_VERSION); ss << newEntry; - if (!walletModel->saveReceiveRequest(recipient.address.toStdString(), newEntry.id, ss.str())) + if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(recipient.address.toStdString()), ToString(newEntry.id), ss.str())) return; addNewRequest(newEntry); diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 1da85c9803..62896ceaa2 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -187,6 +187,7 @@ void TestGUI(interfaces::Node& node) int initialRowCount = requestTableModel->rowCount({}); QPushButton* requestPaymentButton = receiveCoinsDialog.findChild("receiveButton"); requestPaymentButton->click(); + QString address; for (QWidget* widget : QApplication::topLevelWidgets()) { if (widget->inherits("ReceiveRequestDialog")) { ReceiveRequestDialog* receiveRequestDialog = qobject_cast(widget); @@ -195,6 +196,9 @@ void TestGUI(interfaces::Node& node) QString uri = receiveRequestDialog->QObject::findChild("uri_content")->text(); QCOMPARE(uri.count("dash:"), 2); QCOMPARE(receiveRequestDialog->QObject::findChild("address_tag")->text(), QString("Address:")); + QVERIFY(address.isEmpty()); + address = receiveRequestDialog->QObject::findChild("address_content")->text(); + QVERIFY(!address.isEmpty()); QCOMPARE(uri.count("amount=0.00000001"), 2); QCOMPARE(receiveRequestDialog->QObject::findChild("amount_tag")->text(), QString("Amount:")); @@ -221,6 +225,21 @@ void TestGUI(interfaces::Node& node) int currentRowCount = requestTableModel->rowCount({}); QCOMPARE(currentRowCount, initialRowCount+1); + // Check addition to wallet + std::vector requests = walletModel.wallet().getAddressReceiveRequests(); + QCOMPARE(requests.size(), size_t{1}); + RecentRequestEntry entry; + CDataStream{MakeUCharSpan(requests[0]), SER_DISK, CLIENT_VERSION} >> entry; + QCOMPARE(entry.nVersion, int{1}); + QCOMPARE(entry.id, int64_t{1}); + QVERIFY(entry.date.isValid()); + QCOMPARE(entry.recipient.address, address); + QCOMPARE(entry.recipient.label, QString{"TEST_LABEL_1"}); + QCOMPARE(entry.recipient.amount, CAmount{1}); + QCOMPARE(entry.recipient.message, QString{"TEST_MESSAGE_1"}); + QCOMPARE(entry.recipient.sPaymentRequest, std::string{}); + QCOMPARE(entry.recipient.authenticatedMerchant, QString{}); + // Check Remove button QTableView* table = receiveCoinsDialog.findChild("recentRequestsView"); table->selectRow(currentRowCount-1); @@ -228,6 +247,9 @@ void TestGUI(interfaces::Node& node) removeRequestButton->click(); QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1); RemoveWallet(wallet, std::nullopt); + + // Check removal from wallet + QCOMPARE(walletModel.wallet().getAddressReceiveRequests().size(), size_t{0}); } } // namespace diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 2b33db9a66..f0ed7ef0fe 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -571,25 +571,6 @@ void WalletModel::UnlockContext::CopyFrom(UnlockContext&& rhs) rhs.was_mixing = false; } -void WalletModel::loadReceiveRequests(std::vector& vReceiveRequests) -{ - vReceiveRequests = m_wallet->getDestValues("rr"); // receive request -} - -bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest) -{ - CTxDestination dest = DecodeDestination(sAddress); - - std::stringstream ss; - ss << nId; - std::string key = "rr" + ss.str(); // "rr" prefix = "receive request" in destdata - - if (sRequest.empty()) - return m_wallet->eraseDestData(dest, key); - else - return m_wallet->addDestData(dest, key, sRequest); -} - bool WalletModel::isWalletEnabled() { return !gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET); diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 8194c141a8..d15f65af91 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -140,9 +140,6 @@ public: UnlockContext requestUnlock(bool fForMixingOnly=false); - void loadReceiveRequests(std::vector& vReceiveRequests); - bool saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest); - static bool isWalletEnabled(); int getNumISLocks() const; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index c5cf7f2e8c..c98238096b 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -227,22 +227,14 @@ public: } return result; } - bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override - { + std::vector getAddressReceiveRequests() override { + LOCK(m_wallet->cs_wallet); + return m_wallet->GetAddressReceiveRequests(); + } + bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) override { LOCK(m_wallet->cs_wallet); WalletBatch batch{m_wallet->GetDatabase()}; - return m_wallet->AddDestData(batch, dest, key, value); - } - bool eraseDestData(const CTxDestination& dest, const std::string& key) override - { - LOCK(m_wallet->cs_wallet); - WalletBatch batch{m_wallet->GetDatabase()}; - return m_wallet->EraseDestData(batch, dest, key); - } - std::vector getDestValues(const std::string& prefix) override - { - LOCK(m_wallet->cs_wallet); - return m_wallet->GetDestValues(prefix); + return m_wallet->SetAddressReceiveRequest(batch, dest, id, value); } void lockCoin(const COutPoint& output) override { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index c470c33428..86f28641d8 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -478,11 +478,11 @@ BOOST_AUTO_TEST_CASE(LoadReceiveRequests) CTxDestination dest = PKHash(); LOCK(m_wallet.cs_wallet); WalletBatch batch{m_wallet.GetDatabase()}; - m_wallet.AddDestData(batch, dest, "misc", "val_misc"); - m_wallet.AddDestData(batch, dest, "rr0", "val_rr0"); - m_wallet.AddDestData(batch, dest, "rr1", "val_rr1"); + m_wallet.SetAddressUsed(batch, dest, true); + m_wallet.SetAddressReceiveRequest(batch, dest, "0", "val_rr0"); + m_wallet.SetAddressReceiveRequest(batch, dest, "1", "val_rr1"); - auto values = m_wallet.GetDestValues("rr"); + auto values = m_wallet.GetAddressReceiveRequests(); BOOST_CHECK_EQUAL(values.size(), 2U); BOOST_CHECK_EQUAL(values[0], "val_rr0"); BOOST_CHECK_EQUAL(values[1], "val_rr1"); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index efbe2fddbf..0638dadd5c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -803,12 +803,11 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned CTxDestination dst; if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) { if (IsMine(dst)) { - if (used && !GetDestData(dst, "used", nullptr)) { - if (AddDestData(batch, dst, "used", "p")) { // p for "present", opposite of absent (null) + if (used != IsAddressUsed(dst)) { + if (used) { tx_destinations.insert(dst); } - } else if (!used && GetDestData(dst, "used", nullptr)) { - EraseDestData(batch, dst, "used"); + SetAddressUsed(batch, dst, used); } } } @@ -824,14 +823,14 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const if (!ExtractDestination(srctx->tx->vout[n].scriptPubKey, dest)) { return false; } - if (GetDestData(dest, "used", nullptr)) { + if (IsAddressUsed(dest)) { return true; } if (IsLegacy()) { LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); assert(spk_man != nullptr); for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) { - if (GetDestData(PKHash(keyid), "used", nullptr)) { + if (IsAddressUsed(PKHash(keyid))) { return true; } } @@ -4441,45 +4440,45 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const return nTimeSmart; } -bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key, const std::string &value) +bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used) { + const std::string key{"used"}; if (std::get_if(&dest)) return false; + if (!used) { + if (auto* data = util::FindKey(m_address_book, dest)) data->destdata.erase(key); + return batch.EraseDestData(EncodeDestination(dest), key); + } + + const std::string value{"1"}; m_address_book[dest].destdata.insert(std::make_pair(key, value)); return batch.WriteDestData(EncodeDestination(dest), key, value); } -bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key) -{ - if (!m_address_book[dest].destdata.erase(key)) - return false; - return batch.EraseDestData(EncodeDestination(dest), key); -} - void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value) { m_address_book[dest].destdata.insert(std::make_pair(key, value)); } -bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const +bool CWallet::IsAddressUsed(const CTxDestination& dest) const { + const std::string key{"used"}; std::map::const_iterator i = m_address_book.find(dest); if(i != m_address_book.end()) { CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key); if(j != i->second.destdata.end()) { - if(value) - *value = j->second; return true; } } return false; } -std::vector CWallet::GetDestValues(const std::string& prefix) const +std::vector CWallet::GetAddressReceiveRequests() const { + const std::string prefix{"rr"}; std::vector values; for (const auto& address : m_address_book) { for (const auto& data : address.second.destdata) { @@ -4491,6 +4490,20 @@ std::vector CWallet::GetDestValues(const std::string& prefix) const return values; } +bool CWallet::SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) +{ + const std::string key{"rr" + id}; // "rr" prefix = "receive request" in destdata + CAddressBookData& data = m_address_book.at(dest); + if (value.empty()) { + if (!batch.EraseDestData(EncodeDestination(dest), key)) return false; + data.destdata.erase(key); + } else { + if (!batch.WriteDestData(EncodeDestination(dest), key, value)) return false; + data.destdata[key] = value; + } + return true; +} + std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string) { // Do some checking on wallet path. It should be either a: diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 8199dd6fc2..a0c5618ac5 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -983,19 +983,8 @@ public: bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; nWalletMaxVersion = std::max(nWalletMaxVersion, nVersion); return true; } - /** - * Adds a destination data tuple to the store, and saves it to disk - * When adding new fields, take care to consider how DelAddressBook should handle it! - */ - bool AddDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - //! Erases a destination data tuple in the store and on disk - bool EraseDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Adds a destination data tuple to the store, without saving it to disk void LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - //! Look up a destination data tuple in the store, return true if found false otherwise - bool GetDestData(const CTxDestination& dest, const std::string& key, std::string* value) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - //! Get all destination values matching a prefix. - std::vector GetDestValues(const std::string& prefix) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Holds a timestamp at which point the wallet is scheduled (externally) to be relocked. Caller must arrange for actual relocking to occur via Lock(). int64_t nRelockTime GUARDED_BY(cs_wallet){0}; @@ -1201,6 +1190,12 @@ public: bool DelAddressBook(const CTxDestination& address); + bool IsAddressUsed(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + std::vector GetAddressReceiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + unsigned int GetKeyPoolSize() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! signify that a particular wallet feature is now used. this may change nWalletVersion and nWalletMaxVersion if those are lower