State machine will follow the business logic (continuation) #10059

turbolay · 2023-02-04T01:27:09Z

Supersedes #10032
Closes #9957

The point is to NotifyWalletStartedCoinJoin(walletToStart) when ScheduleRestartAutomatically is called, but only once (hence use of WasRestartedAutomatically )

The change with PlebStop is to adapt it to new behavior as it was broken. It makes sure that when a user chose to OverridePlebstop, he won't have to override again until he presses Pause. It should also block when balance becomes lower than the threshold during CJ because of fees (untested).

I was not able to see any issue with this fix, @yahiheb @MarnixCroes can you try to break it?

turbolay · 2023-02-04T01:28:47Z

What have I done??

turbolay · 2023-02-04T07:26:56Z

WalletWasabi.Fluent/ViewModels/Wallets/CoinJoinStateViewModel.cs

@@ -249,6 +251,7 @@ private void ConfigureStateMachine()
 			.Permit(Trigger.PlebStopChanged, State.Playing)
 			.Permit(Trigger.WalletStartedCoinJoin, State.Playing)
 			.Permit(Trigger.WalletStoppedCoinJoin, State.StoppedOrPaused)
+			.Permit(Trigger.StartError, State.Playing)


Same change than #10032 but for PlebStop

turbolay · 2023-02-04T08:34:29Z

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

+			walletToStart.LogDebug($"AutoStart was already scheduled, cancel the last task and do not wait.");
+			trackedAutoStarts[walletToStart].CancellationTokenSource.Cancel();
+			trackedAutoStarts.Remove(walletToStart, out _);
+			delayRestart = 0;


This is to make sure that whenever user tries to restart CoinJoin when it was already scheduled, new arguments are taken into account.

This happens for example when user presses Play in State.PlebStop with only pending coins. A restart task was already scheduled by precedent action with OverridePlebStop as false, so when user presses Play in State.PlebStop we need to cancel the last task to start a new one with OverridePlebStop as true.

I set the delay to 0 as it doesn't really change anything but we could increase it a bit or use a StopWatch if this could be a problem.

I would isolate the case. Compare the two trackedAutoStarts OverridePlebStop. If it is different, do the update if not then just debug the message and return.

MarnixCroes

I was not able to see any issue with this fix, @yahiheb @MarnixCroes can you try to break it?

So far, I'm not able to break it.

molnard · 2023-02-06T14:57:08Z

What have I done??

I don't know but it is not looking good. Can you fix that?

# Conflicts: # WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

yahiheb

Once all your coins are private and you get this message Hurray! Your funds are private the play button does nothing on master. With this PR it works.

tACK

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

molnard · 2023-02-09T13:37:51Z

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

+			walletToStart.LogDebug($"AutoStart was already scheduled, cancel the last task and do not wait.");
+			trackedAutoStarts[walletToStart].CancellationTokenSource.Cancel();
+			trackedAutoStarts.Remove(walletToStart, out _);
+			delayRestart = 0;


I would isolate the case. Compare the two trackedAutoStarts OverridePlebStop. If it is different, do the update if not then just debug the message and return.

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

molnard · 2023-02-09T13:39:28Z

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

-			walletToStart.LogDebug($"AutoStart was already scheduled.");
-			return;
+			walletToStart.LogDebug("AutoStart was already scheduled, cancel the last task and do not wait.");
+			TryRemoveTrackedAutoStart(trackedAutoStarts, walletToStart);


You can just let this go in this case, it will be removed later?

https://github.com/zkSNACKs/WalletWasabi/pull/10059/files#diff-28444f49c77fd0c2cd05f91515914d63fc3c9264ff1f24a56b2fc8e8167df850R317

I am not sure that it will be removed correctly after while being sure the old task is not executed in case for eg of an infortunate timing

turbolay · 2023-02-09T14:14:28Z

I would isolate the case. Compare the two trackedAutoStarts OverridePlebStop. If it is different, do the update if not then just debug the message and return.

@molnard To do that I'd need to access parameters for the last task and hence change record TrackedAutoStart to also take bool StopWhenAllMixed, bool OverridePlebStop as parameters, should I proceed?

molnard · 2023-02-09T15:36:52Z

TrackedAutoStart to also take bool StopWhenAllMixed, bool OverridePlebStop as parameters, should I proceed?

That is a clean-cut. Yes.

