sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-25 12:02:48 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge #21083: wallet: Avoid requesting fee rates multiple times during coin selection

Browse Source 
f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b7f389ffcfae3b602ee3ecee8581acd wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931f86941903e855f831249ff5ec77485 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bbf7098f278c0247b954ecd3ba53cf37 wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb90f9ebd4b86d7934c6aa5594974f5f 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 #19229

ACKs for top commit:
  glozow:
    reACK f9cd2bfbcc
  fjahr:
    Code review re-ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
  Xekyo:
    tACK f9cd2bfbcc
  meshcollider:
    Code review + test run ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3

Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
This commit is contained in:
Samuel Dobson 2021-03-17 11:46:08 +13:00 committed by Konstantin Akimov
parent 9e9975f83b
commit ccac35c89c
No known key found for this signature in database
GPG Key ID: 2176C4A5D01EA524
4 changed files with 45 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
src/bench/coin_selection.cpp
View File

@ -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 */); coins.emplace_back(wtx.get(), 0 /* iIn */, 6 * 24 /* nDepthIn */, true /* spendable */, true /* solvable */, true /* safe */);
} }
const CoinEligibilityFilter filter_standard(1, 6, 0); 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([&] { bench.run([&] {
std::set<CInputCoin> setCoinsRet; std::set<CInputCoin> setCoinsRet;
CAmount nValueRet; CAmount nValueRet;

30
src/wallet/test/coinselector_tests.cpp
View File

@ -33,9 +33,10 @@ static const CoinEligibilityFilter filter_standard(1, 6, 0);
static const CoinEligibilityFilter filter_confirmed(1, 1, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0);
static const CoinEligibilityFilter filter_standard_extra(6, 6, 0); static const CoinEligibilityFilter filter_standard_extra(6, 6, 0);
CoinSelectionParams coin_selection_params(/* use_bnb = */ false, /* change_output_size = */ 0, CoinSelectionParams coin_selection_params(/* use_bnb= */ false, /* change_output_size= */ 0,
/* change_spend_size = */ 0, /* effective_fee = */ CFeeRate(0), /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0),
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); /* 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) static void add_coin(const CAmount& nValue, int nInput, std::vector<CInputCoin>& set)
{ {
@ -254,9 +255,10 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
} }
// Make sure that effective value is working in SelectCoinsMinConf when BnB is used // 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, CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 0,
/* change_spend_size = */ 0, /* effective_fee = */ CFeeRate(3000), /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000),
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); /* 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()); std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /* coinjoin_loader = */ nullptr, "", CreateMockWalletDatabase());
wallet->SetupLegacyScriptPubKeyMan(); wallet->SetupLegacyScriptPubKeyMan();
@ -298,7 +300,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
CCoinControl coin_control; CCoinControl coin_control;
coin_control.fAllowOtherInputs = true; coin_control.fAllowOtherInputs = true;
coin_control.Select(COutPoint(coins.at(0).tx->GetHash(), coins.at(0).i)); 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(wallet->SelectCoins(coins, 10 * CENT, setCoinsRet, nValueRet, coin_control, coin_selection_params_bnb, bnb_used));
BOOST_CHECK(bnb_used); BOOST_CHECK(bnb_used);
BOOST_CHECK(coin_selection_params_bnb.use_bnb); BOOST_CHECK(coin_selection_params_bnb.use_bnb);
@ -642,12 +644,14 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
CAmount target = rand.randrange(balance - 1000) + 1000; CAmount target = rand.randrange(balance - 1000) + 1000;
// Perform selection // Perform selection
CoinSelectionParams coin_selection_params_knapsack(/* use_bnb = */ false, /* change_output_size = */ 34, CoinSelectionParams coin_selection_params_knapsack(/* use_bnb= */ false, /* change_output_size= */ 34,
/* change_spend_size = */ 148, /* effective_fee = */ CFeeRate(0), /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
CoinSelectionParams coin_selection_params_bnb(/* use_bnb = */ true, /* change_output_size = */ 34, /* tx_no_inputs_size= */ 0, /* avoid_partial= */ false);
/* change_spend_size = */ 148, /* effective_fee = */ CFeeRate(0), CoinSelectionParams coin_selection_params_bnb(/* use_bnb= */ true, /* change_output_size= */ 34,
/* tx_noinputs_size = */ 0, /* m_avoid_partial_spends = */ false); /* 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; CoinSet out_set;
CAmount out_value = 0; CAmount out_value = 0;
bool bnb_used = false; bool bnb_used = false;

29
src/wallet/wallet.cpp
View File

@ -2872,26 +2872,20 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, const CoinEligibil
nValueRet = 0; nValueRet = 0;
if (coin_selection_params.use_bnb) { 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. // Get the feerate for effective value.
// When subtracting the fee from the outputs, we want the effective feerate to be 0 // When subtracting the fee from the outputs, we want the effective feerate to be 0
CFeeRate effective_feerate{0}; CFeeRate effective_feerate{0};
if (!coin_selection_params.m_subtract_fee_outputs) { 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 // 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 // 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; bnb_used = true;
return SelectCoinsBnB(groups, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees); return SelectCoinsBnB(groups, nTargetValue, cost_of_change, setCoinsRet, nValueRet, not_input_fees);
} else { } else {
@ -2959,7 +2953,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
if (coin.m_input_bytes <= 0) { if (coin.m_input_bytes <= 0) {
return false; // Not solvable, can't estimate size for fee 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) { if (coin_selection_params.use_bnb) {
value_to_select -= coin.effective_value; value_to_select -= coin.effective_value;
} else { } else {
@ -3548,14 +3542,16 @@ bool CWallet::CreateTransactionInternal(
CMutableTransaction txNew; CMutableTransaction txNew;
FeeCalculation feeCalc; FeeCalculation feeCalc;
CFeeRate discard_rate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this);
CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
coin_selection_params.m_discard_feerate = coin_control.m_discard_feerate ? *coin_control.m_discard_feerate : GetDiscardRate(*this);
// Get the fee rate to use effective values in coin selection // 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 // Do not, ever, assume that it's fine to change the fee rate if the user has explicitly
// provided one // provided one
if (coin_control.m_feerate && nFeeRateNeeded > *coin_control.m_feerate) { 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::DUFF_B), nFeeRateNeeded.ToString(FeeEstimateMode::DUFF_B)); error = strprintf(_("Fee rate (%s) is lower than the minimum fee rate setting (%s)"), coin_control.m_feerate->ToString(FeeEstimateMode::DUFF_B), coin_selection_params.m_effective_feerate.ToString(FeeEstimateMode::DUFF_B));
return false; return false;
} }
@ -3568,7 +3564,6 @@ bool CWallet::CreateTransactionInternal(
CAmount nAmountAvailable{0}; CAmount nAmountAvailable{0};
std::vector<COutput> vAvailableCoins; std::vector<COutput> vAvailableCoins;
AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); AvailableCoins(vAvailableCoins, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
coin_selection_params.use_bnb = false; // never use BnB coin_selection_params.use_bnb = false; // never use BnB
for (auto out : vAvailableCoins) { for (auto out : vAvailableCoins) {
@ -3756,7 +3751,7 @@ bool CWallet::CreateTransactionInternal(
// Never create dust outputs; if we would, just // Never create dust outputs; if we would, just
// add the dust to the fee. // add the dust to the fee.
if (IsDust(newTxOut, discard_rate)) if (IsDust(newTxOut, coin_selection_params.m_discard_feerate))
{ {
nFeeRet = nFeePrev; nFeeRet = nFeePrev;
nChangePosInOut = -1; nChangePosInOut = -1;

16
src/wallet/wallet.h
View File

@ -683,8 +683,13 @@ struct CoinSelectionParams
size_t change_output_size = 0; size_t change_output_size = 0;
/** Size of the input to spend a change output in virtual bytes. */ /** Size of the input to spend a change output in virtual bytes. */
size_t change_spend_size = 0; size_t change_spend_size = 0;
/** The targeted feerate of the transaction being built. */ /** The fee to spend these UTXOs at the long term feerate. */
CFeeRate effective_fee = CFeeRate(0); CFeeRate m_effective_feerate;
/** The feerate estimate used to estimate an upper bound on what should be sufficient to spend
* the change output sometime in the future. */
CFeeRate m_long_term_feerate;
/** If the cost to spend a change output at the discard feerate exceeds its value, drop it to fees. */
CFeeRate m_discard_feerate;
size_t tx_noinputs_size = 0; size_t tx_noinputs_size = 0;
/** Indicate that we are subtracting the fee from outputs */ /** Indicate that we are subtracting the fee from outputs */
bool m_subtract_fee_outputs = false; bool m_subtract_fee_outputs = false;
@ -693,11 +698,14 @@ struct CoinSelectionParams
* reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */ * reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */
bool m_avoid_partial_spends = 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), use_bnb(use_bnb),
change_output_size(change_output_size), change_output_size(change_output_size),
change_spend_size(change_spend_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), tx_noinputs_size(tx_noinputs_size),
m_avoid_partial_spends(avoid_partial) m_avoid_partial_spends(avoid_partial)
{} {}