Coin selection improvement #8938

onvej-sl · 2022-08-15T14:26:20Z

Coauthored with @nopara73 and @andrewkozlik.

nopara73 · 2022-08-18T09:00:17Z

WalletWasabi/WabiSabi/Client/CoinJoinClient.cs

 		int inputCount = Math.Min(
 			privateCoins.Length + allowedNonPrivateCoins.Count,
-			consolidationMode ? MaxInputsRegistrableByWallet : GetInputTarget(utxoCount, minUtxoCountTarget, rnd));


Note that these things got removed because the implementation down aims to address the same issue, just in a more intelligent way.

nopara73 · 2022-08-18T09:01:18Z

WalletWasabi/WabiSabi/Client/CoinJoinClient.cs

 		foreach (var coin in finalCandidate
+			.Except(new [] {selectedNonPrivateCoin})


Note this makes sure that selectedNonPrivateCoin participates, as that's what we want to mix the most.

This is actually unrelated to this PR, just a minor bugfix.

nopara73 · 2022-08-18T09:02:47Z

WalletWasabi/WabiSabi/Client/CoinJoinClient.cs

@@ -815,6 +812,30 @@ private void LogCoinJoinSummary(ImmutableArray<AliceClient> registeredAliceClien
 			}
 		}

+		double winnerCost = 0;


@onvej-sl would you mind adding comments to what we're trying to do here?

nopara73 · 2022-08-18T09:03:39Z

WalletWasabi/WabiSabi/Client/CoinJoinClient.cs

+			winner = bestReducedWinner;
+			winnerCost = minCost;
+		}
+
 		if (winner.Count != finalCandidate.Count())
 		{
 			Logger.LogDebug($"Optimizing selection, removing coins coming from the same tx.");


The logging is now incorrect here. It was tailor made to the previous logic, but the added logic changes things.

I'd suggest to move these logs up before the new code was added, then duplicate the logs here as well so we can see how the new code changed the selection.

nopara73 · 2022-08-18T09:06:53Z

WalletWasabi/WabiSabi/Client/CoinJoinClient.cs

@@ -815,6 +812,30 @@ private void LogCoinJoinSummary(ImmutableArray<AliceClient> registeredAliceClien
 			}
 		}

+		double winnerCost = 0;
+		while ((winner.Sum(x => x.Amount) > liquidityClue) && (winnerCost/winner.Sum(x => x.Amount) < 3))


3 is a magic number here, this I'll test and figure out very well after we have the logs: https://github.com/zkSNACKs/WalletWasabi/pull/8938/files#r948849346

stale · 2022-10-18T23:31:48Z

This has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nopara73 · 2022-10-19T10:09:15Z

FTR @turbolay and @Szpoti are creating unit tests, with which hopefully we can finetune this PR.

Szpoti

