Extend FeeRateService to support multiple API endpoints #4247
Conversation
Add unit test for existing functionality of the Version controller.
Extend the unit tests with more granular checks.
Add unit test which covers the scenario that an ExchangeRateService queries multiple ExchangeRateProviders.
Extend FeeRateService with necessary logic to support multiple BitcoinFeeRateProvider instances. Average the retrieved rates and provide the average via the service getFee() API.
Automatically instantiate up to 5 parallel fee estimation providers based on configured API endpoints in application.properties. By default, two endpoints are active. When none are configured, one primary provider defaults to querying mempool.space.
Add a section explaining how the mining fee average is calculated and how the queried API endpoints can be modified.
|
cc @wiz |
Fix a unit test that failed after a few related classes were refactored.
|
FYI for reviewers: I realise its a relatively big change, so here's a short "changelog" to help with the review:
|
- Wrap comments at 90 chars per bisq-network/style#5 - Wrap code at 120 chars per bisq-network/style#3 - Remove unused imports - Remove extra newlines - Format code where appropriate - Remove unused Javadoc tags, e.g. @return, @param - End Javadoc summary sentence with a period where missing - Remove HTML formatting in Javadoc, e.g. extra <br>s
This class had been made public in order to access the MIN/MAX_FEE_RATE fields and to construct dummy instances in FeeRateServiceTest. This introduced package cycles between bisq.price.mining and bisq.price.mining.provider, making the implementation more difficult to understand and maintain. This commit moves these fields to the already-public FeeRateProvider base class, where they make more sense to reside anyway. It also reworks tests to remove the need to access BitcoinFeeRateProvider directly from FeeRateServiceTest. In the end, BitcoinFeeRateProvider's visibility is returned to package-private as it originally was, and all package cycles have been eliminated.
The BitcoinFeeRateProvider name was never great; it should technically have been something like EarnDotComFeeRateProvider. This change renames the class to reflect that it (and its new subclasses) are specifically designed to query the Mempool API as found at https://mempool.space.
cbeams
force-pushed
the
feeservice-support-multiple-endpoints
branch
from
aee06a9 to
7064f0f
Compare
There was a problem hiding this comment.
ACK including my review commits. Please see each for details. I've tested this locally running against both the default mempool.space and mempool.emzy.de instances, and saw that everything works as expected. After starting up my pricenode locally and getting a couple successful polling results from both endpoints, I configured Little Snitch to deny connections to mempool.emzy.de, and saw the following error show up in the logs:
May-11 11:42:37.189 [Timer-0] ERROR b.p.m.p.MempoolFeeRateProvider$First: Error retrieving bitcoin mining fee estimation: I/O error on GET request for "https://mempool.space/api/v1/fees/recommended": Host is down (connect failed); nested exception is java.net.SocketException: Host is down (connect failed)
After removing the rule to deny connections, polling started working as per usual once again. So the implementation seems to tolerate intermittent failures of an individual endpoint well.
One thing that's not ideal about the implementation is that it doesn't log explicitly what the calculated average fee rate is. It only logs the result of each request against the individual mempool api endpoints. This makes sense, because the implementation only calculates the average on calls to /getFees, and it would create a lot of noise in the logs to log that each time. It's probably better to leave things as-is for now and if debugging errant fee rates becomes a problem in the future, we can add more sophistication to the logging.
Beyond what's mentioned above, I haven't done what I'd consider exhaustive testing of the implementation, but I have looked at everything closely and I think this is in good enough shape to merge and to give a careful trial deployment in production. @bisq-network/pricenode-operators should just be ready to roll back if something unexpected comes up.
Extend
FeeRateServiceand related classes to allow querying multiple fee estimation API endpoints, instead of just one.The API endpoints are queried independently and in parallel. Each will recover gracefully from any connection issues or timeouts, not influencing the others.
The
FeeRateServiceexposes the average of the retrieved fee rates to the Bisq clients. Alternatively, if no fee estimate could be queried, it will returnBitcoinFeeRateProvider.MIN_FEE_RATE.Up to 5 API endpoints are supported. They can be configured in the file
application.properties.When no configured API endpoint is reachable, or none is configured, the logic falls back to asking the https://mempool.space endpoint. By default, 2 endpoints are configured.
Fixes bisq-network/projects#27