Skip to content

Commit

Permalink
Roll static tx fees into nValueToSelect instead of having it be separate
Browse files Browse the repository at this point in the history 
The fees for transaction overhead and recipient outputs are now included
in nTargetValue instead of being a separate parameter. For the coin
selection algorithms, it doesn't matter that these are separate as in
either case, the algorithm needs to select enough to cover these fees.

Note that setting nValueToSelect is changed as it now includes
not_input_fees. Without the change to how nValueToSelect is increased
for KnapsackSolver, this would result in overpaying fees. The change to
increase by the difference between nFeeRet and not_input_fees allows
this to have the same behavior as previously.

Additionally, because we assume that KnapsackSolver will always find a
solution that requires change (we assume that BnB always finds a
non-change solution), we also include the fee for the change output in
KnapsackSolver's target. As part of this, we also use the changeless
nFeeRet when iterating for KnapsackSolver. This is because we include
the change fee when doing KnapsackSolver, so nFeeRet on further
iterations won't include the change fee.
  • Loading branch information
@achow101
achow101 committed May 19, 2021
1 parent cc3f14b commit bf26e01
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 34 deletions.
3 changes: 1 addition & 2 deletions src/bench/coin_selection.cpp
View file
Expand Up @@ -101,12 +101,11 @@ static void BnBExhaustion(benchmark::Bench& bench)
std::vector<OutputGroup> utxo_pool;
CoinSet selection;
CAmount value_ret = 0;
CAmount not_input_fees = 0;

bench.run([&] {
// Benchmark
CAmount target = make_hard_case(17, utxo_pool);
SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret, not_input_fees); // Should exhaust
SelectCoinsBnB(utxo_pool, target, 0, selection, value_ret); // Should exhaust

// Cleanup
utxo_pool.clear();
Expand Down
9 changes: 3 additions & 6 deletions src/wallet/coinselection.cpp
View file
Expand Up @@ -49,28 +49,25 @@ struct {
* @param const std::vector<CInputCoin>& utxo_pool The set of UTXOs that we are choosing from.
* These UTXOs will be sorted in descending order by effective value and the CInputCoins'
* values are their effective values.
* @param const CAmount& target_value This is the value that we want to select. It is the lower
* @param const CAmount& selection_target This is the value that we want to select. It is the lower
* bound of the range.
* @param const CAmount& cost_of_change This is the cost of creating and spending a change output.
* This plus target_value is the upper bound of the range.
* This plus selection_target is the upper bound of the range.
* @param std::set<CInputCoin>& out_set -> This is an output parameter for the set of CInputCoins
* that have been selected.
* @param CAmount& value_ret -> This is an output parameter for the total value of the CInputCoins
* that were selected.
* @param CAmount not_input_fees -> The fees that need to be paid for the outputs and fixed size
* overhead (version, locktime, marker and flag)
*/

static const size_t TOTAL_TRIES = 100000;

bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount not_input_fees)
bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret)
{
out_set.clear();
CAmount curr_value = 0;

std::vector<bool> curr_selection; // select the utxo at this index
curr_selection.reserve(utxo_pool.size());
CAmount selection_target = not_input_fees + target_value;

// Calculate curr_available_value
CAmount curr_available_value = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/coinselection.h
View file
Expand Up @@ -120,7 +120,7 @@ struct OutputGroup
bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
};

bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_value, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret, CAmount not_input_fees);
bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change, std::set<CInputCoin>& out_set, CAmount& value_ret);

// Original coin selection algorithm as a fallback
bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& groups, std::set<CInputCoin>& setCoinsRet, CAmount& nValueRet);
Expand Down
29 changes: 14 additions & 15 deletions src/wallet/test/coinselector_tests.cpp
View file
Expand Up @@ -148,14 +148,13 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
CoinSet selection;
CoinSet actual_selection;
CAmount value_ret = 0;
CAmount not_input_fees = 0;

/////////////////////////
// Known Outcome tests //
/////////////////////////

// Empty utxo pool
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret));
selection.clear();

// Add utxos
Expand All @@ -166,15 +165,15 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)

// Select 1 Cent
add_coin(1 * CENT, 1, actual_selection);
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 0.5 * CENT, selection, value_ret));
BOOST_CHECK(equal_sets(selection, actual_selection));
BOOST_CHECK_EQUAL(value_ret, 1 * CENT);
actual_selection.clear();
selection.clear();

// Select 2 Cent
add_coin(2 * CENT, 2, actual_selection);
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 2 * CENT, 0.5 * CENT, selection, value_ret));
BOOST_CHECK(equal_sets(selection, actual_selection));
BOOST_CHECK_EQUAL(value_ret, 2 * CENT);
actual_selection.clear();
Expand All @@ -183,27 +182,27 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
// Select 5 Cent
add_coin(4 * CENT, 4, actual_selection);
add_coin(1 * CENT, 1, actual_selection);
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret));
BOOST_CHECK(equal_sets(selection, actual_selection));
BOOST_CHECK_EQUAL(value_ret, 5 * CENT);
actual_selection.clear();
selection.clear();

// Select 11 Cent, not possible
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 11 * CENT, 0.5 * CENT, selection, value_ret));
actual_selection.clear();
selection.clear();

// Cost of change is greater than the difference between target value and utxo sum
add_coin(1 * CENT, 1, actual_selection);
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0.5 * CENT, selection, value_ret));
BOOST_CHECK_EQUAL(value_ret, 1 * CENT);
BOOST_CHECK(equal_sets(selection, actual_selection));
actual_selection.clear();
selection.clear();

