Skip to content

Commit e901183

Browse files
committed
merge bitcoin#24560: Use single FastRandomContext when creating a wallet tx
1 parent 5a1850d commit e901183

File tree

5 files changed

+89
-55
lines changed

5 files changed

+89
-55
lines changed

src/bench/coin_selection.cpp

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,17 @@ static void CoinSelection(benchmark::Bench& bench)
4848
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);
4949
}
5050
const CoinEligibilityFilter filter_standard(1, 6, 0);
51-
const CoinSelectionParams coin_selection_params(/* change_output_size= */ 34,
52-
/* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
53-
/* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
54-
/* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
51+
FastRandomContext rand{};
52+
const CoinSelectionParams coin_selection_params{
53+
rand,
54+
/* change_output_size= */ 34,
55+
/* change_spend_size= */ 148,
56+
/* effective_feerate= */ CFeeRate(0),
57+
/* long_term_feerate= */ CFeeRate(0),
58+
/* discard_feerate= */ CFeeRate(0),
59+
/* tx_noinputs_size= */ 0,
60+
/* avoid_partial= */ false,
61+
};
5562
bench.run([&] {
5663
auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params);
5764
assert(result);

src/wallet/coinselection.cpp

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo
169169
return result;
170170
}
171171

172-
std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value)
172+
std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng)
173173
{
174174
if (utxo_pool.empty()) return std::nullopt;
175175

@@ -178,7 +178,7 @@ std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& ut
178178
std::vector<size_t> indexes;
179179
indexes.resize(utxo_pool.size());
180180
std::iota(indexes.begin(), indexes.end(), 0);
181-
Shuffle(indexes.begin(), indexes.end(), FastRandomContext());
181+
Shuffle(indexes.begin(), indexes.end(), rng);
182182

183183
CAmount selected_eff_value = 0;
184184
for (const size_t i : indexes) {
@@ -193,7 +193,7 @@ std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& ut
193193
return std::nullopt;
194194
}
195195

196-
static void ApproximateBestSubset(const std::vector<OutputGroup>& groups, const CAmount& nTotalLower, const CAmount& nTargetValue,
196+
static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::vector<OutputGroup>& groups, const CAmount& nTotalLower, const CAmount& nTargetValue,
197197
std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
198198
{
199199
std::vector<char> vfIncluded;
@@ -202,8 +202,6 @@ static void ApproximateBestSubset(const std::vector<OutputGroup>& groups, const
202202
nBest = nTotalLower;
203203
int nBestInputCount = 0;
204204

205-
FastRandomContext insecure_rand;
206-
207205
for (int nRep = 0; nRep < iterations && nBest != nTargetValue; nRep++)
208206
{
209207
vfIncluded.assign(groups.size(), false);
@@ -266,7 +264,7 @@ bool less_then_denom (const OutputGroup& group1, const OutputGroup& group2)
266264
return (!found1 && found2);
267265
}
268266

269-
std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, bool fFullyMixedOnly, CAmount maxTxFee)
267+
std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, FastRandomContext& rng, bool fFullyMixedOnly, CAmount maxTxFee)
270268
{
271269
if (groups.empty()) return std::nullopt;
272270

@@ -277,7 +275,7 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
277275
std::vector<OutputGroup> applicable_groups;
278276
CAmount nTotalLower = 0;
279277

280-
Shuffle(groups.begin(), groups.end(), FastRandomContext());
278+
Shuffle(groups.begin(), groups.end(), rng);
281279

282280
int tryDenomStart = 0;
283281
CAmount nMinChange = MIN_CHANGE;
@@ -349,9 +347,9 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
349347
std::vector<char> vfBest;
350348
CAmount nBest;
351349

352-
ApproximateBestSubset(applicable_groups, nTotalLower, nTargetValue, vfBest, nBest);
350+
ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue, vfBest, nBest);
353351
if (nBest != nTargetValue && nMinChange != 0 && nTotalLower >= nTargetValue + nMinChange) {
354-
ApproximateBestSubset(applicable_groups, nTotalLower, nTargetValue + nMinChange, vfBest, nBest);
352+
ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + nMinChange, vfBest, nBest);
355353
}
356354

357355
// If we have a bigger coin and (either the stochastic approximation didn't find a good solution,

src/wallet/coinselection.h

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,9 @@ class COutput
8686
};
8787