turbolay · 2023-02-10T06:09:57Z

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

 		if (trackedAutoStarts.ContainsKey(walletToStart))
 		{
-			walletToStart.LogDebug($"AutoStart was already scheduled.");
-			return;
+			if (stopWhenAllMixed == trackedAutoStarts[walletToStart].StopWhenAllMixed && overridePlebStop == trackedAutoStarts[walletToStart].OverridePlebStop)


Dictionnary with IWallet as key. Maybe consider changing? I don't know if necessary, neither what we could use.

I will address this in a subsequent PR probably using WalletName

Or implement making sure the wallet name will be used when checking equality for dictionaries. ACK for doing it later.

Just a comment, wallet references are never changing.

molnard

Purrfect

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

molnard · 2023-02-14T13:16:12Z

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

 		if (trackedAutoStarts.ContainsKey(walletToStart))
 		{
-			walletToStart.LogDebug($"AutoStart was already scheduled.");
-			return;
+			if (stopWhenAllMixed == trackedAutoStarts[walletToStart].StopWhenAllMixed && overridePlebStop == trackedAutoStarts[walletToStart].OverridePlebStop)


Or implement making sure the wallet name will be used when checking equality for dictionaries. ACK for doing it later.

molnard · 2023-02-14T15:03:05Z

@brizik please test this with the help of @MarnixCroes.

yahiheb

tACK

turbolay · 2023-02-15T14:13:18Z

Last 2 commits does not change anything to the behavior:

Use if instead of ternary
Create the task then add to task list it then start it. There was no any issues before but now that we do not wait sometimes there could be a concurrency issues

molnard · 2023-02-17T10:47:54Z

@brizik please test this with the help of @MarnixCroes.

@MarnixCroes can you take a look at this?

molnard · 2023-02-17T11:05:10Z

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs


 #pragma warning disable CA2000 // Dispose objects before losing scope
 		var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken);
 #pragma warning restore CA2000 // Dispose objects before losing scope

-		var restartTask = Task.Run(
+		var restartTask = new Task(


Why the change here?

I explained in my comment:

Create the task then add to task list it then start it. There was no any issues before but now that we do not wait sometimes there could be a concurrency issues

At the end of the task we remove the task from the list, but the line to add the task into the list was after task run, I was afraid of a potential concurrency problem.
Should I revert?

Sorry I did not see that. How can concurrency happen?

https://github.com/molnard/WalletWasabi/blob/fafc8bfa/WalletWasabi/WabiSabi/Client/CoinJoinManager.cs#L256

Between

WalletWasabi/WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

Line 326 in fafc8bf

if (trackedAutoStarts.TryRemove(walletToStart, out _))

and

WalletWasabi/WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

Line 337 in fafc8bf

if (trackedAutoStarts.TryAdd(walletToStart, new TrackedAutoStart(restartTask, stopWhenAllMixed, overridePlebStop, linkedCts)))

because we do not wait anymore in some cases

MarnixCroes

tACK

yahiheb

tACK

molnard

tACK 🥇

molnard added 2 commits February 1, 2023 13:44

State machine will follow the business logic

34247f7

Improve the code

db37a9d

pull-request-size bot added the size/M label Feb 4, 2023

turbolay changed the title ~~Improve NoCoinsToMix business logic~~ State machine will follow the business logic (continuation) Feb 4, 2023

Fix logic for NoCoinsToMix and PlebStop

da90903

turbolay force-pushed the cjWhenNoCoinsToMix branch from 8583ce6 to da90903 Compare February 4, 2023 07:24

turbolay commented Feb 4, 2023

View reviewed changes

Always restart with new arguments

ee9bdfe

turbolay commented Feb 4, 2023

View reviewed changes

turbolay marked this pull request as ready for review February 4, 2023 08:34

MarnixCroes reviewed Feb 6, 2023

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Feb 7, 2023

turbolay force-pushed the cjWhenNoCoinsToMix branch from 51d9a39 to ee9bdfe Compare February 7, 2023 03:16

pull-request-size bot added size/M and removed size/L labels Feb 7, 2023

turbolay added a commit to turbolay/WalletWasabi that referenced this pull request Feb 7, 2023

Fix CF for zkSNACKs#10059

51f1a4a

turbolay mentioned this pull request Feb 7, 2023

Some CF issues for CJManager and CJStateVM #10072

Closed

yahiheb mentioned this pull request Feb 7, 2023

Fix some CodeFactor issues #10073

Merged

pull-request-size bot added size/L and removed size/M labels Feb 7, 2023

turbolay force-pushed the cjWhenNoCoinsToMix branch from 46fd1d5 to ee9bdfe Compare February 7, 2023 05:08

pull-request-size bot added size/M and removed size/L labels Feb 7, 2023

turbolay added 3 commits February 8, 2023 07:52

Merge branch 'master' into cjWhenNoCoinsToMix

3ca0fc0

# Conflicts: # WalletWasabi/WabiSabi/Client/CoinJoinManager.cs

CF Const before all variables

e2f1427

CF Each parameter on its line

cad2e6d

molnard requested changes Feb 8, 2023

View reviewed changes

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs Outdated Show resolved Hide resolved

turbolay added 2 commits February 8, 2023 13:05

Revert useless change

5c065d0

Remove debug artifact

8472e4c

yahiheb approved these changes Feb 9, 2023

View reviewed changes

yahiheb requested changes Feb 9, 2023

View reviewed changes

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs Outdated Show resolved Hide resolved

Remove '$'

974b7d3

yahiheb previously approved these changes Feb 9, 2023

View reviewed changes

molnard requested changes Feb 9, 2023

View reviewed changes

Only relaunch if parameters changed

5971db8

turbolay dismissed yahiheb’s stale review via 5971db8 February 10, 2023 06:08

turbolay commented Feb 10, 2023

View reviewed changes

Merge branch 'master' into cjWhenNoCoinsToMix

cb76792

yahiheb mentioned this pull request Feb 13, 2023

[Trivial] Fix CodeFactor issues #10107

Merged

Merge branch 'master' into cjWhenNoCoinsToMix

76512aa

molnard requested changes Feb 14, 2023

View reviewed changes

yahiheb previously approved these changes Feb 14, 2023

View reviewed changes

turbolay added 2 commits February 15, 2023 08:10

Convert ternary to if

1470eae

Avoid concurrency problems.

fafc8bf

turbolay dismissed yahiheb’s stale review via fafc8bf February 15, 2023 14:10

molnard requested changes Feb 17, 2023

View reviewed changes

MarnixCroes approved these changes Feb 17, 2023

View reviewed changes

yahiheb approved these changes Feb 17, 2023

View reviewed changes

molnard approved these changes Feb 20, 2023

View reviewed changes

molnard merged commit 1a751c8 into zkSNACKs:master Feb 20, 2023

turbolay mentioned this pull request May 15, 2023

Music box is showing incorrect information #9887

Closed

turbolay deleted the cjWhenNoCoinsToMix branch February 26, 2024 04:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State machine will follow the business logic (continuation) #10059

State machine will follow the business logic (continuation) #10059

turbolay commented Feb 4, 2023 •

edited

turbolay commented Feb 4, 2023

turbolay Feb 4, 2023

turbolay Feb 4, 2023 •

edited

molnard Feb 9, 2023

MarnixCroes left a comment

molnard commented Feb 6, 2023

yahiheb left a comment

molnard Feb 9, 2023

molnard Feb 9, 2023

turbolay Feb 13, 2023

turbolay commented Feb 9, 2023 •

edited

molnard commented Feb 9, 2023

turbolay Feb 10, 2023

turbolay Feb 13, 2023

molnard Feb 14, 2023

molnard Feb 20, 2023

molnard left a comment •

edited

molnard Feb 14, 2023

molnard commented Feb 14, 2023

yahiheb left a comment

turbolay commented Feb 15, 2023

molnard commented Feb 17, 2023

molnard Feb 17, 2023

turbolay Feb 17, 2023

molnard Feb 17, 2023

turbolay Feb 17, 2023 •

edited

molnard Feb 20, 2023

MarnixCroes left a comment

yahiheb left a comment

molnard left a comment

State machine will follow the business logic (continuation) #10059

State machine will follow the business logic (continuation) #10059

Conversation

turbolay commented Feb 4, 2023 • edited

turbolay commented Feb 4, 2023

Choose a reason for hiding this comment

turbolay Feb 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

molnard commented Feb 6, 2023

yahiheb left a 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

turbolay commented Feb 9, 2023 • edited

molnard commented Feb 9, 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

molnard left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

molnard commented Feb 14, 2023

yahiheb left a comment

Choose a reason for hiding this comment

turbolay commented Feb 15, 2023

molnard commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbolay Feb 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

yahiheb left a comment

Choose a reason for hiding this comment

molnard left a comment

Choose a reason for hiding this comment

turbolay commented Feb 4, 2023 •

edited

turbolay Feb 4, 2023 •

edited

turbolay commented Feb 9, 2023 •

edited

molnard left a comment •

edited

turbolay Feb 17, 2023 •

edited