Coins are randomly shuffled in coin selection to avoid unintentional privacy leaks regarding the user’s coin set. For the case where a user has a lot of coins with the same destination, these will be grouped into groups of 10 before the shuffling.
It is unclear whether this has any implications at all, but this PR plugs the potential issue, if there ever is one, by shuffling the coins before they are grouped.
Issue brought up in #12257 (review)
GroupOutputs()? , just shuffle each group.m_outputs in the same condition? Currently GroupOutputs result depends on the input order, but that can change. Post-shuffle would be guaranteed.
2514@@ -2515,6 +2515,12 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2515
2516 // form groups from remaining coins; note that preset coins will not
2517 // automatically have their associated (same address) coins included
2518+ if (coin_control.m_avoid_partial_spends && vCoins.size() > 10) {
2519+ // Cases where we have 11+ outputs all pointing to the same destination may result in
2520+ // privacy leaks as they will potentially be deterministically sorted. We solve that by
2521+ // explicitly sorting the outputs before processing
2514@@ -2515,6 +2515,12 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2515
2516 // form groups from remaining coins; note that preset coins will not
2517 // automatically have their associated (same address) coins included
2518+ if (coin_control.m_avoid_partial_spends && vCoins.size() > 10) {
I haven’t checked, but I don’t think you can remove the later shuffle, and you at least want to skip shuffling if !m_avoid_partial_spends, so I don’t think unconditional is ideal here.
I.e. if we could move the shuffle operation to always happen here, rather than later, that would work fine.
If we can’t, we should at least not skip the extra shuffle if we are not grouping, since that’s just no-reason-wasteful.
Issue brought up in https://github.com/bitcoin/bitcoin/pull/12257\#discussion_r204554549
