Detecting and (avoid) Banning Wasabi coinjoins #11946

lontivero · 2023-11-16T01:55:36Z

This PR fixes a few very important things in the process of detecting and banning round disruptions by spending coins participating in coinjoin rounds.

Given a transaction can have multiple inputs and those can be participating in multiple rounds, a single transaction can be used to disrupt more than one round. This fact was not part of the original design and for that reason the RoundDisruption offense only considered one round instead of multiple rounds.
Wasabi was banning the inputs of the attacking transaction but those coins are spent and it makes no sense to ban them, instead it should have banned the coins that the attacking transaction creates.
There was a small time frame where a received wasabi coinjoin transaction was not in the CoinJoinIdStore or in the RoundStates either because of that some coinjoins were detected as double spending attacks. Fortunately the mistake in point 2 (above) made this a non-issue. This is solved by querying Arena.Rounds instead of Arena.RoundStates.

Additionally to these problems this PR adds:

Unit tests for the code
Code refactoring to make it more readable
Heuristic to make absolutely sure we don't ban Wasabi coinjoins. Sounds unnecessary but I want to be 100% sure.

This is because the two mechanisms for detecting incoming coinjoin transactions are not enough in many cases and we were banning our own coinjoin inputs.

Szpoti

LGTM, only nits

WalletWasabi/WabiSabi/Backend/DoSPrevention/Prison.cs

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/UtxoPrisonWardenTests.cs

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/PrisonTests.cs

kiminuo

Only partial review

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/PrisonTests.cs

WalletWasabi/WabiSabi/Backend/DoSPrevention/Offender.cs

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/OffenderSerializationTests.cs

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs

turbolay

A question before submitting feedback: It looks like as an attacker I can create a tx that will pass IsWasabiCoinJoinLookingTx and therefore be able to disrupt the rounds freely with the outputs of this transaction. Is this true? Is this a problem?

If yes to both, maybe we can make sure that an unused key belonging to coordinator's xpub is used as output

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs

turbolay · 2023-11-17T13:34:27Z

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs

+		&& tx.Inputs.Count >= Config.MinInputCountByRound
+		&& tx.Inputs.Count <= Config.MaxInputCountByRound
+		&& tx.Outputs.All(x => Config.AllowedOutputTypes.Any(y => x.ScriptPubKey.IsScriptType(y)))
+		&& tx.Outputs.Zip(tx.Outputs.Skip(1), (a, b) => (First: a.Value, Second: b.Value)).All(p => p.First >= p.Second);


Are these heuristics exactly the same than Dumplings @Szpoti ?

No, this is the logic in dumplings:

tx.Inputs.All(x => x.PrevOutput.ScriptPubKey.IsScriptType(ScriptType.P2WPKH) || x.PrevOutput.ScriptPubKey.IsScriptType(ScriptType.Taproot)) && inputCount >= 50 && inputValues.SequenceEqual(inputValues.OrderByDescending(x => x)) && outputValues.SequenceEqual(outputValues.OrderByDescending(x => x)) && outputValues.Count(x => Wasabi2Denominations.Contains(x.Satoshi)) > outputCount * 0.8;

Take into account that dumplings, in general, knows less than wasabi. Dumplings assumes the inputCount is greater than 50 because it doesn't know what the MinInputCountByRound really is. It assumes all outputs are wpkh or tr because it doesn't know what the AllowedOutputTypes really are.

OTOH, dumplings know the prevOutputs so, it knows the input values and then it can check the order of the inputs, wasabi cannot. For this same reason it could check that all the inputs are wpkh or tr.

The 80% of std denominations is just a bug waiting to be found. Where is that 80% comes from? If tomorrow we implement pay in coinjoins that would need to change.

lontivero · 2023-11-17T13:53:16Z

It looks like as an attacker I can create a tx that will pass IsWasabiCoinJoinLookingTx and therefore be able to disrupt the rounds freely with the outputs of this transaction. Is this true? Is this a problem?

Sure. You can create a huge wasabi coinjoin looking transaction to disrupt rounds but it is hard to imagine a more expensive way to attack.

If yes to both, maybe we can make sure that an unused key belonging to coordinator's xpub is used as output

There are cases where the coordinator cannot take any fee and in that case we risk to ban ourselves.

turbolay · 2023-11-17T15:29:06Z

Sure. You can create a huge wasabi coinjoin looking transaction to disrupt rounds but it is hard to imagine a more expensive way to attack.

You're right, I was under the false feeling that we could disrupt many rounds with one wabisabi's like cj, which is not the case as we can only disrupt the ongoing rounds.

turbolay

cACK and code LGTM

It would make sense to me to create a new offense DisruptedSeveralRoundsInSameTransaction if a tx disrupts several rounds
Please consider it and feel free to either implement it in a follow up PR either don't implement it, both are fine for me.

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/CoordinatorTests.cs

Co-authored-by: Kimi <58662979+kiminuo@users.noreply.github.com>

lontivero · 2023-11-21T14:26:43Z

Who do I have to kill to get this PR approved?

kiminuo · 2023-11-21T22:04:00Z

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs

+		CoinJoinIdStore.Contains(txId) || IsFinishedCoinJoin(txId) || IsWasabiCoinJoinLookingTx(tx);
+
+	private bool IsFinishedCoinJoin(uint256 txId) =>
+		Arena.RoundStates


