← Watch feed
walllet: use CoinsResult instead of PreSelectedInputs
Public commit record
85/100 · Strong
What the developer wrote
Authored by stratospher
walllet: use CoinsResult instead of PreSelectedInputs
PreSelectedInputs is confusing to use. it's `total_amount`
might store total amount or effective amount based on SFFO.
ex: we might accidentally sum preselected inputs effective
amount (named `total_amount`) with automatically selected
inputs actual total amount.
CoinsResult has a cleaner interface with separate fields
for both these amounts.
2 behavioural changes:
1. no more default assert error if effective value is unset
- previously PreSelectedInputs::Insert() called
COutput::GetEffectiveValue() which assert failed
if the optional was unset.
- now we don't default assert anymore.
* in GUI/getAvailableBalance better not to assert.
* SelectCoins's preselected inputs always contain a
feerate, so effective amount should be set.
explicitly added an assertion to ensure this.
2. FetchSelectedInputs uses OutputType::UNKNOWN as key to
populate CoinsResult's coins map. it's discarded later.
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This is a Bitcoin Core wallet code cleanup that replaces an internal data structure called PreSelectedInputs with a cleaner one called CoinsResult. The commit message says the old structure was confusing because a field named total_amount could hold two different kinds of amounts, which could lead to accidentally mixing them. The patch separates those amounts and adds an explicit safety check where it matters. It is a defensive refactor rather than a fix for a known live bug or exploit.
Recommended action
Treat as a normal code-quality and defensive-correctness change. Reviewers should verify that GetAppropriateTotal() is used consistently wherever preset or available coin totals feed into selection_target calculations, and that the new assertion in SelectCoins cannot be triggered by valid wallet states.
Security signals we found
01
Mixing of total_amount and effective_amount in legacy PreSelectedInputs struct
02
Removal of default assertion on unset effective value
03
Addition of explicit assertion in SelectCoins for preset total
04
Refactor of coin selection arithmetic around subtract-fee-from-outputs
05
No explicit bug, CVE, or exploit described in commit or references
Evidence from the diff
The change refactors coin-control preset-input handling. PreSelectedInputs::Insert() mixed total amount and effective amount depending on the subtract-fee-from-outputs flag. CoinsResult exposes separate totals and a GetAppropriateTotal() helper. The patch removes a default assertion on effective value in the insert path, relying on callers to assert when needed (SelectCoins now asserts preset_total.has_value()). FetchSelectedInputs now returns a CoinsResult keyed under OutputType::UNKNOWN, which is later discarded. Test code is updated to use the new API.
Changed components
src/wallet/spend.cppsrc/wallet/spend.hsrc/wallet/interfaces.cppsrc/wallet/test/coinselector_tests.cppInspect captured patch +24 / −18
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index 28c4d639..8e0f240a 100644
--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -411,7 +411,7 @@ public:
CoinSelectionParams params(rng);
// Note: for now, swallow any error.
if (auto res = FetchSelectedInputs(*m_wallet, coin_control, params)) {
- total_amount += res->total_amount;
+ total_amount += res->GetTotalAmount();
}
}
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 30edd62a..675cf623 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -266,10 +266,10 @@ static OutputType GetOutputType(TxoutType type, bool is_from_p2sh)
// Fetch and validate the coin control selected inputs.
// Coins could be internal (from the wallet) or external.
-util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control,
+util::Result<CoinsResult> FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control,
const CoinSelectionParams& coin_selection_params)
{
- PreSelectedInputs result;
+ CoinsResult result;
const bool can_grind_r = wallet.CanGrindR();
std::map<COutPoint, CAmount> map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate);
for (const COutPoint& outpoint : coin_control.ListSelected()) {
@@ -312,7 +312,7 @@ util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const
/* Set some defaults for depth, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */
COutput output(outpoint, txout, /*depth=*/0, input_bytes, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, coin_selection_params.m_effective_feerate);
output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint));
- result.Insert(output, coin_selection_params.m_subtract_fee_outputs);
+ result.Add(OutputType::UNKNOWN, output);
}
return result;
}
@@ -812,12 +812,12 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
return *std::min_element(results.begin(), results.end());
}
-util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs,
+util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CoinsResult& pre_set_inputs,
const CAmount& nTargetValue, const CCoinControl& coin_control,
const CoinSelectionParams& coin_selection_params)
{
// Deduct preset inputs amount from the search target
- CAmount selection_target = nTargetValue - pre_set_inputs.total_amount;
+ CAmount selection_target = nTargetValue - pre_set_inputs.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs).value_or(0);
// Return if automatic coin selection is disabled, and we don't cover the selection target
if (!coin_control.m_allow_other_inputs && selection_target > 0) {
@@ -825,18 +825,22 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
"Please allow other inputs to be automatically selected or include more coins manually")};
}
+ OutputSet preset_coin_set;
+ for (const auto& output: pre_set_inputs.All()) {
+ preset_coin_set.insert(std::make_shared<COutput>(output));
+ }
+
// Return if we can cover the target only with the preset inputs
if (selection_target <= 0) {
SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);
- result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
+ result.AddInputs(preset_coin_set, coin_selection_params.m_subtract_fee_outputs);
result.RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
return result;
}
// Return early if we cannot cover the target with the wallet's UTXO.
// We use the total effective value if we are not subtracting fee from outputs and 'available_coins' contains the data.
- CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() :
- (available_coins.GetEffectiveTotalAmount().has_value() ? *available_coins.GetEffectiveTotalAmount() : 0);
+ CAmount available_coins_total_amount = available_coins.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs).value_or(0);
if (selection_target > available_coins_total_amount) {
return util::Error(); // Insufficient funds
}
@@ -847,8 +851,10 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
// If needed, add preset inputs to the automatic coin selection result
if (!pre_set_inputs.coins.empty()) {
- SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL);
- preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
+ auto preset_total = pre_set_inputs.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs);
+ assert(preset_total.has_value());
+ SelectionResult preselected(preset_total.value(), SelectionAlgorithm::MANUAL);
+ preselected.AddInputs(preset_coin_set, coin_selection_params.m_subtract_fee_outputs);
op_selection_result->Merge(preselected);
op_selection_result->RecalculateWaste(coin_selection_params.min_viable_change,
coin_selection_params.m_cost_of_change,
@@ -1183,7 +1189,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
}
// Fetch manually selected coins
- PreSelectedInputs preset_inputs;
+ CoinsResult preset_inputs;
if (coin_control.HasSelected()) {
auto res_fetch_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params);
if (!res_fetch_inputs) return util::Error{util::ErrorString(res_fetch_inputs)};
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
index c1657087..5a6ad246 100644
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@ -180,7 +180,7 @@ struct PreSelectedInputs
* Fetch and validate coin control selected inputs.
* Coins could be internal (from the wallet) or external.
*/
-util::Result<PreSelectedInputs> FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control,
+util::Result<CoinsResult> FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control,
const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
/**
@@ -202,7 +202,7 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
* Select all coins from coin_control, and if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to
* select a set of coins such that nTargetValue - pre_set_inputs.total_amount is met.
*/
-util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs,
+util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CoinsResult& pre_set_inputs,
const CAmount& nTargetValue, const CCoinControl& coin_control,
const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index b609ce82..f113532b 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -222,8 +222,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
coin_control.m_allow_other_inputs = true;
COutput select_coin = available_coins.All().at(0);
coin_control.Select(select_coin.outpoint);
- PreSelectedInputs selected_input;
- selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs);
+ CoinsResult selected_input;
+ selected_input.Add(OutputType::BECH32, select_coin);
available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint});
LOCK(wallet->cs_wallet);
@@ -253,8 +253,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
coin_control.m_allow_other_inputs = true;
COutput select_coin = available_coins.All().at(1); // pre select 9 coin
coin_control.Select(select_coin.outpoint);
- PreSelectedInputs selected_input;
- selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs);
+ CoinsResult selected_input;
+ selected_input.Add(OutputType::BECH32, select_coin);
available_coins.Erase({(++available_coins.coins[OutputType::BECH32].begin())->outpoint});
const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb);
BOOST_CHECK(EquivalentResult(expected_result, *result13));
Risk score
Our methodology →Why this scored 27/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.