Coin selection improvement

[onvej-sl]

TLDR

This pull request improves the coin selection algorithm. The goal was to reduce the probability of anonymity loss that can occur when the algorithm selects coins whose anonymity scores are too far from each other.

Problem statement

The anonymity score of a transaction output is computed as an anonymity inherited from the inputs of the transaction belonging to the same person as the output plus an anonymity gained from the transaction itself. The inherited anonymity is either

• the average anonymity score of the inputs weighted by their values or
• the minimum anonymity score of the inputs.

Usually the first formula is used. Nevertheless, if there is a suspicion that the coinjoin sudoku problem could be solvable for the output then the second formula is used. The problem with this second case is that the (weighted) average anonymity score of the outputs can be smaller that the (weighted) average anonymity score of the inputs.

Let say, for example, a user registers the following inputs:

• 0.4 BTC with anonymity score 4
• 0.5 BTC with anonymity score 5
• 0.1 BTC with anonymity score 9

The average anonymity score of the inputs weighted by their values is (4 * 0.4 + 5 * 0.5 + 9 * 0.1) / (0.4 + 0.5 + 0.1) = 5. Let say the user registered one output with the value of 1 BTC. Suppose there is a suspicion the coinjon sudoku problem could be solvable for the output therefore the second formula is used. Suppose there is no other output with the value of 1 BTC which means the anonymity gain from the transaction is zero. The anonymity score of the output would be 4 which is decrease in the average anonymity score. From the anonymity point of view, it would be better for the user not to participate in the transaction at all. Moreover, the user sees that they had a utxo whose anonymity score was reduced from 9 to 5.
I agree this is a kind of an extreme example that nevertheless can occur when you register far more coins (in terms of the total amount) than your co-participants.

In e1773b5 and e1773b5, we extended the coin selection algorithm by a piece of code that removes coins from the final set of coins until the worst expected anonymity loss is less than a certain value. The worst expected anonymity loss is computed as the average difference between a coin anonymity and the coin with the minimum anonymity weighted by the value of the coin.

In our experience, it doesn't seem to be enough. The worst expected anonymity loss in the example above would be ((4 - 4) * 0.4 + (5 - 4) * 0.5 + (9 - 4) * 0.1) / (0.4 + 0.5 + 0.1) = 1.

Proposed solution

In my opinion, the formula for the anonymity loss should be modified in the following way:

• Instead of the weighted average it should use something that is more like the maximum. An extreme case would be max(4 - 4, 5 - 4, 9 - 4) = 4.
• Coin values should have a less weight in the weighted average. An extreme case would be ((4 - 4) + (5 - 4) + (9 - 4)) / 3 = 2.

I developed a formula that combines both the ideas. The formula is based on generalized weighted mean. Let say we have coins with anonymity scores a₁, ..., a_n and values v₁, ..., v_n. Let a = min(a₁, ..., a_n). The anonymity loss is computed as follows:

(((a₁ - a)^p * v₁^q + ... + (a_n - a)^p * v_n^q) / (v₁^q + ... + v_n^q))^1/p

The numbers p and q are suitable chosen numbers.

The formula has a few interesting special cases:

• If p = 1 and q = 1 then the formula becomes ((a₁ - a) * v₁ + ... + (a_n - a) * v_n) / (v₁ + ... + v_n), which is the formula used previously.
• If q = 0 and p goes to the infinity then the formula becomes max((a₁ - a), ..., (a_n - a)), which corresponds to the extreme case of the first idea.
• If p = 1 and q = 0 then the formula becomes ((a₁ - a) + ... + (a_n - a)) / n, which corresponds to the extreme case of the second idea.

I computed the anonymity loss for several coin sets and try to choose p and q so it corresponds best to my subjective perception of anonymity loss. I ended up with p = 10 and q = 0.8 which is in compliance with the two ideas that I had before.

[nopara73]

Thanks for this! This is an important PR. Can I get someone to review things before I do? Maybe @turbolay? You're pretty talented in understanding the most complex problems and this is one of the most important part of the code to get familiar with.

[turbolay]

Ty for the details on the context you provided in this PR, they were really useful.

Review/tl;dr

cACK

This PR is an adjustment of an existing concept that tries to mitigate the problem of big AnonScore loss because of big inputs with small AnonScore. It makes a wise decision about potentially removing coins suspected to impact AnonScore for all outputs negatively.
It relies on three magic numbers (MaxWeightedAnonLoss, p, q), so we need to test with #9375, I didn't do it yet.

If we change the AmountDecomposer, we will need to find the best magic numbers again, and I made another proposal on the AmountDecomposer that continues the same idea, so we can:

1. Consider my proposal and implement or not something (I will open dedicated Issur/PR)
2. Test this PR to be sure about magic numbers MaxWeightedAnonLoss, p, q and merge.

Reformulation

I will try to reformulate the problem and the purpose of this to be sure I and everyone else get it:

If an output is: unique && (non-standard || one of the two biggest outputs), it will inherit the lowest AnonScore of the client's inputs, what OP called the second formula. Otherwise (standard, majority of cases), a weighted average of inputs is used, the first formula.

