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

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 https://github.com/bitcoin/bitcoin/commit/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
  fjahr:
    Code review re-ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
  Xekyo:
    tACK https://github.com/bitcoin/bitcoin/pull/21083/commits/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
  meshcollider:
    Code review + test run ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3

Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863

1 files changed, 17 insertions, 5 deletions

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<CInputCoin>& 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;