diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index 376e70ffba..80acdec044 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -49,7 +49,10 @@ static void CoinSelection(benchmark::Bench& bench) } const CoinEligibilityFilter filter_standard(1, 6, 0); - const CoinSelectionParams coin_selection_params(true, 34, 148, CFeeRate(0), 0, false); + const CoinSelectionParams coin_selection_params(/* use_bnb= */ true, /* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); bench.run([&] { std::set setCoinsRet; CAmount nValueRet; diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 0e9f3819eb..7eff6e592d 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -35,7 +35,10 @@ static CAmount balance = 0; CoinEligibilityFilter filter_standard(1, 6, 0); CoinEligibilityFilter filter_confirmed(1, 1, 0); CoinEligibilityFilter filter_standard_extra(6, 6, 0); -CoinSelectionParams coin_selection_params(false, 0, 0, CFeeRate(0), 0, false); +CoinSelectionParams coin_selection_params(/* use_bnb= */ false, /* change_output_size= */ 0, + /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); static void add_coin(const CAmount& nValue, int nInput, std::vector& set) { @@ -269,7 +272,10 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) } // Make sure that effective value is working in SelectCoinsMinConf when BnB is used - CoinSelectionParams coin_selection_params_bnb(true, 0, 0, CFeeRate(3000), 0, false); + CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 0, + /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000), + /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); CoinSet setCoinsRet; CAmount nValueRet; bool bnb_used; @@ -301,7 +307,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) CCoinControl coin_control; coin_control.fAllowOtherInputs = true; coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i)); - coin_selection_params_bnb.effective_fee = CFeeRate(0); + coin_selection_params_bnb.m_effective_feerate = CFeeRate(0); BOOST_CHECK(wallet->SelectCoins(vCoins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used)); BOOST_CHECK(bnb_used); BOOST_CHECK(coin_selection_params_bnb.use_bnb); @@ -639,8 +645,14 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test) CAmount target = rand.randrange(balance - 1000) + 1000; // Perform selection - CoinSelectionParams coin_selection_params_knapsack(false, 34, 148, CFeeRate(0), 0, false); - CoinSelectionParams coin_selection_params_bnb(true, 34, 148, CFeeRate(0), 0, false); + CoinSelectionParams coin_selection_params_knapsack(/* use_bnb= */ false, /* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); + CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 34, + /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0), + /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0), + /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false); CoinSet out_set; CAmount out_value = 0; bool bnb_used = false; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ac1b2a4c0a..2600968cad 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2399,26 +2399,20 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil nValueRet = 0; if (coin_selection_params.use_bnb) { - // Get long term estimate - FeeCalculation feeCalc; - CCoinControl temp; - temp.m_confirm_target = 1008; - CFeeRate long_term_feerate = GetMinimumFeeRate(*this, temp, &feeCalc); - // 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}; if (!coin_selection_params.m_subtract_fee_outputs) { - effective_feerate = coin_selection_params.effective_fee; + effective_feerate = coin_selection_params.m_effective_feerate; } - std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, long_term_feerate, eligibility_filter, true /* positive_only */); + std::vector groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, coin_selection_params.m_long_term_feerate, eligibility_filter, true /* positive_only */); // Calculate cost of change - CAmount cost_of_change = GetDiscardRate(*this).GetFee(coin_selection_params.change_spend_size) + coin_selection_params.effective_fee.GetFee(coin_selection_params.change_output_size); + CAmount cost_of_change = coin_selection_params.m_discard_feerate.GetFee(coin_selection_params.change_spend_size) + coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); // Calculate the fees for things that aren't inputs - CAmount not_input_fees = coin_selection_params.effective_fee.GetFee(coin_selection_params.tx_noinputs_size); + CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size); bnb_used = true; return SelectCoinsBnB(groups, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); } else { @@ -2472,7 +2466,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (coin.m_input_bytes <= 0) { return false; // Not solvable, can't estimate size for fee } - coin.effective_value = coin.txout.nValue - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes); + coin.effective_value = coin.txout.nValue - coin_selection_params.m_effective_feerate.GetFee(coin.m_input_bytes); if (coin_selection_params.use_bnb) { value_to_select -= coin.effective_value; } else { @@ -2840,17 +2834,28 @@ bool CWallet::CreateTransactionInternal( CTxOut change_prototype_txout(0, scriptChange); coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout); - CFeeRate discard_rate = GetDiscardRate(*this); + // Set discard feerate + coin_selection_params.m_discard_feerate = GetDiscardRate(*this); // Get the fee rate to use effective values in coin selection - CFeeRate nFeeRateNeeded = GetMinimumFeeRate(*this, coin_control, &feeCalc); + coin_selection_params.m_effective_feerate = GetMinimumFeeRate(*this, coin_control, &feeCalc); // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly // provided one - if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) { - error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), nFeeRateNeeded.ToString(FeeEstimateMode::SAT_VB)); + if (coin_control.m_feerate && coin_selection_params.m_effective_feerate > *coin_control.m_feerate) { + error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::SAT_VB), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::SAT_VB)); + return false; + } + if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { + // eventually allow a fallback fee + error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); return false; } + // Get long term estimate + CCoinControl cc_temp; + cc_temp.m_confirm_target = chain().estimateMaxBlocks(); + coin_selection_params.m_long_term_feerate = GetMinimumFeeRate(*this, cc_temp, nullptr); + nFeeRet = 0; bool pick_new_inputs = true; CAmount nValueIn = 0; @@ -2924,7 +2929,6 @@ bool CWallet::CreateTransactionInternal( } else { coin_selection_params.change_spend_size = (size_t)change_spend_size; } - coin_selection_params.effective_fee = nFeeRateNeeded; if (!SelectCoins(vAvailableCoins, 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. @@ -2950,7 +2954,7 @@ bool CWallet::CreateTransactionInternal( // Never create dust outputs; if we would, just // add the dust to the fee. // The nChange when BnB is used is always going to go to fees. - if (IsDust(newTxOut, discard_rate) || bnb_used) + if (IsDust(newTxOut, coin_selection_params.m_discard_feerate) || bnb_used) { nChangePosInOut = -1; nFeeRet += nChange; @@ -2988,13 +2992,7 @@ bool CWallet::CreateTransactionInternal( return false; } - nFeeNeeded = GetMinimumFee(*this, nBytes, coin_control, &feeCalc); - if (feeCalc.reason == FeeReason::FALLBACK && !m_allow_fallback_fee) { - // eventually allow a fallback fee - error = _("Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee."); - return false; - } - + nFeeNeeded = coin_selection_params.m_effective_feerate.GetFee(nBytes); if (nFeeRet >= nFeeNeeded) { // Reduce fee to only the needed amount if possible. This // prevents potential overpayment in fees if the coins @@ -3008,8 +3006,8 @@ bool CWallet::CreateTransactionInternal( // change output. Only try this once. if (nChangePosInOut == -1 && nSubtractFeeFromAmount == 0 && pick_new_inputs) { unsigned int tx_size_with_change = nBytes + coin_selection_params.change_output_size + 2; // Add 2 as a buffer in case increasing # of outputs changes compact size - CAmount fee_needed_with_change = GetMinimumFee(*this, tx_size_with_change, coin_control, nullptr); - CAmount minimum_value_for_change = GetDustThreshold(change_prototype_txout, discard_rate); + CAmount fee_needed_with_change = coin_selection_params.m_effective_feerate.GetFee(tx_size_with_change); + CAmount minimum_value_for_change = GetDustThreshold(change_prototype_txout, coin_selection_params.m_discard_feerate); if (nFeeRet >= fee_needed_with_change + minimum_value_for_change) { pick_new_inputs = false; nFeeRet = fee_needed_with_change; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 3a76163dd2..f03afd5066 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -608,17 +608,22 @@ struct CoinSelectionParams bool use_bnb = true; size_t change_output_size = 0; size_t change_spend_size = 0; - CFeeRate effective_fee = CFeeRate(0); + CFeeRate m_effective_feerate; + CFeeRate m_long_term_feerate; + CFeeRate m_discard_feerate; size_t tx_noinputs_size = 0; //! Indicate that we are subtracting the fee from outputs bool m_subtract_fee_outputs = false; bool m_avoid_partial_spends = false; - CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size, bool avoid_partial) : + CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate, + CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) : use_bnb(use_bnb), change_output_size(change_output_size), change_spend_size(change_spend_size), - effective_fee(effective_fee), + m_effective_feerate(effective_feerate), + m_long_term_feerate(long_term_feerate), + m_discard_feerate(discard_feerate), tx_noinputs_size(tx_noinputs_size), m_avoid_partial_spends(avoid_partial) {}