Repository navigation
fix: restore electrum connections after returning from the background - #990
Conversation
Bump ldk-node to 0.7.0-rc.76, which rebuilds a broken Electrum connection and retries a failed broadcast once on the new connection, within each call's deadline.
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. |
[Medium impact] The PR appears safe to merge; no actionable defect was found. SummaryUpdates
No actionable issues were found. Tests were inspected, not run. Acknowledged scope from Reviews (1) · Last reviewed commit: "fix: keep the wallet re-mark for manual ..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Bumps ldk-node to 0.7.0-rc.76 for the Electrum reconnect, and stops a failed scheduled wallet backup from re-marking and rescheduling itself while manual and USDT callers keep the re-mark. Reviewed bc04747, full tier, reasoned from the code and CI (iOS does not build on this reviewer's Linux host).
What I checked, and 4 candidates I ruled out
Read in full: BackupService.swift (trigger, run, persist/write, mark, schedule, observers, restore rewrite), BackupServiceTests.swift around testAutomaticBackupDoesNotRepeatUnrelatedFailures
Call sites traced: persistWalletBackup (UsdtVssBackup.persist, manual runBackup), triggerBackup (BackupViewModel, rewriteMigratedBackups, scheduler)
Dependency pin: Package.resolved revision e494969 is the v0.7.0-rc.76 tag; the release's LDKNodeFFI.xcframework.zip hashes to the Package.swift checksum 34687372...; the v0.7.0-rc.74...v0.7.0-rc.76 compare is 10 commits ahead, 0 behind, touching only src/chain/electrum.rs, its integration test and version files, so no API change for the bindings
CI: validate and Greptile green; Run Tests, Run Integration Tests and build-local still pending at review time
Ruled out
- Scheduled wallet failure is never retried: the job ends with the wallet still required, the same as every non-wallet category on master; the next
markBackupRequired(status observer) or a manual trigger schedules it again, and the 30 minute failure notification still fires fromstatus.required. - Lost version bump on scheduled failure: the old re-mark incremented
backupVersions, but nosyncedupdate happens on the failure path, so nothing reads that bump. - Scheduled cancellation drops the wallet: the
CancellationErrorbranch keepsrequiredfor.wallet. - Snapshot churn stops retrying:
WalletBackupSnapshotErrormaps to.snapshotChanged, whichcanRetryPendingBackupkeeps in the loop.
Merge confidence: 4/5, the test jobs were still running and the reconnect itself is covered only by ldk-node's own tests, not by anything in this repo.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base bd09966, at bc04747.
No new actionable code findings.
ldk-node 0.7.0-rc.76 (e494969a872454be69ae38eb7efa2e57c06d01a5) is tag v0.7.0-rc.76. The compare with v0.7.0-rc.74 replaces the main Electrum client after a broken connection and retries one broadcast inside the original 15s deadline. A timeout stays on the current client and returns an unknown outcome. The published LDKNodeFFI.xcframework.zip SHA-256 matches Package.swift (34687372866db01da601951de2cd106638111a1be2a7642c71719beef2a03aac). The same pin is in bitkit-android#1532 at 0e13a52; that checkout was compared only for this pin and the backup split.
A scheduled wallet backup now writes through writeWalletBackup, so a hard failure stays required until the next wallet change or wallet start. persistWalletBackup still re-marks for manual backups and USDT payments. That matches #988 and Android's split between uploadWalletBackup and persistWalletBackup at the compared revision.
#967's reconnect is in this revision. Showing that a broadcast never reached the server, and rebroadcasting while the Paykit request is still valid, remain in #971, as this PR states.
Validation: Inspected BackupService.swift, its callers, and BackupServiceTests (testAutomaticBackupDoesNotRepeatUnrelatedFailures asserts a single attempt and that the wallet stays required). Inspected ldk-node src/chain/electrum.rs at e494969, including the queued-drop and stalled-reconnect tests. Downloaded the rc.76 xcframework and checked its SHA-256. Did not run the iOS tests or a simulator. Run Tests, Run Integration Tests, and build-local were still in progress.
Recommended before device testing: Wait for Run Tests. #988 says testAutomaticBackupDoesNotRepeatUnrelatedFailures was failing on master.
Device testing: not performed in this review.
Suggested additional test cases
- iOS, regtest or staging, with
scripts/electrum-bridge.pyfrom bitkit-android#1532 (blackhole; this iOS branch does not include the script). Return from the background and start an on-chain send while requests are swallowed. Expected: the send settles within the 15s broadcast deadline, and the outcome stays unknown for as long as the blackhole stays up. The author's drop-mode case remains the check that a broken connection is sent.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at bc04747: no findings. This round was reviewed on Opus because Fable credits ran out.
- Pin:
project.pbxprojchanges onlyexactVersion0.7.0-rc.74 → rc.76, andPackage.resolvedchanges only the ldk-node entry toe494969a872454be69ae38eb7efa2e57c06d01a5. That is the same tag and revision as synonymdev/bitkit-android#1532, whose rc.75 → rc.76 delta I reviewed: every new early exit returnsUnknown, neverRejected, the broadcast is capped at 15 s, the reconnect is detached, andlightningstays 0.2.7. - Backup: the hunks are line for line those of #971's
33d7ca7/55445f9, including the manual re-mark.runScheduledBackupwrites without re-marking, so a failed scheduled backup no longer loops every ~5 s. The wallet staysisRequiredand re-runs on the next data change, at wallet start (scheduleFullBackup) or from the failure notice, as in v2.5.1. Manual, migration and USDT callers keep the re-mark. - Nothing from #971's resend, reconnect-wait or cancel-expired features is included.
- CI:
testAutomaticBackupDoesNotRepeatUnrelatedFailurespasses here and fails on master (runs 38957644856, 38927972797), so this fixes master's red.Run Testsis red only onBitkitUITests.testExample("Failed to get background assertion … Timed out"), a simulator flake; all 2623 unit tests pass. It needs a re-run before merge. - Android parity: Android ships rc.76 via #1532 and does not need the backup fix, since its scheduled and manual backups never re-mark on failure.
Release: issues #967 and #988 carry the 3.0.0 milestone, so this PR likely should too.
Coordination: #971's branch also has changelog.d/next/967.fixed.md. Once this merges, #971's fragment needs its own name and should describe only the 3.0.5 recovery features.
Device gate (partial): bc04747 — not run yet. iOS keeps the node and its Electrum connection alive in the background, which is how #967 was reported, so it is worth one run. Background ~10 min, or drop the connection, then foreground and send on-chain. Expect a reconnect in the log, no repeated Broken pipe, and the send settling within 15 s. The device result will follow.
There was a problem hiding this comment.
Approving at bc04747. The code scan from my earlier comment stands, with no findings. This round was reviewed on Opus because Fable credits ran out.
Device gate: bc04747 — iPhone 17 Pro simulator (iOS 26.5), Debug E2E_BUILD on staging regtest. The Electrum server was pointed at a local scripts/electrum-bridge.py forwarding to staging. The app was backgrounded 90 s with the bridge dropping the connection, then foregrounded with a bitcoin: URI and sent.
| Run | Result | Sync after resume |
|---|---|---|
master bd09966 (rc.74), drop |
Payment Pending: Broken pipe (os error 32) ×3, outcome unknown, and tx 1b03c561 never reached the mempool |
never recovered; on-chain sync failed every 10 s for 2.5+ min |
| this PR (rc.76), drop | Bitcoin Sent: Successfully broadcast 84bc2cef |
Reconnected to Electrum server after a failed call 0.7 s after foreground; on-chain sync finished at +9 s |
| this PR, blackhole | Payment Pending (Unknown): broadcast timed out: deadline has elapsed at exactly 15.0 s |
recovered once the blackhole was lifted |
This reproduces #967 on master and shows it fixed here.
perf: swipe → result 2.6 s after a drop (master baseline with no drop: 2.2 s). Broadcast capped at 15 s.
Seen on both builds, outside this PR: after a drop, getting from foreground to Swipe To Pay takes ~17 s. One fee-rate call left on the dropped connection waits out its full 15 s deadline (Updating fee rate estimates timed out, getChannelFundableBalance … took 15.0 seconds), and the amount → confirm step waits on it. The payment outcome is not affected.
CI: unit, integration and build-local are green at bc04747; e2e-tests-local was still running when I posted.
|
Result: ldk-node rc.76 alone fixes the bug, so the app-side recovery work (#971, synonymdev/bitkit-android#1553) isn't needed and is closed. Device check on this PR's head
Same check on synonymdev/bitkit-android master |
ben-kaufman
left a comment
There was a problem hiding this comment.
Reviewed bc04747. No material findings. Verified the LDK release pin and published artifact checksum, and the scheduled-backup change preserves manual and USDT backup behavior. All 10 BackupServiceTests pass locally. SwiftFormat and whitespace checks pass.
Hosted unit, integration, and build checks pass. E2E is still running. I did not independently rerun the network-fault device scenarios locally. This approval does not mark the E2E check complete.
Closes #967, closes #988
Twin: synonymdev/bitkit-android#1532
Refs:
Description
0.7.0-rc.76. After Bitkit returned from the background, the Electrum server had dropped the idle connection and ldk-node never rebuilt it, so wallet sync, fee updates and every broadcast failed withBroken pipeuntil the app was relaunched, and a Pubky Shop payment never reached the network. ldk-node now replaces the connection once a call finds it broken and retries a failed broadcast once on the new connection, within each call's original deadline.0.7.0-rc.76is0.7.0-rc.74plus this fix, with no API change. The same pin is in fix: reconnect electrum after the server drops the connection bitkit-android#1532.BackupServiceTests.testAutomaticBackupDoesNotRepeatUnrelatedFailuresred. The scheduler now writes the wallet backup without the re-mark, so it retries only by its own rules; the backup stays required, and other callers such as USDT payments keep the re-mark.This is the 3.0.0 RC3 part of #971, split out so RC3 ships only the connection fix and the backup fix. The recovery features stay in #971 for 3.0.5: resending a payment whose outcome is unknown, waiting for the network after a long stay in the background, and cancelling an expired payment.
Out of Scope
Design
N/A — no UI changes.
Preview
N/A
QA Notes
Journeys
N/A — no user-visible behaviour change.
Manual Tests
Broken pipein the log — needs an Electrum connection dropped while a call waits on it (scripts/electrum-bridge.pydrop mode, on fix: recover on-chain sends after the app returns from the background #971's branch).Automated Checks
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; a stalled reconnect keeps calls within their deadlines.BackupServiceTests— a failed scheduled wallet backup stays required and does not queue itself again.