Repository navigation
fix: let an expired payment that never reached the network be cancelled - #998
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aa5ea75ad
ℹ️ 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".
[Critical impact] The change reaches wallet funds through a new on-chain release path. Fix the cancellation amount before merging so small payments made at high fee rates can actually be released. FindingsSummaryAdds cancellation for unresolved Paykit on-chain payments whose requests have ended.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Cancel expired payment] --> B{Request ended?}
B -->|No| C[Keep payment pending]
B -->|Yes| D{Saved release found in wallet?}
D -->|Yes| J[Remove unfinished proof and send block]
D -->|No| E[Request wallet sync]
E --> F{Original candidates absent?}
F -->|No| C
F -->|Yes| G[Prepare payment back to wallet]
G --> H[Approve fee and authenticate if required]
H --> I[Save release transaction ID and broadcast]
I -->|Accepted| J
I -->|Rejected or unknown| C
Reviews (1) · Last reviewed commit: "fix: restore a pending payment from its ..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Adds "Cancel expired payment" to Payment Pending: once a Paykit request has ended and a fresh sync shows none of the payment's transactions in the wallet, the original's coins are spent back to the wallet at a fee above every candidate's, and the send block and started proof are cleared only once that release is accepted or seen. Reviewed 0037ba9c, full tier, reasoned from the code and CI; iOS does not build on this reviewer's Linux host.
What I checked, and 3 candidates I ruled out
Read in full: OnchainSendAttemptService.swift (release, sync wait, restoreBackup, approveRelease/recordRelease/abandonUnbroadcast), the new PaykitPaymentProofService paths, SendPendingScreen.swift; the incremental 0aa5ea75..0037ba9c diff.
Cross-repo: ldk-node 0.7.0-rc.74 (190a7d9): PreparedOnchainSend::broadcast, prepare_send (validate_signed_inputs enforces the exact utxosToSpend set), get_spendable_utxos / BDK list_unspent, get_transaction_details.
CI: validate green; Run Tests, Run Integration Tests, build-local still pending at review time.
Ruled out
- Release could leave an original coin unspent and let a candidate confirm: ldk-node rejects a signed tx whose inputs differ from
utxosToSpend, so the release spends every still-spendable original coin; a coin already spent elsewhere means a candidate is not the one confirming. - A restore wiping local release state or failing on the backup's follow-up stamp:
restoreBackupnow nilsreleaseApproved,releaseTxid,restoredFromBackupandbackupCreatedAtMillison both sides before comparing; covered byOnchainSendAttemptServiceTests. - Second cancel after an
.unknownrelease: if the first sync of the second attempt is what discovers the release,prepareReleasefinds no spendable original coin and shows the generic retry-unavailable toast; a further tap finishes it. Narrow trigger, no funds at risk, misleading copy only, so not posted. A cold-reader pass on it did not return.
Merge confidence: 4/5, the real LightningService release and sync-wait paths are exercised only through ReleaseWalletMock, and the test jobs were still running.
|
Device check on an iPhone sim (regtest staging): a wallet stuck on an expired, never-broadcast Shop payment recovers with "Cancel expired payment", including after a wipe and a restore from backup. Cancel the stuck payment, then send ✅On build 0aa5ea7, the stuck payment
Restore while stuck, then cancel the restored payment (f0dabe8) ✅The Shop payment
|
|
Why this PR is in 3.0.0 RC3 ldk-node rc.76 (#990) fixes the dropped-connection bug, but a broadcast the server never answers still ends as "outcome unknown" after about 24 s (test A, swallowed-request case: #990 (comment)). If that payment's Shop or Paykit request then expires, every later on-chain send opens its Payment Pending screen, and nothing in the app clears it. Test B on RC2 showed that a wipe and restore doesn't help: the restore brings the stuck attempt back from the backup, and the next send is blocked again. Reproduced twice. Recording: lock survives wipe and restorerc3-b-ios-rc2-wipe-restore-lock-returns-3x.mp4So RC3 takes only this narrow escape: "Cancel expired payment" for a payment the network never saw, done as a release transaction that makes the original unconfirmable. The broader recovery work (#971, synonymdev/bitkit-android#1553) stays closed. This PR's test B re-run passes, including after a wipe and restore (see the results comment). |
jvsena42
left a comment
There was a problem hiding this comment.
No HIGH or MEDIUM findings. Two LOWs are inline, each with a failing test.
I reviewed this as a funds change: it adds a new on-chain spend (a release transaction with fee approval and PIN), changes the backup-restore comparison in restoreBackup, and changes the persisted attempt schema.
Checked and clean:
- "Never reached the network" is not trusted on its own: the check needs a fresh sync, no candidate in the wallet graph and every input's parent known (:90-119), and the release then spends every still-spendable original coin at the highest candidate rate + 2 sat/vB, doubling until it outbids. A timeout, an app kill mid-broadcast or an Electrum disconnect all leave the candidate recorded before broadcast.
- A release that lost to a confirmed candidate does not finish the cancel at :609:
getTransactionDetailsgoes through BDKget_tx, which only searches canonical transactions in bdk_wallet 2.4.0. originalCoinSpentElsewherecannot be fooled by channel-funding change: ldk-node refuses funding-tx outpoints when the original send is built, and manual coin selection draws fromlistSpendableOutputs.- Write order:
approveRelease, thenrecordReleasesaved before broadcast, then broadcast, all actor methods with equality guards. - The fee prompt is followed by the same PIN or biometric gate as Retry (
SendPendingScreen:392-396), and Retry and Cancel disable each other. requireRecoveryPayerruns before the latch. Hardware wallets, transfers and ShoporderIdattempts are excluded. The new fields are Optional, so older stored attempts decode. Logs carry txids only.- iOS does not roll back the consumed private payment-list version on cancel; Android does. That is a parity difference noted on synonymdev/bitkit-android#1559, not a finding here.
CI is green at this head.
perf: relevant (a user-facing wait of up to 30 s for the sync on Continue, a paymentRequests() poll every 30 s while Payment Pending is open). To be measured in the device gate.
Device gate (partial): f0f5afc — not run yet. Pending: the cancel and refused cases from the manual steps through the Electrum bridge (blackhole and lose-reply), timing Continue to the fee prompt against master, the poll rate while Payment Pending stays open, and wallet__onchain_abandon_note / wallet__onchain_abandon_checking in long locales on an iPhone SE. The device result will follow.
|
Device gate (partial):
Environment note: the payee's first Contact › Send, about a minute after the contacts were saved, fell through to a direct payment with Still running: the cancel case after expiry, the refused case (if time allows), Continue → fee prompt timings, poll rates, and the new strings. |
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 1dd20a61. No earlier assessment baseline was available.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
An expired payment whose original coins cannot fund a 1,000 sat release still leaves every new on-chain send blocked, the failure reported in #986. The resolved thread already says that remainder stays blocked after the switch away from returning half the inputs. The release still cannot add the wallet's other coins, so a larger balance stays blocked with the small coin.
Non-blocking consistency suggestions. iOS does not return a consumed private payment-list version when this cancel deletes the node-wallet proof. Android at f22c596 releases that version. iOS releases it for a receipt that never reached dispatch, and a node-wallet start does not store the version on the proof, so Shop cancels are unaffected.
Validation: Run Tests, Run Integration Tests, and build-local passed on this head. Those unit tests were inspected and were not re-run here. ReleaseWalletMock returns a caller-chosen amount, so the passing suite does not cover an input set below 1,000 sats. ldk-node 190a7d98 (0.7.0-rc.74 in Package.resolved) was read for preparation and transaction lookup. Device testing was not performed. The release amount and abandon order were compared with bitkit-android#1559 at f22c596; that was not a full review of the Android PR.
Suggested additional test cases
- iOS, regtest. Fund a wallet with a larger coin and one coin that exactly matches a Paykit or Shop on-chain payment of 1,000 sats or less plus its fee, with no change. Swallow the Electrum broadcast, wait until the request expires, then open Payment Pending. Tap Cancel expired payment, Continue, and approve the fee. The original coins should return to the wallet, and the next on-chain send should be admitted. The larger coin should become spendable again when the small coin cannot fund the release.
Device testing: not performed in this review.
Findings
- [HIGH] Cancel stays blocked when the original coins cannot fund 1,000 sats — inline at
Bitkit/Services/OnchainSendAttemptService.swift:105.
|
Device gate (partial):
Still running: the refused case and the timings. |
jvsena42
left a comment
There was a problem hiding this comment.
Delta 1dd20a6..370aabd (extra wallet coins when the original coins cannot fund the release): no HIGH or MEDIUM findings, and one LOW inline. Every set starts with all original coins, and utxos.prefix(originalCount) refuses a release that drops one. releaseOutbids is unchanged, the rate restarts per set, and the fee prompt still shows the whole cost.
The device gate at 1dd20a6 is still running: the cancel case passed (partial comment above), and the refused case and the timings are in progress. That run does not exercise the new extra-coin path, which needs a payment of 1,000 sats or less.
jvsena42
left a comment
There was a problem hiding this comment.
Device gate result at 1dd20a6. The head has since moved to 370aabd (the extra-coin delta, with its LOW open above), so this is not an approval yet. One LOW from the device is inline.
Device gate: 1dd20a6 — Debug build on iPhone 17 (payer) and iPhone 17 Pro (payee) simulators, iOS 26.5, linked as Paykit contacts, staging Electrum through the bridge.
- Cancel: passed. Details in the partial comment above. The button appeared 3 s after expiry on an already-open Payment Pending. Two fee prompts were dismissed (422 sats), and the third was approved and showed "Payment cancelled. Its coins are back in your wallet." The release
570fe66f…confirmed, the original3800c63e…never reached the network, the next payment went out, and the payee never received the original. ⚠️ Refused: not reached. The original reached the mempool with its reply dropped. After expiry the button appeared, but LDK saw the original 16 s after the bridge was unblocked and the screen switched to "Bitcoin Sent" before Continue could be tapped. No release was built, the original confirmed normally, and the payee was paid. A re-run needs another request and an hour; tap Continue while the bridge is still dark, then unblock inside the 30 s wait. The refusal was observed on Android today.- perf: Continue → fee prompt took 5.18 s, 3.96 s and 4.16 s, of which 2.5–2.7 s from the check start to the release being built. The 30 s limit was never approached. Master has no cancel, so there is no baseline. Idle Home: 59.0 Paykit SDK operations/min on the PR and 60.0 on the merge-base build, with 6 on-chain syncs/min on both. Payment Pending with the 30 s re-check: 42.7–45.2/min. The explicit sync runs only on Continue.
- l10n: 8 new keys, English only, so every locale shows English. The English layout already truncates on iPhone 17 (the inline LOW).
⚠️ The iPhone 17e was not checked. - The extra-coin path in
370aabd(payments of 1,000 sats or less) was not exercised by this run.
As on Android, each Continue derives a new wallet address for the release, so dismissed fee prompts leave unused addresses behind. This is an observation, not a finding.
Payment Pending also keeps its text whole when the cancel button shows; the hourglass shrinks instead.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up of caeae98fd4ccb5b566527e950325486babb4bac2 on bitkit-ios#998, covering b0b17f9d and caeae98f since the previous reviewed head 370aabde2ce28c63a16166df958202a277dca7eb. Inherited baseline bc-dcd36580-5121-4c0c-afef-6a3531ea7abd; base and merge base remain bd099662819d59c95736c403017a42e9c67806d8. This pass inspected confirmed-only extra coins, one-coin growth, and the Payment Pending text layout. Fee approval and broadcast handling for an extra-coin release, the cancel sync and restore path, Paykit request-ended checks, the earlier unit tests aside from the set-order test, and Android's 1,000 sat amount and abandon order stay inherited.
No new actionable code findings.
A release still starts from every still-spendable original coin, including when that coin is unconfirmed. Other coins join only when listPayments reports their creating transaction as an on-chain payment with ConfirmationStatus.confirmed, largest first, one coin per set, stopping at the first set that can pay 1,000 sats and outbid. At ldk-node 0.7.0-rc.74 (190a7d98), that status is set when the wallet has a confirmation anchor. The six-confirmation payment-succeeded gate is separate from this filter. listSpendableOutputs still returns unconfirmed coins, and the new filter drops them before they can be added. Sync updates the payment store before it returns, and cancel waits for that sync before building the release. That matches the unconfirmed-extra case in the prior comment.
testReleaseAddsWalletCoinsWhenTheOriginalCoinsFallShort now expects the unconfirmed 500,000 sat coin to stay out and the confirmed extras to grow 90,000, then 20,000, then 5,000. It exercises the set builder only.
The Android comparison was limited to prepareRelease on bitkit-android#1559 at f22c59644ccbdb341939d80277ed669b1827d099. That function still spends only the original coins, so funding a short release from other wallet coins remains iOS-only.
The Payment Pending description, transaction id, and checking line use fixedSize so they keep their height, and the hourglass can shrink. That matches the layout change on the truncation comment. HourglassLoadingView is also used by the USDT send screen. Pixel layout was not rendered in this review.
Validation: Run Tests and Run Integration Tests succeeded on caeae98fd4ccb5b566527e950325486babb4bac2. The updated set-order test was inspected and was not run locally.
Device testing: not performed in this review.
Ready for device testing
Suggested additional test cases
- Platform: iOS. Setup: a Paykit or Shop on-chain payment of 1,000 sats or less whose broadcast never reached the network, plus a larger confirmed wallet coin. Action: open Payment Pending, tap Cancel expired payment, Continue, and approve the fee. Expected: the release spends the original coins and that confirmed coin, the change returns to the wallet, and the next on-chain send can be sent.
- Platform: iOS. Setup: the same blocked payment, where the only coin large enough to fund the release is unconfirmed. Action: tap Cancel expired payment and Continue. Expected: the unconfirmed coin stays unspent, the send block remains, and the payment stays pending.
- Platform: iOS, iPhone 17 and a shorter phone. Setup: Payment Pending with Cancel expired payment visible. Action: tap Continue so the checking line is showing. Expected: the description, transaction id, and checking line are fully readable, and Cancel and Close can be tapped.
jvsena42
left a comment
There was a problem hiding this comment.
Delta 370aabd..caeae98: no findings.
b0b17f9adds only confirmed wallet coins as extras (confirmedTxidsfromlistPayments()), with the original coins unfiltered. My LOW thread is fixed and resolved.caeae98adds coins one at a time, largest first, matching synonymdev/bitkit-android#1562. Theutxos.prefix(originalCount)subset guard still throws rather than falling through.SendPendingScreen: the description, the Transaction ID and the checking line now keep their height (fixedSize(horizontal: false, vertical: true)), and the hourglass shrinks withmaxHeight: sizeand layout priority. My truncation LOW is fixed in code. I will confirm it on device below.
CI is green at this head.
Device gate (partial): caeae98 — not run yet. Pending: the extra-coin release (a 1,000-sat payment from one ~1,200-sat coin, a confirmed 50k coin and an unconfirmed 90k coin; the release should spend the original plus the confirmed coin only), Payment Pending with the cancel button and the checking line on the iPhone 17e with no truncation, and Continue → fee prompt timing. The refused case not reached yesterday is not repeated: the refusal path is unchanged since 1dd20a6, and it was observed on Android. The device result will follow.
|
Device gate (partial):
|
jvsena42
left a comment
There was a problem hiding this comment.
Payment Pending compared against Figma and Android. Figma (Send (Edge cases) › Transaction pending Lightning) only has the Lightning state, so the on-chain-only parts (Retry, Cancel, the Transaction ID, the checking line) have no frame to check against. Where there is no frame, the comparison is with Android. Everything here is non-blocking: most of it predates this PR, and none of it changes behaviour.
| Element | Figma | Android | iOS (caeae98) |
Origin |
|---|---|---|---|---|
| Retry original payment | not in Figma | Secondary (outlined) | Primary (filled) | #844 — inline |
| Cancel expired payment | not in Figma | Secondary | Secondary | matches |
| Details / Close row | Secondary + Primary, 56, radius 64, gap 16 | same | same | matches |
| Description | Inter Tight Regular 17/22, Alpha/White 64 | BodyM, White64 | BodyMText, .textSecondary |
matches; the on-chain copy differs between platforms (see below) |
| Description → Transaction ID | not in Figma | 16 | 0 | #844 — inline |
| Transaction ID | not in Figma | BodyM 17 Regular, White64 | BodySSB 15 SemiBold, white | #844 — inline |
| "Checking the network…" | not in Figma | BodyM, White64 | BodyS | this PR |
| Hourglass | 256, centred between text and buttons | fixed 256 dp, 32 dp under the text | shrinks (maxHeight: size), centred by Spacers |
this PR (iOS) |
| Amount → description, side padding | 32, 16 | 32, 16 | 32, 16 | matches |
Not tied to a line:
- On-chain description copy. Android says "The transaction outcome is unresolved. Check its status before trying again. Other payments remain blocked. If available, use Retry original payment to authorize a fee for the same payment." iOS says "This transaction may have been sent. Its outcome is unknown. Do not start a separate payment." Figma only has the Lightning copy. One wording should be picked for both.
- The checking line is BodyS here and BodyM on Android (
SendPendingScreen.kt:452-456).BodyMTextwith.textSecondarywould match Android and the description. - The hourglass on iOS now gives way when both buttons show, and the text wraps in full on the iPhone 17e. Android's is fixed at 256 dp, and with Retry and Cancel both shown it is clipped behind the button stack on a Pixel 9. That is an Android-side defect from synonymdev/bitkit-android#1559 (merged), which I'll take up separately.
|
@jvsena42 Thanks for the comparison. 634e0d1 covers the two inline threads and moves the "Checking the network…" line to I left the on-chain description copy unchanged here. Both wordings predate this PR, and changing them means new strings and translations in an RC3 fix; aligning them fits a separate copy change on both platforms. |
|
Device gate (partial): The request expired at about 15:35:50, and "Cancel expired payment" showed on the open Payment Pending. The shared staging chain confirmed the 90,000-sat coin Z before the cancel, so I deposited a 150,000-sat coin Z2, which stayed unconfirmed through run 2. Run 2 (approved). Z2 was known to the wallet and unconfirmed. X alone fails, and the next set adds the largest confirmed coin (Z). The larger unconfirmed Z2 is skipped: The release decoded from Electrum has inputs Z ( Staging Electrum still reports the original as a missing transaction. The next payment was sent ( Still to post: the layout check of Payment Pending with the cancel button and the checking line on the iPhone 17e, and the timings. |
jvsena42
left a comment
There was a problem hiding this comment.
Delta caeae98..634e0d1 applies my Payment Pending comments: Retry is .secondary, and the Transaction ID and the checking line use BodyM, with 16 above the ID. Both threads are resolved. No findings. The description copy still differs from Android; that was a note in the body and does not block.
Device gate (partial): caeae98 on the iPhone 17e.
- Extra-coin release: passed (comment above).
- Layout with the cancel button and with the checking line: passed at
caeae98. The description wraps over 3 lines, the full txid over 2, and "Checking the network. This can take up to 30 seconds." over 2. Nothing is cut, and the hourglass gives up the space. The fee prompt and note are system alerts and wrap in full. - Continue → fee prompt: ~4.6 s and ~3.7 s. X alone fails in under 1 ms and X + one coin builds in 8–9 ms, so the extra set adds no visible time. Yesterday's single-set runs took 3.96–5.18 s.
Pending: the same layout at 634e0d1, since the Transaction ID and the checking line are now larger. The device result will follow.
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 634e0d1b.
Every LOW I raised on this PR is fixed and resolved: the restore outbid, the cut-short cancel, confirmed-only extra coins, the truncation, the Retry variant, and the Transaction ID style. The code scan of the latest delta is clean. Unit and integration tests are green at this head. e2e-tests-local is still pending, so check it before merge.
Device gate: 634e0d1 — the same flows ran at caeae98 on an iPhone 17e payer and an iPhone 17 Pro payee. Only SendPendingScreen.swift changed since, and its layout was re-checked at 634e0d1. Details in the partial comments above.
- Extra-coin release (
caeae98): passed. The original spent only a 1,200-sat coin. X alone failed for lack of funds, and the release spent X + the largest confirmed coin (90,000), skipping a larger unconfirmed 150,000 coin. Fee 626 sats, the release confirmed, the original never reached the network, the next payment was sent, and the payee never received the original. - Cancel (
1dd20a6, yesterday): passed. - Layout (
634e0d1, iPhone 17e): passed. A subscription was paid with the broadcast swallowed, then deleted by the payee, and the cancel button appeared about 27 s later. Retry and Cancel are both outlined, and Close is the only filled button. The description (3 lines), the Transaction ID with 16 above it (2 lines) and the checking line (2 lines) wrap in full with no "…". The hourglass shrinks across the Retry-only, cancel-offered and checking states, and nothing overlaps or goes off screen. - perf: Continue → fee prompt ~4.6 s and ~3.7 s. The extra coin set adds 8–9 ms, and the time is almost all the explicit sync.
⚠️ Refused case: not reached on iOS (yesterday the original resolved before Continue could be tapped). The refusal path has not changed since1dd20a6, and it was observed on synonymdev/bitkit-android#1559.- l10n: 8 new keys, English only.
Notes that do not block:
- The on-chain description copy still differs from Android.
- While the check runs, the disabled Retry and Cancel buttons lose their outlines and show only grey labels. It was the same at
caeae98. - Each Continue derives a new release address.


Closes #986
Twin: synonymdev/bitkit-android#1559
Refs:
Description
Out of Scope
SendPendingScreen: on-chain description copy differs from Android: bug: payment pending on-chain copy differs from android #1000 (review).Design
N/A — no design available; adds a secondary "Cancel expired payment" button and two confirmation dialogs to the existing Payment Pending screen.
Preview
Regtest staging, iPhone 17 Pro simulator (iOS 26.5), 2x. The blocked attempt was seeded by a local debug-only hook, not in this PR, because a real one needs a Shop request left to expire.
Cancel released, next payment sent
A Paykit payment with an unknown outcome, whose request has ended and whose transaction never reached the network, blocks a new
bitcoin:payment. Cancel asks for the network fee, sends the coins back to the wallet, and the next payment is sent.cancel-released2x.mp4
Cancel refused: transaction already known
The same blocked state, with a transaction the wallet already knows: the cancel is refused and the payment stays pending.
cancel-refused2x.mp4
QA Notes
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
Cancel a Shop payment whose request ended while its outcome is unknown → its coins return to the wallet and new on-chain sends work again — needs an Electrum server that swallows requests and a Shop request left to expire.
Steps
scripts/electrum-bridge.pyfrom fix: reconnect electrum after the server drops the connection bitkit-android#1532) and point Settings › Advanced › Electrum Server at it.blockchain.transaction.broadcast, swallow it, then swallow everything after; a plainblackholeset before the swipe fails the pre-send sync instead. Swipe and wait for the payment to show as pending.Released on-chain payment <txid> through <release txid>, and the next on-chain payment is sent. The Shop order stays unpaid.Automated Checks
PaykitPaymentProofServiceTests.swift— ended, canceled and rejected requests release a payment that never reached the network; an open request, a submitted proof or a known candidate keeps it; a rejected or unknown release keeps the guard and proof and a later cancel finishes once the release is in the wallet; the release must spend an original coin and outbid every candidate; the release adds confirmed wallet coins largest first, one at a time, and never an unconfirmed one; an approved release finishes after its proof was removed; subscription periods and expired quotes; only a sync after the request countsOnchainSendAttemptServiceTests.swift— ordinary and restored payments are never released; a restore keeps the local release state and restores a pending attempt from its own backup5705f068…accepted and the next paymentab23b7b0…sent; a known candidate refused withmayStillConfirm.