8888
/** Parameters for one iteration of Coin Selection. */
89-
struct CoinSelectionParams
90-
{
89+
struct CoinSelectionParams {
90+
/** Randomness to use in the context of coin selection. */
91+
FastRandomContext& rng_fast;
9192
/** Size of a change output in bytes, determined by the output type. */
9293
size_t change_output_size = 0;
9394
/** Size of the input to spend a change output in virtual bytes. */
@@ -111,17 +112,20 @@ struct CoinSelectionParams
111112
* reuse. Dust outputs are not eligible to be added to output groups and thus not considered. */
112113
bool m_avoid_partial_spends = false;
113114

114-
CoinSelectionParams(size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate,
115-
CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial) :
116-
change_output_size(change_output_size),
117-
change_spend_size(change_spend_size),
118-
m_effective_feerate(effective_feerate),
119-
m_long_term_feerate(long_term_feerate),
120-
m_discard_feerate(discard_feerate),
121-
tx_noinputs_size(tx_noinputs_size),
122-
m_avoid_partial_spends(avoid_partial)
123-
{}
124-
CoinSelectionParams() {}
115+
CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size, CFeeRate effective_feerate,
116+
CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial)
117+
: rng_fast{rng_fast},
118+
change_output_size(change_output_size),
119+
change_spend_size(change_spend_size),
120+
m_effective_feerate(effective_feerate),
121+
m_long_term_feerate(long_term_feerate),
122+
m_discard_feerate(discard_feerate),
123+
tx_noinputs_size(tx_noinputs_size),
124+
m_avoid_partial_spends(avoid_partial)
125+
{
126+
}
127+
CoinSelectionParams(FastRandomContext& rng_fast)
128+
: rng_fast{rng_fast} {}
125129
};
126130

127131
/** Parameters for filtering which OutputGroups we may use in coin selection.
@@ -257,9 +261,9 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo
257261
* @param[in] target_value The target value to select for
258262
* @returns If successful, a SelectionResult, otherwise, std::nullopt
259263
*/
260-
std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value);
264+
std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& utxo_pool, CAmount target_value, FastRandomContext& rng);
261265

262266
// Original coin selection algorithm as a fallback
263-
std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, bool fFullyMixedOnly, CAmount maxTxFee);
267+
std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, FastRandomContext& rng, bool fFullyMixedOnly, CAmount maxTxFee);
264268

265269
#endif // BITCOIN_WALLET_COINSELECTION_H

src/wallet/spend.cpp

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -386,15 +386,15 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
386386
if (!coin_selection_params.m_subtract_fee_outputs && nCoinType != CoinType::ONLY_FULLY_MIXED) {
387387
target_with_change += coin_selection_params.m_change_fee;
388388
}
389-
if (auto knapsack_result{KnapsackSolver(all_groups, target_with_change, nCoinType == CoinType::ONLY_FULLY_MIXED, wallet.m_default_max_tx_fee)}) {
389+
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)}) {
390390
knapsack_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
391391
results.push_back(*knapsack_result);
392392
}
393393

