Repository navigation
Conversation
Once the wallet syncs again, rebroadcast a payment whose broadcast outcome is unknown with the same coins at no higher fee, while its Paykit request is still valid, so a send cut off by a dropped connection is not silently dropped.
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. |
 [Medium impact] No blocking issue was identified in the reviewed changes. SummaryUpdates
Reviews (2) · Last reviewed commit: "fix: keep electrum reconnects within eac..." · Reviewed by Greptile |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Pins 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 once on the new connection. Reviewed c56fb64, full tier (dependency manifest). The Swift side was reasoned from the code and the ldk-node diff; iOS does not build here.
What I checked, and 3 candidates I ruled out
Pin: tag v0.7.0-rc.75 points at dc3e53d, the revision in Package.resolved; pbxproj and Package.resolved agree on the version.
Binary: the Package.swift checksum at that tag (7075b2aa…05da) matches the sha256 GitHub reports for the release's LDKNodeFFI.xcframework.zip.
Scope of rc.74 to rc.75: rc.74 is an ancestor. Between them are the Electrum reconnect commit, the release commit, and two merges (#125, #126: sign-foreign-psbt-inputs, a force-close HTLC test). The only source change outside tests is src/chain/electrum.rs. bindings/ldk_node.udl and src/lib.rs are unchanged, so the Swift API does not change.
ldk-node change: read in synonymdev/ldk-node#127 at dc3e53d.
Ruled out
- Duplicate broadcast from the retry: a rebroadcast of the same transaction has the same txid, so a second accept does not create a second payment.
- API break in the generated bindings: no UDL or public API change between the two tags.
- Unreviewed code in the pinned range: the extra merges are already in synonymdev/ldk-node master.
Context, not a finding: the pinned tag was cut from the synonymdev/ldk-node#127 branch. That PR is still open, with a change request from @ben-kaufman about reconnect time limits.
Merge confidence: 4/5, Run Tests and Run Integration Tests were still running at this head when I reviewed, and nothing here builds iOS.
Background wallet syncs report only their completion and never set isSyncingWallet, so the resend never ran outside a manual sync. Trigger it from the on-chain sync-completed event instead.
…sume After a stay in the background long enough for the Electrum server to drop the connection, show the send sheet's connecting screen for on-chain payments until a wallet sync succeeds again, then continue on its own; after 20 s it shows the existing connection-issues screen.
Only a send with an on-chain target waits for the network after resume; the options screen stays reachable for scan, paste and Lightning.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1277827b39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Recovery check for #986 on the build at deeec53, using the same stuck wallet from the earlier run on an iPhone 26.5 sim (regtest staging): the wallet sends on-chain again. Cancel the expired payment, then send ✅Before the install, payment
|
|
@ben-kaufman this PR now also carries the fix for #988, in 33d7ca7 and 55445f9, which touch your #950 backup code. The scheduled path now calls |
ben-kaufman
left a comment
ben-kaufman
left a comment
There was a problem hiding this comment.
Reviewed e690f4a. The earlier reconnect findings are addressed, but the three recovery issues below remain. Isolated Swift helper checks reproduce stale-sync acceptance and loss of recurring cancellation eligibility. These are helper tests, not device or funded-payment tests.
SwiftFormat also reports one introduced hoistTry finding in BitkitTests/PaykitPaymentProofServiceTests.swift:178. The other 16 diagnostics were verified against the merge base. Unit, integration, and build checks are still running.
| let node = onchainDispatchNode as? Node, let inputs = attempt.recoveryContext?.inputs, !inputs.isEmpty | ||
| else { return false } | ||
| let requestedAt = UInt64(Date().timeIntervalSince1970) | ||
| Task { try? await self.sync() } |
There was a problem hiding this comment.
A sync completed earlier in this same second passes this comparison before the new sync finishes, and the task discards sync errors. That lets cancellation remove the send guard using stale wallet state when the requested sync fails. Could we await a confirmed post-request sync completion before checking candidates, and cover an old same-second timestamp with a failed sync?
There was a problem hiding this comment.
Fixed in 7ba0406: the check now reads the last on-chain sync timestamp before requesting the sync and waits, up to 30 s, for one strictly later. A sync that finished earlier in the same second no longer counts, and a failed sync never moves the timestamp, so cancellation reports that it could not sync instead. testOnlyASyncFinishedAfterTheRequestCountsAsFresh covers the same-second case.
| (proof.onchainWalletId == nil || proof.onchainWalletId == WalletScope.default) && | ||
| (proof.paymentIdentifier == nil || attempt.containsCandidate(proof.paymentIdentifier))) | ||
| } | ||
| if remaining != proofs { try await persist(remaining) } |
There was a problem hiding this comment.
If the app closes after this save and before the attempt is deleted, a recurring payment cannot finish cancellation after restart. recurringPeriodHasEnded needs the billing period from the proof we just removed, so it returns false even when the subscription is canceled and the attempt keeps blocking new sends. Could we retain enough release evidence with the attempt to resume this cleanup, and cover that restart state?
There was a problem hiding this comment.
Fixed in 7ba0406: once the request is found ended, the attempt is marked releaseApproved before the proof is removed, so after a restart onchainRequestHasEnded returns true from the attempt alone and the release finishes after a fresh network check. A canceled or rejected subscription also counts without the local proof now, matching the record's proofs by the attempt's billing-period start. testApprovedReleaseFinishesAfterItsProofWasRemoved covers the restart state for an active subscription.
| return true | ||
| case .activeRecurring: | ||
| guard let subscription = PaykitSubscription(record: record) else { return false } | ||
| return subscription.isExpired(at: now) || |
There was a problem hiding this comment.
This recurring branch ignores the original quote's expiry. If that quote expires before the period payment deadline, recovery rejects it but this still returns false, so the unresolved payment cannot be retried or canceled. Could we check the stored proof's conversionQuoteId against the materialized period's submission deadline too, and cover an expired quote on an otherwise active subscription?
There was a problem hiding this comment.
Fixed in 7ba0406: for an active subscription, the period also counts as ended once the stored proof's conversionQuoteId deadline (submissionDeadline(quoteId:) on the materialized period request) has passed. testExpiredQuoteEndsAnActiveSubscriptionPeriod covers a live and an expired quote. The hoistTry finding in the tests is fixed too.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of changes since 4cd8f432, at e690f4a3. Inherited baseline a0a05e2a-9bd8-4a9d-a26a-b311007889fa, with the same base and merge base eb3156ab. This pass reviewed the scheduled wallet-backup re-mark split, restoredFromBackup, subscription period release, the Payment Pending cancel recheck, and the new unit tests. Inherited coverage is the unchanged ldk-node 0.7.0-rc.76 pin, rebroadcaster rules, baseline predicate tests, electrum bridge, reconnect monitor and pre-send routes, and the bitkit-android#1532 reconnect contract at 0e13a52e.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
An ended subscription payment cannot be released once its local proof is gone, so the crash window documented on abandonUnbroadcastOnchainPayment leaves that attempt blocking on-chain sends. bitkit-android#1553 at e6cc735 still releases a canceled or rejected subscription from the attempt's billing-period start. The scheduled-backup change matches testAutomaticBackupDoesNotRepeatUnrelatedFailures: a failed scheduled wallet backup stays required and does not queue itself again. Earlier reconnect-wait threads are unchanged since 4cd8f43.
Validation: source inspection only. unit-tests and integration-tests were still in progress on e690f4a. Device testing: not performed in this review.
Suggested additional test cases
- iOS unit test. Setup: a canceled subscription on-chain attempt with an unknown outcome whose local proof was already removed. Action: release it through
abandonUnbroadcastOnchainPayment. Expected: the attempt is removed and no longer blocks a new on-chain send.
Findings
- [HIGH] Release an ended subscription payment after its proof is already gone — inline at
Bitkit/Services/PaykitPaymentProofService.swift:573.
| private func recurringPeriodHasEnded( | ||
| _ record: Paykit.PaymentRequestRecord, requestId: PaykitPaymentRequest.ID, payer: String, now: Date | ||
| ) async throws -> Bool { | ||
| guard let period = try await loadProofs().last(where: { |
There was a problem hiding this comment.
[HIGH] Release an ended subscription payment after its proof is already gone
Trigger: cancel an ended Paykit subscription payment whose on-chain broadcast never reached the network, and the app stops after the started proof is deleted and before the attempt is removed. A second tap sees that same state.
Mechanism: recurringPeriodHasEnded returns false unless a local on-chain proof still carries a billing period. abandonUnbroadcastOnchainPayment removes that proof before abandonUnbroadcast. The next call takes the billing-period branch and treats the request as still open, so loadAbandonableAttempt hides Cancel expired payment. The comment on abandonUnbroadcastOnchainPayment says a later call must still be able to release the attempt. One-time canceled, rejected, and expired requests still return true without a local proof. Android recurringPeriodHasEnded at e6cc735 matches remote proofs from the attempt's billingPeriodStartsAt and still returns true for a canceled or rejected subscription when the local proof is gone.
Consequence: the unresolved attempt keeps blocking every new on-chain send, and Payment Pending no longer offers a way to release it. The coins never reached the network, so a later sync does not clear the attempt.
Expected: a later cancel releases the attempt from the request's billing period, including a canceled or rejected subscription whose local proof is already gone.
Evidence basis: source analysis at e690f4a. This review did not run the unit tests or a device session. testReleaseFinishesWhenTheProofWasAlreadyRemoved covers this window only for a one-time request, and testEndedSubscriptionPeriodCountsOnlyWithoutItsOwnProof keeps the local proof seeded.
There was a problem hiding this comment.
Fixed in 7ba0406: the attempt records releaseApproved before its proof is removed, so a later cancel finishes from the attempt alone. A canceled or rejected subscription now counts as ended without the local proof, matching the record's proofs by the attempt's billing-period start. testCanceledSubscriptionReleasesAfterItsProofWasRemoved is the suggested test; testApprovedReleaseFinishesAfterItsProofWasRemoved covers the restart state for an active subscription.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of changes since e690f4a3, at 7ba0406d. Inherited baseline review 5553204785.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
This commit persists releaseApproved before the started proof is removed, treats a canceled or rejected subscription as ended without that local proof, and ends an active period once the stored quote's submissionDeadline(quoteId:) has passed. Cancel now waits for an on-chain sync timestamp strictly later than the one read before the request. Those paths match the open comments on same-second sync, restart after proof removal, quote expiry, and the baseline release finding. Wallet restore still compares the whole attempt after clearing only restoredFromBackup and candidate fees, so a release-approved attempt fails the restore and can leave the app on the restore-failed screen.
The new release, quote, and sync predicates were compared with bitkit-android#1553 at e3688a622719658be4c6ae43434c8253b473fb20. Unit tests were inspected and not executed. Run Tests was still pending. Device testing: not performed in this review.
Suggested additional test cases
- iOS, staging regtest. Leave a Paykit on-chain attempt in the keychain with
releaseApprovedtrue and its started proof already removed, and keep a wallet backup of that same attempt from before the approval. Run a full wallet restore. The restore completes, the local attempt stays release-approved, and a later cancel on Payment Pending removes it after a fresh sync. The app does not remain on the restore-failed screen.
Findings
- [MEDIUM] Wallet restore rejects a release-approved attempt — inline at
Bitkit/Services/OnchainSendAttemptService.swift:196.
| /// Set on an attempt restored from a backup, which may not hold evidence that its transaction was broadcast. | ||
| var restoredFromBackup: Bool? = nil | ||
| /// Set once its request was found ended, so a release cut short after its proof was removed can finish. | ||
| var releaseApproved: Bool? = nil |
There was a problem hiding this comment.
[MEDIUM] Wallet restore rejects a release-approved attempt
Trigger: a wallet restore runs while the keychain still holds a Paykit on-chain attempt with releaseApproved true. approveRelease sets that flag before the proof is removed, and it stays until abandonUnbroadcast deletes the attempt, including when the app stops in between.
Mechanism: synthesized equality includes releaseApproved. The shared ActiveOnchainAttempt backup has no field for it, and restored() leaves it nil. restoreBackup clears restoredFromBackup and candidate fees, then requires the local attempt to equal the restored one. The flagged attempt fails that check and throws OnchainSendAttemptError.unresolved. WalletBackupRestoreGate.beginRestore has already stored a pending restore, and the throw returns before completeRestore.
Consequence: performFullRestoreFromLatestBackup stops. The app stays on the restore-failed screen, and Try Again repeats the same restore. Payment Pending is not reachable from that screen, so the release cannot finish and the retry cannot succeed.
Expected: a restore of the same attempt keeps the local release approval and continues, the same way restoredFromBackup is ignored in this comparison.
Evidence basis: source analysis at 7ba0406d. This review did not execute the restore. Android restoreActive at e3688a622719658be4c6ae43434c8253b473fb20 also leaves releaseApproved out of the fields it normalizes before equality.
There was a problem hiding this comment.
Fixed in 6ff2cee: restoreBackup now leaves the local releaseApproved out of the comparison, as it already does restoredFromBackup, and keeps the local attempt with its approval. testRestoreKeepsALocalReleaseApproval restores the same attempt over an approved one. The Android twin is fixed in synonymdev/bitkit-android#1553 as well.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Delta review aedeceb..7ba0406: my two threads are fixed and resolved, and there is one new MEDIUM inline. This round was reviewed on Opus because Fable credits ran out.
- My MEDIUM is fixed in
4cd8f43.waitsForReconnect(SendSheet.swift:226-235) skips the reconnect wait while submitting and on outcome routes. Confirm still revalidates, andtestReconnectWaitCoversOnlyTheStepsBeforeSendingcovers it. - My LOW is fixed in
4cd8f43. The monitor callswallet.sync(), which dedupes with the foreground sync. A waiter without a fresh on-chain sync throwsOnchainSyncPendingand retries. - Release flow (
deeec53d,65e5210,e690f4a,7ba0406):approveReleaseandabandonUnbroadcastrequire no dispatch in progress and the same attempt.- Racing resends are refused by
authorizeOnchainRecoveryand the dispatch-time deadline. - Write order is release flag, then proof removal, then attempt deletion, with rollback.
- Restored attempts are excluded, and recurring periods need no remote proof for the period.
- Outside the MEDIUM, this is clean.
- Backup (
33d7ca7,55445f9): a failed scheduled wallet backup no longer reschedules itself. The wallet staysisRequiredand re-runs on the next data change, at wallet start or on the periodic failure notice, as in v2.5.1.
Scope: 33d7ca7 and 55445f9 fix #988 in the backup code and are unrelated to this PR's Electrum reconnect. The "Cancel expired payment" flow (#986) is a new Paykit funds feature landing under an "ldk-node electrum reconnect" title. Splitting it out, or saying so in the description, would help reviewers.
Performance-relevant: one sync per resume now. Payment Pending polls every 30 s, and Cancel waits up to 30 s for a fresh sync.
The device gate waits for the MEDIUM fix. It will also re-run the Max-send-and-background check and count Syncing LDK pairs per resume.
| guard nativeDispatchInProgress == nil, let attempt = try currentAttempt(), attempt == expected, | ||
| attempt.canBeAbandoned | ||
| else { throw OnchainSendAttemptError.unresolved } | ||
| try store.save([]) |
There was a problem hiding this comment.
MEDIUM: cancelling an expired payment frees the original inputs without invalidating the signed original, so a later payment with other coins and the original can both confirm.
This is the same mechanism as synonymdev/bitkit-android#1553. abandonUnbroadcast runs try store.save([]) once the attempt is canBeAbandoned and onchainCandidatesNeverReachedNetwork passes (OnchainSendAttemptService.swift:64-81). Nothing keeps the original inputs reserved, and nothing forces a conflicting spend.
Cancel trusts a single Electrum server's history, which ldk-node documents it should not. walletKnowsNoCandidate passes when the configured Electrum server's history has no candidate txid but has every input's parent. That shows only that this server never reported the signed original, not that its bytes reached nobody. ldk-node rc.76 payment/onchain.rs:209 says: "neither Rejected nor Unknown authorizes clearing the original payment guard."
Nothing keeps the original inputs reserved after cancel. prepare_send_to_address does not add the signed tx to the BDK graph or lock its outpoints. Once the guard is gone, the next send's coin selection can pick disjoint UTXOs. On master, the only way out of an Unknown attempt was a retry, and retainPreparedReceipt required receipt.inputs.toSet() == originalInputs.toSet(), so every later spend conflicted with the original.
The button can show within seconds of an Unknown broadcast. The payee controls CANCELED, or the quote deadline may already have passed, and there is no minimum age.
Failure path:
- The broadcast outcome is Unknown.
- The request ends.
- The user taps Cancel and reads "Its coins are available again".
- The user checks out again and pays with other coins.
- The original surfaces later, from a withholding server, a plaintext-TCP custom server that is MITM'd, or a late delivery.
- The payee is paid twice, and Bitkit has already deleted the original's proof.
Against an honest server, the forced fresh sync would see a tx that reached a mempool, so the exposure needs a dishonest or interposed server, or a narrow overload race. But the cost is a second full payment in a common flow, and the cancel copy promises more than the check proves.
Fix: make cancel conflict with the original rather than forget it. Either spend the original inputs to a wallet-owned output and delete the guard only after that replacement is Accepted or Observed, or keep a persisted tombstone of the original inputs and force the next send to include one of them until they are seen spent. At minimum, word the dialog and toast as "may still confirm" until then. The retry path cannot be reused as-is, because retainPreparedReceipt(isRecovery=true) requires the same address and amount.
Reproduction
The Android twin's store test reproduces the gap. After an abandon, a new send's receipt that spends inputs disjoint from the abandoned original is admitted:
java.lang.AssertionError: disjoint send admitted while abandoned original is still valid. Expected an exception of class java.lang.IllegalArgumentException to be thrown, but was completed successfully
On iOS the same path is abandonUnbroadcast → store.save([]) (:994-998). The next prepare places no constraint on inputs. An iOS unit test was not run.
There was a problem hiding this comment.
HIGH: A timed-out local broadcast can still arrive after cancellation
This overlaps @jvsena42's finding above. Adding the concrete timeout evidence from my independently saved source review.
The pinned native implementation can return Unknown while its spawn_blocking(transaction_broadcast) continues; electrum_timeout_returns_unknown_while_blocking_rpc_remains_in_flight explicitly demonstrates that lifetime. A newer sync can observe no candidate before that delayed broadcast is processed.
At Bitkit/Services/OnchainSendAttemptService.swift:78–79, absence from wallet history still permits deleting the started proof and send guard without stopping the old broadcast or invalidating its signed transaction. A later payment using different UTXOs can therefore confirm alongside it.
Retain the guard until dispatch is definitively ruled out or the original candidate is invalidated. Cover a broadcast held past timeout and released after the cancellation check.
Evidence basis: source tracing and the existing native timeout regression test; I have not run a funded/device reproduction of this cancellation path.
There was a problem hiding this comment.
Agreed: cancelling must make the original unconfirmable rather than forget it. This PR moved to 3.0.5, and the RC3 part split out to #990, so the fix lands here for 3.0.5. The planned approach: after the fresh-sync check, cancel spends at least one of the original's coins back to the wallet at a fee above every candidate's, behind the same fee approval and PIN as Retry, and keeps the guard and the started proof until that release is accepted or the wallet sees it. A Rejected or Unknown release keeps both. The dialog will say that cancelling sends the coins back to the wallet and costs a network fee.
There was a problem hiding this comment.
The release transaction planned in the reply above covers this too: it spends one of the original's coins at a higher fee, so a broadcast still in flight can no longer confirm, and the guard stays until that release is accepted or the wallet sees it. It lands here for 3.0.5.
|
This PR now holds the on-chain recovery features 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. The 3.0.0 RC3 part, the ldk-node |
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of changes since 7ba0406d, at 6ff2cee0. Inherited baseline review 5553590165. This pass rechecked restoreBackup equality for a local release approval, the decoded ActiveOnchainAttempt, and testRestoreKeepsALocalReleaseApproval. The other surfaces from that baseline are unchanged and inherited.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Leaving releaseApproved out of the comparison does not let a production wallet restore finish. The decoded backup's follow-up still carries backupCreatedAtMillis, and the live attempt does not, so equality still throws. The earlier restore thread remains open at this revision.
Two cancel threads on this head are outside this delta and were not re-assessed: cancelling frees the original inputs and a timed-out broadcast can still arrive.
Validation: source inspection only. This environment has no Swift toolchain, so OnchainSendAttemptServiceTests was not executed. No device or simulator was started. Android restoreActive was read on bitkit-android#1553 at 5e40108887f75c2451db81f107acd4e00ef86b61.
Device testing: not performed in this review.
Suggested additional test cases
- iOS simulator, staging regtest. Send a Paykit on-chain payment through Confirm so the attempt stores follow-up metadata, and leave the broadcast outcome unknown. End the request and approve release so the attempt remains in the keychain. Run a full wallet restore from the latest backup. Expected: restore completes, the stored attempt still has release approval, and Payment Pending can still cancel that payment after the wallet starts.
Findings
- [MEDIUM] Wallet restore still rejects a release-approved Paykit payment — inline at
Bitkit/Services/OnchainSendAttemptService.swift:439.
| previous.restoredFromBackup = nil | ||
| // A local release approval is not part of the shared backup record. | ||
| previous.releaseApproved = nil | ||
| guard previous == incoming else { throw OnchainSendAttemptError.unresolved } |
There was a problem hiding this comment.
[MEDIUM] Wallet restore still rejects a release-approved Paykit payment
Trigger: a wallet restore runs while the keychain still holds the original Paykit on-chain attempt with releaseApproved true. Confirm stores follow-up metadata on that attempt, and approveRelease sets the flag before the proof is removed.
Mechanism: restoreBackup clears releaseApproved, restoredFromBackup, and candidate fees on the local copy, then requires the rest of the attempt to equal ActiveOnchainAttempt.restored(). That decoder sets followupContext.backupCreatedAtMillis from the wire timestamp. The live attempt leaves that field nil. Synthesized equality fails and throws OnchainSendAttemptError.unresolved. testRestoreKeepsALocalReleaseApproval replays the in-memory pre-approval attempt, which has no follow-up timestamp provenance, so it never decodes a backup.
Consequence: WalletBackupRestoreGate has already stored a pending restore, and the throw returns before completeRestore. The app stays on the restore-failed screen, and Try Again repeats the same comparison. Payment Pending is not reachable there, so the release cannot finish and the retry cannot succeed.
Expected: a backup of the same attempt restores and keeps the local release approval, including the follow-up timestamp the wire format adds. Android restoreActive at 5e40108887f75c2451db81f107acd4e00ef86b61 treats release approval as local-only and keeps the stored approval.
Evidence basis: source analysis at 6ff2cee0ca97991fb3256b2e7fb1cf766b9bc27e. This review did not execute the restore or the unit test.
There was a problem hiding this comment.
Confirmed from the source: the restored copy's followupContext.backupCreatedAtMillis comes from the wire, while the live attempt leaves it nil, so the comparison still fails, and testRestoreKeepsALocalReleaseApproval never decodes a backup. This PR's cancel and resend paths are paused while the RC3 part ships in #990. The fix lands here with the 3.0.5 cancel rework: compare the attempt without the local-only and backup-only follow-up fields, and test it with a real ActiveOnchainAttempt round trip.
|
Closing: ldk-node rc.76 alone fixes the dropped-connection bug, shown on device in #990 (#990 (comment)) and on synonymdev/bitkit-android master. The recovery features here aren't needed for 3.0.0. The branch stays, so this can be reopened if a real case shows up. |
6 participants
Closes #976, closes #986
Twin:
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.bitcoin:link) shows the send sheet's existing "Connecting to network" screen until a wallet sync succeeds, retrying every 3 s, then continues on its own. After 20 s it shows the existing "Connection issues" screen, which keeps retrying. The wait shares the foreground wallet sync and ends once an on-chain sync finishes after the resume. It covers only the steps before sending: the empty Send sheet and the Sent, Pending and Failure screens are not affected, and no new copy needs translation.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.Out of Scope
Design
N/A — no design available; reuses the existing send sheet connecting and connection-issues screens, and adds a secondary "Cancel expired payment" button to Payment Pending.
Preview
Regtest staging, iPhone 17 Pro simulator (iOS 26.5), 2x, through a local Electrum bridge that can drop or swallow requests.
Dropped connection with a call waiting
Connection dropped with a call waiting on it, the state RC2 hit: ldk-node rebuilds the connection and the next send is accepted (recorded on rc.75; rc.76 only bounds it by each call's deadline).
a2x.mp4
Unknown outcome resent after reconnect
Broadcast outcome unknown, then the connection comes back: the same payment is resent at the first sync and accepted.
b2x.mp4
Return from background via a bitcoin: link
Return from 40 s in the background through a
bitcoin:link: "Connecting to network" until a sync succeeds, then the payment is sent.c2x.mp4
Broadcast reply lost
The broadcast reaches the server but its reply is lost: the outcome shows as unknown, the next sync recognizes the payment from the mempool, and nothing is resent.
e2x.mp4
Open confirmation backgrounded while offline
An open, already validated confirmation goes to the background for 40 s while the Electrum server swallows requests: on return it shows Connecting to network, then Connection Issues, and goes back to Confirm once the server answers again.
reval2x.mp4
Cancel refused: transaction already known
A Paykit payment with an unknown outcome whose request has ended blocks a new
bitcoin:payment. Its transaction is already known to the wallet, so "Cancel expired payment" is refused and the payment stays pending.abandon-refused2x.mp4
Cancel released, next payment sent
The same blocked state, with a transaction that never reached the network: after a fresh sync the payment is cancelled, and a new on-chain payment is sent.
abandon-released2x.mp4
Bitcoin Sent kept after background
A payment is sent and Bitcoin Sent stays open while Bitkit spends 45 s in the background: on return the success screen stays as it was, with no connecting screen or balance re-check, and one wallet sync runs.
jv-success2x.mp4
QA Notes
Journeys
N/A
Setup for all three tests (staging regtest, about 10 min)
Debugapp (staging regtest is its default backend) and run it on an iOS Simulator; create a new wallet.xcrun simctl pbpaste <udid>), then127.0.0.1:50099, forwards over TLS to the staging Electrum server and takes control files in/tmp/electrum-bridge:127.0.0.1, port50099, TCP. On a simulator you can instead stop the app and runopenlines.tail -f "$(ls -t ~/Library/Developer/CoreSimulator/Devices/<udid>/data/Containers/Shared/AppGroup/*/logs/*.log | head -1)"Manual Tests
Return from the background into an on-chain payment → "Connecting to network" shows until a sync succeeds, then the payment opens and is sent — needs an Electrum connection dropped while a call waits on it.
Steps
xcrun simctl launch <udid> com.apple.Preferences) and arm a drop:touch /tmp/electrum-bridge/drop.xcrun simctl openurl <udid> "bitcoin:<address>?amount=0.00002"(or a Pubky Shop checkout), and tap Open.drop: … -> closing; the log showsIncremental sync of on-chain wallet failed(allEOF while parsing) followed within a second byReconnected to Electrum server after a failed call.;shouldShowSyncOverlay: true, thenfalseonce a sync succeeds (up to ~20 s, see bug: on-chain payment waits up to 20 s on the connecting screen after resume #978); continue and swipe → "Bitcoin Sent" andSuccessfully broadcast transaction <txid>.Broken pipewith noReconnectedline, and the payment ends as "outcome unknown".Leave an open on-chain confirmation in the background while the server is unreachable → back in front, "Connecting to network" falls back to Connection Issues within 20 s and the confirmation returns once a sync succeeds — needs an Electrum server that swallows requests.
Steps
touch /tmp/electrum-bridge/blackhole, send Bitkit to the background (xcrun simctl launch <udid> com.apple.Preferences) and wait 40 s.xcrun simctl launch <udid> to.bitkit).rm /tmp/electrum-bridge/blackholethe Confirm screen returns.Lose the broadcast reply, then bring the connection back → the same payment is resent once and no second transaction appears — needs an Electrum server that swallows requests.
Steps
touch /tmp/electrum-bridge/blackhole, then swipe. After ~15 s the sheet shows Payment Pending, "Its outcome is unknown", and the log showsFailed to broadcast transaction due to timeoutandOn-chain broadcast outcome unknown for <txid>.rm /tmp/electrum-bridge/blackhole.Rebroadcasting on-chain payment <txid> with an unknown outcomeandRebroadcast of on-chain payment <txid> ended as accepted(txid: "<new txid>"); the pending sheet shows the new transaction ID; Activity has one new Sent row and the balance drops once.Let the broadcast reach the server but lose its reply, then bring the connection back → the payment is recognized once and no second transaction is built — needs an Electrum server that drops only the broadcast reply.
Steps
touch /tmp/electrum-bridge/lose-reply, then swipe. The bridge printslose-reply: forwarding broadcast …anddropped reply …with the txid; after ~15 s the sheet shows Payment Pending, "Its outcome is unknown".rm /tmp/electrum-bridge/lose-reply.New unconfirmed transaction <txid> detected in mempoolandCreated sent onchain activity for txid <txid>; the pending sheet shows "The earlier payment's local details were restored. No new payment was sent."; the log has no furtherCreated new transactionline; the balance dropped by the payment and fee exactly once.Let a Paykit request's deadline pass while its payment's outcome is unknown → no resend and the request is not marked paid — needs a Pubky Shop request and the bridge.
Steps (not run for this PR)
touch /tmp/electrum-bridge/blackhole, swipe, and wait for "Its outcome is unknown".rm /tmp/electrum-bridge/blackhole.Could not rebroadcast on-chain payment <txid>with an expired or unavailable request, noRebroadcast … ended asline, and the Shop order stays unpaid.Cancel a Paykit payment whose request has ended while its outcome is unknown → it is cancelled only if none of its transactions reached the network, and new on-chain sends work again — needs a Pubky Shop request and the bridge.
Steps
touch /tmp/electrum-bridge/blackhole, swipe, and wait for "Its outcome is unknown". Keep the blackhole until the Shop checkout ends or its request expires (about 30 min), thenrm /tmp/electrum-bridge/blackhole.xcrun simctl openurl <udid> "bitcoin:<address>?amount=0.00001". Payment Pending opens with "Cancel expired payment".Released on-chain payment <txid> after its request ended without it reaching the network, and opening the payment again reaches Swipe To Pay and sends it. The Shop order stays unpaid.lose-replyinstead ofblackholein step 2, so the transaction reaches the mempool: the toast says "This payment may still reach the network, so it cannot be cancelled." and the log showsmayStillConfirm; or, once the next sync recognizes the transaction, the payment resolves as sent and no cancel is offered.Open Send from the tab bar right after returning from the background → the options screen opens without waiting.
Steps
Automated Checks
OnchainSendAttemptServiceTests.swift— only an unknown original with one candidate and no order is resent, at its approved fee; a try that stopped before dispatch is retried after the throttle.OnchainConnectionMonitorTests.swift— a short background stay never waits; a long one waits until a sync succeeds; an unfinished wait resumes after a short background stay.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.scripts/electrum-bridge.py— the bridge used for the manual tests and recordings.0.7.0-rc.76: the server accepted589e9c20…while its replies were dropped, the next sync recognized it from the mempool, no resend was built, and the balance dropped once (39,285 → 37,142).f7d56c6d…sent, 45 s in the background on Bitcoin Sent, back in front with no connecting screen, toast or sheet dismissal, and oneSyncing LDK…/LDK syncedpair after the resume.mayStillConfirm), attempt kept; a candidate that never reached the network was released about 4 s after the tap, and the nextbitcoin:paymentef107777…was broadcast.0.7.0-rc.76: dropped with a fee estimate queued, reconnected 1 ms after the failed sync, connecting screen for 19 s, payment1f1f68d9…sent.