Formula choice happens here (I inverted the if in my short explanation above so that it's easier to understand):

WalletWasabi/WalletWasabi/Blockchain/Analysis/BlockchainAnalyzer.cs

Lines 141 to 144 in a695f7d

	if (anonymityGain < 1)
	{
	// When WW2 denom output isn't too large, then it's not change.
	if (tx.IsWasabi2Cj is true && StdDenoms.Contains(virtualOutput.Amount.Satoshi) && virtualOutput.Amount < secondLargestOutputAmount)

This PR tries to be more precise on whether or not we should recursively remove an input from the selection if one selected coin can potentially have a big negative impact on the AnonymityScore outputs will inherit, regardless if they use the first or second formula.

The formula implemented in this PR based on generalized weight mean gives a score to the selection regarding the potential negative impact on AnonScore. It is used to decide if we should remove a coin (max acceptable threshold) or to rank subsets between them (the less the score, the better) to see which coin is the best to remove.

The parameters are for adjusting the balance between the weight of the amount (q) and the weight of AnonScore (p).

Go further proposal

Intuitively, a different approach than your PR (not mutually exclusive) would be for the AmountDecomposer to try to make "change" output (big non-standard) as small in amount possible, because they are the ones degrading outputs' AnonScore the most common case: first formula

AnonScore gain dynamics will be improved the same way as they are with this PR: if the "change" (low AnonScore big coin) from last CJ is smaller in amount, well in the next CJ it will take part as an input it will impact less negatively the WeigthedAverage than a bigger change like currently. The potential for improvement here seems really consequent.

For example, this TX: https://mempool.space/tx/3e3fbad73bbca60615b5fe1724c94a675192b10f468aa58d728303e5d19b6eef#flow=&vin=0
Output#1 42..BTC used the second formula (biggest and unique).

Instead of one 42 BTC output, AmountDecomposer could probably have made three more standards, 11.62..BTC, with still output in the second formula but of only 7.14..BTC.

This is decided here:

WalletWasabi/WalletWasabi/WabiSabi/Client/AmountDecomposer.cs

Lines 296 to 298 in 5660501

	var orderedCandidates = preCandidates
	.OrderBy(x => x.Cost) // Less cost is better.
	.ThenBy(x => x.Decomp.All(x => denomHashSet.Contains(x)) ? 0 : 1) // Prefer no change.

My proposition is: change less cost is better to lower change is better if cost is acceptable

[kiminuo]

An alternative would be:

Suggested change

	var coinsToSelectFrom = Enumerable
	.Empty<SmartCoin>()
	.Append(bigCoinWithSmallAnonymity1)
	.Append(bigCoinWithSmallAnonymity2)
	.Append(smallCoinWithBigAnonymity)
	.ToList();
	List<SmartCoin> coinsToSelectFrom = new() { bigCoinWithSmallAnonymity1, bigCoinWithSmallAnonymity2, smallCoinWithBigAnonymity };

or

Suggested change

	var coinsToSelectFrom = Enumerable
	.Empty<SmartCoin>()
	.Append(bigCoinWithSmallAnonymity1)
	.Append(bigCoinWithSmallAnonymity2)
	.Append(smallCoinWithBigAnonymity)
	.ToList();
	List<SmartCoin> coinsToSelectFrom = new()
	{
	bigCoinWithSmallAnonymity1,
	bigCoinWithSmallAnonymity2,
	smallCoinWithBigAnonymity
	};

[kiminuo]

These two checks make me think that the asserts are more general because we use random number generator but the rng returns a fixed number. Would it make sense to run this test, eg, 100 times with random RNG seed (i.e. make it random instead of deterministic) so that the test can actually fail if something is not as expected?

[onvej-sl]

Yes, it would make sense. And I suppose the same holds true for other tests in this file. For this reason I suggest you to address it in another issue or pull request.

[kiminuo]

Would it make sense to switch from [Fact] to [Theory] with multiple AnonymitySet values to make the test more robust?

[onvej-sl]

Yes, it would make sense. Nevertheless, it is not easy to generate such test vectors, because the coin selection algorithm is very complex and doesn't behave deterministically.

[kiminuo]

I would check the parameters more:

Suggested change

	if (p == 0)
	{
	throw new ArgumentException("Non-zero value is expected.", nameof(p));
	}

If this is not added, it will crash later in because 1 / 0 is not defined (AFAIK).

[onvej-sl]

Done in 3cd7ea2.

[kiminuo]

The changes look great.

I would like to re-review this PR in the afternoon.

[kiminuo]

LGTM. Very nice description.

Another pair of eyes for reviewing would be certainly useful as this annon stuff in general is non-trivial.

[nopara73]

Unless other comments arrive I am merging this a week from now.

[nopara73]

@turbolay asked me to hold on with the merge, so I won't be merging this for now.

[turbolay]

The reason is that it seems this PR, even if the concept makes total sense, tends to create more coins for the user than master, which result in no gain on mining fees/time, even if the PR reduces the AnonScore loss.
Also, in the standard case, the positive impact seems to exist but is relatively low.
Because testing and understanding the complete impact of code related to Coin Selection is complex, I don't want to rush a conclusion, wither positive or negative. The safe thing to do is wait and try to understand unforeseeable consequences.

[nopara73]

@turbolay any news here?

[turbolay]

I didn't test more after my previous statement because of prioritization.
What I'm wondering about with this change is its potential negative impact on crumbling, something we are already suffering from.
I can be easily convinced of a merge because the PR makes total sense and is a direct improvement.
But I still think that merging without further testing for this specific case (does it or not make crumbling worse?) would be, I quote:

[A] leap into the abyss of the unknown, surrendering the security of our familiar existence to the whims of chance and entropy while creating the tower of Babel.

-> I fear an unforeseeable consequence, something bad we do not see coming. Yet, I agree and understand that it can also tremendously improve the current consolidation and privacy loss issues.

[nopara73]

No need for concern, my intention in commenting wasn't to press you. I simply wanted to ensure that the matter isn't overlooked.

[stale]

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.

[turbolay]

We basically ended up implementing what I explained in my comment:

Intuitively, a different approach than your PR (not mutually exclusive) would be for the AmountDecomposer to try to make "change" output (big non-standard) as small as possible

Now that the work on the AmountDecomposer is finished, it would be interesting to consider again this PR.

At first glance, I believe that its impact will now be minimal, but I think the PR still makes sense.

[stale]

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.