Skip to content

refactor: carry the coin type in CoinSelectionParams #7651

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
Aug 31, 2026
Merged

refactor: carry the coin type in CoinSelectionParams #7651

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
26d12fa
refactor: carry the coin type in CoinSelectionParams
knst Aug 30, 2026
4ae4ab0
refactor: drop CoinType threading from AttemptSelection/ChooseSelecti…
knst Aug 30, 2026
22441a3
refactor: fold SelectTxDSInsByDenomination overloads into one
knst Aug 30, 2026
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
14 changes: 1 addition & 13 deletions src/wallet/coincontrol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,14 @@
#include <script/keyorigin.h>
#include <script/signingprovider.h>
#include <script/standard.h>
#include <wallet/coinselection.h>

#include <algorithm>
#include <map>
#include <optional>
#include <set>

namespace wallet {
enum class CoinType : uint8_t
{
ALL_COINS,
ONLY_FULLY_MIXED,
ONLY_READY_TO_MIX,
ONLY_NONDENOMINATED,
ONLY_MASTERNODE_COLLATERAL, // find masternode outputs including locked ones (use with caution)
ONLY_COINJOIN_COLLATERAL,
// Attributes
MIN_COIN_TYPE = ALL_COINS,
MAX_COIN_TYPE = ONLY_COINJOIN_COLLATERAL,
};

//! Default for -avoidpartialspends
static constexpr bool DEFAULT_AVOIDPARTIALSPENDS = false;

Expand Down
6 changes: 1 addition & 5 deletions src/wallet/coinjoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <wallet/coinjoin.h>

#include <key_io.h>
#include <wallet/coincontrol.h>
#include <wallet/receive.h>
#include <wallet/spend.h>
#include <wallet/transaction.h>
Expand Down Expand Up @@ -45,11 +46,6 @@ bool CWallet::SetCoinJoinSalt(const uint256& cj_salt)
return false;
}

bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet)
{
return SelectTxDSInsByDenomination(nDenom, nValueMax, vecTxDSInRet, CoinType::ONLY_READY_TO_MIX);
}

bool CWallet::SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet, CoinType nCoinType)
{
LOCK(cs_wallet);
Expand Down
16 changes: 16 additions & 0 deletions src/wallet/coinselection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <random.h>
#include <util/result.h>

#include <cstdint>
#include <optional>

namespace wallet {
Expand All @@ -19,6 +20,19 @@ static constexpr CAmount CHANGE_LOWER{50000};
//! upper bound for randomly-chosen target change amount
static constexpr CAmount CHANGE_UPPER{1000000};

enum class CoinType : uint8_t
{
ALL_COINS,
ONLY_FULLY_MIXED,
ONLY_READY_TO_MIX,
ONLY_NONDENOMINATED,
ONLY_MASTERNODE_COLLATERAL, // find masternode outputs including locked ones (use with caution)
ONLY_COINJOIN_COLLATERAL,
// Attributes
MIN_COIN_TYPE = ALL_COINS,
MAX_COIN_TYPE = ONLY_COINJOIN_COLLATERAL,
};

/** A UTXO under consideration for use in funding a new transaction. */
struct COutput {
private:
Expand Down Expand Up @@ -146,6 +160,8 @@ struct CoinSelectionParams {
* associated with the same address. This helps reduce privacy leaks resulting from address
* reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */
bool m_avoid_partial_spends = false;
/** Dash: which class of coins may be selected (from CCoinControl::nCoinType). */
CoinType m_coin_type{CoinType::ALL_COINS};
Comment on lines +163 to +164

@thepastaclaw thepastaclaw Aug 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Nitpick: Document the CoinType consistency requirement for direct SelectCoins callers

SelectCoins still reads coin_control.nCoinType when filtering preset inputs, while ChooseSelectionResult and the solvers now read coin_selection_params.m_coin_type. CreateTransactionInternal synchronizes the two values, but a direct internal caller that sets only CCoinControl::nCoinType now gets inconsistent behavior; before this PR, SelectCoins itself passed that value through every selection attempt. The current parenthetical identifies the field's source but does not state the new caller invariant. Document that callers constructing CoinSelectionParams separately must keep the values synchronized.

Suggested change
/** Dash: which class of coins may be selected (from CCoinControl::nCoinType). */
CoinType m_coin_type{CoinType::ALL_COINS};
/** Dash: which class of coins may be selected. CreateTransactionInternal syncs this
* from CCoinControl::nCoinType; other callers of SelectCoins() that build
* CoinSelectionParams directly must keep it consistent with their coin control. */
CoinType m_coin_type{CoinType::ALL_COINS};

source: ['codex']


CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size,
CAmount min_change_target, CFeeRate effective_feerate,
Expand Down
31 changes: 17 additions & 14 deletions src/wallet/spend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,19 +443,19 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C
}

std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins,
const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types, CoinType nCoinType)
const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types)
{
// Run coin selection on each OutputType and compute the Waste Metric
std::vector<SelectionResult> results;
if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params, nCoinType)}) {
if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.legacy, coin_selection_params)}) {
results.push_back(*result);
}

