From ab80f3a8a199f2bcd22bc8fa7c62b566ec4be9f1 Mon Sep 17 00:00:00 2001 From: Murch Date: Fri, 29 Jul 2022 13:42:52 -0400 Subject: [PATCH] wallet: Prefer smaller change amount in tiebreaker Requires #25647 --- src/wallet/coinselection.cpp | 30 ++++++++++++++++++++---------- src/wallet/coinselection.h | 14 ++++++++++++-- src/wallet/spend.cpp | 2 +- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index d8fe4c445f70a4..84a9ed61c16002 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -408,9 +408,9 @@ CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng) void SelectionResult::ComputeAndSetWaste(const CAmount min_change, const CAmount change_cost, const CAmount change_fee) { - const CAmount change = GetChange(min_change, change_fee); + m_change = ComputeAndSetChange(min_change, change_fee); - if (change > 0) { + if (m_change > 0) { m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective); } else { m_waste = GetSelectionWaste(m_selected_inputs, 0, m_target, m_use_effective); @@ -470,8 +470,16 @@ bool SelectionResult::operator<(SelectionResult other) const { Assert(m_waste.has_value()); Assert(other.m_waste.has_value()); - // As this operator is only used in std::min_element, we want the result that has more inputs when waste are equal. - return *m_waste < *other.m_waste || (*m_waste == *other.m_waste && m_selected_inputs.size() > other.m_selected_inputs.size()); + // As this operator is only used in std::min_element, we want the result that has more inputs when waste are equal, and other things equal we want to reveal fewer funds to the counter party as well as unconfirm less liquidity + if (*m_waste != *other.m_waste) { + return *m_waste < *other.m_waste; + } else if (m_selected_inputs.size() != other.m_selected_inputs.size()) { + return (m_selected_inputs.size() > other.m_selected_inputs.size()); + } else { + Assert(m_change.has_value()); + Assert(other.m_change.has_value()); + return m_change < other.m_change; + } } std::string COutput::ToString() const @@ -492,7 +500,12 @@ std::string GetAlgorithmName(const SelectionAlgorithm algo) assert(false); } -CAmount SelectionResult::GetChange(const CAmount min_change, const CAmount change_fee) const +CAmount SelectionResult::GetChange() const +{ + return *Assert(m_change); +} + +CAmount SelectionResult::ComputeAndSetChange(const CAmount min_change, const CAmount change_fee) { // change_budget = SUM(inputs) - SUM(outputs) - fees // 1) With SFFO we don't pay any fees @@ -504,11 +517,8 @@ CAmount SelectionResult::GetChange(const CAmount min_change, const CAmount chang ? GetSelectedEffectiveValue() - m_target - change_fee : GetSelectedValue() - m_target; - if (change_budget < min_change) { - return 0; - } - - return change_budget; + m_change = (change_budget < min_change) ? 0 : change_budget; + return *m_change; } } // namespace wallet diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 40c24f3250824e..0fcfecd9037543 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -287,6 +287,8 @@ struct SelectionResult std::set m_selected_inputs; /** The target the algorithm selected for. Equal to the recipient amount plus non-input fees */ CAmount m_target; + /** The computed change for this selection result under the selection attempt's circumstances */ + std::optional m_change; /** The algorithm used to produce this result */ SelectionAlgorithm m_algo; /** Whether the input values for calculations should be the effective value (true) or normal value (false) */ @@ -320,9 +322,17 @@ struct SelectionResult /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */ std::vector GetShuffledInputVector() const; + /** Sorting order for SelectionResults + * 1) Prefer lower waste, + * 2) at equal waste prefer higher input count, + * 3) at equal waste and input count prefer lower change + */ bool operator<(SelectionResult other) const; - /** Get the amount for the change output after paying needed fees. + /** Get m_change */ + CAmount GetChange() const; + + /** Calculate the amount for the change output after paying needed fees. * * The change amount is not 100% precise due to discrepancies in fee calculation. * The final change amount (if any) should be corrected after calculating the final tx fees. @@ -339,7 +349,7 @@ struct SelectionResult * @returns Amount for change output, 0 when there is no change. * */ - CAmount GetChange(const CAmount min_change, const CAmount change_fee) const; + CAmount ComputeAndSetChange(const CAmount min_change, const CAmount change_fee); CAmount GetTarget() const { return m_target; } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 6702645c7829f6..ef2c27e30dc411 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -792,7 +792,7 @@ static BResult CreateTransactionInternal( } TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->GetAlgo()).c_str(), result->GetTarget(), result->GetWaste(), result->GetSelectedValue()); - const CAmount change_amount = result->GetChange(coin_selection_params.min_change, coin_selection_params.m_change_fee); + const CAmount change_amount = result->ComputeAndSetChange(coin_selection_params.min_change, coin_selection_params.m_change_fee); if (change_amount > 0) { CTxOut newTxOut(change_amount, scriptChange); if (nChangePosInOut == -1)