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

[PoC] Exclude Coins from CoinJoining #9925

Merged
merged 6 commits into from Jan 27, 2023

Conversation

Szpoti
Copy link
Collaborator

@Szpoti Szpoti commented Jan 12, 2023

TL;DR

Proof of Concept for #7487

With this PR, one can ❄️"freeze"/exclude🧊 his or her coins from being selected for a CoinJoin by the CoinJoinManager.
You can achive it by double-clicking on a Coin🪙 in the CoinList🗒️(CTRL+C+D).

Description

As this PR is only but a PoC, there is still work to do, mainly on the UI side (*wink* @zkSNACKs/visual-design-group 😉 ).
Right now nothing indicates that a coin is excluded, you can check the Wallet file for the "ExcludedCoinsFromCoinJoin" = [ ].
But the main concept in the bussiness logic is this.

  • Select and/or exclude Coins from the coinlist;
  • These Coins won't be participating in CoinJoins;
  • These Coins's Outpoint will be saved in the KeyManager (including the Wallet file);
  • Thanks to that, upon loading the Wallet, we also set by Outpoint, which Coins should still be excluded;
  • This is good, as one might move his/her wallet to another computer, but don't have to re-select 10s if not 100s Coins.

TODOs and Questions

  • The UI should have an icon next to the Confirmation check and CoinJoining icon, indicationg if it's "frozen"? (maybe an icecube 🧊 , or snowflake ❄️)?
  • Excluding a Coin should be a more obvious thing then double clicking (wow).
  • There needs to be a way to select-deselect more coins in one step.

And the question:

If we add this feature, the proper place for it is the CoinList. Which all started as a developer feature, but as I heard, there were user complains about this, "why I need to come to GitHub to find out that this feature exists". Until now it's like they found an easter egg🥚. At this point, we need to reconsider making a dedicated button for the Coinlist in the Wallet.

@turbolay
Copy link
Collaborator

turbolay commented Jan 12, 2023

IMO the concept of freezing UTXOs should be implemented in combination with the ability of freezing specific labels from coinjoining. If you think about it on a purely theoretical matter, freezing labels instead of individual UTXOs makes much more sense.

Both concepts are enabling use cases but both are not perfect on their own, that's why I think there must be a combination of both.
For example with your PoC, If I send a frozen coin, I need to re-freeze the change. Also, what if I don't want to coinjoin a coin that I receive while I'm afk but with wallet launched and auto coinjoin on?

Only label freezing is not enough either because users might misuse labels, it's less granular and it's not possible to use it for wallet recovered by seed.

I didn't look into the specific implementation of your PoC so won't review that, just imo we need to consider also freezing labels otherwise the feature will be incomplete and frustrating.

@nopara74
Copy link
Collaborator

IMO the concept of freezing UTXOs should be implemented in combination with the ability of freezing specific labels from coinjoining. If you think about it on a purely theoretical matter, freezing labels instead of individual UTXOs makes much more sense.

Both concepts are enabling use cases but both are not perfect on their own, that's why I think there must be a combination of both. For example with your PoC, If I send a frozen coin, I need to re-freeze the change. Also, what if I don't want to coinjoin a coin that I receive while I'm afk but with wallet launched and auto coinjoin on?

Only label freezing is not enough either because users might misuse labels, it's less granular and it's not possible to use it for wallet recovered by seed.

I didn't look into the specific implementation of your PoC so won't review that, just imo we need to consider also freezing labels otherwise the feature will be incomplete and frustrating.

Agreed!
This is where a unification needs to happen between the new coinselection dialog (which would support this out the box as it is designed) and this coinlist control that was rushed in.

@Szpoti you should continue as is, and UI team an look into that. IMO this PR should only implement the backend support:

wabisabi part, supporting enabling / disabling,
persisting the selection to disk, etc

The ui part we can handle.

@molnard
Copy link
Collaborator

molnard commented Jan 13, 2023

I didn't look into the specific implementation of your PoC so won't review that, just imo we need to consider also freezing labels otherwise the feature will be incomplete and frustrating.

What you are saying makes sense if you concentrate on the feature itself. However, the feature is only available in the coinlist which is a developer function so this is an edge case of an edge case. Here is my thinking. There are questions here about the concept, for example how to display this and where to store the excluded coins (wallet file). Those are already controversial so let's first push ourself through those questions - until that do not expand the specification.

BTW if the PoC is accepted it will be easy to do the same with labels by using the same pattern.

@Szpoti
Copy link
Collaborator Author

Szpoti commented Jan 16, 2023

So @turbolay's idea is ACK, just out of scope right now. The main questions:

How to display this?

This is mainly a UI question, but I would stand next to my ideas I said in the description, in addition of a tooltip maybe on the icons saying "Exclude this from being selected for Coinjoin" or similar.

Where to store the excluded coins (wallet file)?

I think the wallet file is one of the best place:

as one might move his/her wallet to another computer, but don't have to re-select 10s if not 100s Coins.

What do you guys think, where to move forward?

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.

cACK.

WalletWasabi/Blockchain/TransactionOutputs/SmartCoin.cs Outdated Show resolved Hide resolved
@molnard
Copy link
Collaborator

molnard commented Jan 17, 2023

@nopara74 the business logic is OK. How shall we proceed? Merge this and you will add the UI later, or you will add it here?

@nopara74
Copy link
Collaborator

@nopara74 the business logic is OK. How shall we proceed? Merge this and you will add the UI later, or you will add it here?

Lets merge the buisness logic, and we will work out the ui, @SuperJMN has done work on the coin selection, and that work can be leveraged here also.

@nopara74
Copy link
Collaborator

@Szpoti please resolve conflicts, or remove the ui parts completly ready for final review and merge.

@molnard
Copy link
Collaborator

molnard commented Jan 25, 2023

Go for it @Szpoti !

@Szpoti
Copy link
Collaborator Author

Szpoti commented Jan 25, 2023

Done 🙏

@molnard molnard requested a review from yahiheb January 25, 2023 15:48
@brizik
Copy link

brizik commented Jan 27, 2023

Testing result:

Business logic function works as expected (coin excluded from CoinJoin process) ✅

positive test:

  1. open coin list with CTRL +C +D
    » result: Wallet Coins (UTXOs) opens

image

  1. double click on all coins in the coin list »» UI part missing ❗ make it visible for the user that coin is selected to be excluded
  2. click on Done
  3. go to Search bar on the top
  4. type: Wallet Folder
  5. open Wallet Folder
  6. open json file with text file viewer
    » result: Excluded coin's outpoint appears in the log

image

  1. run the code from Terminal (WalletWasabi.Fluent.Desktop - ps « dotnet run)
  2. check CoinJoin Manager log message in terminal:

image

» result: Wallet (xy): No candidate coins available to mix.

inverse test:

  1. go back to Wallet
  2. open coin list with CTRL +C +D
    » result: Wallet Coins (UTXOs) opens
  3. double click on the coin in the coin list that was previously excluded
  4. click on Done
  5. click refresh to check json file with text file viewer / browser / Notepad ++
  6. check that no/previously selected coin's outpoint appears in the Wallet json
    field name: ExcludedCoinsFromCoinJoin

image

  1. go to Terminal
  2. check that CoinJoin started
    » result:
    Manager log contains the following lines:
    Coin selection started
    Selected the final selection candidate: 1 coins, x.xxxxxxx BTC

image

  1. Check Wallet if coin join progress started / in progress
    Music box text message appears: Waiting for CoinJoin to start

In this test case transaction immediately finished Music box text message appears:
Hurray! Your funds are private

image

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.

tACK

@molnard molnard merged commit de6d271 into zkSNACKs:master Jan 27, 2023
@molnard molnard deleted the freezeCoinFromCoinJoin branch January 27, 2023 15:45
@molnard
Copy link
Collaborator

molnard commented Jan 27, 2023

@nopara74 passed this task to the VDG team to finish the UI part.

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

5 participants