Add more variance to coin selection in PS mixing #2261

UdjinM6 · 2018-09-04T21:46:18Z

This should make it harder to analyse mixing sessions. See individual commits, should be self-explanatory. Client side only, requires no protobump.

~~Note: this includes #2259 , will rebase after #2259 is merged.~~

@PastaPastaPasta @Antti-Kaikkonen

PastaPastaPasta · 2018-09-04T23:30:08Z

concept ACK, I think the selection could be a little bit better than this in such a way that any input could be selected (rather than highest, middlest(¯\(ツ)/¯), lowest). But with this in conjunction with 8fe3811, this should be a major improvement, while not decreasing speed much. The only situation I am worried about here is the mixing of a single input, but even that should be fine with 8fe3811

PastaPastaPasta

See inline: Spacing after if and for

Looks like these files as a whole need a redo, I might do that after this gets merged

PastaPastaPasta · 2018-09-04T23:31:31Z

src/privatesend-client.cpp

        // Try to match their denominations if possible, select exact number of denominations
-        if(!pwalletMain->SelectCoinsByDenominations(dsq.nDenom, nMinAmount, nMaxAmount, vecTxDSInTmp, vCoinsTmp, nValueInTmp, 0, privateSendClient.nPrivateSendRounds)) {
-            LogPrintf("CPrivateSendClientSession::JoinExistingQueue -- Couldn't match %d denominations %d %d (%s)\n", dsq.nInputCount, vecBits.front(), dsq.nDenom, CPrivateSend::GetDenominationsToString(dsq.nDenom));
+        if(!pwalletMain->SelectCoinsByDenominations(dsq.nDenom, nMinAmount, nMaxAmount, vecTxDSInTmp, vCoinsTmp, nValueInTmp, 0, privateSendClient.nPrivateSendRounds, true)) {


if( -> if ( ;)

PastaPastaPasta · 2018-09-04T23:32:28Z

src/privatesend-client.cpp

+    if (privateSendClient.nLiquidityProvider || fMixLowest) {
+        // Try to use only inputs with the same number of rounds, from low to high
+        while (true) {
+            for(int i = nRoundStart; i < privateSendClient.nPrivateSendRounds; i++) {


for( -> for (

PastaPastaPasta · 2018-09-04T23:33:04Z

src/privatesend-client.cpp

+        // Try to use only inputs with the same number of rounds, from low to high
+        while (true) {
+            for(int i = nRoundStart; i < privateSendClient.nPrivateSendRounds; i++) {
+                if(PrepareDenominate(i, i + 1, strError, vecTxDSInRet, vecTxOutRet)) {


if( -> if (

PastaPastaPasta · 2018-09-04T23:33:30Z

src/privatesend-client.cpp

-                return SendDenominate(vecTxDSInRet, vecTxOutRet, connman);
+        // Try to use only inputs with the same number of rounds, from high to low
+        while (true) {
+            for(int i = nRoundStart; i > 0; i--) {


for( -> for (

PastaPastaPasta · 2018-09-04T23:33:47Z

src/privatesend-client.cpp

+        // Try to use only inputs with the same number of rounds, from high to low
+        while (true) {
+            for(int i = nRoundStart; i > 0; i--) {
+                if(PrepareDenominate(i - 1, i, strError, vecTxDSInRet, vecTxOutRet)) {


if( -> if (

PastaPastaPasta · 2018-09-04T23:35:18Z

src/wallet/wallet.cpp

@@ -3037,6 +3039,7 @@ bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount
    {
        // masternode-like input should not be selected by AvailableCoins now anyway
        //if(out.tx->vout[out.i].nValue == 1000*COIN) continue;
+        if(fNoDuplicateTxIds && mapRecentTxIds.find(out.tx->GetHash()) != mapRecentTxIds.end()) continue;


if( -> if (

I'm following the surrounding code format here intentionally (to improve readability).

Agree on that, if a general code cleanup is needed for the touched code, that's better in a separate PR. If a small code cleanup is enough, e.g. in just the line that is being touched, then in the same commit is ok IMHO.

… high" one someties

If failed when started from the middle try mixing from the edges right away. Note: liqudity providers always start from 0

UdjinM6 · 2018-09-05T11:24:24Z

Rebased and force-pushed.

Re all the formatting comments: new line has such for readability reasons and I didn't fix code style in others (old ones) because of the "do not mix functionality and refactoring in one PR" principle https://github.com/dashpay/dash/blob/master/CONTRIBUTING.md#pull-request-philosophy

codablock · 2018-09-05T11:17:59Z

src/privatesend-client.cpp

+    // Note: liqudity providers always start from 0
+    int nRoundStart = GetRandInt(4) == 0
+                        ? (privateSendClient.nLiquidityProvider ? 0 : (privateSendClient.nPrivateSendRounds / 2))
+                        : (fMixLowest ? 0 : privateSendClient.nPrivateSendRounds);


Can you convert this into if/else statements? Hard to read and using if/else would make this easier.

Not exactly if/else but should be simpler to read now imo.

What about this?

diff --git a/src/privatesend-client.cpp b/src/privatesend-client.cpp index bf1d60f2f..ac0f74fe1 100644 --- a/src/privatesend-client.cpp +++ b/src/privatesend-client.cpp @@ -1185,9 +1185,13 @@ bool CPrivateSendClientSession::SubmitDenominate(CConnman& connman) // lean towards edges but still mix starting from the middle someties // Note: liqudity providers always start from 0 bool fScanFromTheMiddle = (privateSendClient.nLiquidityProvider == 0) && (GetRandInt(4) == 0); - int nRoundStart = fScanFromTheMiddle - ? privateSendClient.nPrivateSendRounds / 2 - : (fMixLowest ? 0 : privateSendClient.nPrivateSendRounds); + + int nRoundStart = privateSendClient.nPrivateSendRounds; + if (fScanFromTheMiddle) { + nRoundStart = privateSendClient.nPrivateSendRounds / 2; + } else if (fMixLowest) { + nRoundStart = 0; + } // Submit transaction to the pool if we get here if (privateSendClient.nLiquidityProvider || fMixLowest) {

I find this much more clear, as it's immediately visible that there are 3 possible cases. With the nested ternary operators, it's very hard to see this.

Ok, makes sense. replaced with e0b0f4e

codablock · 2018-09-05T11:25:00Z

src/wallet/wallet.cpp

 {
+    std::map<uint256, uint8_t> mapRecentTxIds;


Why std::map and not std::set?

ooops, initial idea was to count them and then maybe sort etc. but then I figured out that I just need unique ones and... forgot about the map thing completely 🙈 Good catch 👍

codablock · 2018-09-05T11:26:13Z

src/wallet/wallet.cpp

@@ -3037,6 +3039,7 @@ bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount
    {
        // masternode-like input should not be selected by AvailableCoins now anyway
        //if(out.tx->vout[out.i].nValue == 1000*COIN) continue;
+        if(fNoDuplicateTxIds && mapRecentTxIds.find(out.tx->GetHash()) != mapRecentTxIds.end()) continue;


Agree on that, if a general code cleanup is needed for the touched code, that's better in a separate PR. If a small code cleanup is enough, e.g. in just the line that is being touched, then in the same commit is ok IMHO.

codablock

utACK

nmarley

utACK

InhumanPerfection · 2018-09-05T19:40:50Z

src/wallet/wallet.cpp

@@ -3037,6 +3039,7 @@ bool CWallet::SelectCoinsByDenominations(int nDenom, CAmount nValueMin, CAmount
    {
        // masternode-like input should not be selected by AvailableCoins now anyway
        //if(out.tx->vout[out.i].nValue == 1000*COIN) continue;
+        if(fNoDuplicateTxIds && setRecentTxIds.find(out.tx->GetHash()) != setRecentTxIds.end()) continue;


But will not this lead to the situation that most mixing transactions will contain only 1 input from each participant?

No, it shouldn't. Even if it does that's fine for privacy.

Yes, it's very fine for privacy. But mixing time will increase significantly. And mixing will cost much more.

This sounds like a valid concern. I'll have a look.

Fees would go up, but I see no situation where a participant would only be able to submit one denom, maybe the first mix when they are all fresh out of create denoms

@PastaPastaPasta not only after create denoms.

What we have: in most cases (nRoundStart = privateSendClient.nPrivateSendRounds) mixing function will select inputs with highest round originating only from different Txs.

After "create denoms" the participant will be able to provide from 1 to several inputs (depending on amount he is trying to mix - can be several "create denoms" Txs).

And after every mixing round where participant will be able to provide more than 1 input, on the next round in most cases he will not be able to provide more than 1 input because all inputs with highest round will be from the same Tx.

The number of inputs in this situation will always tends to one.

Antti-Kaikkonen · 2018-09-10T20:51:23Z

I like the concept but what is the motivation behind randomly choosing to scan from

lowest to highest
highest to lowest
middle to lowest
?

Wouldn't this approach make certain ~~denominations~~rounds more probable for no apparent reason? For example, the ~~denominations~~rounds at the middle and below middle would be chosen at a higher probability than the ~~denominations~~rounds slightly above the middle.

I think it might be better and more simple if we sometimes choose a random input instead of scanning. SelectCoinsByDenominations used by PrepareDenominate seems to choose a random input between nPrivateSendRoundsMin and nPrivateSendRoundsMax.

My idea would look something like this: (totally untested)

if (privateSendClient.nLiquidityProvider) {
  //scan from lowest
} else if (GetRandInt(4) == 0) {//pick randomly
  if(PrepareDenominate(0, privateSendClient.nPrivateSendRounds, strError, vecTxDSInRet, vecTxOutRet)) {
    LogPrintf("CPrivateSendClientSession::SubmitDenominate -- Running PrivateSend denominate for %d rounds, success\n", i);
    return SendDenominate(vecTxDSInRet, vecTxOutRet, connman);
  }
} else {
  //scan from highest
}

UdjinM6 · 2018-09-10T21:11:41Z

@Antti-Kaikkonen lowest to highest, vice versa or from the middle to whatever edge - all of them are not about denominations but about number of rounds, so no, there should be no preferences for specific denoms I think. Motivation for slightly randomising it for rounds is basically to make it harder to guess what's going on, otherwise you know it's highest to lowest all the time, so maybe someone could use this as a heuristic or smth.

Also, pls see an extension of this idea in #2275 (avoiding single input mix) and an alternative solution in #2278 (mixing rounds with most inputs first).

Antti-Kaikkonen · 2018-09-10T23:56:26Z

@UdjinM6 yeah rounds is what I mean and not denominations. I think it might be better to sometimes simply pick a random input rather than changing the scan direction because it will randomly choose from a larger set of inputs.

UdjinM6 · 2018-09-11T00:45:21Z

@Antti-Kaikkonen Probably. That was one of the ideas I had before #2278 tbh ;) but then I realized that picking random rounds have to be balanced with the speed of mixing somehow and picking rounds with the most inputs improves mixing speed while also achieving some kind of shuffling/randomization as a free bonus, see PR description there.

UdjinM6 added this to the 12.4 milestone Sep 4, 2018

PastaPastaPasta requested changes Sep 4, 2018

View reviewed changes

UdjinM6 added 3 commits September 5, 2018 14:19

Lean towards "from high to low" branch but still mix via "from low to…

0a8a127

… high" one someties

Lean towards edges but still mix starting from the middle sometimes

1dc7e9a

If failed when started from the middle try mixing from the edges right away. Note: liqudity providers always start from 0

Try to mix non-duplicate txids only

8025de8

UdjinM6 force-pushed the fixps branch from 8fe3811 to 8025de8 Compare September 5, 2018 11:20

codablock suggested changes Sep 5, 2018

View reviewed changes

map -> set

6eb1cc5

UdjinM6 mentioned this pull request Sep 5, 2018

Mixing Transactions with all inputs from the same transaction #2092

Closed

Refactor nRoundStart calculations

e0b0f4e

UdjinM6 force-pushed the fixps branch from df4bac3 to e0b0f4e Compare September 5, 2018 11:57

codablock approved these changes Sep 5, 2018

View reviewed changes

nmarley approved these changes Sep 5, 2018

View reviewed changes

UdjinM6 merged commit ad31dbb into dashpay:develop Sep 5, 2018

InhumanPerfection reviewed Sep 5, 2018

View reviewed changes

UdjinM6 mentioned this pull request Sep 8, 2018

Try to mix more than one input by default #2275

Closed

UdjinM6 deleted the fixps branch November 26, 2020 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more variance to coin selection in PS mixing #2261

Add more variance to coin selection in PS mixing #2261

UdjinM6 commented Sep 4, 2018 •

edited

Loading

PastaPastaPasta commented Sep 4, 2018 •

edited

Loading

PastaPastaPasta left a comment

PastaPastaPasta Sep 4, 2018

PastaPastaPasta Sep 4, 2018

PastaPastaPasta Sep 4, 2018

PastaPastaPasta Sep 4, 2018

PastaPastaPasta Sep 4, 2018

PastaPastaPasta Sep 4, 2018

UdjinM6 Sep 5, 2018

codablock Sep 5, 2018

UdjinM6 commented Sep 5, 2018 •

edited

Loading

codablock Sep 5, 2018

UdjinM6 Sep 5, 2018

codablock Sep 5, 2018

UdjinM6 Sep 5, 2018

codablock Sep 5, 2018

UdjinM6 Sep 5, 2018

codablock Sep 5, 2018

codablock left a comment

nmarley left a comment

InhumanPerfection Sep 5, 2018

PastaPastaPasta Sep 5, 2018

InhumanPerfection Sep 5, 2018 •

edited

Loading

UdjinM6 Sep 5, 2018

PastaPastaPasta Sep 5, 2018

InhumanPerfection Sep 6, 2018 •

edited

Loading

Antti-Kaikkonen commented Sep 10, 2018 •

edited

Loading

UdjinM6 commented Sep 10, 2018

Antti-Kaikkonen commented Sep 10, 2018

UdjinM6 commented Sep 11, 2018

Add more variance to coin selection in PS mixing #2261

Add more variance to coin selection in PS mixing #2261

Conversation

UdjinM6 commented Sep 4, 2018 • edited Loading

PastaPastaPasta commented Sep 4, 2018 • edited Loading

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UdjinM6 commented Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codablock left a comment

Choose a reason for hiding this comment

nmarley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InhumanPerfection Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InhumanPerfection Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Antti-Kaikkonen commented Sep 10, 2018 • edited Loading

UdjinM6 commented Sep 10, 2018

Antti-Kaikkonen commented Sep 10, 2018

UdjinM6 commented Sep 11, 2018

UdjinM6 commented Sep 4, 2018 •

edited

Loading

PastaPastaPasta commented Sep 4, 2018 •

edited

Loading

UdjinM6 commented Sep 5, 2018 •

edited

Loading

InhumanPerfection Sep 5, 2018 •

edited

Loading

InhumanPerfection Sep 6, 2018 •

edited

Loading

Antti-Kaikkonen commented Sep 10, 2018 •

edited

Loading