merge bitcoin#24560: Use single FastRandomContext when creating a wallet tx

kwvg · kwvg · commit 0e33db23e275 · 2025-05-07T14:22:33.000Z
diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp
@@ -48,10 +48,17 @@ static void CoinSelection(benchmark::Bench& bench)
         coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/ 6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true);
     }
     const CoinEligibilityFilter filter_standard(1, 6, 0);
-    const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34,
-                                                    /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
-                                                    /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
-                                                    /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
+    FastRandomContext rand{};
+    const CoinSelectionParams coin_selection_params{
+        rand,
+        /* change_output_size= */ 34,
+        /* change_spend_size= */ 148,
+        /* effective_feerate= */ CFeeRate(0),
+        /* long_term_feerate= */ CFeeRate(0),
+        /* discard_feerate= */ CFeeRate(0),
+        /* tx_noinputs_size= */ 0,
+        /* avoid_partial= */ false,
+    };
     bench.run([&] {
         auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params);
         assert(result);
diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp
@@ -167,14 +167,14 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo
     return result;
 }
 
-std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value)
+std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng)
 {
     SelectionResult result(target_value);
 
     std::vector<size_t> indexes;
     indexes.resize(utxo_pool.size());
     std::iota(indexes.begin(), indexes.end(), 0);
-    Shuffle(indexes.begin(), indexes.end(), FastRandomContext());
+    Shuffle(indexes.begin(), indexes.end(), rng);
 
     CAmount selected_eff_value = 0;
     for (const size_t i : indexes) {
@@ -189,7 +189,7 @@ std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& ut
     return std::nullopt;
 }
 
-static void ApproximateBestSubset(const std::vector<OutputGroup>& groups, const CAmount& nTotalLower, const CAmount& nTargetValue,
+static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::vector<OutputGroup>& groups, const CAmount& nTotalLower, const CAmount& nTargetValue,
                                   std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
 {
     std::vector<char> vfIncluded;
@@ -198,8 +198,6 @@ static void ApproximateBestSubset(const std::vector<OutputGroup>& groups, const
     nBest = nTotalLower;
     int nBestInputCount = 0;
 
-    FastRandomContext insecure_rand;
-
     for (int nRep = 0; nRep < iterations && nBest != nTargetValue; nRep++)
     {
         vfIncluded.assign(groups.size(), false);
@@ -262,7 +260,7 @@ bool less_then_denom (const OutputGroup& group1, const OutputGroup& group2)
     return (!found1 && found2);
 }
 
-std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, bool fFullyMixedOnly, CAmount maxTxFee)
+std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, FastRandomContext& rng, bool fFullyMixedOnly, CAmount maxTxFee)
 {
     SelectionResult result(nTargetValue);
 
@@ -271,7 +269,7 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
     std::vector<OutputGroup> applicable_groups;
     CAmount nTotalLower = 0;
 
-    Shuffle(groups.begin(), groups.end(), FastRandomContext());
+    Shuffle(groups.begin(), groups.end(), rng);
 
     int tryDenomStart = 0;
     CAmount nMinChange = MIN_CHANGE;
@@ -343,9 +341,9 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
     std::vector<char> vfBest;
     CAmount nBest;
 
-    ApproximateBestSubset(applicable_groups, nTotalLower, nTargetValue, vfBest, nBest);
+    ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue, vfBest, nBest);
     if (nBest != nTargetValue && nMinChange != 0 && nTotalLower >= nTargetValue + nMinChange) {
-        ApproximateBestSubset(applicable_groups, nTotalLower, nTargetValue + nMinChange, vfBest, nBest);
+        ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + nMinChange, vfBest, nBest);
     }
 
     // If we have a bigger coin and (either the stochastic approximation didn't find a good solution,
diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h
@@ -86,8 +86,9 @@ class COutput
 };
 
 /** Parameters for one iteration of Coin Selection. */
-struct CoinSelectionParams
-{
+struct CoinSelectionParams {
+    /** Randomness to use in the context of coin selection. */
+    FastRandomContext& rng_fast;
     /** Size of a change output in bytes, determined by the output type. */
     size_t change_output_size = 0;
     /** Size of the input to spend a change output in virtual bytes. */
@@ -111,17 +112,20 @@ 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(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) :
-        change_output_size(change_output_size),
-        change_spend_size(change_spend_size),
-        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)
-    {}
-    CoinSelectionParams() {}
+    CoinSelectionParams(FastRandomContext& rng_fast, 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)
+        : rng_fast{rng_fast},
+          change_output_size(change_output_size),
+          change_spend_size(change_spend_size),
+          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)
+    {
+    }
+    CoinSelectionParams(FastRandomContext& rng_fast)
+        : rng_fast{rng_fast} {}
 };
 
 /** Parameters for filtering which OutputGroups we may use in coin selection.
@@ -257,9 +261,9 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo
  * @param[in]  target_value The target value to select for
  * @returns If successful, a SelectionResult, otherwise, std::nullopt
  */
