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

Refactor fee provider #5643

Merged
merged 4 commits into from Jan 18, 2024
Merged

Conversation

Kukks
Copy link
Member

@Kukks Kukks commented Jan 3, 2024

The fee provider ended up glued with a hardcoded factory. This PR:

@Kukks Kukks force-pushed the mempool-graceful branch 2 times, most recently from 29538d4 to b705ba3 Compare January 3, 2024 09:53
@Kukks Kukks added the Bug label Jan 9, 2024
@Kukks Kukks added this to the 1.12.x milestone Jan 9, 2024
@NicolasDorier
Copy link
Member

NicolasDorier commented Jan 18, 2024

I will remove the refactor you did to focus on the bug fix.

The refactor still hardcode the mempool fee provider, but on top of this limit fee providers to depends solely on bootstrap services. We should probably do something later, but for now let's just focus on fixing the bug.

On top of this, fee provider itself might require a refactoring sooner or later, as I don't think it should depends on BTCPayNetwork, probably should depends only to a cryptoCode, but still need to think about it.

Last, it doesn't fix #5641 I believe.
The issue came because mempool space was just hanging, rather than crashing this is fixed by a timeout and cancellationtoken.

Your PR is also doing some linear interpolation stuff you didn't really explained why. Looking into it.

The fee provider ended up glued with a hardcoded factory. This PR:
* removes this glue and uses the DI to register fee provider for a network. (allows plugins to add their own fee providers, for any network
* Add a 10 second timeout to mempoolspace fee fetching as they are slow at times
@NicolasDorier NicolasDorier force-pushed the mempool-graceful branch 4 times, most recently from c8e6f48 to cf0bbb3 Compare January 18, 2024 05:24
@NicolasDorier
Copy link
Member

NicolasDorier commented Jan 18, 2024

  • Rewrote the RandomizeByPercentage, it was bugged: This was adding or removing exactly 10% rather than picking random value on a range of +-10%
  • Added tests of InterpolateBound and RandomizeByPercentage
  • Rewrote the InterpolateBound to make it more concise, removing branches on the way. I believe the previous version had an issue because it was dividing a decimal with an integer.
  • Removed the registration of providers in DI for the moment.
  • Dispose of the CancellationTokenSource for the timeout

I will do a followup PR so that, like the rates, the fees are fetched in the background, avoiding a checkout page load hanging

@NicolasDorier NicolasDorier merged commit 3eec9cb into btcpayserver:master Jan 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

[Bug]: Gateway Timeout (504) on Invoice Creation because Mempool.space is down
2 participants