cryptogera/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-16 16:32:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34037: wallet, doc: clarify the coin selection filters that enforce cluster count

Browse Source 
a067ca34106817565e02daca52b3175266714c25 [doc] coin selection filters by max cluster count, not descendant (glozow)
f7be5fb8fc7d2a5831810a0b91666fc774b64b8f [refactor] rename variable to clarify it is unused and cluster count (glozow)

Pull request description:

  Followup to #33629.

  Fix misleading docs and variable names. Namely, `getTransactionAncestry` returns the cluster count, not max descendant count of ancestor set (not worth reimplementing as it is merely a heuristic). No behavior changes - I don’t think much needs to be changed for the first release containing cluster mempool.

  Current `CoinEligibilityFilter`s enforce a maximum ancestor count (summed across all outputs, potentially overestimating) and max descendant count across ancestors of the output.

  Since #33629, these filters have started using cluster count instead of max desc count across ancestors. The change isn’t dangerous, as the cluster count bounds descendant count as well. Currently, the wallet is essentially enforcing a mixture of both limits - this is good while we are transitioning. Note that the cluster count enforced is 25, not 64, since it's grabbing the node's descendant count limit. While it is not an apples-to-apples comparison, a cluster count limit of 25 helps us avoid busting legacy descendant limits (which will be common on the network for a while).

  Potential things for the future, out of scope for this PR:
  - When we get rid of the ancestor/descendant config options, `getPackageLimits` can probably be replaced with hard-coded values.
  - Change the `OutputGroup`s to track the actual cluster count that results from spending these outputs and merging their clusters.
  - Loosen from 25 after that policy is no longer common.
  - Clean up `getPackageLimits`.

ACKs for top commit:
  achow101:
    ACK a067ca34106817565e02daca52b3175266714c25
  ismaelsadeeq:
    reACK a067ca34106817565e02daca52b3175266714c25
  rkrux:
    crACK a067ca34106817565e02daca52b3175266714c25

Tree-SHA512: d7cacd5bf668d42e26e8b83e42a42c280929c3bfd554c3db1de605e5939f8b36c14ecfd2839abeb4eec352363df1891b3420a693c250916391ab10a5ce26396b
This commit is contained in:
merge-script 2026-03-09 10:11:08 +00:00
commit 3a222507fd
No known key found for this signature in database
GPG Key ID: 2EEB9F5CC09526C1
8 changed files with 52 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/bench/coin_selection.cpp
View File

@ -107,7 +107,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>
tx.vout[nInput].nValue = nValue;
COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/0, /*input_bytes=*/-1, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/true, /*fees=*/0);
set.emplace_back();
set.back().Insert(std::make_shared<COutput>(output), /*ancestors=*/ 0, /*descendants=*/ 0);
set.back().Insert(std::make_shared<COutput>(output), /*ancestors=*/ 0, /*cluster_count=*/ 0);
}
// Copied from src/wallet/test/coinselector_tests.cpp
static CAmount make_hard_case(int utxos, std::vector<OutputGroup>& utxo_pool)

10
src/wallet/coinselection.cpp
View File

