Sort out the Coin Selectors

📜 Preamble:

Sort out the Coin Selectors by @molnard

📖 TL;DR - Too Long, Didn't Read:

When our user wants to send Bitcoin, wasabi needs to find the most optimal set of coins, according to the user's demand, privacy, cost, and speed. There are two problems here, first - sometimes these are against each other, where is the golden mean? Second - there are multiple logic in Wasabi and they barely work together. There are many cases where the selection could be better or what is worse harming privacy. The idea here is to unify these logics into one and write proper tests to clarify the behavior, which will also reduce issues in the future.

🌟 Specification, Scope, and Features:

Discover all the algorithms that are affecting coin selection from the UI to the broadcast.
Refactor send-related coin selectors by unifying them into one algorithm.
Make sure we use all relevant data everywhere when coin selection happens, like the labels.
Fix the current and add more tests.
Clarify edge cases and define the golden mean in questionable situations. Back up these with tests as well.
Not touching the coin selection for coinjoins.

⚔️ Motivation:

Issues about this are coming back again and again. The reason is that is so complex and scattered that when we fix something on one side, it breaks on the other side. Nobody at the company has a comprehensive detailed knowledge of how we are selecting coins. With a unified model and proper tests, this could be detected and avoided. Below you can see a few concrete issues and there were many user cases where the coin selection was clearly not optimal. However, the problem is not so visible for the users but it is there and will be noticed more when privacy warnings are more and more detailed.

📜 Rationale:

Basically, there are three main modules regarding coin selection. Pocket selector, Branch&Bound and SmartCoinSelector. My idea is to dismiss SmartCoinSelector and move all the relevant logic into another module. SmartCoinSelector is called by NBitcoin and we are not in 100% control there - we ran into mystical cases in the past - but currently, nobody dares to touch it significantly. After that Pocket selector should be melted into this new module as well. Tests are very important - so the implementation must focus on that as well. All cases have to be covered. The final part is to make B&B respect the same rules as we have in our new Module.

Many tests have to be added to ensure the best privacy we can reach with a given coin set.

🌌 Backward Compatibility:

I see no problems here. Everything will look the same for the user. Just the logic will be replaced under the hood.

📚 Reference Implementation (optional):

None but there are a couple of issues related.

👥 Team involvement

Lead: ? Code team Partially UI team

💪 Subtasks

We spent 2:30 hours figuring out a few actual steps in the right direction but we generally failed to provide a comprehensive plan. Coin selection happens on many levels and involves many other classes, like pockets, txbuilder, smartcoins, nbitcoin.

Primary direction: Simplify! - no improvements!

⛔ NOT in the scope of this Quest
- CoinJoin-related coin selection.
- Payments in CoinJoin coin selection.
- Any UI change - no change in the UI at all!
- Speed Up tx coin selection
- Improvements in coin selection by adding new logic or algorithms
- Introduce more coin selectors
- Introduce more parameters to coin selectors, like optimizing to speed/privacy/cost

✅

[ ] Investigate this idea - https://github.com/zkSNACKs/WalletWasabi/pull/10618 @kiminuo 🕐 don't know
[ ] Dismiss SmartCoinSelector @Szpoti 🕐 don't know
- SmartCoinSelector is a function that is called by NBitcoin a couple of times. NBitcoin tries to create valid/optimized transactions by applying different parameters to the SmartCoinSelector.Select function. The algorithm behind this is not clear. That is bringing uncertainty every time we touch this.
- Move the logic out from under NBitcoin. Make SmartCoinSelector a standalone class that does the selection before and passes the coins to NBitcoin. The concept is clear, but the details are not.
- Disable NBitcoin coin selection. Ensure that it won't modify the coins. If the coin selection is not good, throw instead. If we let NBitcoin touch the coin selection in any way there will be always uncertainty in privacy.
- Add Tests for SmartCoinSelector.
- Add Tests for TransactionBuilder. NBitcoin built-in class where you can apply SetCoinSelector.
- Make sure that tx size limit is respected.
- Make sure that fee calculation is OK as well - NBitcoin was playing with that as well.
[ ] Research what we do now. @kiminuo 🕐 don't know
- How does wasabi work from the point of user enters his intention until the broadcast of the tx.
- How does coin selection happen, which classes are involved, and where are optional modifiers to the selection, like B&B or Pocket selection dialog?
[ ] Research if B&B takes labels into consideration or not. @kiminuo 🕐 don't know
- It can also happen before B&B gets the coins, which is OK as well.
[ ] Use a simplified class for coins instead of SmartCoin when doing coinselections. 🕐 don't know
- Ensure mapping between SmartCoin and simplified coin, so one can start the simplification of selector classes.
- Research if this makes sense.
[ ] Research if the usage of TransactionBuilder can be avoided when doing the selection. 🕐 don't know
[ ] Have one superclass that can be instructed to do the selection. 🕐 don't know
- This would be the class that can be tested by unit test for every scenario.

The quest seems to be too big to estimate - so the success rate and the time estimation are not determined at all => the risk is high. However, I do not see any other way to solve the problem any other way. So the question: is it the right time now to take the risk?

molnard / The-release-game-v2-0-4-1

Sort out the Coin Selectors #4