Skip to content

Commit

Permalink
Fold GetSelectionWaste() into ComputeAndSetWaste()
Browse files Browse the repository at this point in the history 
Both `GetSelectionWaste()` and `ComputeAndSetWaste()` now are part of
`SelectionResult`. Instead of `ComputeAndSetWaste()` being a wrapper for
`GetSelectionWaste()`, we combine them to a new function
`RecalculateWaste()`.

As I was combining the logic of the two functions, I noticed that
`GetSelectionWaste()` was making the odd assumption that the
`change_cost` being set to zero means that no change is created.
However, if we build transactions at a feerate of zero with the
`discard_feerate` also set to zero, we'd organically have a
`change_cost` of zero, even when we create change on a transaction.

This commit cleans up this duplicate meaning of `change_cost` and relies
on `GetChange()` to figure out whether there is change on basis of the
`min_viable_change` and whatever is left after deducting fees.

Since this broke a bunch of tests that relied on the double-meaning of
`change_cost` a bunch of tests had to be fixed.
  • Loading branch information
@murchandamus
murchandamus committed May 24, 2024
1 parent d186c64 commit e1ab2fd
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 96 deletions.
8 changes: 4 additions & 4 deletions src/bench/coin_selection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,15 @@ static void CoinSelection(benchmark::Bench& bench)
/*change_output_size=*/ 34,
/*change_spend_size=*/ 148,
/*min_change_target=*/ CHANGE_LOWER,
/*effective_feerate=*/ CFeeRate(0),
/*long_term_feerate=*/ CFeeRate(0),
/*discard_feerate=*/ CFeeRate(0),
/*effective_feerate=*/ CFeeRate(20'000),
/*long_term_feerate=*/ CFeeRate(10'000),
/*discard_feerate=*/ CFeeRate(3000),
/*tx_noinputs_size=*/ 0,
/*avoid_partial=*/ false,
};
auto group = wallet::GroupOutputs(wallet, available_coins, coin_selection_params, {{filter_standard}})[filter_standard];
bench.run([&] {
auto result = AttemptSelection(wallet.chain(), 1003 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true);
auto result = AttemptSelection(wallet.chain(), 1002.99 * COIN, group, coin_selection_params, /*allow_mixed_output_types=*/true);
assert(result);
assert(result->GetSelectedValue() == 1003 * COIN);
assert(result->GetInputSet().size() == 2);
Expand Down
60 changes: 26 additions & 34 deletions src/wallet/coinselection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool
for (const size_t& i : best_selection) {
result.AddInput(utxo_pool.at(i));
}
result.ComputeAndSetWaste(cost_of_change, cost_of_change, CAmount{0});
result.RecalculateWaste(cost_of_change, cost_of_change, CAmount{0});
assert(best_waste == result.GetWaste());

return result;
Expand Down Expand Up @@ -792,35 +792,6 @@ void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool in
}
}

CAmount SelectionResult::GetSelectionWaste(CAmount change_cost, CAmount target, bool use_effective_value)
{
// This function should not be called with empty inputs as that would mean the selection failed
assert(!m_selected_inputs.empty());

// Always consider the cost of spending an input now vs in the future.
CAmount waste = 0;
for (const auto& coin_ptr : m_selected_inputs) {
const COutput& coin = *coin_ptr;
waste += coin.GetFee() - coin.long_term_fee;
}
// Bump fee of whole selection may diverge from sum of individual bump fees
waste -= bump_fee_group_discount;

if (change_cost) {
// Consider the cost of making change and spending it in the future
// If we aren't making change, the caller should've set change_cost to 0
assert(change_cost > 0);
waste += change_cost;
} else {
// When we are not making change (change_cost == 0), consider the excess we are throwing away to fees
CAmount selected_effective_value = use_effective_value ? GetSelectedEffectiveValue() : GetSelectedValue();
assert(selected_effective_value >= target);
waste += selected_effective_value - target;
}

return waste;
}

CAmount GenerateChangeTarget(const CAmount payment_value, const CAmount change_fee, FastRandomContext& rng)
{
if (payment_value <= CHANGE_LOWER / 2) {
Expand All @@ -839,11 +810,32 @@ void SelectionResult::SetBumpFeeDiscount(const CAmount discount)
bump_fee_group_discount = discount;
}


void SelectionResult::ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee)
void SelectionResult::RecalculateWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee)
{
bool makes_change = {GetChange(min_viable_change, change_fee) != 0};
m_waste = GetSelectionWaste(makes_change ? change_cost : 0, m_target, m_use_effective);
// This function should not be called with empty inputs as that would mean the selection failed
assert(!m_selected_inputs.empty());

// Always consider the cost of spending an input now vs in the future.
CAmount waste = 0;
for (const auto& coin_ptr : m_selected_inputs) {
const COutput& coin = *coin_ptr;
waste += coin.GetFee() - coin.long_term_fee;
}
// Bump fee of whole selection may diverge from sum of individual bump fees
waste -= bump_fee_group_discount;

if (GetChange(min_viable_change, change_fee)) {
// if we have a minimum viable amount after deducting fees, account for
// cost of creating and spending change
waste += change_cost;
} else {
// When we are not making change (GetChange(…) == 0), consider the excess we are throwing away to fees
CAmount selected_effective_value = m_use_effective ? GetSelectedEffectiveValue() : GetSelectedValue();
assert(selected_effective_value >= m_target);
waste += selected_effective_value - m_target;
}

m_waste = waste;
}

