phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-10 20:03:34 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#25337: refactor: encapsulate wallet's address book access

Browse source 
d69045e291 test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy)
324f00a642 refactor: 'ListReceived' use optional for filtered address (furszy)
b459fc122f refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy)
fa9f2ab8fd refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy)
83e42c4b94 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy)
2b48642499 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy)
032842ae41 wallet: implement ForEachAddrBookEntry method (furszy)
09649bc95d refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy)
192eb1e61c refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy)

Pull request description:

  ### Context

  The wallet's `m_address_book` field is being accessed directly from several places across the sources.

  ### Problem

  Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own.

  ### Solution

  Encapsulate `m_address_book` access inside the wallet.

  -------------------------------------------------------

  Extra Note:

  This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR).

ACKs for top commit:
  achow101:
    ACK d69045e291
  theStack:
    ACK d69045e291 ✅
  w0xlt:
    ACK d69045e291

Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
This commit is contained in:
Andrew Chow 2022-07-08 10:09:25 -04:00
commit b9f9ed4640
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
8 changed files with 108 additions and 92 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/interfaces/wallet.h
View file

@ -114,7 +114,7 @@ public:
std::string* purpose) = 0;
//! Get wallet address list.
virtual std::vector<WalletAddress> getAddresses() = 0;
virtual std::vector<WalletAddress> getAddresses() const = 0;
//! Get receive requests.
virtual std::vector<std::string> getAddressReceiveRequests() = 0;

20
src/wallet/interfaces.cpp
View file

@ -191,29 +191,27 @@ 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;
}
std::vector<WalletAddress> getAddresses() override
std::vector<WalletAddress> getAddresses() const override
{
LOCK(m_wallet->cs_wallet);
std::vector<WalletAddress> 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<std::string> getAddressReceiveRequests() override {

33
src/wallet/rpc/addresses.cpp
View file

@ -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<std::string> addresses;
for (const std::pair<const CTxDestination, CAddressBookData>& 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));
@ -742,13 +733,7 @@ RPCHelpMan listlabels()
}
// Add to a set to sort by label name, then insert into Univalue array
std::set<std::string> label_set;
for (const std::pair<const CTxDestination, CAddressBookData>& 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<std::string> label_set = pwallet->ListAddrBookLabels(purpose);
UniValue ret(UniValue::VARR);
for (const std::string& name : label_set) {

6
src/wallet/rpc/coins.cpp
View file

@ -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<CTxDestination> addresses;
std::vector<CTxDestination> 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

67
src/wallet/rpc/transactions.cpp
View file

@ -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<CTxDestination> 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];
@ -138,70 +134,55 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
UniValue ret(UniValue::VARR);
std::map<std::string, tallyitem> 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<int>::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<int>::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());
}
}
obj.pushKV("txids", transactions);
ret.push_back(obj);
}
};
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);

42
src/wallet/wallet.cpp
View file

@ -2368,21 +2368,45 @@ void CWallet::MarkDestinationsDirty(const std::set<CTxDestination>& destinations
}
}
std::set<CTxDestination> CWallet::GetLabelAddresses(const std::string& label) const
void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const
{
AssertLockHeld(cs_wallet);
std::set<CTxDestination> result;
for (const std::pair<const CTxDestination, CAddressBookData>& item : m_address_book)
{
if (item.second.IsChange()) continue;
const CTxDestination& address = item.first;
const std::string& strName = item.second.GetLabel();
if (strName == label)
result.insert(address);
for (const std::pair<const CTxDestination, CAddressBookData>& item : m_address_book) {
const auto& entry = item.second;
func(item.first, entry.GetLabel(), entry.purpose, entry.IsChange());
}
}
std::vector<CTxDestination> CWallet::ListAddrBookAddresses(const std::optional<AddrBookFilter>& _filter) const
{
AssertLockHeld(cs_wallet);
std::vector<CTxDestination> result;
AddrBookFilter filter = _filter ? *_filter : AddrBookFilter();
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;
}
std::set<std::string> CWallet::ListAddrBookLabels(const std::string& purpose) const
{
AssertLockHeld(cs_wallet);
std::set<std::string> 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);

25
src/wallet/wallet.h
View file

@ -635,7 +635,30 @@ public:
std::optional<int64_t> GetOldestKeyPoolTime() const;
std::set<CTxDestination> 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<std::string> 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<CTxDestination> ListAddrBookAddresses(const std::optional<AddrBookFilter>& filter) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/**
* Retrieve all the known labels in the address book
*/
std::set<std::string> ListAddrBookLabels(const std::string& purpose) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
/**
* Walk-through the address book entries.
* Stops when the provided 'ListAddrBookFunc' returns false.
*/
using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change)>;
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

5
test/functional/wallet_listreceivedby.py
View file

@ -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, ]}