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

"Failed to signal ready to sign" - Prison extension #11422

Conversation

adamPetho
Copy link
Collaborator

This PR is made to address this https://github.com/zkSNACKs/WalletWasabi/pull/11115/files#r1294451295
and to push forward #11115 PR.

This PR adds a new method to Prison called FailedToSignalReadyToSign.
On master this method is not yet used. It will be used in #11115 AFAIK.

FailedToSignalReadyToSign has the same penalty factor values as FailedToSign. Please let me know if this should be different.

Copy link
Collaborator

@Szpoti Szpoti left a comment

Choose a reason for hiding this comment

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

ACK.
Code LGTM

Copy link
Collaborator

@lontivero lontivero left a comment

Choose a reason for hiding this comment

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

Missing parts:

  • You have to serialize and deserialize the new type of disruption; otherwise they will not be persisted.
  • Update unit tests for this new type of disruption.

WalletWasabi.Tests/Helpers/WabiSabiFactory.cs Outdated Show resolved Hide resolved
WalletWasabi/WabiSabi/Backend/DoSPrevention/Offender.cs Outdated Show resolved Hide resolved
WalletWasabi/WabiSabi/Backend/WabiSabiConfig.cs Outdated Show resolved Hide resolved
@adamPetho
Copy link
Collaborator Author

@lontivero There is a bit more complex unit test called BanningTime. I have a hard time updating it, because I'm not sure how hard do we want to punish this misbehavior.
Is it OK to handle this misbehavior as the FailedToSign? Or should it be a bit lesser than FailedToSign?

@lontivero
Copy link
Collaborator

Is it OK to handle this misbehavior as the FailedToSign? Or should it be a bit lesser than FailedToSign?

I think it is just a bit less than FailedToSign because it wastes less time from everybody else but don't worry about that, you can make it the same as FailedToSing for the test because in the end these factors have to be tweaked in production.

@lontivero lontivero merged commit 794f129 into WalletWasabi:master Sep 5, 2023
7 checks passed
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.

None yet

3 participants