-
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
Pick rounds with the most inputs available to mix first #2278
Conversation
Does this mean that only one round is / will be mixed at once? For ex a 2 round input won't be able to be submitted with a 3 round input in the same mix? |
@PastaPastaPasta yes, this was always the case and it is the same here too, not touching this part. |
6cebae0
to
541f1f3
Compare
541f1f3
to
7c632e7
Compare
I think this solution might increase mixing throughput, but it could take longer to get a positive mixing balance, especially if the user has hundreds of inputs. What is the reason for trying to select inputs with the same number of rounds? Have you considered a solution like this? |
Because that's the shortest path to get some PS balance. When we calculate the number of rounds each input was mixed, we pick the worst possible estimation to make sure that each input was mixed at least required number of times. So when you pick 2 inputs inputs, one with 2 rounds and the other one with 5 rounds to mix in the same session, they'll end up both having min(2,5)+1=3 rounds. |
12c441a
to
3cfb25e
Compare
@UdjinM6 Oh that's right. I didn't think about that before. |
c3f52b6
to
7ab14f2
Compare
7ab14f2
to
617581e
Compare
Rebased. Note: this is an alternative to #2275 |
I think "no duplicate txids" rule making it easier to analyse. Not easy to explain this only with words so I made some slides ;) |
Interesting :) what about 33c12af as of a countermeasure? |
Yes, 33c12af will solve this exact issue (however it will speed down mixing by half), but my suggestion is in PrepareDenominate instead of: int nStepsMax = 5 + GetRandInt(PRIVATESEND_ENTRY_MAX_SIZE - 5 + 1); do something like this: int nPairsMatch = 0;
for (const auto& pair: vecPSInOutPairsIn) {
if (pair.second.nRounds >= nMinRounds && pair.second.nRounds <= nMaxRounds) {
nPairsMatch++;
}
}
int nStepsMax = 1 + GetRandInt(std::min(nPairsMatch, PRIVATESEND_ENTRY_MAX_SIZE)); it will:
(IMHO the whole idea of putting input selection in strict constraints (like with "no duplicate txids" rule) reduces randomness and probably facilitates analysis, idk...) |
0d8f845
to
33c12af
Compare
Pushed your suggestion @InhumanPerfection (f8d4a51) but then realized that it's not going to work for sessions with different denoms. We don't do this anymore(/yet?) but strictly saying the code was incorrect for this specific case. How about ab86bd0 instead? And we can also play with |
Indeed, my example was incorrect for sessions with different denoms (and every time I read this legacy part of code, I wonder why it's still here?). And ab86bd0 looks good. |
src/privatesend-client.cpp
Outdated
CAmount nValueDenom = vecStandardDenoms[nBit]; | ||
if (pair.second.nValue == nValueDenom) { | ||
CScript scriptDenom; | ||
if (fDryRun) { | ||
scriptDenom = CScript(); | ||
} else { | ||
// randomly skip some inputs when we have at least one of the same denom already | ||
if (vecSteps[nBit] > 1 && GetRandInt(2)) break; | ||
if (vecSteps[nBit] > 1 && GetRandInt(2)) { |
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.
vecSteps[nBit] >= 1 ?
…peed i.e. `GetRandInt(5) == 0`
Yeah, that an old legacy thing. Smth to refactor in another PR I guess ;)
Good idea, added
Ooops 🙈 This indeed doesn't match the comment above. Fixed :) |
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.
utACK (only looked for general errors...PS remains a black box for me)
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.
I feel same as @codablock , but didn't see any glaring issues. A couple typos identified inline.
src/privatesend-client.cpp
Outdated
int nRoundStart = GetStartRound(fMixLowest, fScanFromTheMiddle); | ||
int nRoundEdge = GetStartRound(fMixLowest, false); | ||
std::vector< std::pair<int, size_t> > vecInputsByRounds; | ||
// Note: liqudity providers are fine whith whatever numner of inputs they've got |
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.
nit: s/whith/with/
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.
s/numner/number/
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.
omg, 2 typos in one sentence 🙈 fixed :)
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.
Actually 3 typos 😆 "liquidity"
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.
This is the old one actually https://github.com/dashpay/dash/pull/2278/files#diff-8bdec64a6a2adb83b9696e5ca6d5522bL1196 but yeah..
Fixed typos, pls re-review. |
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.
utACK
* Alternative solution: pick rounds with the most inputs available to mix first * randomly skip some inputs when we have at least one of this denom already * More randomization in PrepareDenominate * fix `vecSteps[nBit] >= 1 ?` and adjust speed/privacy ratio for more speed i.e. `GetRandInt(5) == 0` * fix typos * no comments
After discussion in previous PRs I can't stop thinking that maybe we shouldn't introduce some arbitrary variance and fight the consequences of mixing with "no duplicate txids" rule and instead we could use it to our benefit somehow and the variance will appear naturally. So here is the idea: scan all coins with all rounds first to figure out how many inputs available for each of them and pick the one that has max inputs. For example, let's say that we have
<R, N1>
,<R+1, N2>
(where R is some number of rounds; N1, N2 is some number of available inputs and N1 > N2). We mix M inputs, so after this we'll have something from<R, N1-M>
to<R, N1>
(it depends because previous txes could have more than 1 output) for R and<R+1, N2+1>
for R+1. Note, that this way we consumed up to M inputs with unique txids for R rounds but we only added 1 input to R+1 rounds (because of "no duplicate txids" rule). What's interesting is that 1) there is already a variance for R which comes from previous txes and 2) ifN2+1
is greater thanN1-M
then we kind of shuffled them and R+1 now has higher mixing priority (hence even more variance).I wonder if this makes any sense or if I'm missing smth :)
See
6cebae0eddb4d7 for the implementation, ignore all the commits from previous PRs included here, will rebase later.UPDATE1: we can include inputs from the very next round too if there are some, this should bring more variance I think, see
541f1f37c632e7f3. This will however "squash" them into the same single tx which will decrease mixing speed due to "no same txid" rule.UPDATE2: reverted 7c632e7f3, it tends to have the same issue with lower and lower number of inputs on higher rounds which could probably make cluster analysis easier.
UPDATE3: also randomly skip some inputs and try to add up to PRIVATESEND_ENTRY_MAX_SIZE of every needed denomination to balance out "no duplicate txids" rule and make it harder to analyse.