Just to make sure: I don't see any RoundState.Phase == Phase.Ended. Is it not necessary?

No because if it is in signing state and matches the txid then it doesn't matter whether it already switched to Ended or not, it is one of our coinjoins.
In the first versions i tested the Phase but here we use RoundStates which is not updated immediately but after all the steps in the round have happen so, it only added problem

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs

kiminuo · 2023-11-21T22:22:41Z

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/CoordinatorTests.cs

+
+	private static Transaction CreateTransaction(Money amount, OutPoint? outPoint = default)
+	{
+		var tx = Network.RegTest.CreateTransaction();


It may be good to use Main because that's closer to what is actually being done in the production then:

Suggested change

var tx = Network.RegTest.CreateTransaction();

var tx = Network.Main.CreateTransaction();

NewMockRpcClient below also relies on Main.

Yes, it can be. I saw in my mobile phone that the pr was approved and then i merged it immediately. I didn't see the comments, sorry.

kiminuo

Me?

lontivero added 3 commits November 15, 2023 11:26

Add and heuristic for detecting Wasabi coinjoins.

0125cea

This is because the two mechanisms for detecting incoming coinjoin transactions are not enough in many cases and we were banning our own coinjoin inputs.

Refactor BanDoubleSpenders to make it more readable.

e69b75e

Ban created outputs instead of already spent inputs.

424c5bf

pull-request-size bot added the size/L label Nov 16, 2023

lontivero force-pushed the detect-wasabi-coinjoins branch from a89c9bb to da2095b Compare November 16, 2023 02:05

Add Unit Test for BanDoubleSpenders

f9f1f9d

lontivero force-pushed the detect-wasabi-coinjoins branch from da2095b to f9f1f9d Compare November 16, 2023 02:08

lontivero changed the title ~~Detect wasabi coinjoins~~ Detecting and (avoid) Banning Wasabi coinjoins Nov 16, 2023

lontivero marked this pull request as ready for review November 16, 2023 02:25

lontivero requested review from Szpoti and kiminuo November 16, 2023 02:26

Szpoti approved these changes Nov 16, 2023

View reviewed changes

Code Review suggestions

4cac950

kiminuo reviewed Nov 17, 2023

View reviewed changes

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/PrisonTests.cs Outdated Show resolved Hide resolved

WalletWasabi/WabiSabi/Backend/DoSPrevention/Offender.cs Show resolved Hide resolved

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/OffenderSerializationTests.cs Show resolved Hide resolved

kiminuo reviewed Nov 17, 2023

View reviewed changes

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs Show resolved Hide resolved

turbolay reviewed Nov 17, 2023

View reviewed changes

CR suggestions

a6859e7

lontivero force-pushed the detect-wasabi-coinjoins branch from 5d18d43 to a6859e7 Compare November 17, 2023 13:55

turbolay approved these changes Nov 17, 2023

View reviewed changes

kiminuo reviewed Nov 20, 2023

View reviewed changes

WalletWasabi.Tests/UnitTests/WabiSabi/Backend/CoordinatorTests.cs Outdated Show resolved Hide resolved

Kimi test

0b7c8f6

Co-authored-by: Kimi <58662979+kiminuo@users.noreply.github.com>

lontivero requested a review from kiminuo November 21, 2023 12:37

kiminuo reviewed Nov 21, 2023

View reviewed changes

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs Show resolved Hide resolved

kiminuo reviewed Nov 21, 2023

View reviewed changes

WalletWasabi/WabiSabi/WabiSabiCoordinator.cs Show resolved Hide resolved

kiminuo reviewed Nov 21, 2023

View reviewed changes

kiminuo approved these changes Nov 21, 2023

View reviewed changes

lontivero merged commit fcf32f3 into WalletWasabi:master Nov 22, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detecting and (avoid) Banning Wasabi coinjoins #11946

Detecting and (avoid) Banning Wasabi coinjoins #11946

lontivero commented Nov 16, 2023 •

edited

Szpoti left a comment

kiminuo left a comment

turbolay left a comment •

edited

turbolay Nov 17, 2023

Szpoti Nov 20, 2023

lontivero Nov 20, 2023

lontivero commented Nov 17, 2023

turbolay commented Nov 17, 2023

turbolay left a comment

lontivero commented Nov 21, 2023

kiminuo Nov 21, 2023

lontivero Nov 22, 2023

kiminuo Nov 21, 2023

kiminuo Nov 21, 2023

lontivero Nov 22, 2023

kiminuo left a comment

	var tx = Network.RegTest.CreateTransaction();
	var tx = Network.Main.CreateTransaction();

Detecting and (avoid) Banning Wasabi coinjoins #11946

Detecting and (avoid) Banning Wasabi coinjoins #11946

Conversation

lontivero commented Nov 16, 2023 • edited

Szpoti left a comment

Choose a reason for hiding this comment

kiminuo left a comment

Choose a reason for hiding this comment

turbolay left a comment • edited

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

lontivero commented Nov 17, 2023

turbolay commented Nov 17, 2023

turbolay left a comment

Choose a reason for hiding this comment

lontivero commented Nov 21, 2023

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

kiminuo left a comment

Choose a reason for hiding this comment

lontivero commented Nov 16, 2023 •

edited

turbolay left a comment •

edited