From a067ca34106817565e02daca52b3175266714c25 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 9 Dec 2025 12:45:01 -0800 Subject: [PATCH] [doc] coin selection filters by max cluster count, not descendant Avoid confusion by clarifying the docs and renaming the variables that now hold cluster count rather than descendant count. No behavior change. --- src/bench/coin_selection.cpp | 2 +- src/wallet/coinselection.cpp | 10 +++--- src/wallet/coinselection.h | 21 +++++++------ src/wallet/spend.cpp | 42 ++++++++++++++----------- src/wallet/test/coinselection_tests.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 6 ++-- src/wallet/test/fuzz/coinselection.cpp | 6 ++-- 7 files changed, 48 insertions(+), 41 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 72b3d70ec68..2f108a310b4 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -107,7 +107,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector 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(output), /*ancestors=*/ 0, /*descendants=*/ 0); + set.back().Insert(std::make_shared(output), /*ancestors=*/ 0, /*cluster_count=*/ 0); } // Copied from src/wallet/test/coinselector_tests.cpp static CAmount make_hard_case(int utxos, std::vector& utxo_pool) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 8977d999382..d7fde38991c 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -752,7 +752,7 @@ util::Result KnapsackSolver(std::vector& groups, c ******************************************************************************/ -void OutputGroup::Insert(const std::shared_ptr& output, size_t ancestors, size_t descendants) { +void OutputGroup::Insert(const std::shared_ptr& 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& 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 diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 22103de167c..2457cf95ee2 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -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& output, size_t ancestors, size_t descendants); + void Insert(const std::shared_ptr& output, size_t ancestors, size_t cluster_count); bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const; CAmount GetSelectionAmount() const; }; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index c91d0f47778..70044da195b 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -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(output), ancestors, descendants); + group.Insert(std::make_shared(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::vector> ScriptPubKeyToOutgroup; const auto& insert_output = [&]( - const std::shared_ptr& output, OutputType type, size_t ancestors, size_t descendants, + const std::shared_ptr& output, OutputType type, size_t ancestors, size_t cluster_count, ScriptPubKeyToOutgroup& groups_map) { std::vector& 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(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 SelectCoins(const CWallet& wallet, CoinsResult& av util::Result 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(1, limit_ancestor_count); - const size_t max_descendants = (size_t)std::max(1, limit_descendant_count); + const size_t max_cluster_count = (size_t)std::max(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 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::max(), std::numeric_limits::max(), @@ -930,7 +936,7 @@ util::Result 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) { diff --git a/src/wallet/test/coinselection_tests.cpp b/src/wallet/test/coinselection_tests.cpp index 6aa473bc260..4a0eed8f090 100644 --- a/src/wallet/test/coinselection_tests.cpp +++ b/src/wallet/test/coinselection_tests.cpp @@ -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(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(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; } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 7f55d76585b..04d64ae241b 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -43,7 +43,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(output), /*ancestors=*/ 0, /*descendants=*/ 0); + group.Insert(std::make_shared(output), /*ancestors=*/ 0, /*cluster_count=*/ 0); result.AddInput(group); } @@ -55,7 +55,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result, tx.nLockTime = nextLockTime++; // so all transactions get different hashes std::shared_ptr coin = std::make_shared(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); } @@ -129,7 +129,7 @@ inline std::vector& GroupCoins(const std::vector& availabl for (auto& coin : available_coins) { static_groups.emplace_back(); OutputGroup& group = static_groups.back(); - group.Insert(std::make_shared(coin), /*ancestors=*/ 0, /*descendants=*/ 0); + group.Insert(std::make_shared(coin), /*ancestors=*/ 0, /*cluster_count=*/ 0); group.m_subtract_fee_outputs = subtract_fee_outputs; } return static_groups; diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 084777f8490..8b30423c6f2 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -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(coin), /*ancestors=*/0, /*descendants=*/0); + output_group.Insert(std::make_shared(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(temp_utxo_pool.at(0)), /*ancestors=*/0, /*descendants=*/0); + output_group.Insert(std::make_shared(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(temp_utxo_pool.at(0)), /*ancestors=*/0, /*descendants=*/0); + output_group.Insert(std::make_shared(temp_utxo_pool.at(0)), /*ancestors=*/0, /*cluster_count=*/0); group_pos.push_back(output_group); } size_t num_groups = group_pos.size();