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

On shutdown prevention, stop the CJM to participate in new rounds. On cancel, restart them. #10664

Merged
merged 13 commits into from Jun 20, 2023

Conversation

adamPetho
Copy link
Collaborator

@adamPetho adamPetho commented May 10, 2023

Fixes: #10653

This PR stops all wallets in InputReg phase on shutdown prevention.
Also, it signals to the CJM not to participate in new rounds, once they are finished with the Critical phase.

If the user cancels the shutdown prevention (changed his mind about quitting), restart the CJs.

How to test:

  1. Do CJs with 4-5 wallets and make sure they are participating in 2-3 different rounds parallel.
  2. Try to shutdown the app when one of the wallets in Critical Phase.
  3. See that wallets in input reg phase are stopped/cancelled.
  4. See that wallets in Critical phase don't participate in new rounds.
  5. See that the app can shut down after every wallet leaves the Critical phase.

You can also test the case when you cancel the shutdown prevention, the wallets that were in input reg are restarted.

@turbolay
Copy link
Collaborator

turbolay commented May 10, 2023

I think it should restart the CJ if we leave the dialog.

I actually worked a bit on the feature this morning (not because it was easy, but because I thought it would), but it was not working so I didn't make a PR.
Here's what I've done if it can help, especially with the NextCommand: 7abbb96

@soosr
Copy link
Collaborator

soosr commented May 11, 2023

I think it should restart the CJ if we leave the dialog.

Yepp

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 11, 2023
@adamPetho adamPetho changed the title On shutdown prevention, stop the CJM to participate in new rounds. On shutdown prevention, stop the CJM to participate in new rounds. On cancel, restart them. May 11, 2023
@adamPetho
Copy link
Collaborator Author

I think it should restart the CJ if we leave the dialog.

Yepp

I updated the PR + the description.
Restart should happen if user cancels his shutdown intention and leaves the shutdown prevention dialog.

@adamPetho
Copy link
Collaborator Author

I'm blocked until this gets resovled #10657 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adamPetho adamPetho marked this pull request as ready for review May 15, 2023 14:27
@adamPetho
Copy link
Collaborator Author

adamPetho commented May 15, 2023

The merge of #10671 unblocked this PR.

It's ready for tests and reviews.

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wallet in critical phase doesn't continue coinjoining after pressing cancel?
(And neither does the wallet below auto start cj treshold, but that might be an edge case)

Video
Screencast.from.18-05-2023.13.49.46.webm

@adamPetho
Copy link
Collaborator Author

@MarnixCroes could you please re-test both scenarios? Both should be fixed by my latest 2 commit. Ty.

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarnixCroes could you please re-test both scenarios? Both should be fixed by my latest 2 commit. Ty.

@adamPetho
Copy link
Collaborator Author

Ty for the testing @MarnixCroes, I will check what's going on with this PlebThreshold.

@adamPetho
Copy link
Collaborator Author

adamPetho commented Jun 16, 2023

@MarnixCroes could you please re-test this PR with the latest commit? Thanks 💖
The plebstop edge-case should be fixed now.

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 3d7422f

@adamPetho adamPetho requested a review from molnard June 19, 2023 12:33
Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

async () =>
{
await _coinJoinManager.RestartAbortedCoinjoinsAsync();
Navigate().Clear();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only good until somebody not touches CancelCommand.

Shouldn't be this more correct?

Suggested change
Navigate().Clear();
CancelCommand.Execute(null);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know. I didn't see the usage of CancelCommand.Execute(null) in the codebase before, so I went with this.

Maybe @soosr knows more about this.

@molnard molnard merged commit 3f51604 into zkSNACKs:master Jun 20, 2023
8 of 9 checks passed
@adamPetho adamPetho deleted the signal_to_stop_cjs_on_shutdown branch June 20, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Software can never shuts down due to continuous CJs
5 participants