Skip to content

Commit

Permalink
Merge bitcoin#21083: wallet: Avoid requesting fee rates multiple time…
Browse files Browse the repository at this point in the history 
…s during coin selection

f9cd2bf Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c29 wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0 wallet: Use existing feerate instead of getting a new one (Andrew Chow)

Pull request description:

  During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.

  Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.

  While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.

  Fixes bitcoin#19229

ACKs for top commit:
  glozow:
    reACK bitcoin@f9cd2bf
  fjahr:
    Code review re-ACK f9cd2bf
  Xekyo:
    tACK bitcoin@f9cd2bf
  meshcollider:
    Code review + test run ACK f9cd2bf

Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
  • Loading branch information
@meshcollider @vijaydasmp
meshcollider authored and vijaydasmp committed Sep 6, 2024
1 parent 63b63aa commit 130d781
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 34 deletions.
5 changes: 4 additions & 1 deletion src/bench/coin_selection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ static void CoinSelection(benchmark::Bench& bench)
coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */);
}
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<CInputCoin> setCoinsRet;
CAmount nValueRet;
Expand Down
30 changes: 17 additions & 13 deletions src/wallet/test/coinselector_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ static const CoinEligibilityFilter filter_standard(1, 6, 0);
static const CoinEligibilityFilter filter_confirmed(1, 1, 0);
static const CoinEligibilityFilter filter_standard_extra(6, 6, 0);

CoinSelectionParams coin_selection_params(/* use_bnb = */ false, /* change_output_size = */ 0,
/* change_spend_size = */ 0, /* effective_fee = */ CFeeRate(0),
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ 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<CInputCoin>& set)
{
Expand Down Expand Up @@ -254,9 +255,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(/* use_bnb = */ true, /* change_output_size = */ 0,
/* change_spend_size = */ 0, /* effective_fee = */ CFeeRate(3000),
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ 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);
{
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
wallet->SetupLegacyScriptPubKeyMan();
Expand Down Expand Up @@ -298,7 +300,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
CCoinControl coin_control;
coin_control.fAllowOtherInputs = true;
coin_control.Select(COutPoint(coins.at(0).tx->GetHash(), coins.at(0).i));
coin_selection_params_bnb.effective_fee = CFeeRate(0);
coin_selection_params_bnb.m_effective_feerate = CFeeRate(0);
BOOST_CHECK(wallet->SelectCoins(coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used));
BOOST_CHECK(bnb_used);
BOOST_CHECK(coin_selection_params_bnb.use_bnb);
Expand Down Expand Up @@ -642,12 +644,14 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
CAmount target = rand.randrange(balance - 1000) + 1000;

// Perform selection
CoinSelectionParams coin_selection_params_knapsack(/* use_bnb = */ false, /* change_output_size = */ 34,
/* change_spend_size = */ 148, /* effective_fee = */ CFeeRate(0),
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false);
CoinSelectionParams coin_selection_params_bnb(/* use_bnb = */ true, /* change_output_size = */ 34,
/* change_spend_size = */ 148, /* effective_fee = */ CFeeRate(0),
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ 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;
Expand Down
28 changes: 11 additions & 17 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2872,26 +2872,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<OutputGroup> groups = GroupOutputs(coins, !coin_selection_params.m_avoid_partial_spends, effective_feerate, long_term_feerate, eligibility_filter, true /* positive_only */);
std::vector<OutputGroup> 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 {
Expand Down Expand Up @@ -2959,7 +2953,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& 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 {
Expand Down Expand Up @@ -3597,6 +3591,11 @@ bool CWallet::CreateTransactionInternal(
CHECK_NONFATAL(IsValidDestination(dest) != scriptChange.empty());
}

// 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;
Expand Down Expand Up @@ -3746,7 +3745,7 @@ bool CWallet::CreateTransactionInternal(

// Never create dust outputs; if we would, just
// add the dust to the fee.
if (IsDust(newTxOut, discard_rate))
if (IsDust(newTxOut, coin_selection_params.m_discard_feerate))
{
nFeeRet = nFeePrev;
nChangePosInOut = -1;
Expand Down Expand Up @@ -3809,11 +3808,6 @@ bool CWallet::CreateTransactionInternal(
}
}

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;
}

if (nAmountLeft == nFeeRet) {
// We either added the change amount to nFeeRet because the change amount was considered
Expand Down
11 changes: 8 additions & 3 deletions src/wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,9 @@ struct CoinSelectionParams
/** Size of the input to spend a change output in virtual bytes. */
size_t change_spend_size = 0;
/** The targeted feerate of the transaction being built. */
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;
Expand All @@ -693,11 +695,14 @@ struct CoinSelectionParams
* reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */
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)
{}
Expand Down

0 comments on commit 130d781

Please sign in to comment.