// Cost of change is less than the difference between target value and utxo sum
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0, selection, value_ret, not_input_fees));
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.9 * CENT, 0, selection, value_ret));
actual_selection.clear();
selection.clear();

Expand All @@ -212,7 +211,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
add_coin(5 * CENT, 5, actual_selection);
add_coin(4 * CENT, 4, actual_selection);
add_coin(1 * CENT, 1, actual_selection);
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 0.5 * CENT, selection, value_ret));
BOOST_CHECK(equal_sets(selection, actual_selection));
BOOST_CHECK_EQUAL(value_ret, 10 * CENT);
actual_selection.clear();
Expand All @@ -223,21 +222,21 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
add_coin(5 * CENT, 5, actual_selection);
add_coin(3 * CENT, 3, actual_selection);
add_coin(2 * CENT, 2, actual_selection);
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000, selection, value_ret, not_input_fees));
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 10 * CENT, 5000, selection, value_ret));
BOOST_CHECK_EQUAL(value_ret, 10 * CENT);
// FIXME: this test is redundant with the above, because 1 Cent is selected, not "too small"
// BOOST_CHECK(equal_sets(selection, actual_selection));

// Select 0.25 Cent, not possible
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 0.25 * CENT, 0.5 * CENT, selection, value_ret));
actual_selection.clear();
selection.clear();

// Iteration exhaustion test
CAmount target = make_hard_case(17, utxo_pool);
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret, not_input_fees)); // Should exhaust
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should exhaust
target = make_hard_case(14, utxo_pool);
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret, not_input_fees)); // Should not exhaust
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret)); // Should not exhaust

// Test same value early bailout optimization
utxo_pool.clear();
Expand All @@ -254,7 +253,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
for (int i = 0; i < 50000; ++i) {
add_coin(5 * CENT, 7, utxo_pool);
}
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000, selection, value_ret, not_input_fees));
BOOST_CHECK(SelectCoinsBnB(GroupCoins(utxo_pool), 30 * CENT, 5000, selection, value_ret));
BOOST_CHECK_EQUAL(value_ret, 30 * CENT);
BOOST_CHECK(equal_sets(selection, actual_selection));

Expand All @@ -268,7 +267,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
}
// Run 100 times, to make sure it is never finding a solution
for (int i = 0; i < 100; ++i) {
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret, not_input_fees));
BOOST_CHECK(!SelectCoinsBnB(GroupCoins(utxo_pool), 1 * CENT, 2 * CENT, selection, value_ret));
}

// Make sure that effective value is working in SelectCoinsMinConf when BnB is used
Expand Down
26 changes: 16 additions & 10 deletions src/wallet/wallet.cpp
View file
Expand Up @@ -2399,9 +2399,6 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
setCoinsRet.clear();
nValueRet = 0;

// Calculate the fees for things that aren't inputs, excluding the change output
const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);

// Get the feerate for effective value.
// When subtracting the fee from the outputs, we want the effective feerate to be 0
CFeeRate effective_feerate{0};
Expand All @@ -2412,14 +2409,17 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
if (coin_selection_params.use_bnb) {
std::vector<OutputGroup> positive_groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */);
bnb_used = true;
return SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet, not_input_fees);
// Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output.
return SelectCoinsBnB(positive_groups, nTargetValue, coin_selection_params.m_cost_of_change, setCoinsRet, nValueRet);
} else {
// 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.
// The knapsack solver currently does not use effective values, so we give GroupOutputs feerates of 0 so it sets the effective values to be the same as the real value.
std::vector<OutputGroup> groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, CFeeRate(0), CFeeRate(0), eligibility_filter, false /* positive_only */);

bnb_used = false;
return KnapsackSolver(nTargetValue, groups, setCoinsRet, nValueRet);
// While nTargetValue includes the transaction fees for non-input things, it does not include the fee for creating a change output.
// So we need to include that for KnapsackSolver as well, as we are expecting to create a change output.
return KnapsackSolver(nTargetValue + coin_selection_params.m_change_fee, groups, setCoinsRet, nValueRet);
}
}

Expand Down Expand Up @@ -2931,10 +2931,6 @@ bool CWallet::CreateTransactionInternal(
txNew.vin.clear();
txNew.vout.clear();

CAmount nValueToSelect = nValue;
if (nSubtractFeeFromAmount == 0)
nValueToSelect += nFeeRet;

// vouts to the payees
if (!coin_selection_params.m_subtract_fee_outputs) {
coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size)
Expand All @@ -2956,11 +2952,21 @@ bool CWallet::CreateTransactionInternal(
txNew.vout.push_back(txout);
}

// Include the fees for things that aren't inputs, excluding the change output
const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
CAmount nValueToSelect = nValue + not_input_fees;

// For KnapsackSolver, when we are not subtracting the fee from the recipients, we also want to include the fees for the
// inputs that we found in the previous iteration.
if (!coin_selection_params.use_bnb && nSubtractFeeFromAmount == 0) {
nValueToSelect += std::max(CAmount(0), nFeeRet - not_input_fees);
}

// Choose coins to use
bool bnb_used = false;
nValueIn = 0;
setCoins.clear();
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used))
if (!SelectCoins(vAvailableCoins, /* nTargetValue */ nValueToSelect, setCoins, nValueIn, coin_control, coin_selection_params, bnb_used))
{
// If BnB was used, it was the first pass. No longer the first pass and continue loop with knapsack.
if (bnb_used) {
Expand Down

0 comments on commit bf26e01

Please sign in to comment.