Skip to content

Conversation

@sstone
Copy link
Member

@sstone sstone commented Jun 27, 2023

When we connect to an electrum server we perform a "handshake" that includes exchanging protocol version messages and retrieving the server's current tip, and now we also retrieve onchain fees.

@sstone
Copy link
Member Author

sstone commented Jun 29, 2023

Moving fee retrieval into the electrum handshake means that if the server returns an error it will be caught by our coroutine exception handler and will not crash the application (as in the case now), I included a test for that.

@sstone sstone force-pushed the electrum-get-feerate branch 5 times, most recently from a51c29a to 636a768 Compare July 6, 2023 16:35
When we connect to an electrum sever we perform a "handshake" that includes exchanging protocol version messages and
retrieving the server's current tip, and now we also retrieve onchain fees.

We also add an extra, optional CoroutineExceptionHandler to ElectrumClient's constructor, which can be used for testing or to specify a different
behaviour to the one that is currently hard-coded.

Since this is done during the connection handshake, errors will be caught by the corouting exception handler that
we use in the client and will not crash the application.
@sstone sstone force-pushed the electrum-get-feerate branch from 636a768 to fed39c7 Compare July 10, 2023 09:08
@sstone sstone marked this pull request as ready for review July 10, 2023 13:45
@sstone sstone requested review from pm47 and t-bast July 10, 2023 14:18
@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?

@sstone
Copy link
Member Author

sstone commented Sep 18, 2024

I don't think so since we still ask the electrum server we're connected to for fee estimates and it may return an error ? The point here was do we add this to the handshake we perform when we connect to the server or do we try a different fix.

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