diff options
| -rw-r--r-- | src/bench/coin_selection.cpp | 3 | ||||
| -rw-r--r-- | src/wallet/coinselection.cpp | 9 | ||||
| -rw-r--r-- | src/wallet/coinselection.h | 2 | ||||
| -rw-r--r-- | src/wallet/test/coinselector_tests.cpp | 29 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 26 |
5 files changed, 35 insertions, 34 deletions
diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 80acdec044..b42939ffdc 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -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(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 2228536f29..db770f6a45 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -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; diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 5645e6db46..af23178d04 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -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); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 7eff6e592d..1a2454479d 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -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 @@ -166,7 +165,7 @@ 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(); @@ -174,7 +173,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) // 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(); @@ -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(); @@ -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(); @@ -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(); @@ -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)); @@ -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 diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 51ff683c2e..3383b3e96b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -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}; @@ -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); } } @@ -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) @@ -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) { |