@ -752,7 +752,7 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
******************************************************************************/
void OutputGroup::Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t descendants) {
void OutputGroup::Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t cluster_count) {
m_outputs.push_back(output);
auto& coin = *m_outputs.back();
@ -770,9 +770,9 @@ void OutputGroup::Insert(const std::shared_ptr<COutput>& output, size_t ancestor
// the sum, rather than the max; this will overestimate in the cases where multiple inputs
// have common ancestors
m_ancestors += ancestors;
// descendants is the count as seen from the top ancestor, not the descendants as seen from the
// coin itself; thus, this value is counted as the max, not the sum
m_descendants = std::max(m_descendants, descendants);
// This is the maximum cluster count among all outputs. If these outputs are from distinct clusters but spent in the
// same transaction, their clusters will be merged, potentially exceeding the mempool's max cluster count.
m_max_cluster_count = std::max(m_max_cluster_count, cluster_count);
if (output->input_bytes > 0) {
m_weight += output->input_bytes * WITNESS_SCALE_FACTOR;
@ -783,7 +783,7 @@ bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_f
{
return m_depth >= (m_from_me ? eligibility_filter.conf_mine : eligibility_filter.conf_theirs)
&& m_ancestors <= eligibility_filter.max_ancestors
&& m_descendants <= eligibility_filter.max_descendants;
&& m_max_cluster_count <= eligibility_filter.max_cluster_count;
}
CAmount OutputGroup::GetSelectionAmount() const

21
src/wallet/coinselection.h
View File

@ -207,19 +207,20 @@ struct CoinEligibilityFilter
const int conf_theirs;
/** Maximum number of unconfirmed ancestors aggregated across all UTXOs in an OutputGroup. */
const uint64_t max_ancestors;
/** Maximum number of descendants that a single UTXO in the OutputGroup may have. */
const uint64_t max_descendants;
/** Maximum cluster count that a single UTXO in the OutputGroup may have. In practice, this filter also caps the
* maximum descendant count, as a transaction's descendant count is never larger than its cluster count. */
const uint64_t max_cluster_count;
/** When avoid_reuse=true and there are full groups (OUTPUT_GROUP_MAX_ENTRIES), whether or not to use any partial groups.*/
const bool m_include_partial_groups{false};
CoinEligibilityFilter() = delete;
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_ancestors) {}
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants) {}
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_descendants, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_descendants(max_descendants), m_include_partial_groups(include_partial) {}
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_cluster_count(max_ancestors) {}
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_cluster_count) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_cluster_count(max_cluster_count) {}
CoinEligibilityFilter(int conf_mine, int conf_theirs, uint64_t max_ancestors, uint64_t max_cluster_count, bool include_partial) : conf_mine(conf_mine), conf_theirs(conf_theirs), max_ancestors(max_ancestors), max_cluster_count(max_cluster_count), m_include_partial_groups(include_partial) {}
bool operator<(const CoinEligibilityFilter& other) const {
return std::tie(conf_mine, conf_theirs, max_ancestors, max_descendants, m_include_partial_groups)
< std::tie(other.conf_mine, other.conf_theirs, other.max_ancestors, other.max_descendants, other.m_include_partial_groups);
return std::tie(conf_mine, conf_theirs, max_ancestors, max_cluster_count, m_include_partial_groups)
< std::tie(other.conf_mine, other.conf_theirs, other.max_ancestors, other.max_cluster_count, other.m_include_partial_groups);
}
};
@ -239,8 +240,8 @@ struct OutputGroup
/** The aggregated count of unconfirmed ancestors of all UTXOs in this
* group. Not deduplicated and may overestimate when ancestors are shared. */
size_t m_ancestors{0};
/** The maximum count of descendants of a single UTXO in this output group. */
size_t m_descendants{0};
/** The maximum cluster count of a single UTXO in this output group. */
size_t m_max_cluster_count{0};
/** The value of the UTXOs after deducting the cost of spending them at the effective feerate. */
CAmount effective_value{0};
/** The fee to spend these UTXOs at the effective feerate. */
@ -263,7 +264,7 @@ struct OutputGroup
m_subtract_fee_outputs(params.m_subtract_fee_outputs)
{}
void Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t descendants);
void Insert(const std::shared_ptr<COutput>& output, size_t ancestors, size_t cluster_count);
bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
CAmount GetSelectionAmount() const;
};

4
src/wallet/rpc/coins.cpp
View File

