Upgrading ThirdPartyFeeProvider to support list of fee providers

[csiki2]

Upgrading ThirdPartyFeeProvider to support list of fee providers.

Later HybridFeeProvider can be removed since it's a sort of duplicate.

[lontivero]

We are planning to move the responsibility of fetching the mining fee rates from the server to the client. The ultimate goal is to make Wasabi to work without a central server, or at least that's the direction I think we have to go. Each user should be able to chose which fee provider to trust on, mempool.space is pretty popular and I am sure it would be among the most chosen ones.

We cannot maintain tens of fee providers and, even when they would all look exactly the same, because after all they all have to make a http request to get a json containing a list of (target, fee rate) pairs, we should support many of these fee rate providers.

So, the idea is to simplify that task. Imagine something like a WebFeeRateProvider, a PeriodicRunner as it is the one that created in this PR, that can be used with any fee rate source. Something like this:

class WebFeeProvider : PeriodicRunner
{
        private IFeeProviderHandler _feeProviderHandler;
        public FeeRateCollection FeeRates { get; private set; }

        public async Task ActionAsync()
	{
             var json = _httpClient.SendAsync(HttpMethod.Get, _feeProviderHandler.ApiUrl)
             FeeRates = _feeProviderHandler.Parse(json);
        }
}


interface IFeeProviderHandler {
   string ApiUrl { get; }
   FeeRateCollection Parse(JObject /*or string*/ apiResponse);
}

Then, if you implement this, your MempoolSpaceFeeRateProvider can be implemented with a MempoolFeeRateProviderHandler that maps, using your BlockMap, the mempool.space response to the FeeRateCollection (AllFeeEstimates is in the code btw, sorry for the confusion)

[csiki2]

This is on the client side.
The whole point of the ThirdPartyFeeProvider change that the WasabiSyncronizer is just one of the list and later we can remove it anytime from the list.
We already have an interface for the fee provider, I don't understand why we intend to make it more complex than needed, is there any immediate benefit?
Ofc, if we begin to use many-many similar approach it might worth to make an ancestor (or even better solution).
To be fair I plan to remove the HybridFeeProvider later as well as it's a duplicate.
I recommend to accept and merge this in, and then we can discuss what else should be done at this area of code.

[csiki2]

Created #13022 as a feature request.

[turbolay]

First, it's nice that you split this PR with the integration of mempool.space, that we can easily do later.

The code is a bit hard to understand but the concept is OK. Code mostly LGTM. I just have 2 small questions that I wrote as comments.

You mentioned in the meeting that the codebase in general lacks useful comments, maybe adding some to this kind of PR is a good way to not worsen the problem. FYI most uncommented features were coded early, and most new features are properly commented.

I made turbolay@6c4a066. You can choose whether to apply it or not, it applies some suggestions from Rider and fixes few warnings.

As @kiminuo mentioned, it would be great to have Unit Tests as the concept is not trivial (eg a test that shows that priority is respected, another one to ensure that priority goes back to first provider when it's not OnError anymore and that other providers are paused)

[csiki2]

First, it's nice that you split this PR with the integration of mempool.space, that we can easily do later.

You mentioned in the meeting that the codebase in general lacks useful comments, maybe adding some to this kind of PR is a good way to not worsen the problem. FYI most uncommented features were coded early, and most new features are properly commented.

You have a point there. I will create a commit that has the comments you already asked.

I made turbolay@6c4a066. You can choose whether to apply it or not, it applies some suggestions from Rider and fixes few warnings.

I have mixed feelings here.
In general I like the early returns, but the extra brackets greatly reduces it's usefulness and in some cases even worsen the readability.
For example, the
'''
if (inError && !InError && LastStatusChange - DateTimeOffset.UtcNow > TimeSpan.FromMinutes(1))
'''
is easier to read (at least to me) then it's negated logic.
I added some of your recommendations.

As @kiminuo mentioned, it would be great to have Unit Tests as the concept is not trivial (eg a test that shows that priority is respected, another one to ensure that priority goes back to first provider when it's not OnError anymore and that other providers are paused)

Began to write a unit test (and caught a bug, khm).

[turbolay]

I pushed 2 commits on this PR, harmless simply style and a small addition to the test

However, upon testing there are 2 things not really great with the implementation:

• It's too long to wait for the backup when a provider is OnError
• Excluding for the WasabiSynchronizer (which long term won't be a fee provider), it's useless to wait 1 minute when being OnError, because the period of the other provider (currently only Blockstream) is higher than 1 minute

I can suggest 2 things:

• Reduce the period of the providers, but return immediately if last result was less than 3 minutes ago turbolay@257d242 -> This change might be enough by itself.
• Expose TriggerRound in IThirdPartyFeeProvider and call it when IsPaused goes from false to true in SetPauseStates turbolay@bea838a

As mentioned, 2nd suggestion might be overkill, as first one would already improve (and we would need to wait max 10s). So you can choose whether to implement or not, modify my suggestions etc... Your call.

With one modification of the kind, this PR can be merged.

[csiki2]

I would go with the second as its cleaner.
Let me check whether it is an issue that we willingly add the same method name for the 2 interfaces.

[csiki2]

I went with the second approach, but with separate method name for cleaner interface (TriggerOutOfOrderUpdate) + unit test.
If you have other method name suggestion, please let me know.

[molnard]

We talked with Csiki on Slack. The idea here is to make ThirdPartyFeeProvider the one and only API for providing fees. There are multiple approaches IMO this is reasonable first step, because it is not breaking anything and using the current structure but opens the space for increments.

[turbolay]

Why did you override my commits?

[csiki2]

You told me, I can choose.
I chose the second one, but needed to change (and fix) it, so ended up removing them.
Sorry for the inconvenience.

[turbolay]

No, I pushed commits on your PR to fix style that you overrode with a forced push, mentioned here:

I pushed 2 commits on this PR, harmless simply style and a small addition to the test

Anyway, lost too much time on this, won't happen again.

[yahiheb]

Isn't this PR now obsolete with #13035?

[csiki2]

"No, I pushed commits on your PR to fix style that you overrode with a forced push, mentioned here:"
I remember only these two commits with your suggestions. Way earlier was a squash.

[csiki2]

Isn't this PR now obsolete with #13035?

Different branch.

[csiki2]

No, I pushed commits on your PR to fix style that you overrode with a forced push, mentioned here:

I pushed 2 commits on this PR, harmless simply style and a small addition to the test

Anyway, lost too much time on this, won't happen again.

Argh. My fault. Terribly sorry about that. :(

[yahiheb]

Isn't this PR now obsolete with #13035?

Different branch.

That branch is the future of Wasabi Wallet that's why others are working on it.
AFAIK soon it will be merged to master and all of this will be overridden.

[molnard]

I am not sure what is the future of this, meanwhile the other PR was borned - anyway I tested it and reviewed the code.

tACK

[turbolay]

See why I said I would merge #13031 after

[csiki2]

I don't have high hopes that this change will be merged. Still, I fixed the conflict.