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

electrum: ping server before returning client #1282

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Sep 6, 2024

Ping server as an extra connectivity check.

@jp1ac4 jp1ac4 self-assigned this Sep 6, 2024
@jp1ac4 jp1ac4 marked this pull request as draft September 6, 2024 15:56
@nondiremanuel nondiremanuel added this to the v7 - Liana milestone Sep 6, 2024
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Sep 7, 2024

I haven't been able to reproduce the test failures on my local machine. Still investigating.

@darosior
Copy link
Member

lianad logs for CI failure:

b"[1725971004][liana][INFO][thread main] Created a new data directory at '/tmp/lianad-tests-5getftrr/test_coinbase_deposit_1/lianad/regtest'"
b'[1725971004][liana::database::sqlite][INFO][thread main] Created a fresh database at /tmp/lianad-tests-5getftrr/test_coinbase_deposit_1/lianad/regtest/lianad.sqlite3.'
b'[1725971004][liana::database::sqlite][INFO][thread main] Checking if the database needs upgrading.'
b'[1725971004][liana][INFO][thread main] Database initialized and checked.'
b'[1725971004][electrum_client::raw_client][INFO][thread main] Trying to connect to 127.0.0.1:39451 (attempt 1/1) with timeout 3s'
b'[1725971004][lianad][ERROR][thread main] Error starting Liana daemon: Error setting up Electrum interface: \'Electrum client error: \'Electrum error: \'Electrum server error: {"code":-32603,"message":"unavailable index"}\'.\'.\'.'

@darosior
Copy link
Member

Please don't re-push just now, i'm trying to debug on Cirrus. Also, we should address the root cause of the issue, not just throw more retries until it passes CI.

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Sep 10, 2024

lianad logs for CI failure:

Thanks 🙏

Please don't re-push just now, i'm trying to debug on Cirrus. Also, we should address the root cause of the issue, not just throw more retries until it passes CI.

I agree. We probably need to look for something else in the logs when starting electrs to consider it ready to receive the ping request.

@darosior
Copy link
Member

darosior commented Sep 10, 2024 via email

@darosior
Copy link
Member

darosior commented Sep 10, 2024

So we want Electrs to not start until its index has is_ready set to true. Helpfully, they don't log anything when they actually set it to true.

Maybe waiting for finished full compaction could be a good enough approximation. But a robust alternative would be to simply (Electrum-)ping the server until it replies with a proper pong (to be clear: in the Python framework, not in the software).

@darosior
Copy link
Member

We could let the more robust solutions as a followup.

@darosior
Copy link
Member

utACK 537ebb2

@darosior darosior merged commit 61bc690 into wizardsardine:master Sep 10, 2024
29 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants