Repository navigation
fix: reconnect electrum after the server drops the connection - #1532
Conversation
Bump ldk-node to 0.7.0-rc.75, which rebuilds the shared Electrum connection once a call finds it broken and retries a failed broadcast on the new connection.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
[Critical impact] No blocking issue was found in the reviewed changes. SummaryUpdates
Reviews (2) · Last reviewed commit: "fix: keep electrum reconnects within eac..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Bumps ldk-node-android to 0.7.0-rc.75, which replaces the shared Electrum client after a broken connection and retries a broadcast once on the new connection, plus a changelog fragment for #1526. Reviewed a22e4c6, full tier.
What I checked, and 4 candidates I ruled out
Read in full: gradle/libs.versions.toml, changelog.d/next/1526.fixed.md, the configureChainSource call in LightningService.kt
Upstream diff: v0.7.0-rc.74...v0.7.0-rc.75 in synonymdev/ldk-node changes src/chain/electrum.rs, tests/integration_tests_rust.rs and version strings only. The bindings are unchanged apart from the version, so the body's "rc.74 plus this fix, with no API change" holds. src/chain/electrum.rs at the tag (dc3e53d) is the head I reviewed on synonymdev/ldk-node#127.
CI: CI build, lint, detekt and build-local green on a22e4c6, so rc.75 resolves from the registry and compiles against the app. The E2E run was still in progress when I reviewed.
Ruled out
- Unrelated changes riding along in rc.75: the merge commits for synonymdev/ldk-node#125 and #126 between the two tags add no file changes beyond the test, and the tag sits 2 commits ahead of ldk-node
mainwith nothing behind. - Reconnect and retry stretching a send past its deadline: in ldk-node the reconnect runs after the failed call, with the configured socket timeout. Bitkit sets
connectionTimeoutSecs = Env.walletSyncTimeoutSecs(10 s), not the 255 s ceiling, so the reconnect and the retried broadcast are each bounded by a 10 s socket timeout, not the 255 s ceiling. The open review point on synonymdev/ldk-node#127 about keeping this inside the original deadline would land in a later rc, not this pin. - The deferred concurrent-reconnect race (failed connect alongside a successful one): its outcome is an
Unknownbroadcast that the next call recovers from, and the ldk-node PR explicitly leaves it for a later rc. nativeDebugSymbolspinned to a different version: it reads the samelibs.ldk.node.androidentry, so both move together.
Merge confidence: 4/5, the app has no test that drops an Electrum connection (the manual check in the body is still open) and the E2E run had not finished.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at a22e4c6: no findings. I reviewed it as a wallet-SDK bump.
Checked:
ldk-node-android0.7.0-rc.74 → rc.75. Upstream,v0.7.0-rc.74..v0.7.0-rc.75is571a1da fix: reconnect electrum after the connection brokeplus the release commit. It touches onlysrc/chain/electrum.rsand an integration test. The UDL, persistence, KV/VSS and channel-monitor code are unchanged, andCargo.lockchanges only the ldk-node version (bdk, electrum-client 0.24.1 and lightning unchanged). There is no migration surface.- The v2.5.1 security fix is carried:
lightningis 0.2.7 at rc.66.1, rc.72, rc.74 and rc.75. - Reconnect logic: only "connection broken" errors reconnect (io errors other than WouldBlock/TimedOut, JSON EOF).
Arc::ptr_eqcollapses concurrent failures into one reconnect, and a failed connect keeps the old client. A broadcast reconnects once and retries once.TimedOutstaysUnknownwith no retry. An already-delivered tx maps to Accepted, or to Unknown via "already known", never to Rejected. A slow server does not cause reconnect churn. - No app API change:
OnchainSendResultmapping (LightningService.kt:1007-1009) is unchanged, and the build is green. - iOS twin synonymdev/bitkit-ios#971 (draft) pins the same rc.75.
Performance-relevant (reconnect of background work): at most one connect per failed call, no loop and no sleep. Worst case is about 2–3 connects every 10 s with the server unreachable.
Note: the rc.75 tag sits on the still-open synonymdev/ldk-node#127, while rc.74 was tagged on main. If #127 changes before it merges, rc.75 becomes an orphan tag.
Device gate (partial): a22e4c6 — not run yet. Pending: the #1526 path on master and on this PR (break the connection with airplane mode, foreground, send on-chain; expect Reconnected to Electrum server after a failed call. and an Accepted result), the reconnect rate per minute while offline, the first sync after reconnect, and onchain-accepted-result.xml / onchain-original-payment-retry.xml. The device result will follow.
|
Device gate (partial): Emulator
The #1526 symptom (persistent Seen on master, outside this PR: a native SIGABRT at 14:48:36 UTC, |
jvsena42
left a comment
There was a problem hiding this comment.
Approving at a22e4c6. The code scan from my earlier review stands, with no findings.
Device gate: a22e4c6 — journeys: 2 passed. perf: the first sync after an airplane drop took 4.4 s (master 16.2 s), and after the PR's reconnect 3.5 s; steady state was 657–693 ms idle on both builds. While offline the PR made 7 reconnect attempts in 2 min (one per failed call, no loop). No ANR, OOM or crash on the PR build. l10n: no UI text changed.
onchain-accepted-result.xml: 11/11 pass. Both sends showedSendSuccess, and theTXIDmatchedSuccessfully broadcast transactionin the log. The Max send went through the >50% fee dialog.onchain-original-payment-retry.xml: 12/12 pass against a real network fault. A local proxy swallowed the broadcast and refused connections for 40 s, givingFailed to broadcast transaction due to timeout … deadline has elapsedand Payment Pending. The retry kept the original input, Back without the PIN broadcast nothing, and the approved retry broadcast3a132712..fd82at 281 sats.- Broken connection during a send, the same server-side FIN 5 s after a request on both builds: master refused the send with
SyncUnhealthyError='Wallet sync failed before send', and the PR sent it (Accepted). The PR's path fired asReconnected to Electrum server after a failed call.227 ms after a failed sync.
- The #1526 symptom itself (persistent
Broken pipe, outcome unknown) did not reproduce on master in 5 attempts on the emulator. - Android stops the node in the background, so that state can only arise in the foreground.
Retrying broadcast ofand two calls queued on one dropped connection were never reached.
Details and logs: #1532 (comment)
Bump ldk-node to 0.7.0-rc.76, which adds the deadline bound to the Electrum reconnect and broadcast retry from 0.7.0-rc.75, and point the marketplace journey at it.
|
@jvsena42 thanks for the device gate. This PR now pins |
|
Delta review Checked:
Device gate (partial):
|
|
@jvsena42 thanks for the delta check. GitHub still needs an approval on |
|
Device results on The CI APK was run on
No crash or ANR. |
Closes #1526
Twin: synonymdev/bitkit-ios#971
Refs: synonymdev/ldk-node#127
Description
0.7.0-rc.76for 3.0.0 RC3, and points the marketplace journey at it. When the Electrum server dropped the connection while a call was waiting on it, ldk-node never rebuilt it: wallet sync, fee updates and every broadcast failed withBroken pipeuntil the node restarted. Bitkit iOS RC2 hit this with a Pubky Shop payment. ldk-node now replaces the connection once a call finds it broken, and retries a failed broadcast once on the new connection, all within each call's original deadline.0.7.0-rc.76is0.7.0-rc.74plus this fix, with no API change.Out of Scope
Design
N/A — no UI changes.
Preview
N/A — no user-visible changes.
QA Notes
Journeys
N/A
Manual Tests
Drop the Electrum connection while a call waits on it, then pay on-chain → the payment is sent — needs an Electrum connection dropped mid-call.
Steps (staging regtest, about 15 min)
devDebugAPK on an emulator (staging regtest is its default backend) and create a new wallet.adb shell uiautomator dump), then127.0.0.1:50099, forwards over TLS to the staging Electrum server and takes control files in/tmp/electrum-bridge:adb reverseif adb reconnects.127.0.0.1, port50099, TCP, Connect To Host. Pass: "Currently connected to 127.0.0.1:50099" and the bridge prints twoopenlines. (adb shell input textcan drop characters; type the host in pieces and check it.)adb logcat -s LDK.touch /tmp/electrum-bridge/drop, and wait for the bridge to printdrop: queued … -> closing(the next sync, up to a minute).adb shell am start -a android.intent.action.VIEW -d "bitcoin:<address>?amount=0.00002" to.bitkit.dev, Continue, Swipe To Pay.Incremental sync of on-chain wallet failed(EOF while parsing, thenBroken pipe) followed within a second byReconnected to Electrum server after a failed call.; the send shows "Bitcoin Sent" and the logSuccessfully broadcast transaction <txid>.Broken pipewith noReconnectedline, and the send ends as pending with an unknown outcome.Automated Checks
scripts/electrum-bridge.py— the bridge used for the manual test.0.7.0-rc.76: sync failed withEOFandBroken pipe, reconnected 2 ms later, paymentc316914f…sent.electrum.rsin fix: reconnect electrum after the server drops the connection ldk-node#127 — a dropped connection with a queued call no longer leaves broadcasts failing.