From 192eb1e61c3c43baec7f32c498ab0ce0656a58f7 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 10:45:08 -0300 Subject: refactor: getAddress don't access m_address_book, use FindAddressEntry function --- src/wallet/interfaces.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index e1203817e0..5cfcf16e16 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -191,18 +191,16 @@ public: std::string* purpose) override { LOCK(m_wallet->cs_wallet); - auto it = m_wallet->m_address_book.find(dest); - if (it == m_wallet->m_address_book.end() || it->second.IsChange()) { - return false; - } + const auto& entry = m_wallet->FindAddressBookEntry(dest, /*allow_change=*/false); + if (!entry) return false; // addr not found if (name) { - *name = it->second.GetLabel(); + *name = entry->GetLabel(); } if (is_mine) { *is_mine = m_wallet->IsMine(dest); } if (purpose) { - *purpose = it->second.purpose; + *purpose = entry->purpose; } return true; } -- cgit v1.2.3 From 09649bc95d5f2855a54a8cf02e65215a3b333c92 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 11:06:22 -0300 Subject: refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup --- src/wallet/rpc/coins.cpp | 6 +++--- src/wallet/wallet.cpp | 15 +++++++-------- src/wallet/wallet.h | 13 ++++++++++++- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 2649fa586c..6050ad7b4b 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -18,10 +18,10 @@ namespace wallet { static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { - std::set addresses; + std::vector addresses; if (by_label) { // Get the set of addresses assigned to label - addresses = wallet.GetLabelAddresses(LabelFromValue(params[0])); + addresses = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{LabelFromValue(params[0])}); if (addresses.empty()) throw JSONRPCError(RPC_WALLET_ERROR, "Label not found in wallet"); } else { // Get the address @@ -29,7 +29,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b if (!IsValidDestination(dest)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address"); } - addresses.insert(dest); + addresses.emplace_back(dest); } // Filter by own scripts only diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 910562e669..8ca8ef0a19 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2348,17 +2348,16 @@ void CWallet::MarkDestinationsDirty(const std::set& destinations } } -std::set CWallet::GetLabelAddresses(const std::string& label) const +std::vector CWallet::ListAddrBookAddresses(const std::optional& _filter) const { AssertLockHeld(cs_wallet); - std::set result; - for (const std::pair& item : m_address_book) - { - if (item.second.IsChange()) continue; - const CTxDestination& address = item.first; + std::vector result; + AddrBookFilter filter = _filter ? *_filter : AddrBookFilter(); + for (const std::pair& item : m_address_book) { + if (filter.ignore_change && item.second.IsChange()) continue; const std::string& strName = item.second.GetLabel(); - if (strName == label) - result.insert(address); + if (filter.m_op_label && *filter.m_op_label != strName) continue; + result.emplace_back(item.first); } return result; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7da601c3b7..970d3e2e75 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -635,7 +635,18 @@ public: std::optional GetOldestKeyPoolTime() const; - std::set GetLabelAddresses(const std::string& label) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + // Filter struct for 'ListAddrBookAddresses' + struct AddrBookFilter { + // Fetch addresses with the provided label + std::optional m_op_label{std::nullopt}; + // Don't include change addresses by default + bool ignore_change{true}; + }; + + /** + * Filter and retrieve destinations stored in the addressbook + */ + std::vector ListAddrBookAddresses(const std::optional& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Marks all outputs in each one of the destinations dirty, so their cache is -- cgit v1.2.3 From 032842ae4196aaed5ea3567ea01a61ed75ab2edd Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 11:35:14 -0300 Subject: wallet: implement ForEachAddrBookEntry method --- src/wallet/wallet.cpp | 23 +++++++++++++++++------ src/wallet/wallet.h | 7 +++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8ca8ef0a19..f7eb0bbc03 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2348,17 +2348,28 @@ void CWallet::MarkDestinationsDirty(const std::set& destinations } } +void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const +{ + AssertLockHeld(cs_wallet); + for (const std::pair& item : m_address_book) { + const auto& entry = item.second; + func(item.first, entry.GetLabel(), entry.purpose, entry.IsChange()); + } +} + std::vector CWallet::ListAddrBookAddresses(const std::optional& _filter) const { AssertLockHeld(cs_wallet); std::vector result; AddrBookFilter filter = _filter ? *_filter : AddrBookFilter(); - for (const std::pair& item : m_address_book) { - if (filter.ignore_change && item.second.IsChange()) continue; - const std::string& strName = item.second.GetLabel(); - if (filter.m_op_label && *filter.m_op_label != strName) continue; - result.emplace_back(item.first); - } + ForEachAddrBookEntry([&result, &filter](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) { + // Filter by change + if (filter.ignore_change && is_change) return; + // Filter by label + if (filter.m_op_label && *filter.m_op_label != label) return; + // All good + result.emplace_back(dest); + }); return result; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 970d3e2e75..3775f325ba 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -648,6 +648,13 @@ public: */ std::vector ListAddrBookAddresses(const std::optional& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** + * Walk-through the address book entries. + * Stops when the provided 'ListAddrBookFunc' returns false. + */ + using ListAddrBookFunc = std::function; + void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** * Marks all outputs in each one of the destinations dirty, so their cache is * reset and does not return outdated information. -- cgit v1.2.3 From 2b48642499016cb357e4bcec32481cd50361194e Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 11:38:19 -0300 Subject: refactor: use ForEachAddrBookEntry in interfaces::getAddresses --- src/interfaces/wallet.h | 2 +- src/wallet/interfaces.cpp | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index f26ac866dc..b3cb0ae387 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -114,7 +114,7 @@ public: std::string* purpose) = 0; //! Get wallet address list. - virtual std::vector getAddresses() = 0; + virtual std::vector getAddresses() const = 0; //! Get receive requests. virtual std::vector getAddressReceiveRequests() = 0; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 5cfcf16e16..823deed71b 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -204,14 +204,14 @@ public: } return true; } - std::vector getAddresses() override + std::vector getAddresses() const override { LOCK(m_wallet->cs_wallet); std::vector result; - for (const auto& item : m_wallet->m_address_book) { - if (item.second.IsChange()) continue; - result.emplace_back(item.first, m_wallet->IsMine(item.first), item.second.GetLabel(), item.second.purpose); - } + m_wallet->ForEachAddrBookEntry([&](const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change) EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet) { + if (is_change) return; + result.emplace_back(dest, m_wallet->IsMine(dest), label, purpose); + }); return result; } std::vector getAddressReceiveRequests() override { -- cgit v1.2.3 From 83e42c4b94e376a19d3eb0a2379769b8b8ac5fc8 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 11:39:25 -0300 Subject: refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' --- src/wallet/rpc/addresses.cpp | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index f25ad59528..55dd2f935c 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -637,17 +637,6 @@ RPCHelpMan getaddressinfo() }; } -/** Convert CAddressBookData to JSON record. */ -static UniValue AddressBookDataToJSON(const CAddressBookData& data, const bool verbose) -{ - UniValue ret(UniValue::VOBJ); - if (verbose) { - ret.pushKV("name", data.GetLabel()); - } - ret.pushKV("purpose", data.purpose); - return ret; -} - RPCHelpMan getaddressesbylabel() { return RPCHelpMan{"getaddressesbylabel", @@ -680,10 +669,10 @@ RPCHelpMan getaddressesbylabel() // Find all addresses that have the given label UniValue ret(UniValue::VOBJ); std::set addresses; - for (const std::pair& item : pwallet->m_address_book) { - if (item.second.IsChange()) continue; - if (item.second.GetLabel() == label) { - std::string address = EncodeDestination(item.first); + pwallet->ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label, const std::string& _purpose, bool _is_change) { + if (_is_change) return; + if (_label == label) { + std::string address = EncodeDestination(_dest); // CWallet::m_address_book is not expected to contain duplicate // address strings, but build a separate set as a precaution just in // case it does. @@ -693,9 +682,11 @@ RPCHelpMan getaddressesbylabel() // and since duplicate addresses are unexpected (checked with // std::set in O(log(N))), UniValue::__pushKV is used instead, // which currently is O(1). - ret.__pushKV(address, AddressBookDataToJSON(item.second, false)); + UniValue value(UniValue::VOBJ); + value.pushKV("purpose", _purpose); + ret.__pushKV(address, value); } - } + }); if (ret.empty()) { throw JSONRPCError(RPC_WALLET_INVALID_LABEL_NAME, std::string("No addresses with label " + label)); -- cgit v1.2.3 From fa9f2ab8fd53075d2a3ec93ddac4908e73525c46 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 11:46:14 -0300 Subject: refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality Mainly to not access 'm_address_book' externally. --- src/wallet/rpc/addresses.cpp | 8 +------- src/wallet/wallet.cpp | 14 ++++++++++++++ src/wallet/wallet.h | 5 +++++ 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 55dd2f935c..da4cc44ee6 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -733,13 +733,7 @@ RPCHelpMan listlabels() } // Add to a set to sort by label name, then insert into Univalue array - std::set label_set; - for (const std::pair& entry : pwallet->m_address_book) { - if (entry.second.IsChange()) continue; - if (purpose.empty() || entry.second.purpose == purpose) { - label_set.insert(entry.second.GetLabel()); - } - } + std::set label_set = pwallet->ListAddrBookLabels(purpose); UniValue ret(UniValue::VARR); for (const std::string& name : label_set) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f7eb0bbc03..3c211fb348 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2373,6 +2373,20 @@ std::vector CWallet::ListAddrBookAddresses(const std::optional CWallet::ListAddrBookLabels(const std::string& purpose) const +{ + AssertLockHeld(cs_wallet); + std::set label_set; + ForEachAddrBookEntry([&](const CTxDestination& _dest, const std::string& _label, + const std::string& _purpose, bool _is_change) { + if (_is_change) return; + if (purpose.empty() || _purpose == purpose) { + label_set.insert(_label); + } + }); + return label_set; +} + bool ReserveDestination::GetReservedDestination(CTxDestination& dest, bool internal, bilingual_str& error) { m_spk_man = pwallet->GetScriptPubKeyMan(type, internal); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 3775f325ba..8bc1189bec 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -648,6 +648,11 @@ public: */ std::vector ListAddrBookAddresses(const std::optional& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** + * Retrieve all the known labels in the address book + */ + std::set ListAddrBookLabels(const std::string& purpose) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** * Walk-through the address book entries. * Stops when the provided 'ListAddrBookFunc' returns false. -- cgit v1.2.3 From b459fc122feace9e9a738c48aab21961cf15dddc Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 12:00:33 -0300 Subject: refactor: RPC 'ListReceived', encapsulate m_address_book access --- src/wallet/rpc/transactions.cpp | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index fae9bf3ea5..5d9b0965bc 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -138,26 +138,12 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons UniValue ret(UniValue::VARR); std::map label_tally; - // Create m_address_book iterator - // If we aren't filtering, go from begin() to end() - auto start = wallet.m_address_book.begin(); - auto end = wallet.m_address_book.end(); - // If we are filtering, find() the applicable entry - if (has_filtered_address) { - start = wallet.m_address_book.find(filtered_address); - if (start != end) { - end = std::next(start); - } - } + const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) { + if (is_change) return; // no change addresses - for (auto item_it = start; item_it != end; ++item_it) - { - if (item_it->second.IsChange()) continue; - const CTxDestination& address = item_it->first; - const std::string& label = item_it->second.GetLabel(); auto it = mapTally.find(address); if (it == mapTally.end() && !fIncludeEmpty) - continue; + return; CAmount nAmount = 0; int nConf = std::numeric_limits::max(); @@ -196,6 +182,14 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons obj.pushKV("txids", transactions); ret.push_back(obj); } + }; + + if (has_filtered_address) { + const auto& entry = wallet.FindAddressBookEntry(filtered_address, /*allow_change=*/false); + if (entry) func(filtered_address, entry->GetLabel(), entry->purpose, /*is_change=*/false); + } else { + // No filtered addr, walk-through the addressbook entry + wallet.ForEachAddrBookEntry(func); } if (by_label) -- cgit v1.2.3 From 324f00a6420bbd64c67c264e50632e6fa36ae732 Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 11 Jun 2022 12:05:54 -0300 Subject: refactor: 'ListReceived' use optional for filtered address Plus remove open bracket jump line --- src/wallet/rpc/transactions.cpp | 45 +++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp index 5d9b0965bc..6e42c0736d 100644 --- a/src/wallet/rpc/transactions.cpp +++ b/src/wallet/rpc/transactions.cpp @@ -85,14 +85,12 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons filter |= ISMINE_WATCH_ONLY; } - bool has_filtered_address = false; - CTxDestination filtered_address = CNoDestination(); + std::optional filtered_address{std::nullopt}; if (!by_label && !params[3].isNull() && !params[3].get_str().empty()) { if (!IsValidDestinationString(params[3].get_str())) { throw JSONRPCError(RPC_WALLET_ERROR, "address_filter parameter was invalid"); } filtered_address = DecodeDestination(params[3].get_str()); - has_filtered_address = true; } // Tally @@ -106,23 +104,21 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons // Coinbase with less than 1 confirmation is no longer in the main chain if ((wtx.IsCoinBase() && (nDepth < 1)) - || (wallet.IsTxImmatureCoinBase(wtx) && !include_immature_coinbase)) - { + || (wallet.IsTxImmatureCoinBase(wtx) && !include_immature_coinbase)) { continue; } - for (const CTxOut& txout : wtx.tx->vout) - { + for (const CTxOut& txout : wtx.tx->vout) { CTxDestination address; if (!ExtractDestination(txout.scriptPubKey, address)) continue; - if (has_filtered_address && !(filtered_address == address)) { + if (filtered_address && !(filtered_address == address)) { continue; } isminefilter mine = wallet.IsMine(address); - if(!(mine & filter)) + if (!(mine & filter)) continue; tallyitem& item = mapTally[address]; @@ -148,34 +144,27 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons CAmount nAmount = 0; int nConf = std::numeric_limits::max(); bool fIsWatchonly = false; - if (it != mapTally.end()) - { + if (it != mapTally.end()) { nAmount = (*it).second.nAmount; nConf = (*it).second.nConf; fIsWatchonly = (*it).second.fIsWatchonly; } - if (by_label) - { + if (by_label) { tallyitem& _item = label_tally[label]; _item.nAmount += nAmount; _item.nConf = std::min(_item.nConf, nConf); _item.fIsWatchonly = fIsWatchonly; - } - else - { + } else { UniValue obj(UniValue::VOBJ); - if(fIsWatchonly) - obj.pushKV("involvesWatchonly", true); + if (fIsWatchonly) obj.pushKV("involvesWatchonly", true); obj.pushKV("address", EncodeDestination(address)); obj.pushKV("amount", ValueFromAmount(nAmount)); obj.pushKV("confirmations", (nConf == std::numeric_limits::max() ? 0 : nConf)); obj.pushKV("label", label); UniValue transactions(UniValue::VARR); - if (it != mapTally.end()) - { - for (const uint256& _item : (*it).second.txids) - { + if (it != mapTally.end()) { + for (const uint256& _item : (*it).second.txids) { transactions.push_back(_item.GetHex()); } } @@ -184,18 +173,16 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons } }; - if (has_filtered_address) { - const auto& entry = wallet.FindAddressBookEntry(filtered_address, /*allow_change=*/false); - if (entry) func(filtered_address, entry->GetLabel(), entry->purpose, /*is_change=*/false); + if (filtered_address) { + const auto& entry = wallet.FindAddressBookEntry(*filtered_address, /*allow_change=*/false); + if (entry) func(*filtered_address, entry->GetLabel(), entry->purpose, /*is_change=*/false); } else { // No filtered addr, walk-through the addressbook entry wallet.ForEachAddrBookEntry(func); } - if (by_label) - { - for (const auto& entry : label_tally) - { + if (by_label) { + for (const auto& entry : label_tally) { CAmount nAmount = entry.second.nAmount; int nConf = entry.second.nConf; UniValue obj(UniValue::VOBJ); -- cgit v1.2.3 From d69045e291e32e02d105d1b5ff1c8b86db0ae69e Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 22 Jun 2022 12:46:43 -0300 Subject: test: add coverage for 'listreceivedbyaddress' no change addrs return --- test/functional/wallet_listreceivedby.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/functional/wallet_listreceivedby.py b/test/functional/wallet_listreceivedby.py index db1d8eb54a..7ae3a40eaf 100755 --- a/test/functional/wallet_listreceivedby.py +++ b/test/functional/wallet_listreceivedby.py @@ -57,6 +57,11 @@ class ReceivedByTest(BitcoinTestFramework): {"address": empty_addr}, {"address": empty_addr, "label": "", "amount": 0, "confirmations": 0, "txids": []}) + # No returned addy should be a change addr + for node in self.nodes: + for addr_obj in node.listreceivedbyaddress(): + assert_equal(node.getaddressinfo(addr_obj["address"])["ischange"], False) + # Test Address filtering # Only on addr expected = {"address": addr, "label": "", "amount": Decimal("0.1"), "confirmations": 10, "txids": [txid, ]} -- cgit v1.2.3