@ -663,9 +663,9 @@ RPCHelpMan listunspent()
entry.pushKV("amount", ValueFromAmount(out.txout.nValue));
entry.pushKV("confirmations", out.depth);
if (!out.depth) {
size_t ancestor_count, descendant_count, ancestor_size;
size_t ancestor_count, unused_cluster_count, ancestor_size;
CAmount ancestor_fees;
pwallet->chain().getTransactionAncestry(out.outpoint.hash, ancestor_count, descendant_count, &ancestor_size, &ancestor_fees);
pwallet->chain().getTransactionAncestry(out.outpoint.hash, ancestor_count, unused_cluster_count, &ancestor_size, &ancestor_fees);
if (ancestor_count) {
entry.pushKV("ancestorcount", ancestor_count);
entry.pushKV("ancestorsize", ancestor_size);

46
src/wallet/spend.cpp
View File

@ -405,8 +405,8 @@ CoinsResult AvailableCoins(const CWallet& wallet,
if (wtx.truc_child_in_mempool.has_value()) continue;
// this unconfirmed v3 transaction has a parent: spending would create a third generation
size_t ancestors, descendants;
wallet.chain().getTransactionAncestry(wtx.tx->GetHash(), ancestors, descendants);
size_t ancestors, unused_cluster_count;
wallet.chain().getTransactionAncestry(wtx.tx->GetHash(), ancestors, unused_cluster_count);
if (ancestors > 1) continue;
} else {
if (wtx.tx->version == TRUC_VERSION) continue;
@ -582,12 +582,12 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
for (const auto& [type, outputs] : coins.coins) {
for (const COutput& output : outputs) {
// Get mempool info
size_t ancestors, descendants;
wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants);
size_t ancestors, cluster_count;
wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, cluster_count);
// Create a new group per output and add it to the all groups vector
OutputGroup group(coin_sel_params);
group.Insert(std::make_shared<COutput>(output), ancestors, descendants);
group.Insert(std::make_shared<COutput>(output), ancestors, cluster_count);
// Each filter maps to a different set of groups
bool accepted = false;
@ -611,7 +611,7 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
// OUTPUT_GROUP_MAX_ENTRIES COutputs, a new OutputGroup is added to the end of the vector.
typedef std::map<std::pair<CScript, OutputType>, std::vector<OutputGroup>> ScriptPubKeyToOutgroup;
const auto& insert_output = [&](
const std::shared_ptr<COutput>& output, OutputType type, size_t ancestors, size_t descendants,
const std::shared_ptr<COutput>& output, OutputType type, size_t ancestors, size_t cluster_count,
ScriptPubKeyToOutgroup& groups_map) {
std::vector<OutputGroup>& groups = groups_map[std::make_pair(output->txout.scriptPubKey,type)];
@ -632,24 +632,24 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
group = &groups.back();
}
group->Insert(output, ancestors, descendants);
group->Insert(output, ancestors, cluster_count);
};
ScriptPubKeyToOutgroup spk_to_groups_map;
ScriptPubKeyToOutgroup spk_to_positive_groups_map;
for (const auto& [type, outs] : coins.coins) {
for (const COutput& output : outs) {
size_t ancestors, descendants;
wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, descendants);
size_t ancestors, cluster_count;
wallet.chain().getTransactionAncestry(output.outpoint.hash, ancestors, cluster_count);
const auto& shared_output = std::make_shared<COutput>(output);
// Filter for positive only before adding the output
if (output.GetEffectiveValue() > 0) {
insert_output(shared_output, type, ancestors, descendants, spk_to_positive_groups_map);
insert_output(shared_output, type, ancestors, cluster_count, spk_to_positive_groups_map);
}
// 'All' groups
insert_output(shared_output, type, ancestors, descendants, spk_to_groups_map);
insert_output(shared_output, type, ancestors, cluster_count, spk_to_groups_map);
}
}
@ -871,11 +871,16 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, CoinsResult& available_coins, const CAmount& value_to_select, const CoinSelectionParams& coin_selection_params)
{
// Try to enforce a mixture of cluster limits and ancestor/descendant limits on transactions we create by limiting
// the ancestors and the maximum cluster count of any UTXO we use. We use the ancestor/descendant limits, which are
// lower than the cluster limits, to avoid exceeding any ancestor/descendant limits of legacy nodes. This filter is safe
// because a transaction's ancestor or descendant count cannot be larger than its cluster count.
// TODO: these limits can be relaxed in the future, and we can replace the ancestor filter with a cluster equivalent.
unsigned int limit_ancestor_count = 0;
unsigned int limit_descendant_count = 0;
wallet.chain().getPackageLimits(limit_ancestor_count, limit_descendant_count);
const size_t max_ancestors = (size_t)std::max<int64_t>(1, limit_ancestor_count);
const size_t max_descendants = (size_t)std::max<int64_t>(1, limit_descendant_count);
const size_t max_cluster_count = (size_t)std::max<int64_t>(1, limit_descendant_count);
const bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
// Cases where we have 101+ outputs all pointing to the same destination may result in
@ -900,20 +905,21 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
// possible) if we cannot fund the transaction otherwise.
if (wallet.m_spend_zero_conf_change) {
ordered_filters.push_back({CoinEligibilityFilter(0, 1, 2)});
ordered_filters.push_back({CoinEligibilityFilter(0, 1, std::min(size_t{4}, max_ancestors/3), std::min(size_t{4}, max_descendants/3))});
ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2)});
ordered_filters.push_back({CoinEligibilityFilter(0, 1, std::min(size_t{4}, max_ancestors/3), std::min(size_t{4}, max_cluster_count/3))});
ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors/2, max_cluster_count/2)});
// If partial groups are allowed, relax the requirement of spending OutputGroups (groups
// of UTXOs sent to the same address, which are obviously controlled by a single wallet)
// in their entirety.
ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, /*include_partial=*/true)});
ordered_filters.push_back({CoinEligibilityFilter(0, 1, max_ancestors-1, max_cluster_count-1, /*include_partial=*/true)});
// Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs
// received from other wallets.
if (coin_selection_params.m_include_unsafe_inputs) {
ordered_filters.push_back({CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs=*/0, max_ancestors-1, max_descendants-1, /*include_partial=*/true)});
ordered_filters.push_back({CoinEligibilityFilter(/*conf_mine=*/0, /*conf_theirs=*/0, max_ancestors-1, max_cluster_count-1, /*include_partial=*/true)});
}
// Try with unlimited ancestors/descendants. The transaction will still need to meet
// mempool ancestor/descendant policy to be accepted to mempool and broadcasted, but
// OutputGroups use heuristics that may overestimate ancestor/descendant counts.
// Try with unlimited ancestors/clusters. The transaction will still need to meet
// local mempool policy (i.e. cluster limits) to be accepted to mempool and broadcasted, and
// limits of other nodes (e.g. ancestor/descendant limits) to propagate, but OutputGroups
// use heuristics that may overestimate.
if (!fRejectLongChains) {
ordered_filters.push_back({CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max(),
std::numeric_limits<uint64_t>::max(),
@ -930,7 +936,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
CAmount total_unconf_long_chain = 0;
for (const auto& group : discarded_groups) {
total_discarded += group.GetSelectionAmount();
if (group.m_ancestors >= max_ancestors || group.m_descendants >= max_descendants) total_unconf_long_chain += group.GetSelectionAmount();
if (group.m_ancestors >= max_ancestors || group.m_max_cluster_count >= max_cluster_count) total_unconf_long_chain += group.GetSelectionAmount();
}
if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select) {

2
src/wallet/test/coinselection_tests.cpp
View File

@ -53,7 +53,7 @@ static OutputGroup MakeCoin(const CAmount& amount, bool is_eff_value = true, Coi
tx.vout[0].nValue = amount + int(is_eff_value) * fees;
tx.nLockTime = next_lock_time++; // so all transactions get different hashes
OutputGroup group(cs_params);
group.Insert(std::make_shared<COutput>(COutPoint(tx.GetHash(), 0), tx.vout.at(0), /*depth=*/1, /*input_bytes=*/custom_spending_vsize, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/fees), /*ancestors=*/0, /*descendants=*/0);
group.Insert(std::make_shared<COutput>(COutPoint(tx.GetHash(), 0), tx.vout.at(0), /*depth=*/1, /*input_bytes=*/custom_spending_vsize, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/fees), /*ancestors=*/0, /*cluster_count=*/0);
return group;
}

6
src/wallet/test/coinselector_tests.cpp
View File

@ -44,7 +44,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result)
tx.nLockTime = nextLockTime++; // so all transactions get different hashes
COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, /*input_bytes=*/-1, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/ 0);
OutputGroup group;
group.Insert(std::make_shared<COutput>(output), /*ancestors=*/ 0, /*descendants=*/ 0);
group.Insert(std::make_shared<COutput>(output), /*ancestors=*/ 0, /*cluster_count=*/ 0);
result.AddInput(group);
}
@ -56,7 +56,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result,
tx.nLockTime = nextLockTime++; // so all transactions get different hashes
std::shared_ptr<COutput> coin = std::make_shared<COutput>(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, /*input_bytes=*/148, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, fee);
OutputGroup group;
group.Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0);
group.Insert(coin, /*ancestors=*/ 0, /*cluster_count=*/ 0);
coin->long_term_fee = long_term_fee; // group.Insert() will modify long_term_fee, so we need to set it afterwards
result.AddInput(group);
}
@ -130,7 +130,7 @@ inline std::vector<OutputGroup>& GroupCoins(const std::vector<COutput>& availabl
for (auto& coin : available_coins) {
static_groups.emplace_back();
OutputGroup& group = static_groups.back();
group.Insert(std::make_shared<COutput>(coin), /*ancestors=*/ 0, /*descendants=*/ 0);
group.Insert(std::make_shared<COutput>(coin), /*ancestors=*/ 0, /*cluster_count=*/ 0);
group.m_subtract_fee_outputs = subtract_fee_outputs;
}
return static_groups;

6
src/wallet/test/fuzz/coinselection.cpp
View File

@ -33,7 +33,7 @@ static void GroupCoins(FuzzedDataProvider& fuzzed_data_provider, const std::vect
bool valid_outputgroup{false};
for (auto& coin : coins) {
if (!positive_only || (positive_only && coin.GetEffectiveValue() > 0)) {
output_group.Insert(std::make_shared<COutput>(coin), /*ancestors=*/0, /*descendants=*/0);
output_group.Insert(std::make_shared<COutput>(coin), /*ancestors=*/0, /*cluster_count=*/0);
}
// If positive_only was specified, nothing was inserted, leading to an empty output group
// that would be invalid for the BnB algorithm
@ -167,7 +167,7 @@ FUZZ_TARGET(coin_grinder_is_optimal)
max_spendable += eff_value;
auto output_group = OutputGroup(coin_params);
output_group.Insert(std::make_shared<COutput>(temp_utxo_pool.at(0)), /*ancestors=*/0, /*descendants=*/0);
output_group.Insert(std::make_shared<COutput>(temp_utxo_pool.at(0)), /*ancestors=*/0, /*cluster_count=*/0);
group_pos.push_back(output_group);
}
size_t num_groups = group_pos.size();
@ -264,7 +264,7 @@ FUZZ_TARGET(bnb_finds_min_waste)
max_spendable += eff_value;
auto output_group = OutputGroup(coin_params);
output_group.Insert(std::make_shared<COutput>(temp_utxo_pool.at(0)), /*ancestors=*/0, /*descendants=*/0);
output_group.Insert(std::make_shared<COutput>(temp_utxo_pool.at(0)), /*ancestors=*/0, /*cluster_count=*/0);
group_pos.push_back(output_group);
}
size_t num_groups = group_pos.size();