Require all participants to submit equal number of inputs #2075

UdjinM6 · 2018-05-15T09:51:51Z

Fixes privacy issues for some edge cases (mixing single input) and should improve privacy in general making each mixing rounds more uniform.

Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability. Should be backwards compatible until then.

Since it requires both spork6 (or smth similar) and protobump I think it's ok to include it in 12.3 - spork6 was not yet activated on testnet).

Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability

codablock

utACK in the sense that I didn't find obvious errors. I'm not deep enough in PS code to do a proper review however

gladcow · 2018-05-20T20:18:46Z

utACK

Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability

InhumanPerfection · 2018-05-29T13:20:18Z

src/privatesend-server.cpp

    if(!fUnitTest) {
        //broadcast that I'm accepting entries, only if it's the first entry through
-        CDarksendQueue dsq(dsa.nDenom, activeMasternode.outpoint, GetAdjustedTime(), false);
+        CDarksendQueue dsq(dsa.nDenom, dsa.nInputCount, activeMasternode.outpoint, GetAdjustedTime(), false);


nSessionInputCount instead of dsa.nInputCount here?

Due to the way it's activated this shouldn't really matter but I agree that using nSession* here would improve readability.

InhumanPerfection · 2018-05-29T14:19:14Z

Fixes privacy issues for some edge cases (mixing single input) and should improve privacy in general making each mixing rounds more uniform.

What privacy issue with mixing single input? Can I read somewhere about this?

In my opinion this will reduce privacy, by making it more easy for statistical analysis (which is one of biggest threats to PS). After activation of this code if I see mixing transaction with 12 inputs/outputs - I can clearly say: every participant have 4 inputs and outputs (and I can use this in statistical analysis). Before these changes it was impossible to reliably identify how many inputs/outputs have each participant.

UdjinM6 · 2018-05-29T15:02:05Z

That's exactly the issue - knowing exact number of inputs helps in statistical analysis if participants have different number of inputs. If number of inputs is the same for all of them then information leakage is reduced. If inputs are not carefully selected and/or outputs are not carefully spent then this can reveal the link (or at least this can provide some info for the guesses/analysis). For this cases you can think of number of (equal) inputs as of amounts e.g. participants with 3, 5 and 7 of 0.1 inputs are actually mixing 0.3, 0.5, 0.7. If all participants would instead mix say 3 inputs each (i.e. 3 x 0.3) then input selection/output spending would not leak any additional info.

InhumanPerfection · 2018-05-30T11:37:55Z

src/privatesend-client.cpp

+        std::vector<COutput> vCoinsTmp;
+        std::vector<CAmount> vecStandardDenoms = CPrivateSend::GetStandardDenominations();
+
+        bool fSelected = pwalletMain->SelectCoinsByDenominations(nSessionDenom, vecStandardDenoms[vecBits.front()], vecStandardDenoms[vecBits.front()] * PRIVATESEND_ENTRY_MAX_SIZE, vecTxDSInTmp, vCoinsTmp, nValueInTmp, 0, nPrivateSendRounds);


Here and in JoinExistingQueue you do selection for whole range of PS rounds. And you assume this amount of inputs to set nInputCount (or to check in JoinExistingQueue)

But the more value of nInputCount, the more likely failure of this code, since in PrepareDenominate selection is limited to only one PS round.

Yes, that's a good point but the code that follows the one you pointed to covers such cases, so shouldn't be an issue imo.

I know, but now mixing will be complete mess:

in some cases will be selected inputs with highest number of rounds

in some cases will be selected inputs with different(!) number of rounds

Edit: however I'm not sure is it good or bad for privacy ;-)

Ah, I see what you mean. But the former one is just the optimal strategy to get some PS balance quicker, while the later one is still a good one because it either advances low PS rounds inputs (which will help to get some more PS balance later) or mixes inputs which already have enough rounds and this shouldn't hurt either.

Or am I missing some another/new vulnerability here?

For the best possible privacy, the inputs likely should be selected randomly. Any other setup has the potential for data leakage and trace-ability, maybe there is a nice way to make it look random while also maintaining speed of getting a balance. However, the random setup would also have a problem for single input private-sending, unless there was very high liquidity of single input only mixing. Maybe we need to say you can't only mix one input?

However, having it be that "in some cases inputs will be selected with highest number of rounds" is likely a very bad idea as there will likely be grouping (when you look at Redeemed in and previous output) happening allowing for the breaking of a mix.

I agree with PaulieD. If the inputs were selected randomly then I think you couldn't really deduce this:

other 2 participants each have 3 inputs from other mixing transactions (txY and txZ)

. But if the most recent inputs are always used then I think that InhumanPerfection is correct.

I don't really understand the original issue. If someone could come up with an example scenario where the privacy is reduced by participants using a different number of inputs, then I might be able to understand it. The way I see it currently is that since a blockchain observer shouldn't be able to know which inputs belong to which mixing participant, then the observer also shouldn't be able to tell how many inputs belong to each participant.

@paulied Do you think prioritizing inputs with the highest number of rounds might have also caused #2092 or made it significantly more common?

I can confirm that there is a definite problem that this pr should fix. @UdjinM6 might be able to provide more details.

@Antti-Kaikkonen yeah, if the selections were random you certainly wouldn't see that

…shpay#2075)" This reverts commit 7ac4b97.

* Revert "Require all participants to submit equal number of inputs (#2075)" This reverts commit 7ac4b97. * Backward compatibility for nInputCount

Related to dashpay/dash#2075, dashpay/dash#2259, and dashpay/dash#2318

* P2P - Remove dsa and dsq input count Related to dashpay/dash#2075, dashpay/dash#2259, and dashpay/dash#2318 * P2P - Update dsq and dstx to include BLS signature Related to dashpay/dash#2352 * P2P - Update govobj and govobjvote to include BLS signature Related to dashpay/dash#2352

Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability

Require all participants to submit equal number of inputs

84f61ca

Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability

UdjinM6 added this to the 12.3 milestone May 15, 2018

UdjinM6 added the enhancement label May 15, 2018

UdjinM6 requested a review from codablock May 19, 2018 06:59

codablock approved these changes May 20, 2018

View reviewed changes

UdjinM6 merged commit 7ac4b97 into dashpay:develop May 26, 2018

Yoyae added a commit to monacocoin-net/monoeci-core that referenced this pull request May 28, 2018

Require all participants to submit equal number of inputs (dashpay#2075)

6cf5cc7

Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability

InhumanPerfection reviewed May 29, 2018

View reviewed changes

InhumanPerfection reviewed May 30, 2018

View reviewed changes

Antti-Kaikkonen mentioned this pull request Jun 7, 2018

Mixing Transactions with all inputs from the same transaction #2092

Closed

thephez mentioned this pull request Jun 11, 2018

[PrivateSend] Require all participants to submit equal number of inputs dash-docs/dash-docs#70

Closed

3 tasks

InhumanPerfection mentioned this pull request Jun 21, 2018

Fix an edge case in PrepareDenominate #2138

Merged

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Sep 4, 2018

Revert "Require all participants to submit equal number of inputs (da…

2f097e5

…shpay#2075)" This reverts commit 7ac4b97.

UdjinM6 mentioned this pull request Sep 4, 2018

Revert 2075 #2259

Merged

UdjinM6 added a commit that referenced this pull request Sep 5, 2018

Revert 2075 (#2259)

8c9cb29

* Revert "Require all participants to submit equal number of inputs (#2075)" This reverts commit 7ac4b97. * Backward compatibility for nInputCount

thephez added a commit to dash-docs/dash-docs that referenced this pull request Dec 26, 2018

P2P - Remove dsa and dsq input count

7f829d4

Related to dashpay/dash#2075, dashpay/dash#2259, and dashpay/dash#2318

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Require all participants to submit equal number of inputs (dashpay#2075)

8bd7249

Activates via SPORK_6_NEW_SIGS to avoid nInputCount malleability

UdjinM6 deleted the eqcount branch November 26, 2020 11:36

Uh oh!

Require all participants to submit equal number of inputs #2075

Require all participants to submit equal number of inputs #2075

Uh oh!

Conversation

UdjinM6 commented May 15, 2018

Uh oh!

codablock left a comment

Choose a reason for hiding this comment

Uh oh!

gladcow commented May 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InhumanPerfection commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented May 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InhumanPerfection Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Antti-Kaikkonen Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Antti-Kaikkonen Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

InhumanPerfection commented May 29, 2018 •

edited

Loading

InhumanPerfection Jun 1, 2018 •

edited

Loading

UdjinM6 Jun 1, 2018 •

edited

Loading

Antti-Kaikkonen Jun 1, 2018 •

edited

Loading

Antti-Kaikkonen Jun 1, 2018 •

edited

Loading