Skip to content

backport: bitcoin#25383, 25535, 25337 #6524

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 3 commits into from
Apr 3, 2025
Merged

backport: bitcoin#25383, 25535, 25337 #6524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
037ee56
Merge bitcoin/bitcoin#25383: wallet: don't read db every time that a …
Jun 30, 2022
dc1e8a4
Merge bitcoin/bitcoin#25535: test: pass `dustrelayfee=0` option for t…
Jul 4, 2022
a7d4127
Merge bitcoin/bitcoin#25337: refactor: encapsulate wallet's address b…
achow101 Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/interfaces/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ class Wallet
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;
Expand Down
6 changes: 0 additions & 6 deletions src/wallet/bdb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,12 +324,6 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, b
env = database.env.get();
pdb = database.m_db.get();
strFile = database.strFile;
if (!Exists(std::string("version"))) {
bool fTmp = fReadOnly;
fReadOnly = false;
Write(std::string("version"), CLIENT_VERSION);
fReadOnly = fTmp;
}
}

void BerkeleyDatabase::Open()
Expand Down
20 changes: 9 additions & 11 deletions src/wallet/interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,29 +205,27 @@ class WalletImpl : public Wallet
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 {
Expand Down
110 changes: 38 additions & 72 deletions src/wallet/rpcwallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,12 +540,12 @@ static RPCHelpMan listaddressbalances()

static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
{
std::set<CTxDestination> address_set;
std::vector<CTxDestination> address_vector;

if (by_label) {
// Get the set of addresses assigned to label
std::string label = LabelFromValue(params[0]);
address_set = wallet.GetLabelAddresses(label);
address_vector = wallet.ListAddrBookAddresses(CWallet::AddrBookFilter{label});
} else {
// Get the address
CTxDestination dest = DecodeDestination(params[0].get_str());
Expand All @@ -556,7 +556,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
if (!wallet.IsMine(script_pub_key)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Address not found in wallet");
}
address_set.insert(dest);
address_vector.emplace_back(dest);
}

// Minimum confirmations
Expand Down Expand Up @@ -590,7 +590,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b

for (const CTxOut& txout : wtx.tx->vout) {
CTxDestination address;
if (ExtractDestination(txout.scriptPubKey, address) && wallet.IsMine(address) && address_set.count(address)) {
if (ExtractDestination(txout.scriptPubKey, address) && wallet.IsMine(address) && std::find(address_vector.begin(), address_vector.end(), address) != address_vector.end()) {
amount += txout.nValue;
}
}
Expand Down Expand Up @@ -971,14 +971,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[4].isNull() && !params[4].get_str().empty()) {
if (!IsValidDestinationString(params[4].get_str())) {
throw JSONRPCError(RPC_WALLET_ERROR, "address_filter parameter was invalid");
}
filtered_address = DecodeDestination(params[4].get_str());
has_filtered_address = true;
}

// Excluding coinbase outputs is deprecated
Expand All @@ -1005,18 +1003,17 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
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];
Expand All @@ -1032,74 +1029,58 @@ 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);
}
}

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();
const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) {
if (is_change) return; // no change addresses
auto it = mapTally.find(address);
if (it == mapTally.end() && !fIncludeEmpty)
continue;
return;

isminefilter mine = wallet.IsMine(address);
if(!(mine & filter))
continue;
if (!(mine & filter))
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);
Expand Down Expand Up @@ -3849,17 +3830,6 @@ static UniValue DescribeWalletAddress(const CWallet& wallet, const CTxDestinatio
return ret;
}

/** 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 getaddressinfo()
{
return RPCHelpMan{"getaddressinfo",
Expand Down Expand Up @@ -4030,10 +4000,10 @@ static 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.
Expand All @@ -4043,9 +4013,11 @@ static 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));
Expand Down Expand Up @@ -4092,13 +4064,7 @@ static 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) {
Expand Down
43 changes: 33 additions & 10 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2903,21 +2903,44 @@ 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
{
LOCK(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) {
Copy link
Collaborator

@knst knst Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for myself: missing AssertLockHeld(cs_wallet); due to non-backported bitcoin#19833

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 fInternalIn)
{
m_spk_man = pwallet->GetScriptPubKeyMan(fInternalIn);
Expand Down
25 changes: 24 additions & 1 deletion src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,30 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
std::set<std::set<CTxDestination>> GetAddressGroupings() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
std::map<CTxDestination, CAmount> GetAddressBalances() const;

std::set<CTxDestination> GetLabelAddresses(const std::string& label) const;
// 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);
Comment on lines 812 to 817
Copy link

@coderabbitai coderabbitai bot Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Potential inconsistency in ForEachAddrBookEntry documentation

The documentation comment states that iteration "Stops when the provided 'ListAddrBookFunc' returns false", but the function type is defined with a void return type, which can't return a boolean value to control iteration.

Verify that either:

  1. The callback function should return a boolean (in which case the function type should be changed), or
  2. The documentation should be updated to match the actual behavior

🏁 Script executed:

#!/bin/bash
# Check the implementation of ForEachAddrBookEntry to see if it expects a boolean return
grep -A 15 "void CWallet::ForEachAddrBookEntry" src/wallet/wallet.cpp

Length of output: 840

Update documentation for ForEachAddrBookEntry to correctly reflect its behavior

There is an inconsistency: the documentation claims that iteration stops when the callback returns false, yet the callback type is defined as void and the implementation iterates over all entries without checking any return value. Please update the comment to indicate that the address book is fully iterated without an early termination condition.

  • Location: src/wallet/wallet.h (Lines 814–819)
  • Action: Revise the documentation comment to remove any reference to stopping on a false return value.


/**
* Marks all outputs in each one of the destinations dirty, so their cache is
Expand Down
Loading
Loading