-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try to mix more than one input by default #2275
Conversation
What is the problem with low round count per mix (with likely improved privacy per round) if there are many rounds per block/minute thanks to your recent improvements? |
@PastaPastaPasta The thing is that mixing 2 inputs per session is still 2 times faster (in general) than mixing just one regardless of all the other improvements. |
src/privatesend-client.cpp
Outdated
for(int i = nRoundStart; i < privateSendClient.nPrivateSendRounds; i++) { | ||
if(PrepareDenominate(i, i + 1, strError, vecTxDSInRet, vecTxOutRet)) { | ||
for (int i = nRoundStartTmp; i >= 0 && i < privateSendClient.nPrivateSendRounds; i += nLoopStep) { | ||
if (PrepareDenominate(i, i + 1, strError, vecPSInOutPairs, vecPSInOutPairs, fAvoidSingle)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... vecPSInOutPairs, vecPSInOutPairsTmp, ...
Looks like you need to run new tests ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😬 Good catch! I believe it's an issue because of solving merge conflicts due to rebase and I compiled it with correct version but worth mixing again I guess :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, just to remain consistent this condition should be also changed to the same logic (nRounds >= nPrivateSendRoundsMax
-> nRounds > nPrivateSendRoundsMax
, and in call of SelectCoinsDark change to privateSendClient.nPrivateSendRounds - 1
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. And it deserves its own PR imo, pls see #2277
fe7c966
to
824c584
Compare
824c584
to
600d689
Compare
Rebased. |
Closing this one in fav of #2278 |
Turned out that @InhumanPerfection was partially right there #2261 (comment) and wallet indeed tends to mix lower number of inputs in general. Not as bad as predicted ("mostly 1") but still lots of mixing sessions have 1-3 inputs, sometimes 4 but that's really rare. It's not like a huge drawback, considering the fact that privacy is not affected, but it's just going to take more time to mix in general, so still not so good. This situation can be improved by simply trying to skip the worst case (when we found 1 input only) in the first run. From what I observed, this works pretty good and it's much more common to see 3+ inputs per session now. Would be interesting to see if that's the case for someone else, pls test :)
This requires #2274 because otherwise GUI becomes unresponsive due to way too many loops over the heavy coin selection. Will rebase after 2274 is merged. Meaningful changes are pretty trivial here if you ignore commits from previous PRs and whitespaces: 44f53f1?w=1