-std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value);
+std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng);
 
 // Original coin selection algorithm as a fallback
-std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, bool fFullyMixedOnly, CAmount maxTxFee);
+std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, FastRandomContext& rng, bool fFullyMixedOnly, CAmount maxTxFee);
 
 #endif // BITCOIN_WALLET_COINSELECTION_H
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
@@ -386,7 +386,7 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
     if (!coin_selection_params.m_subtract_fee_outputs && nCoinType != CoinType::ONLY_FULLY_MIXED) {
         target_with_change += coin_selection_params.m_change_fee;
     }
-    if (auto knapsack_result{KnapsackSolver(all_groups, target_with_change, nCoinType == CoinType::ONLY_FULLY_MIXED, wallet.m_default_max_tx_fee)}) {
+    if (auto knapsack_result{KnapsackSolver(all_groups, target_with_change, coin_selection_params.rng_fast, nCoinType == CoinType::ONLY_FULLY_MIXED, wallet.m_default_max_tx_fee)}) {
         knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
         results.push_back(*knapsack_result);
     }
@@ -395,7 +395,7 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
     // KnapsackSolver does not need this because it includes MIN_CHANGE internally.
     const CAmount srd_target = nTargetValue + coin_selection_params.m_change_fee + MIN_FINAL_CHANGE;
     // Note: SRD is disabled because it is unaware of mixed coins
-    if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target)}; /* DISABLES CODE */ (false)) {
+    if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast)}; /* DISABLES CODE */ (false)) {
         srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
         results.push_back(*srd_result);
     }
@@ -503,7 +503,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
         // Cases where we have 101+ outputs all pointing to the same destination may result in
         // privacy leaks as they will potentially be deterministically sorted. We solve that by
         // explicitly shuffling the outputs before processing
-        Shuffle(vCoins.begin(), vCoins.end(), FastRandomContext());
+        Shuffle(vCoins.begin(), vCoins.end(), coin_selection_params.rng_fast);
     }
     // Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the
     // transaction at a target feerate. If an attempt fails, more attempts may be made using a more
