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

Retrieve feerates everytime we reconnect to electrum #489

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Jun 23, 2023

Before this change, we were only retrieving the feerates once during the lifetime of the app. If we happen to connect to an unhealthy Electrum server (connection succeeds, but then we get ServerError(request=EstimateFees(confirmations=18), error=JsonRPCError(code=-102, message=server busy - request timed out))), then the app will fail to start and actually crash.

This change is more subtle than it may look. Not only do we catch and ignore errors, but we also retrieve feerates at each reconnection. The goal is that in the scenario above we would try another electrum server and get back on our feet.

⚠️ It's very hard to test and I'm not sure of the side effects.

And catch errors, so if current Electrum server has a problem we can
retry with a new one.
@pm47 pm47 requested a review from t-bast June 23, 2023 14:56
@sstone
Copy link
Member

sstone commented Jun 27, 2023

Since we want to retrieve onchain fee rate each time we connect to an electrum server, and that some may fail to return a valid response, another approach would be to add retrieving fee rates to the handshake we perform when we establish a new connection, see #490 ?

@t-bast
Copy link
Member

t-bast commented Sep 18, 2024

Isn't this obsolete after #649 where we let our peer advertise the feerates they accept through the recommended_feerates message?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants