Skip to content

Retry initial client connections #111

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

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Mar 26, 2025

I'm not sure what of the behavior really changed since the bitcoind days, but here's yet another thing I stumbled across while trying to making the switch to corepc-node:

Previously, corerpc-node would try to establish a connection via the given Auth cookie, and would panic if it couldn't setup a initial connection. Only the rest of the initial setup steps would be retried with time-delay.

This however was race-y, as the auth cookie might not already be synced to disk on first startup of a fresh bitcoind instance (with fresh data dir), when we try to connect for the first time. Here, we move the intial connection logic into the retry loop to simply also retry it instead of panicking.

@tcharding Seems this might be the last issue (fingers crossed) that keeps LDK and LDK Node from switching our CI over to corepc-node. As the switch is also blocking some features, it would be much appreciated if this could be released soonish after it's reviewed.

@tnull tnull force-pushed the 2025-03-retry-initial-client-connections branch 2 times, most recently from 610cb55 to cadcd6a Compare March 26, 2025 12:42
Previously, `corerpc-node` would try to establish a connection via the
given `Auth` cookie, and would panic if it couldn't setup a initial
connection. Only the rest of the initial setup steps would be retried
with time-delay.

This however was race-y, as the auth cookie might not already be synced
to disk on first startup of a fresh `bitcoind` instance (with fresh data
dir), when we try to connect for the first time. Here, we move the
intial connection logic into the retry loop to simply also retry it
instead of panicking.
@nervana21
Copy link
Contributor

tACK 26ceb4d

Thanks @tnull!

PR tested and reviewed by cloning main (2b410f2), then applying two patches:

failing-tests.patch – adds two tests that fail on main.
pr-111.patch – fixes those failures by adding retry logic implemented in this PR.

Below is the reproduction process (requires sleep and bitcoind in PATH on Unix-like systems):

# 1) Clone the repo and switch to main
git clone https://github.com/rust-bitcoin/corepc.git
cd corepc
git checkout main

# 2) Apply the failing-tests patch (two tests that fail on main)
git apply failing-tests.patch

# 3) Run tests -- these new tests should fail before fix is applied
cargo test --lib test_with_conf_ -- --test-threads=1

# 4) Apply pr-111.patch (the retry logic fix)
git apply pr-111.patch

# 5) Run tests again -- now they pass, after the fix has been applied
cargo test --lib test_with_conf_ -- --test-threads=1

@tcharding
Copy link
Member

Mad, thanks man. You'll have to give me a minute please. I'll get to this soon as I can.

@tcharding
Copy link
Member

utACK 26ceb4d

@tcharding
Copy link
Member

As the switch is also blocking some features, it would be much appreciated if this could be released soonish after it's reviewed.

Sure can. I've got a bunch of patches locally that I might push up first but I'll try and release tomorrow for you. No guarantees though.

@tcharding tcharding merged commit 3ff2294 into rust-bitcoin:master Apr 3, 2025
27 checks passed
@tnull
Copy link
Contributor Author

tnull commented Apr 3, 2025

As the switch is also blocking some features, it would be much appreciated if this could be released soonish after it's reviewed.

Sure can. I've got a bunch of patches locally that I might push up first but I'll try and release tomorrow for you. No guarantees though.

Thank you!

@tcharding
Copy link
Member

Tagged and published.

@tnull
Copy link
Contributor Author

tnull commented Apr 4, 2025

Tagged and published.

Thanks again! As it was published as a minor release, not a patch, I'm afraid it will need another round of bumping the dependent crates. But I'll open a PR for electrsd at least.

@tnull
Copy link
Contributor Author

tnull commented Apr 4, 2025

Done: RCasatta/electrsd#100

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