@@ -586,7 +586,8 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
  * Set a height-based locktime for new transactions (uses the height of the
  * current chain tip unless we are not synced with the current chain
  */
-static void DiscourageFeeSniping(CMutableTransaction& tx, interfaces::Chain& chain, const uint256& block_hash, int block_height)
+static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
+                                 interfaces::Chain& chain, const uint256& block_hash, int block_height)
 {
     // All inputs must be added by now
     assert(!tx.vin.empty());
@@ -617,8 +618,8 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, interfaces::Chain& cha
         // that transactions that are delayed after signing for whatever reason,
         // e.g. high-latency mix networks and some CoinJoin implementations, have
         // better privacy.
-        if (GetRandInt(10) == 0) {
-            tx.nLockTime = std::max(0, int(tx.nLockTime) - GetRandInt(100));
+        if (rng_fast.randrange(10) == 0) {
+            tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100)));
         }
     } else {
         // If our chain is lagging behind, we can't discourage fee sniping nor help
@@ -653,9 +654,10 @@ static bool CreateTransactionInternal(
 {
     AssertLockHeld(wallet.cs_wallet);
 
+    FastRandomContext rng_fast;
     CMutableTransaction txNew; // The resulting transaction that we make
 
-    CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
+    CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
     coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
 
     // Set the long term feerate estimate to the wallet's consolidate feerate
@@ -789,10 +791,9 @@ static bool CreateTransactionInternal(
     assert(change_and_fee >= 0);
     CTxOut newTxOut(change_and_fee, scriptChange);
 
-    if (nChangePosInOut == -1)
-    {
+    if (nChangePosInOut == -1) {
         // Insert change txn at random position:
-        nChangePosInOut = GetRandInt(txNew.vout.size()+1);
+        nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1);
     }
     else if ((unsigned int)nChangePosInOut > txNew.vout.size())
     {
@@ -828,11 +829,11 @@ static bool CreateTransactionInternal(
     for (const auto& coin : result->GetInputSet()) {
         txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence));
     }
-    DiscourageFeeSniping(txNew, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
+    DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
 
     // Fill in final vin and shuffle/sort it
     if (sort_bip69) { std::sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69()); }
-    else { Shuffle(txNew.vin.begin(), txNew.vin.end(), FastRandomContext()); }
+    else { Shuffle(txNew.vin.begin(), txNew.vin.end(), coin_selection_params.rng_fast); }
 
     // Calculate the transaction fee
     int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly);
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
@@ -144,17 +144,24 @@ inline std::vector<OutputGroup>& GroupCoins(const std::vector<COutput>& coins)
     return static_groups;
 }
 
-inline std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue)
+inline std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, FastRandomContext& rng)
 {
-    return KnapsackSolver(groups, nTargetValue, /*fFullyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE);
+    return KnapsackSolver(groups, nTargetValue, rng, /*fFullyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE);
 }
 
 inline std::vector<OutputGroup>& KnapsackGroupOutputs(const std::vector<COutput>& coins, CWallet& wallet, const CoinEligibilityFilter& filter)
 {
-    CoinSelectionParams coin_selection_params(/* change_output_size= */ 0,
-                                              /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0),
-                                              /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
-                                              /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
+    FastRandomContext rand{};
+    CoinSelectionParams coin_selection_params{
+        rand,
+        /* change_output_size= */ 0,
+        /* change_spend_size= */ 0,
+        /* effective_feerate= */ CFeeRate(0),
+        /* long_term_feerate= */ CFeeRate(0),
+        /* discard_feerate= */ CFeeRate(0),
+        /* tx_noinputs_size= */ 0,
+        /* avoid_partial= */ false,
+    };
     static std::vector<OutputGroup> static_groups;
     static_groups = GroupOutputs(wallet, coins, coin_selection_params, filter, /*positive_only=*/false);
     return static_groups;
@@ -163,6 +170,7 @@ inline std::vector<OutputGroup>& KnapsackGroupOutputs(const std::vector<COutput>
 // Branch and bound coin selection tests
 BOOST_AUTO_TEST_CASE(bnb_search_test)
 {
+    FastRandomContext rand{};
     // Setup
     std::vector<COutput> utxo_pool;
     SelectionResult expected_result(CAmount(0));
@@ -288,10 +296,16 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
     }
 
     // Make sure that effective value is working in AttemptSelection when BnB is used
-    CoinSelectionParams coin_selection_params_bnb(/* change_output_size= */ 0,
-                                                  /* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000),
-                                                  /* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000),
-                                                  /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
+    CoinSelectionParams coin_selection_params_bnb{
+        rand,
+        /* change_output_size= */ 0,
+        /* change_spend_size= */ 0,
+        /* effective_feerate= */ CFeeRate(3000),
+        /* long_term_feerate= */ CFeeRate(1000),
+        /* discard_feerate= */ CFeeRate(1000),
+        /* tx_noinputs_size= */ 0,
+        /* avoid_partial= */ false,
+    };
     {
         std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
         wallet->LoadWallet();
@@ -338,6 +352,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
 
 BOOST_AUTO_TEST_CASE(knapsack_solver_test)
 {
+    FastRandomContext rand{};
+    const auto temp1{[&rand](std::vector<OutputGroup>& g, const CAmount& v) { return KnapsackSolver(g, v, rand); }};
+    const auto KnapsackSolver{temp1};
     std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
     wallet->LoadWallet();
     LOCK(wallet->cs_wallet);
@@ -647,6 +664,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
 
 BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
 {
+    FastRandomContext rand{};
     std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
     wallet->LoadWallet();
     LOCK(wallet->cs_wallet);
@@ -660,7 +678,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
         add_coin(coins, *wallet, 1000 * COIN);
     add_coin(coins, *wallet, 3 * COIN);
 
-    const auto result = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 1003 * COIN);
+    const auto result = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 1003 * COIN, rand);
     BOOST_CHECK(result);
     BOOST_CHECK_EQUAL(result->GetSelectedValue(), 1003 * COIN);
     BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U);
@@ -701,10 +719,16 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
         CAmount target = rand.randrange(balance - 1000) + 1000;
 
         // Perform selection
-        CoinSelectionParams cs_params(/* change_output_size= */ 34,
-                                      /* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
-                                      /* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
-                                      /* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
+        CoinSelectionParams cs_params{
+            rand,
+            /* change_output_size= */ 34,
+            /* change_spend_size= */ 148,
+            /* effective_feerate= */ CFeeRate(0),
+            /* long_term_feerate= */ CFeeRate(0),
+            /* discard_feerate= */ CFeeRate(0),
+            /* tx_noinputs_size= */ 0,
+            /* avoid_partial= */ false,
+        };
         CCoinControl cc;
         const auto result = SelectCoins(*wallet, coins, target, cc, cs_params);
         BOOST_CHECK(result);
