Repository navigation
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. |
![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: 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
talosmachina
left a comment
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
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
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
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
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.
4 participants
Closes #986
Twin: synonymdev/bitkit-android#1559
Refs:
Description
Out of Scope
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; 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.