-
Notifications
You must be signed in to change notification settings - Fork 29
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
Retry initial client connections #111
Conversation
610cb55
to
cadcd6a
Compare
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.
cadcd6a
to
26ceb4d
Compare
electrum_client
dependency and add new TLS-backend features
lightningdevkit/rust-lightning#3587
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. Below is the reproduction process (requires # 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 |
Mad, thanks man. You'll have to give me a minute please. I'll get to this soon as I can. |
utACK 26ceb4d |
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! |
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 |
Done: RCasatta/electrsd#100 |
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 tocorepc-node
:Previously,
corerpc-node
would try to establish a connection via the givenAuth
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.