Skip rounds randomly depending on network conditions #10910
Conversation
There was a problem hiding this comment.
The concept by itself is good, even tho it’s a bit misleading as one reading the code might think it’s helpful to save on fees while it’s in fact only useful for extra randomization, and we keep relying on the legacy implementation to save on fees.
I have at least a problem with the current implementation, related to the profiles. In fact, it’s a problem I already described here #10468 (comment) about retro compatibility, but it’s worse here and is inherent to how profiles equality work:
With your PR, ALL users would have the default values “CoinjoinSkipFactors”: “0.7_0.8_0.9”, and all users that previously had profiles Privacy and Economic would now see that they have profile Custom.
I believe an extra function must be added to check the current profile so that the default value for CoinjoinSkipFactors would be adequate for the current conjoin profile.
Additionally, isn’t it necessary to add a UI to change the values in Coinjoin Strategy Settings screen (Customize)? Otherwise, how would users with Custom profile already selected change their value? Or how would users disable the feature? This raises an extra problem: It’s really hard to understand what the values mean. For eg, 0.6 for the daily parameter of the constructor means 20% probability to skip the round if the current fee rate is lower than the daily median.... definitely not direct.
| @@ -50,6 +53,13 @@ public WalletSettingsModel(KeyManager keyManager, bool isNewWallet = false) | |||
| .Skip(1) | |||
| .Do(_ => SetValues()) | |||
| .Subscribe(); | |||
|
|
|||
| // This should go to the previous WhenAnyValue, it's just that it's not working for some reason. | |||
There was a problem hiding this comment.
I didn't understand the problem either. There is an overload for 8 parameters, IDK why it's not working directly.
|
Thanks @turbolay! I'll get back to this PR after the release. |
|
@Kruwed: Investigate it here. |
Are we getting back to this? |
|
I am working on this. |
After investigation, the answer is "Awaiting Coinjoin". |
|
@Kruwed, the investigation is to be done by the author of this PR, not you 🤣 |
My understanding is it is helpful to save on fees: this makes it less likely to participate in the middle of a mempool spike and more likely to do so down the hill. |
That's a compromise. Correct. The reason why I don't mind it is because I did not touch existing features here like the
I even wonder if there's an argument to be made for introducing the smallest possible change (default configuration of CoinjoinSkipFactors) to existing wallets? In any case, even if not, that's a compromise I'm fine with. |
Necessary? No. Desirable? Maybe. I have no time for this, and your additional challenge makes it not even clear if there's a good way to do so, so this is something I skipped. |
@Kruwed I misunderstood what you were investigating. This investigation is about what should it say and how to make it say that. Now that I've been investigation I realized your investigation isn't a normative prescription, but you're saying what it currently says. |
I didn't implement this. It was too hard for me. I opened an issue for that instead: #11673 |
Closes #10586
Closes #10678
Closes #10906
Closes #8211
Less ambitious version of #10678
This pull request does not aim to replace the coinjoin prevention mechanism that happens during high fee environment, rather it introduces random round skipping depending on the profile and the network conditions, exactly as specified by #10678. The reason why I gave up on replacing the current cost saving mechanism is because I couldn't solve the Spread Problem, noted by @turbolay.
While the focus of this PR is randomization, it does it smartly as it considers the profile and daily, weekly and monthly fee environment in its randomization of round skipping.