Good changes, actually works better, but in cases when the MaxWeightedAnonLoss is low (as it's expected for some to be low) this can cause an infinite loop, as removing coins from the winner doesn't decrease the anonLoss below the threshold.

Szpoti · 2022-11-04T12:13:20Z

WalletWasabi/WabiSabi/Client/CoinJoinClient.cs

+				if (anonLoss <= bestAnonLoss)
+				{
+					bestAnonLoss = anonLoss;
+					bestReducedWinner = reducedWinner.ToList();
+				}


Suggested change

if (anonLoss <= bestAnonLoss)

{

bestAnonLoss = anonLoss;

bestReducedWinner = reducedWinner.ToList();

}

if (anonLoss <= bestAnonLoss)

{

bestAnonLoss = anonLoss;

bestReducedWinner = reducedWinner.ToList();

}

if (winner == bestReducedWinner)

{

break;

}

My thinking: Prevent the infinite loop, by checking if we couldn't reduce our anonLoss no further below our expected threshold before. If so, use our remaining coins in the round.

as removing coins from the winner doesn't decrease the anonLoss below the threshold

I don't think this is the problem. You can always remove the coin with the highest anonymity, which decreases the anonymity loss.

The problem arises when winner contains exactly the same coins as selectedNonPrivateCoin, which means there is no coin that can be removed.

Anyway, the change you suggested does solve the problem.

Do you agree, @andrewkozlik?

Good changes, actually works better, but in cases when the MaxWeightedAnonLoss is low (as it's expected for some to be low) this can cause an infinite loop, as removing coins from the winner doesn't decrease the anonLoss below the threshold.

By the way, did you actually experienced the algorithm ends up in an endless loop? @Szpoti

The reason why Ondřej is asking is because I think this should not enter an infinite loop.

Firstly selectedNonPrivateCoin is a single coin. There are two possible cases:

winner contains one coin. Then this coin is selectedNonPrivateCoin and winnerAnonLoss is 0, therefore the while-loop will end, because the second condition isn't satisfied (assuming that MaxWeightedAnonLoss is non-negative, which it certainly shouldn't be).

winner contains more than one coin. Then the foreach-loop will select one of the coins in winner which is not selectedNonPrivateCoin and it will remove it.

So either the while-cycle ends or a coin is removed. So the change proposed by @Szpoti is OK, but it seems to me that it's unreachable.

Yes, we are running into an infinite loop in the case that all GetAnonLoss(reducedWinner); are higher than winnerAnonLoss. I get that intuitively it should not happen, but it seems that it can.

During testing, here's a case where it happened:

winner = (Index, Amount, AnonymitySet) (0, 0.04782969, 29), (1, 0.11665525, 9), (2, 0.04782969, 9) winnerAnonLoss = 4.51 selectedNonPrivateCoin = coin with index 0 anonLoss without 1 = 10 anonLoss without 2 = 5.82

-> infinite loop

Good job guys! We didn't realize that after the change in 346ed7c the removal of coins is no longer monotonic in the anon loss...

@andrewkozlik do you ACK @Szpoti 's suggested change or is there further work to do to fix this problem?
This PR makes a great difference it would be nice to merge it.

I discussed it with Andrew a bit before he went on vacation and we concluded that the suggested change is OK.

Failed to push to the remote repository. See the Output window for more details. Pushing pr/8938 Pushing refs/heads/pr/8938 Remote: Permission to trezor/WalletWasabi.git denied to molnard. Error encountered while pushing to the remote repository: Git failed with a fatal error. Git failed with a fatal error. unable to access 'https://github.com/trezor/WalletWasabi.git/': The requested URL returned error: 403

Created another PR https://github.com/zkSNACKs/WalletWasabi/pull/9555/files?diff=split&w=1

MaxHillebrand · 2022-11-15T08:06:31Z

It seems that this greatly reduces the client side randomness in regards to input and output registration. Even though this increases reliability and speed, I think it does decrease privacy. Maybe it's worth to consider to keep the randomization for the privacy profile only?

(this might be a nonsense comment, I didn't look at the code)

turbolay · 2022-11-15T15:15:14Z

It seems that this greatly reduces the client side randomness in regards to input and output registration. Even though this increases reliability and speed, I think it does decrease privacy. Maybe it's worth to consider to keep the randomization for the privacy profile only?

On master, we could select more or less any coin randomly, now we will remove some of these coins if they harm AnonScore.
With figures from the test, we are going from a 150% spread in term of number of rounds necessary to reach a certain value of AnonScore to a 10% spread, with a great optimization in any case.

Also, it only really applies for big values of required AnonScore, so only “Anonymous” profile, but this profile is by itself random (from 50 to 100) so the spread is still huge.

I can't think of any heuristics we could use to recognize WW inputs now that the AnonScore gain will be more consistent than before. Lot of things are still random (we select a random order of coins, number of inputs is also more or less random, we alternate coin selection etc…).

molnard · 2022-11-16T12:37:35Z

#8938 (comment)

molnard · 2022-11-16T13:07:14Z

Continuation #8938

molnard · 2022-11-16T13:44:06Z

#9555 got merged.

@nopara73 @andrewkozlik @onvej-sl @turbolay @Szpoti @MaxHillebrand awesome work 🥇

onvej-sl added 2 commits August 15, 2022 16:12

Remove coins that harm anonymity

e1773b5

Always target MaxInputsRegistrableByWallet

9947c75

pull-request-size bot added the size/M label Aug 15, 2022

nopara73 reviewed Aug 18, 2022

View reviewed changes

molnard mentioned this pull request Oct 17, 2022

Fix privacy mode coinjoin for whale users #9332

Closed

3 tasks

stale bot added the stale label Oct 18, 2022

nopara73 removed the stale label Oct 19, 2022

turbolay mentioned this pull request Oct 24, 2022

[Test] Deterministic AnonScoreGainTests #9375

Closed

fixup! Remove coins that harm anonymity

346ed7c

Szpoti suggested changes Nov 4, 2022

View reviewed changes

molnard mentioned this pull request Nov 14, 2022

Remove unused method from LiquidityClue #9484

Closed

molnard mentioned this pull request Nov 16, 2022

Coin selection improvement #9555

Merged

molnard closed this Nov 16, 2022

turbolay mentioned this pull request May 16, 2023

[Breakthrough] Occasional Anonymity Loss #10708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coin selection improvement #8938

Coin selection improvement #8938

onvej-sl commented Aug 15, 2022

nopara73 Aug 18, 2022

nopara73 Aug 18, 2022

nopara73 Aug 18, 2022 •

edited

Loading

nopara73 Aug 18, 2022

nopara73 Aug 18, 2022

nopara73 Aug 18, 2022

nopara73 Aug 18, 2022

stale bot commented Oct 18, 2022

nopara73 commented Oct 19, 2022

Szpoti left a comment

Szpoti Nov 4, 2022 •

edited

Loading

onvej-sl Nov 4, 2022

onvej-sl Nov 7, 2022

andrewkozlik Nov 7, 2022

turbolay Nov 8, 2022 •

edited

Loading

andrewkozlik Nov 8, 2022

turbolay Nov 15, 2022

onvej-sl Nov 15, 2022

molnard Nov 16, 2022

MaxHillebrand commented Nov 15, 2022

turbolay commented Nov 15, 2022

molnard commented Nov 16, 2022

molnard commented Nov 16, 2022

molnard commented Nov 16, 2022

		foreach (var coin in finalCandidate
		.Except(new [] {selectedNonPrivateCoin})

Coin selection improvement #8938

Coin selection improvement #8938

Conversation

onvej-sl commented Aug 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopara73 Aug 18, 2022 • 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

stale bot commented Oct 18, 2022

nopara73 commented Oct 19, 2022

Szpoti left a comment

Choose a reason for hiding this comment

Szpoti Nov 4, 2022 • 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

turbolay Nov 8, 2022 • 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

MaxHillebrand commented Nov 15, 2022

turbolay commented Nov 15, 2022

molnard commented Nov 16, 2022

molnard commented Nov 16, 2022

molnard commented Nov 16, 2022

nopara73 Aug 18, 2022 •

edited

Loading

Szpoti Nov 4, 2022 •

edited

Loading

turbolay Nov 8, 2022 •

edited

Loading