394394
// We include the minimum final change for SRD as we do want to avoid making really small change.
395395
// KnapsackSolver does not need this because it includes MIN_CHANGE internally.
396396
const CAmount srd_target = nTargetValue + coin_selection_params.m_change_fee + MIN_FINAL_CHANGE;
397-
if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target)}) {
397+
if (auto srd_result{SelectCoinsSRD(positive_groups, srd_target, coin_selection_params.rng_fast)}) {
398398
srd_result->ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
399399
results.push_back(*srd_result);
400400
}
@@ -502,7 +502,7 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
502502
// Cases where we have 101+ outputs all pointing to the same destination may result in
503503
// privacy leaks as they will potentially be deterministically sorted. We solve that by
504504
// explicitly shuffling the outputs before processing
505-
Shuffle(vCoins.begin(), vCoins.end(), FastRandomContext());
505+
Shuffle(vCoins.begin(), vCoins.end(), coin_selection_params.rng_fast);
506506
}
507507
// Coin Selection attempts to select inputs from a pool of eligible UTXOs to fund the
508508
// transaction at a target feerate. If an attempt fails, more attempts may be made using a more
@@ -585,7 +585,8 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
585585
* Set a height-based locktime for new transactions (uses the height of the
586586
* current chain tip unless we are not synced with the current chain
587587
*/
588-
static void DiscourageFeeSniping(CMutableTransaction& tx, interfaces::Chain& chain, const uint256& block_hash, int block_height)
588+
static void DiscourageFeeSniping(CMutableTransaction& tx, FastRandomContext& rng_fast,
589+
interfaces::Chain& chain, const uint256& block_hash, int block_height)
589590
{
590591
// All inputs must be added by now
591592
assert(!tx.vin.empty());
@@ -616,8 +617,8 @@ static void DiscourageFeeSniping(CMutableTransaction& tx, interfaces::Chain& cha
616617
// that transactions that are delayed after signing for whatever reason,
617618
// e.g. high-latency mix networks and some CoinJoin implementations, have
618619
// better privacy.
619-
if (GetRandInt(10) == 0) {
620-
tx.nLockTime = std::max(0, int(tx.nLockTime) - GetRandInt(100));
620+
if (rng_fast.randrange(10) == 0) {
621+
tx.nLockTime = std::max(0, int(tx.nLockTime) - int(rng_fast.randrange(100)));
621622
}
622623
} else {
623624
// If our chain is lagging behind, we can't discourage fee sniping nor help
@@ -652,9 +653,10 @@ static bool CreateTransactionInternal(
652653
{
653654
AssertLockHeld(wallet.cs_wallet);
654655

656+
FastRandomContext rng_fast;
655657
CMutableTransaction txNew; // The resulting transaction that we make
656658

657-
CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy
659+
CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
658660
coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
659661

660662
// Set the long term feerate estimate to the wallet's consolidate feerate
@@ -788,10 +790,9 @@ static bool CreateTransactionInternal(
788790
assert(change_and_fee >= 0);
789791
CTxOut newTxOut(change_and_fee, scriptChange);
790792

791-
if (nChangePosInOut == -1)
792-
{
793+
if (nChangePosInOut == -1) {
793794
// Insert change txn at random position:
794-
nChangePosInOut = GetRandInt(txNew.vout.size()+1);
795+
nChangePosInOut = rng_fast.randrange(txNew.vout.size() + 1);
795796
}
796797
else if ((unsigned int)nChangePosInOut > txNew.vout.size())
797798
{
@@ -827,11 +828,11 @@ static bool CreateTransactionInternal(
827828
for (const auto& coin : result->GetInputSet()) {
828829
txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence));
829830
}
830-
DiscourageFeeSniping(txNew, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
831+
DiscourageFeeSniping(txNew, rng_fast, wallet.chain(), wallet.GetLastBlockHash(), wallet.GetLastBlockHeight());
831832

832833
// Fill in final vin and shuffle/sort it
833834
if (sort_bip69) { std::sort(txNew.vin.begin(), txNew.vin.end(), CompareInputBIP69()); }
834-
else { Shuffle(txNew.vin.begin(), txNew.vin.end(), FastRandomContext()); }
835+
else { Shuffle(txNew.vin.begin(), txNew.vin.end(), coin_selection_params.rng_fast); }
835836

836837
// Calculate the transaction fee
837838
int nBytes = CalculateMaximumSignedTxSize(CTransaction(txNew), &wallet, coin_control.fAllowWatchOnly);

src/wallet/test/coinselector_tests.cpp

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -144,17 +144,24 @@ inline std::vector<OutputGroup>& GroupCoins(const std::vector<COutput>& coins)
144144
return static_groups;
145145
}
146146

147-
inline std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue)
147+
inline std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, FastRandomContext& rng)
148148
{
149-
return KnapsackSolver(groups, nTargetValue, /*fFullyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE);
149+
return KnapsackSolver(groups, nTargetValue, rng, /*fFullyMixedOnly=*/false, /*maxTxFee=*/DEFAULT_TRANSACTION_MAXFEE);
150150
}
151151

152152
inline std::vector<OutputGroup>& KnapsackGroupOutputs(const std::vector<COutput>& coins, CWallet& wallet, const CoinEligibilityFilter& filter)
153153
{
154-
CoinSelectionParams coin_selection_params(/* change_output_size= */ 0,
155-
/* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(0),
156-
/* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
157-
/* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
154+
FastRandomContext rand{};
155+
CoinSelectionParams coin_selection_params{
156+
rand,
157+
/* change_output_size= */ 0,
158+
/* change_spend_size= */ 0,
159+
/* effective_feerate= */ CFeeRate(0),
160+
/* long_term_feerate= */ CFeeRate(0),
161+
/* discard_feerate= */ CFeeRate(0),
162+
/* tx_noinputs_size= */ 0,
163+
/* avoid_partial= */ false,
164+
};
158165
static std::vector<OutputGroup> static_groups;
159166
static_groups = GroupOutputs(wallet, coins, coin_selection_params, filter, /*positive_only=*/false);
160167
return static_groups;
@@ -163,6 +170,7 @@ inline std::vector<OutputGroup>& KnapsackGroupOutputs(const std::vector<COutput>
163170
// Branch and bound coin selection tests
164171
BOOST_AUTO_TEST_CASE(bnb_search_test)
165172
{
173+
FastRandomContext rand{};
166174
// Setup
167175
std::vector<COutput> utxo_pool;
168176
SelectionResult expected_result(CAmount(0));
@@ -288,10 +296,16 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
288296
}
289297

290298
// Make sure that effective value is working in AttemptSelection when BnB is used
291-
CoinSelectionParams coin_selection_params_bnb(/* change_output_size= */ 0,
292-
/* change_spend_size= */ 0, /* effective_feerate= */ CFeeRate(3000),
293-
/* long_term_feerate= */ CFeeRate(1000), /* discard_feerate= */ CFeeRate(1000),
294-
/* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
299+
CoinSelectionParams coin_selection_params_bnb{
300+
rand,
301+
/* change_output_size= */ 0,
302+
/* change_spend_size= */ 0,
303+
/* effective_feerate= */ CFeeRate(3000),
304+
/* long_term_feerate= */ CFeeRate(1000),
305+
/* discard_feerate= */ CFeeRate(1000),
306+
/* tx_noinputs_size= */ 0,
307+
/* avoid_partial= */ false,
308+
};
295309
{
296310
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
297311
wallet->LoadWallet();
@@ -338,6 +352,9 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
338352

339353
BOOST_AUTO_TEST_CASE(knapsack_solver_test)
340354
{
355+
FastRandomContext rand{};
356+
const auto temp1{[&rand](std::vector<OutputGroup>& g, const CAmount& v) { return KnapsackSolver(g, v, rand); }};
357+
const auto KnapsackSolver{temp1};
341358
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
342359
wallet->LoadWallet();
343360
LOCK(wallet->cs_wallet);
@@ -647,6 +664,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
647664

648665
BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
649666
{
667+
FastRandomContext rand{};
650668
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(m_node.chain.get(), /*coinjoin_loader=*/nullptr, "", m_args, CreateMockWalletDatabase());
651669
wallet->LoadWallet();
652670
LOCK(wallet->cs_wallet);
@@ -660,7 +678,7 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
660678
add_coin(coins, *wallet, 1000 * COIN);
661679
add_coin(coins, *wallet, 3 * COIN);
662680

663-
const auto result = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 1003 * COIN);
681+
const auto result = KnapsackSolver(KnapsackGroupOutputs(coins, *wallet, filter_standard), 1003 * COIN, rand);
664682
BOOST_CHECK(result);
665683
BOOST_CHECK_EQUAL(result->GetSelectedValue(), 1003 * COIN);
666684
BOOST_CHECK_EQUAL(result->GetInputSet().size(), 2U);
@@ -701,10 +719,16 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
701719
CAmount target = rand.randrange(balance - 1000) + 1000;
702720

703721
// Perform selection
704-
CoinSelectionParams cs_params(/* change_output_size= */ 34,
705-
/* change_spend_size= */ 148, /* effective_feerate= */ CFeeRate(0),
706-
/* long_term_feerate= */ CFeeRate(0), /* discard_feerate= */ CFeeRate(0),
707-
/* tx_noinputs_size= */ 0, /* avoid_partial= */ false);
722+
CoinSelectionParams cs_params{
723+
rand,
724+
/* change_output_size= */ 34,
725+
/* change_spend_size= */ 148,
726+
/* effective_feerate= */ CFeeRate(0),
727+
/* long_term_feerate= */ CFeeRate(0),
728+
/* discard_feerate= */ CFeeRate(0),
729+
/* tx_noinputs_size= */ 0,
730+
/* avoid_partial= */ false,
731+
};
708732
CCoinControl cc;
709733
const auto result = SelectCoins(*wallet, coins, target, cc, cs_params);
710734
BOOST_CHECK(result);

0 commit comments

Comments
 (0)