From 985430d9b2e183c1f59a34472e413a8d00a7e6da Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 2 Mar 2021 07:47:57 -0500 Subject: test: Add gui test for wallet receive requests Make sure wallet receive requests are saved and deleted correctly by GUI code Co-authored-by: Jarol Rodriguez --- src/qt/test/wallettests.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'src') diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index d6d2d0e3df..0ef1fb318a 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -225,6 +225,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); @@ -233,6 +234,9 @@ void TestGUI(interfaces::Node& node) QString uri = receiveRequestDialog->QObject::findChild("uri_content")->text(); QCOMPARE(uri.count("bitcoin:"), 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:")); @@ -259,12 +263,30 @@ void TestGUI(interfaces::Node& node) int currentRowCount = requestTableModel->rowCount({}); QCOMPARE(currentRowCount, initialRowCount+1); + // Check addition to wallet + std::vector requests = walletModel.wallet().getDestValues("rr"); + 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); QPushButton* removeRequestButton = receiveCoinsDialog.findChild("removeRequestButton"); removeRequestButton->click(); QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1); + + // Check removal from wallet + QCOMPARE(walletModel.wallet().getDestValues("rr").size(), size_t{0}); } } // namespace -- cgit v1.2.3 From 62252c95e5aa55f33a5ef22292d5d8161fcb892a Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Sun, 12 Apr 2020 13:40:43 -0400 Subject: interfaces: Stop exposing wallet destdata to gui 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. Note: No user-visible behavior is changing in this commit. New CWallet::SetAddressReceiveRequest() implementation avoids a bug in CWallet::AddDestData() where a modification would leave the previous value in memory while writing the new value to disk. But it doesn't matter because the GUI doesn't currently expose the ability to modify receive requests, only to add and erase them. --- src/interfaces/wallet.h | 11 ++++------- src/qt/recentrequeststablemodel.cpp | 12 +++++++----- src/qt/test/wallettests.cpp | 4 ++-- src/qt/walletmodel.cpp | 19 ------------------- src/qt/walletmodel.h | 3 --- src/wallet/interfaces.cpp | 16 ++++------------ src/wallet/test/wallet_tests.cpp | 6 +++--- src/wallet/wallet.cpp | 17 ++++++++++++++++- src/wallet/wallet.h | 5 +++-- 9 files changed, 39 insertions(+), 54 deletions(-) (limited to 'src') diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 6ccfd7fc20..88f93321f9 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -112,14 +112,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 03531a1381..c47b34a2b0 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 0ef1fb318a..708c3cc92b 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -264,7 +264,7 @@ void TestGUI(interfaces::Node& node) QCOMPARE(currentRowCount, initialRowCount+1); // Check addition to wallet - std::vector requests = walletModel.wallet().getDestValues("rr"); + std::vector requests = walletModel.wallet().getAddressReceiveRequests(); QCOMPARE(requests.size(), size_t{1}); RecentRequestEntry entry; CDataStream{MakeUCharSpan(requests[0]), SER_DISK, CLIENT_VERSION} >> entry; @@ -286,7 +286,7 @@ void TestGUI(interfaces::Node& node) QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1); // Check removal from wallet - QCOMPARE(walletModel.wallet().getDestValues("rr").size(), size_t{0}); + QCOMPARE(walletModel.wallet().getAddressReceiveRequests().size(), size_t{0}); } } // namespace diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 02254da3ce..381d1ce654 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -463,25 +463,6 @@ void WalletModel::UnlockContext::CopyFrom(UnlockContext&& rhs) rhs.relock = 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::bumpFee(uint256 hash, uint256& new_hash) { CCoinControl coin_control; diff --git a/src/qt/walletmodel.h b/src/qt/walletmodel.h index 9a3c3f2f66..72a1671146 100644 --- a/src/qt/walletmodel.h +++ b/src/qt/walletmodel.h @@ -135,9 +135,6 @@ public: UnlockContext requestUnlock(); - void loadReceiveRequests(std::vector& vReceiveRequests); - bool saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest); - bool bumpFee(uint256 hash, uint256& new_hash); static bool isWalletEnabled(); diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 1fb789b128..9a4b077b5a 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -199,22 +199,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); - WalletBatch batch{m_wallet->GetDatabase()}; - return m_wallet->AddDestData(batch, dest, key, value); + return m_wallet->GetAddressReceiveRequests(); } - bool eraseDestData(const CTxDestination& dest, const std::string& key) override - { + 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->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 5480f3ab22..932a0c2eca 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -391,10 +391,10 @@ BOOST_AUTO_TEST_CASE(LoadReceiveRequests) 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.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 08e480225d..56c1ea7fa6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3798,8 +3798,9 @@ bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, st 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) { @@ -3811,6 +3812,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 eb797938cd..58538df0af 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -876,8 +876,6 @@ public: 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}; @@ -1084,6 +1082,9 @@ public: bool DelAddressBook(const CTxDestination& address); + 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. -- cgit v1.2.3 From f5ba424cd44619d9b9be88b8593d69a7ba96db26 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Sun, 12 Apr 2020 13:40:43 -0400 Subject: wallet: Add IsAddressUsed / SetAddressUsed methods This simplifies code and adds a less cumbersome interface for accessing address used information than CWallet AddDestData / EraseDestData / GetDestData methods. There is no change in behavior. Lower-level walletdb DestData methods are also still available and not affected by this change. If there is interest in consolidating destdata logic more and making it internal to walletdb, #18608 could be considered as a followup. --- src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 36 +++++++++++++++++------------------- src/wallet/wallet.h | 12 +++--------- 3 files changed, 21 insertions(+), 29 deletions(-) (limited to 'src') diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 932a0c2eca..471ab67b32 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -390,7 +390,7 @@ 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.SetAddressUsed(batch, dest, true); m_wallet.SetAddressReceiveRequest(batch, dest, "0", "val_rr0"); m_wallet.SetAddressReceiveRequest(batch, dest, "1", "val_rr1"); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 56c1ea7fa6..4732057708 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -813,12 +813,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); } } } @@ -834,7 +833,7 @@ 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()) { @@ -842,15 +841,15 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const assert(spk_man != nullptr); for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) { WitnessV0KeyHash wpkh_dest(keyid); - if (GetDestData(wpkh_dest, "used", nullptr)) { + if (IsAddressUsed(wpkh_dest)) { return true; } ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest)); - if (GetDestData(sh_wpkh_dest, "used", nullptr)) { + if (IsAddressUsed(sh_wpkh_dest)) { return true; } PKHash pkh_dest(keyid); - if (GetDestData(pkh_dest, "used", nullptr)) { + if (IsAddressUsed(pkh_dest)) { return true; } } @@ -3761,37 +3760,36 @@ 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; } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 58538df0af..798993fa84 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -865,17 +865,8 @@ public: bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = 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); //! 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}; @@ -1082,6 +1073,9 @@ 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); -- cgit v1.2.3