wallet: shuffle coins before grouping, where warranted #13808
Conversation
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
src/wallet/wallet.cpp
Outdated
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.
bea78f2 to
c3b47e3
Compare
|
Addressed @instagibbs nits (except unconditional shuffle). |
c3b47e3 to
bc9ef87
Compare
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
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)