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

Local prison should have an invalidation mechanism #11910

Closed
turbolay opened this issue Nov 9, 2023 · 12 comments
Closed

Local prison should have an invalidation mechanism #11910

turbolay opened this issue Nov 9, 2023 · 12 comments
Assignees
Milestone

Comments

@turbolay
Copy link
Collaborator

turbolay commented Nov 9, 2023

Problem

Because the client caches banning time, it has no way to know if the banning time changed (because we tweaked Prison parameters, for example) or if the coin is unbanned (because we cleared the Prison for some reason).
Some clients are in the case where they think some of their coins have been banned for a long time, while it's not the case anymore.

Release?

Except if a new argument is raised, we will not have a silent release for this, but a fix or mitigation must be included in the next major release.

Mitigation

An example mitigation we can have at low cost would be to invalidate information from LocalPrison if it was received more than 2 days ago.

Solution

All solutions have trade-offs, and finding a good one is challenging because it has to respect both:

  • Privacy: (malicious) coordinator shouldn't be able to have extra clues on common ownership, banning information shouldn't be publicly available...
  • Integrations: It must be helpful for other software, not only Wasabi.

Some ideas:

Filters for ownership proofs in Prison

This solution is proposed and described here: #10700 (comment). It has the caveats of potentially harming privacy, as any active observer could know that a coin is in Prison at a given time, considering that the ownership proofs are made public during the round.

Endpoint

An endpoint that the clients call with the ownership proof of one random coin that is supposed to be banned for a round. The endpoint gives back the actual banning information. The whole cache is invalidated if the information received differs from locally cached information. I believe that this idea is the most interesting.

Same without endpoint

It is the same as the previous solution, except this process is done directly when registering inputs. This has the caveat of making a bad input selection if the coin is still banned.

@adamPetho
Copy link
Collaborator

With #11958 merged and released, we made a quick fix for this issue.

Should we proceed with one of the ideas?

If yes, which one?

If no, then I think we can close this issue.

@MaxHillebrand MaxHillebrand added this to the v2.0.6 milestone Dec 25, 2023
@MaxHillebrand
Copy link
Collaborator

Should we proceed with one of the ideas?

@molnard
Copy link
Collaborator

molnard commented Mar 15, 2024

It is the same as the previous solution, except this process is done directly when registering inputs. This has the caveat of making a bad input selection if the coin is still banned.

I am catching up on this, I was abroad at this time - sorry for the confusion. Local invalidation is a good solution on the client side. What happens after that? The client tries to register and finds out there are banned coins, which leads to suboptimal coin selection.

There is a simple naive solution for that. If the client detects there is a banned coin in the selection - it will just stop registering and leave the round by letting it's inputs timeout. The implementation is already done for leaving a round without privacy loss. If it is too late to leave, then proceed with the round (rare case).

WDYT?

@turbolay
Copy link
Collaborator Author

For me this solution is also the best, as it's a proper fix and doesn't introduce much complexity. I like the idea of filters etc and it might be more convenient for integrations but I don't feel an absolute urge and need to have it. For Wasabi clients, waiting 2-4 days in case we ban incorrectly is not really a big deal as it doesn't really happen often.

It's a bit implementation detail, but something to consider:
I have 10 coins banned for 1 week. After some day, I try to register them again, all at once.
If we implement this solution, and don't continue with registering the coins, we will need 10 successive rounds that we unregister from to get rid of this incorrect information, as we will do 1 by 1.

So we have to choose between:

  • Do 1 by 1 -> drawback: time, UX
  • If we detect a banned coin in the selection, keep trying to register the selection to detect the other banned coins, then unregister all -> drawback: privacy

@molnard
Copy link
Collaborator

molnard commented Mar 18, 2024

I vote for the second option.

@adamPetho
Copy link
Collaborator

If no one else wants to vote on this, then I will start to figure out how to implement the second option.

Altho, I'm not a fan of choosing a solution with privacy drawbacks over UX drawbacks...

@molnard
Copy link
Collaborator

molnard commented Apr 1, 2024

Well there is a complex solution. The client has a set of coins to register into the coinjoin. It starts registering one by one. If at least one coin is banned we set a flag.

If the flag is set we just register the coins but we never confirm, so coins will time out in input reg phase. Before the input reg ends we bail out and remember the banned coins.

The solution has no drawback except that it is complex...

@MaxHillebrand
Copy link
Collaborator

proposal: change how often the client attempts to register a banned coin from 2 days to 2 weeks.

@molnard
Copy link
Collaborator

molnard commented Apr 8, 2024

@adamPetho please change the parameter to 2 weeks.

@turbolay
Copy link
Collaborator Author

turbolay commented Apr 8, 2024

@adamPetho please change the parameter to 2 weeks.

Why?
Didn't this discussion happen last week?

@molnard
Copy link
Collaborator

molnard commented Apr 9, 2024

According to the title Local prison should have an invalidation mechanism this is done.

One more thing remained to mitigate the suboptimal coinselection. @adamPetho will create a separate issue for that.

@turbolay
Copy link
Collaborator Author

turbolay commented Apr 9, 2024

You know I could also change the title. Closing issue for the sake of closing issues is ridiculous, this is not fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants