Skip to content
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

privatesend: Implement Random Round Mixing #3661

Merged
merged 13 commits into from Aug 30, 2020

Conversation

PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Aug 17, 2020

The first commit simply uses "GetRealOutpointPrivateSendRounds" in a few behind the scenes places where I saw it

The second commit:

Add "ps_salt" value to walletdb
This value is used to deterministically pick a random number of rounds to mix, between N and N + GetRandomRounds. A salt is needed in addition to the inputs hash to ensure that an attacker learns nothing from looking at the blockchain.

Third Commit:
Implement Random Round Mixing

This implements "Random Round Mixing." Previously, attempted attacks on PrivateSend assumed that all inputs had been mixed for the same number of rounds. Normally assuming 2,4,8 or 16.

While none of these attacks have been successful, they still teach what we can do to make our system more robust, and one of those ways is to implement "Random Round Mixing".

Under the previous system, inputs were mixed up until N rounds (configured by user). At this point, the input was considered mixed, and could be private-sent. Under this new system, an input will be mixed to N rounds like prior. However, at this point, Sha256d(input, salt) will be calculated (note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed for one more round.
This results in an exponential decay where if you mix a set of inputs, half of those inputs will be mixed for N rounds, 1/4 will be mixed N+1, 1/8 will be mixed N+2, etc. This current implementation caps it at N+3. This results in mixing an average of N+0.875 rounds. If you removed the cap, you would mix on average N+1 rounds.

I tested this by starting three wallets mixing, and the distribution (while not perfect) was exactly how I would expect with about 50%, 25%, 12.5%, 12.5%. This also fixes #3525

Signed-off-by: pasta <pasta@dashboost.org>
This value is used to deterministically pick a random number of rounds to mix, between N and N + GetRandomRounds. A salt is needed in addition to the inputs hash to ensure that an attacker learns nothing from looking at the blockchain.

Signed-off-by: pasta <pasta@dashboost.org>
This implements "Random Round Mixing." Previously, attempted attacks on PrivateSend assumed that all inputs had been mixed for the same number of rounds. Noramlly assuming 2,4,8 or 16.

While none of these attacks have been successful, they still teach what we can do to make our system more robust, and one of those ways is to implement "Random Round Mixing".

Under the previous system, inputs were mixed up until N rounds (configured by user). At this point, the input was considered mixed, and could be private-sent. Under this new system, an input will be mixed to N rounds like prior. However, at this point, Sha256d(input, salt) will be calculated (note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed again.
This results in an exponential decay where if you mix a set of inputs, half of those inputs will be mixed for N rounds, 1/4 will be mixed N+1, 1/8 will be mixed N+2, etc. This current implementation caps it at N+2. This results in mixing an average of N+0.875 rounds. If you removed the cap, you would mix on average N+1 rounds.

Signed-off-by: pasta <pasta@dashboost.org>
@PastaPastaPasta PastaPastaPasta changed the title Implement Random Round Mixing [PrivateSend] Implement Random Round Mixing Aug 17, 2020
@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 Aug 17, 2020
@PastaPastaPasta PastaPastaPasta changed the title [PrivateSend] Implement Random Round Mixing privatesend: Implement Random Round Mixing Aug 17, 2020
@UdjinM6
Copy link

UdjinM6 commented Aug 21, 2020

Pls see https://github.com/UdjinM6/dash/commits/pr3661

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Aug 21, 2020

Added your commits, plus some modifications @UdjinM6

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
Signed-off-by: pasta <pasta@dashboost.org>
Signed-off-by: pasta <pasta@dashboost.org>
@PastaPastaPasta PastaPastaPasta added this to the 16 milestone Aug 23, 2020
@xdustinface
Copy link

xdustinface commented Aug 24, 2020

Nice 👍 Didn't test it yet but looks good at a first glance.

(note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed for one more

What about at least adding c1b7027 to this PR?

Another thought i had which i didn't implemented yet because i don't think its absolutely needed is: We could dump the salt to the dump-file in dumpwallet rpc and write it back to the wallet in importwallet rpc. However, without having it in the dump/import process i guess nothing bad would happen, just some more mixing after the import (which probably can then still happen if you import to a "new" salt to a non empty wallet with already mixed funds...), other thoughts?

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Aug 24, 2020

Nice +1 Didn't test it yet but looks good at a first glance.

(note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed for one more

What about at least adding c1b7027 to this PR?

Another thought i had which i didn't implemented yet because i don't think its absolutely needed is: We could dump the salt to the dump-file in dumpwallet rpc and write it back to the wallet in importwallet rpc. However, without having it in the dump/import process i guess nothing bad would happen, just some more mixing after the import (which probably can then still happen if you import to a "new" salt to a non empty wallet with already mixed funds...), other thoughts?

I didn't use single Sha256 cause it adds ~7 LOC and makes it a bit less readable imo, and I don't know if that's actually valuable... Maybe in the future we could modify the Hash function to take a "HashType" but we'd probably want to wait until we diverge from BTC codebase to do that. Or we could add a HashSha256S function like the existing Hash function, so that the code is more readable

@UdjinM6
Copy link

UdjinM6 commented Aug 24, 2020

Good point on dumpwallet 👍 but I'm not sure if parsing dump file for "ps_salt" in importwallet rpc is a good idea - it might break some custom 3rd-party parsers if there are any. Maybe we could just add yet another comment line in the header of the dump file (like we do for mnemonic etc. of hd wallets already) and also allow users to change ps salt on the fly via some new rpc (e.g. setprivatesendsalt) instead. This way you could use the same salt in any wallet, including a newly generated/imported one (could this be helpful for tests?). You could even drop the old salt and force your wallet to generate a new one by specifying the null hash (or simply "0").

Could also add "ps_salt" entry in getprivatesendinfo rpc output I guess.

@xdustinface
Copy link

xdustinface commented Aug 24, 2020

I didn't use single Sha256 cause it adds ~7 LOC and makes it a bit less readable imo, and I don't know if that's actually valuable..

So you prefer to have some less lines of code (which aren't hard to read in any way imo) and therefor run SHA256 twice instead of just running once which basically cuts execution time for hashing in IsFullyMixed by half? 🙈

Good point on dumpwallet 👍 but I'm not sure if parsing dump file for "ps_salt" in importwallet rpc is a good idea - it might break some custom 3rd-party parsers if there are any. Maybe we could just add yet another comment line in the header of the dump file (like we do for mnemonic etc. of hd wallets already) and also allow users to change ps salt on the fly via some new rpc (e.g. setprivatesendsalt) instead. This way you could use the same salt in any wallet, including a newly generated/imported one (could this be helpful for tests?). You could even drop the old salt and force your wallet to generate a new one by specifying the null hash (or simply "0").

Could also add "ps_salt" entry in getprivatesendinfo rpc output I guess.

Oh right, adding it as comment there + RPC sounds good imo. I think i like the idea of having setprivatesendsalt 👍 Not sure though if adding it to getprivatesendinfo is the best.. maybe also have a getprivatesendsalt instead to require a explicit call of it as it's kind of sensible data, no?

@UdjinM6
Copy link

UdjinM6 commented Aug 26, 2020

@PastaPastaPasta re hashing in IsFullyMixed: how about e093978?

Uses just 1 sha256 instead of 3 (1 in SerializeHash + 2 in Hash)
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Aug 27, 2020

@PastaPastaPasta re hashing in IsFullyMixed: how about e093978?

Done

UdjinM6
UdjinM6 previously approved these changes Aug 28, 2020
Copy link

@UdjinM6 UdjinM6 left a comment

ACK

Signed-off-by: pasta <pasta@dashboost.org>
Copy link

@xdustinface xdustinface left a comment

ACK

Copy link

@UdjinM6 UdjinM6 left a comment

re-ACK

@UdjinM6 UdjinM6 merged commit 533b2fe into dashpay:develop Aug 30, 2020
1 check passed
@PastaPastaPasta PastaPastaPasta deleted the randomize-ps-rounds branch Sep 8, 2020
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Sep 12, 2020
* Use GetRealOut... instead of Capped

Signed-off-by: pasta <pasta@dashboost.org>

* Add "ps_salt" value to walletdb

This value is used to deterministically pick a random number of rounds to mix, between N and N + GetRandomRounds. A salt is needed in addition to the inputs hash to ensure that an attacker learns nothing from looking at the blockchain.

Signed-off-by: pasta <pasta@dashboost.org>

* Implement Random Round Mixing

This implements "Random Round Mixing." Previously, attempted attacks on PrivateSend assumed that all inputs had been mixed for the same number of rounds. Noramlly assuming 2,4,8 or 16.

While none of these attacks have been successful, they still teach what we can do to make our system more robust, and one of those ways is to implement "Random Round Mixing".

Under the previous system, inputs were mixed up until N rounds (configured by user). At this point, the input was considered mixed, and could be private-sent. Under this new system, an input will be mixed to N rounds like prior. However, at this point, Sha256d(input, salt) will be calculated (note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed again.
This results in an exponential decay where if you mix a set of inputs, half of those inputs will be mixed for N rounds, 1/4 will be mixed N+1, 1/8 will be mixed N+2, etc. This current implementation caps it at N+2. This results in mixing an average of N+0.875 rounds. If you removed the cap, you would mix on average N+1 rounds.

Signed-off-by: pasta <pasta@dashboost.org>

* Make PS salt a private member of CWallet, tweak the way it's initialized

* Introduce `CWallet::IsFullyMixed` and use it everywhere instead of comparing rounds directly to ensure consistency between coin selection logic, balance calculations and gui

* Tweak `GetRealOutpointPrivateSendRounds` to respect random rounds

* Tweak IsFullyMixed to make decision on a per-outpoint basis instead of a per-tx one

* make a comment doxygen

Signed-off-by: pasta <pasta@dashboost.org>

* Rename GetPrivateSendSalt InitPrivateSendSalt, since it is not a getter

Signed-off-by: pasta <pasta@dashboost.org>

* move the comment below GetRounds call

Signed-off-by: pasta <pasta@dashboost.org>

* don't use GetCappedOutpointPrivateSendRounds when printing to RPC

Signed-off-by: pasta <pasta@dashboost.org>

* Simplify hashing in IsFullyMixed

Uses just 1 sha256 instead of 3 (1 in SerializeHash + 2 in Hash)

* undo comment change

Signed-off-by: pasta <pasta@dashboost.org>

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Sep 12, 2020
* Use GetRealOut... instead of Capped

Signed-off-by: pasta <pasta@dashboost.org>

* Add "ps_salt" value to walletdb

This value is used to deterministically pick a random number of rounds to mix, between N and N + GetRandomRounds. A salt is needed in addition to the inputs hash to ensure that an attacker learns nothing from looking at the blockchain.

Signed-off-by: pasta <pasta@dashboost.org>

* Implement Random Round Mixing

This implements "Random Round Mixing." Previously, attempted attacks on PrivateSend assumed that all inputs had been mixed for the same number of rounds. Noramlly assuming 2,4,8 or 16.

While none of these attacks have been successful, they still teach what we can do to make our system more robust, and one of those ways is to implement "Random Round Mixing".

Under the previous system, inputs were mixed up until N rounds (configured by user). At this point, the input was considered mixed, and could be private-sent. Under this new system, an input will be mixed to N rounds like prior. However, at this point, Sha256d(input, salt) will be calculated (note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed again.
This results in an exponential decay where if you mix a set of inputs, half of those inputs will be mixed for N rounds, 1/4 will be mixed N+1, 1/8 will be mixed N+2, etc. This current implementation caps it at N+2. This results in mixing an average of N+0.875 rounds. If you removed the cap, you would mix on average N+1 rounds.

Signed-off-by: pasta <pasta@dashboost.org>

* Make PS salt a private member of CWallet, tweak the way it's initialized

* Introduce `CWallet::IsFullyMixed` and use it everywhere instead of comparing rounds directly to ensure consistency between coin selection logic, balance calculations and gui

* Tweak `GetRealOutpointPrivateSendRounds` to respect random rounds

* Tweak IsFullyMixed to make decision on a per-outpoint basis instead of a per-tx one

* make a comment doxygen

Signed-off-by: pasta <pasta@dashboost.org>

* Rename GetPrivateSendSalt InitPrivateSendSalt, since it is not a getter

Signed-off-by: pasta <pasta@dashboost.org>

* move the comment below GetRounds call

Signed-off-by: pasta <pasta@dashboost.org>

* don't use GetCappedOutpointPrivateSendRounds when printing to RPC

Signed-off-by: pasta <pasta@dashboost.org>

* Simplify hashing in IsFullyMixed

Uses just 1 sha256 instead of 3 (1 in SerializeHash + 2 in Hash)

* undo comment change

Signed-off-by: pasta <pasta@dashboost.org>

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
UdjinM6 added a commit that referenced this pull request Sep 13, 2020
ckti pushed a commit to ckti-ioncore-current/ion that referenced this pull request Mar 28, 2021
* Use GetRealOut... instead of Capped

Signed-off-by: pasta <pasta@dashboost.org>

* Add "ps_salt" value to walletdb

This value is used to deterministically pick a random number of rounds to mix, between N and N + GetRandomRounds. A salt is needed in addition to the inputs hash to ensure that an attacker learns nothing from looking at the blockchain.

Signed-off-by: pasta <pasta@dashboost.org>

* Implement Random Round Mixing

This implements "Random Round Mixing." Previously, attempted attacks on PrivateSend assumed that all inputs had been mixed for the same number of rounds. Noramlly assuming 2,4,8 or 16.

While none of these attacks have been successful, they still teach what we can do to make our system more robust, and one of those ways is to implement "Random Round Mixing".

Under the previous system, inputs were mixed up until N rounds (configured by user). At this point, the input was considered mixed, and could be private-sent. Under this new system, an input will be mixed to N rounds like prior. However, at this point, Sha256d(input, salt) will be calculated (note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed again.
This results in an exponential decay where if you mix a set of inputs, half of those inputs will be mixed for N rounds, 1/4 will be mixed N+1, 1/8 will be mixed N+2, etc. This current implementation caps it at N+2. This results in mixing an average of N+0.875 rounds. If you removed the cap, you would mix on average N+1 rounds.

Signed-off-by: pasta <pasta@dashboost.org>

* Make PS salt a private member of CWallet, tweak the way it's initialized

* Introduce `CWallet::IsFullyMixed` and use it everywhere instead of comparing rounds directly to ensure consistency between coin selection logic, balance calculations and gui

* Tweak `GetRealOutpointPrivateSendRounds` to respect random rounds

* Tweak IsFullyMixed to make decision on a per-outpoint basis instead of a per-tx one

* make a comment doxygen

Signed-off-by: pasta <pasta@dashboost.org>

* Rename GetPrivateSendSalt InitPrivateSendSalt, since it is not a getter

Signed-off-by: pasta <pasta@dashboost.org>

* move the comment below GetRounds call

Signed-off-by: pasta <pasta@dashboost.org>

* don't use GetCappedOutpointPrivateSendRounds when printing to RPC

Signed-off-by: pasta <pasta@dashboost.org>

* Simplify hashing in IsFullyMixed

Uses just 1 sha256 instead of 3 (1 in SerializeHash + 2 in Hash)

* undo comment change

Signed-off-by: pasta <pasta@dashboost.org>

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 5, 2022
* Use GetRealOut... instead of Capped

Signed-off-by: pasta <pasta@dashboost.org>

* Add "ps_salt" value to walletdb

This value is used to deterministically pick a random number of rounds to mix, between N and N + GetRandomRounds. A salt is needed in addition to the inputs hash to ensure that an attacker learns nothing from looking at the blockchain.

Signed-off-by: pasta <pasta@dashboost.org>

* Implement Random Round Mixing

This implements "Random Round Mixing." Previously, attempted attacks on PrivateSend assumed that all inputs had been mixed for the same number of rounds. Noramlly assuming 2,4,8 or 16.

While none of these attacks have been successful, they still teach what we can do to make our system more robust, and one of those ways is to implement "Random Round Mixing".

Under the previous system, inputs were mixed up until N rounds (configured by user). At this point, the input was considered mixed, and could be private-sent. Under this new system, an input will be mixed to N rounds like prior. However, at this point, Sha256d(input, salt) will be calculated (note: this likely could be a more efficient hash function than double sha256, but that can be done in another PR / version if needed). If (hash % 2 == 0), then the input will be mixed again.
This results in an exponential decay where if you mix a set of inputs, half of those inputs will be mixed for N rounds, 1/4 will be mixed N+1, 1/8 will be mixed N+2, etc. This current implementation caps it at N+2. This results in mixing an average of N+0.875 rounds. If you removed the cap, you would mix on average N+1 rounds.

Signed-off-by: pasta <pasta@dashboost.org>

* Make PS salt a private member of CWallet, tweak the way it's initialized

* Introduce `CWallet::IsFullyMixed` and use it everywhere instead of comparing rounds directly to ensure consistency between coin selection logic, balance calculations and gui

* Tweak `GetRealOutpointPrivateSendRounds` to respect random rounds

* Tweak IsFullyMixed to make decision on a per-outpoint basis instead of a per-tx one

* make a comment doxygen

Signed-off-by: pasta <pasta@dashboost.org>

* Rename GetPrivateSendSalt InitPrivateSendSalt, since it is not a getter

Signed-off-by: pasta <pasta@dashboost.org>

* move the comment below GetRounds call

Signed-off-by: pasta <pasta@dashboost.org>

* don't use GetCappedOutpointPrivateSendRounds when printing to RPC

Signed-off-by: pasta <pasta@dashboost.org>

* Simplify hashing in IsFullyMixed

Uses just 1 sha256 instead of 3 (1 in SerializeHash + 2 in Hash)

* undo comment change

Signed-off-by: pasta <pasta@dashboost.org>

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants