wallet: shuffle coins before grouping, where warranted #13808
Conversation
kallewoof
force-pushed
the
random-ordering-output-groups
branch
2 times, most recently
from
60df230
to
bea78f2
Compare
|
Could shuffle after |
|
@promag You would have to not cap the size of groups and create them with unlimited size, then shuffle each one and break it into groups.. don't see the benefit to this, honestly, unless it has a huge impact on speed for big wallets or something? |
src/wallet/wallet.cpp
Outdated
| if (coin_control.m_avoid_partial_spends && vCoins.size() > 10) { | ||
| // Cases where we have 11+ outputs all pointing to the same destination may result in | ||
| // privacy leaks as they will potentially be deterministically sorted. We solve that by | ||
| // explicitly sorting the outputs before processing |
src/wallet/wallet.cpp
Outdated
| @@ -2515,6 +2515,12 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm | |||
|
|
|||
| // form groups from remaining coins; note that preset coins will not | |||
| // automatically have their associated (same address) coins included | |||
| if (coin_control.m_avoid_partial_spends && vCoins.size() > 10) { | |||
There was a problem hiding this comment.
can we make this 10 a constant to make it easier for the reader/future writer, or make it a parameter to the grouping function itself?
There was a problem hiding this comment.
Second thought: Would an unconditional shuffle just be easier here? We shuffle inputs later post-selection, so nothing of value should be lost.
There was a problem hiding this comment.
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.
kallewoof
force-pushed
the
random-ordering-output-groups
branch
from
bea78f2
to
c3b47e3
Compare
|
Addressed @instagibbs nits (except unconditional shuffle). |
kallewoof
force-pushed
the
random-ordering-output-groups
branch
from
c3b47e3
to
bc9ef87
Compare
Issue brought up in bitcoin#12257
kallewoof
force-pushed
the
random-ordering-output-groups
branch
from
bc9ef87
to
18f690e
Compare
|
utACK 18f690e |
|
utACK 18f690e |
|
utACK 18f690e, but can you update the PR description to explain the issue and solution? It isn't at all clear from the commit message or description why this is necessary. |
|
@sipa I updated the description. |
|
utACK 18f690e |
|
utACK 18f690e |
…ranted 18f690e wallet: shuffle coins before grouping, where warranted (Karl-Johan Alm) Pull request description: 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 bitcoin#12257 (comment) Tree-SHA512: fb50ed4b5fc03ab4853d45b76e1c64476ad5bcd797497179bc37b9262885c974ed6811159fd8e581f1461b6cc6d0a66146f4b70a2777c0f5e818d1322e0edb89
…ranted 18f690e wallet: shuffle coins before grouping, where warranted (Karl-Johan Alm) Pull request description: 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 bitcoin#12257 (comment) Tree-SHA512: fb50ed4b5fc03ab4853d45b76e1c64476ad5bcd797497179bc37b9262885c974ed6811159fd8e581f1461b6cc6d0a66146f4b70a2777c0f5e818d1322e0edb89
…ranted 18f690e wallet: shuffle coins before grouping, where warranted (Karl-Johan Alm) Pull request description: 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 bitcoin#12257 (comment) Tree-SHA512: fb50ed4b5fc03ab4853d45b76e1c64476ad5bcd797497179bc37b9262885c974ed6811159fd8e581f1461b6cc6d0a66146f4b70a2777c0f5e818d1322e0edb89
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 (comment)