// If we can't fund the transaction from any individual OutputType, run coin selection
// over all available coins, else pick the best solution from the results
if (results.size() == 0) {
if (allow_mixed_output_types) {
if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params, nCoinType)}) {
if (auto result{ChooseSelectionResult(wallet, nTargetValue, eligibility_filter, available_coins.all(), coin_selection_params)}) {
return result;
}
}
Expand All @@ -466,7 +466,7 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
};

std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
const CoinSelectionParams& coin_selection_params, CoinType nCoinType)
const CoinSelectionParams& coin_selection_params)
{
// Vector of results. We will choose the best one based on waste.
std::vector<SelectionResult> results;
Expand All @@ -484,13 +484,15 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons
// The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here.
std::vector<OutputGroup> all_groups = GroupOutputs(wallet, available_coins, coin_selection_params, eligibility_filter, /*positive_only=*/false);
if (auto knapsack_result{KnapsackSolver(all_groups, nTargetValue, coin_selection_params.m_min_change_target,
coin_selection_params.rng_fast, max_inputs_weight, nCoinType == CoinType::ONLY_FULLY_MIXED,
coin_selection_params.rng_fast, max_inputs_weight,
coin_selection_params.m_coin_type == CoinType::ONLY_FULLY_MIXED,
wallet.m_default_max_tx_fee)}) {
knapsack_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
results.push_back(*knapsack_result);
}

if (auto srd_result{SelectCoinsSRD(positive_groups, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight, nCoinType == CoinType::ONLY_FULLY_MIXED)}) {
if (auto srd_result{SelectCoinsSRD(positive_groups, nTargetValue, coin_selection_params.rng_fast, max_inputs_weight,
coin_selection_params.m_coin_type == CoinType::ONLY_FULLY_MIXED)}) {
srd_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
results.push_back(*srd_result);
}
Expand Down Expand Up @@ -637,35 +639,35 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a

// If possible, fund the transaction with confirmed UTXOs only. Prefer at least six
// confirmations on outputs received from other wallets and only spend confirmed change.
if (auto r1{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 6, 0), available_coins, coin_selection_params, /*allow_mixed_output_types=*/false, nCoinType)}) return r1;
if (auto r1{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 6, 0), available_coins, coin_selection_params, /*allow_mixed_output_types=*/false)}) return r1;
// Allow mixing only if no solution from any single output type can be found
if (auto r2{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 1, 0), available_coins, coin_selection_params, /*allow_mixed_output_types=*/true, nCoinType)}) return r2;
if (auto r2{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(1, 1, 0), available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) return r2;

// Fall back to using zero confirmation change (but with as few ancestors in the mempool as
// possible) if we cannot fund the transaction otherwise.
if (wallet.m_spend_zero_conf_change) {
if (auto r3{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, 2), available_coins, coin_selection_params, /*allow_mixed_output_types=*/true, nCoinType)}) return r3;
if (auto r3{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, 2), available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) return r3;
if (auto r4{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, std::min(size_t{4}, max_ancestors/3), std::min(size_t{4}, max_descendants/3)),
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true, nCoinType)}) {
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) {
return r4;
}
if (auto r5{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2),
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true, nCoinType)}) {
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) {
return r5;
}
// 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.
if (auto r6{AttemptSelection(wallet, value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1, true /* include_partial_groups */),
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true, nCoinType)}) {
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) {
return r6;
}
// Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs
// received from other wallets.
if (coin_control.m_include_unsafe_inputs) {
if (auto r7{AttemptSelection(wallet, value_to_select,
CoinEligibilityFilter(0 /* conf_mine */, 0 /* conf_theirs */, max_ancestors-1, max_descendants-1, true /* include_partial_groups */),
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true, nCoinType)}) {
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) {
return r7;
}
}
Expand All @@ -675,7 +677,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
if (!fRejectLongChains) {
if (auto r8{AttemptSelection(wallet, value_to_select,
CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max(), std::numeric_limits<uint64_t>::max(), true /* include_partial_groups */),
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true, nCoinType)}) {
available_coins, coin_selection_params, /*allow_mixed_output_types=*/true)}) {
return r8;
}
}
Expand Down Expand Up @@ -827,6 +829,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(

CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
coin_selection_params.m_coin_type = coin_control.nCoinType;

@thepastaclaw thepastaclaw Aug 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Suggestion: Add coverage for CoinType propagation through automatic selection

This assignment is the only production bridge from CCoinControl::nCoinType to the m_coin_type value that controls KnapsackSolver's fully-mixed behavior. Existing rpc_coinjoin coverage does not execute this path with selectable mixed coins: the automatic failure case stops during AvailableCoins filtering, the successful send uses a preset input and manual selection, and sendall constructs its inputs without SelectCoins. If this assignment were dropped, AvailableCoins would still restrict the pool to mixed denominations, but knapsack would run in ordinary mode, changing its denomination ordering, change target, and excess-fee rejection behavior without a focused test failing. The final transaction-level maximum-fee check prevents an excessive fee from being committed, but it does not test the intended fully-mixed solver behavior. Add a CreateTransaction regression using an automatically selected fully mixed denomination and a target difference above the configured maximum fee so losing this propagation changes the asserted result.

source: ['claude', 'codex']


// Set the long term feerate estimate to the wallet's consolidate feerate
coin_selection_params.m_long_term_feerate = wallet.m_consolidate_feerate;
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/spend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ std::vector<OutputGroup> GroupOutputs(const CWallet& wallet, const std::vector<C
* If failed, a nullopt
*/
std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const CoinsResult& available_coins,
const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types, CoinType nCoinType = CoinType::ALL_COINS);
const CoinSelectionParams& coin_selection_params, bool allow_mixed_output_types);

/**
* Attempt to find a valid input set that meets the provided eligibility filter and target.
Expand All @@ -120,7 +120,7 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
* If failed, a nullopt
*/
std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
const CoinSelectionParams& coin_selection_params, CoinType nCoinType = CoinType::ALL_COINS);
const CoinSelectionParams& coin_selection_params);

/**
* Select a set of coins such that nTargetValue is met and at least
Expand Down
3 changes: 1 addition & 2 deletions src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,8 +616,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
bool CanSupportFeature(enum WalletFeature wf) const override EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return IsFeatureSupported(nWalletVersion, wf); }

// Coin selection
bool SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet);
bool SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet, CoinType nCoinType);
bool SelectTxDSInsByDenomination(int nDenom, CAmount nValueMax, std::vector<CTxDSIn>& vecTxDSInRet, CoinType nCoinType = CoinType::ONLY_READY_TO_MIX);
bool SelectDenominatedAmounts(CAmount nValueMax, std::set<CAmount>& setAmountsRet) const;

std::vector<CompactTallyItem> SelectCoinsGroupedByAddresses(bool fSkipDenominated = true, bool fAnonymizable = true, bool fSkipUnconfirmed = true, int nMaxOupointsPerAddress = -1) const;
Expand Down