void SelectionResult::SetAlgoCompleted(bool algo_completed)
Expand Down
31 changes: 13 additions & 18 deletions src/wallet/coinselection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,22 +350,6 @@ struct SelectionResult
}
}

/** Compute the waste for this result given the cost of change
* and the opportunity cost of spending these inputs now vs in the future.
* If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate)
* If no change, waste = excess + inputs * (effective_feerate - long_term_feerate)
* where excess = selected_effective_value - target
* change_cost = effective_feerate * change_output_size + long_term_feerate * change_spend_size
*
* @param[in] change_cost The cost of creating change and spending it in the future.
* Only used if there is change, in which case it must be positive.
* Must be 0 if there is no change.
* @param[in] target The amount targeted by the coin selection algorithm.
* @param[in] use_effective_value Whether to use the input's effective value (when true) or the real value (when false).
* @return The waste
*/
[[nodiscard]] CAmount GetSelectionWaste(CAmount change_cost, CAmount target, bool use_effective_value = true);

public:
explicit SelectionResult(const CAmount target, SelectionAlgorithm algo)
: m_target(target), m_algo(algo) {}
Expand All @@ -387,8 +371,19 @@ struct SelectionResult
/** How much individual inputs overestimated the bump fees for shared ancestries */
void SetBumpFeeDiscount(const CAmount discount);

/** Calculates and stores the waste for this selection via GetSelectionWaste */
void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee);
/** Calculates and stores the waste for this result given the cost of change
* and the opportunity cost of spending these inputs now vs in the future.
* If change exists, waste = change_cost + inputs * (effective_feerate - long_term_feerate) - bump_fee_group_discount
* If no change, waste = excess + inputs * (effective_feerate - long_term_feerate) - bump_fee_group_discount
* where excess = selected_effective_value - target
* change_cost = effective_feerate * change_output_size + long_term_feerate * change_spend_size
*
* @param[in] min_viable_change The minimum amount necessary to make a change output economic
* @param[in] change_cost The cost of creating a change output and spending it in the future. Only
* used if there is change, in which case it must be non-negative.
* @param[in] change_fee The fee for creating a change output
*/
void RecalculateWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee);
[[nodiscard]] CAmount GetWaste() const;

/** Tracks that algorithm was able to exhaustively search the entire combination space before hitting limit of tries */
Expand Down
8 changes: 4 additions & 4 deletions src/wallet/spend.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co

if (coin_selection_params.m_effective_feerate > CFeeRate{3 * coin_selection_params.m_long_term_feerate}) { // Minimize input set for feerates of at least 3×LTFRE (default: 30 ṩ/vB+)
if (auto cg_result{CoinGrinder(groups.positive_group, nTargetValue, coin_selection_params.m_min_change_target, max_inputs_weight)}) {
cg_result->ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
cg_result->RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
results.push_back(*cg_result);
} else {
append_error(std::move(cg_result));
Expand Down Expand Up @@ -746,7 +746,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
if (bump_fee_overestimate) {
result.SetBumpFeeDiscount(bump_fee_overestimate);
}
result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
result.RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
}

// Choose the result with the least waste
Expand All @@ -771,7 +771,7 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
if (selection_target <= 0) {
SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);
result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
result.ComputeAndSetWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
result.RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee);
return result;
}

Expand All @@ -792,7 +792,7 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av
SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL);
preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs);
op_selection_result->Merge(preselected);
op_selection_result->ComputeAndSetWaste(coin_selection_params.min_viable_change,
op_selection_result->RecalculateWaste(coin_selection_params.min_viable_change,
coin_selection_params.m_cost_of_change,
coin_selection_params.m_change_fee);
}
Expand Down
Loading

0 comments on commit e1ab2fd

Please sign in to comment.