Skip to content

fix: let an expired payment that never reached the network be cancelled - #998

Merged
ovitrif merged 15 commits into
masterfrom
fix/cancel-expired-onchain-payment
Oct 18, 2026
Merged

ovitrif merged 15 commits into
masterfrom
fix/cancel-expired-onchain-payment

Conversation

@ovitrif

@ovitrif ovitrif commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #986
Twin: synonymdev/bitkit-android#1559
Refs:

Description

  • Adds "Cancel expired payment" to Payment Pending, the one way out of a Paykit or Shop on-chain payment whose outcome is unknown and whose request has ended. Today that payment blocks every new on-chain send for good, and a wipe and restore brings the block back, because the backup restores the attempt.
  • The cancel is offered only when the request was canceled, rejected or expired, or its payment deadline or quote passed, with no proof for it (for a subscription, no proof for that billing period). It re-checks every 30 s while the screen stays open.
  • On Continue, Bitkit asks for a wallet sync and waits, up to 30 s, for an on-chain sync that finishes after the tap. It goes on only if the wallet then knows none of the payment's transactions while it still knows the transactions its coins came from. A known candidate refuses the cancel.
  • The cancel then spends every spendable original coin back to the wallet (1,000 sats to a new wallet address, the rest as change), at a fee above every candidate's. When those coins cannot pay that, it adds the wallet's other confirmed coins one at a time, largest first, until they do; it never adds an unconfirmed coin, which nodes holding a candidate would refuse as a replacement. This happens after the user approves the network fee and, when payments need it, their PIN or biometrics. The original can therefore never confirm alongside a later payment. The release transaction is saved before it is broadcast. The send block and the Paykit proof stay until the release is accepted or a sync finds it in the wallet. A rejected or unknown release keeps both.
  • After a wipe and restore the payment can still be cancelled: the release transaction, not the restore, is what keeps it safe. If the fresh sync finds none of the payment's transactions but one of its coins already spent, an earlier release or another transaction made the original unconfirmable, so the cancel finishes without a new release, including when a restart or a restore lost the earlier release's ID.
  • The attempt is marked approved before anything changes, so an app stop mid-release finishes on the next cancel. A wallet restore keeps that local state, and a restore of a pending attempt no longer fails on the follow-up time the backup adds.

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
    1. Run an Electrum bridge in front of the staging server (scripts/electrum-bridge.py from fix: reconnect electrum after the server drops the connection bitkit-android#1532) and point Settings › Advanced › Electrum Server at it.
    2. Start a Pubky Shop checkout and continue to Swipe To Pay. The bridge must pass traffic until the first blockchain.transaction.broadcast, swallow it, then swallow everything after; a plain blackhole set before the swipe fails the pre-send sync instead. Swipe and wait for the payment to show as pending.
    3. Keep requests swallowed until the checkout ends or the request expires (about 30 min), then let traffic through again.
    4. Open any new on-chain payment and swipe: Payment Pending for the earlier payment shows "Cancel expired payment".
    5. Tap it, Continue, then Yes, Send on the network fee. Pass: "Payment cancelled. Its coins are back in your wallet.", the log shows Released on-chain payment <txid> through <release txid>, and the next on-chain payment is sent. The Shop order stays unpaid.
    6. Variant where the bridge forwards the broadcast but drops its reply, so the transaction reaches the mempool: the cancel is refused with "This payment may still reach the network, so it cannot be cancelled."

Automated Checks

  • added 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 counts
  • added OnchainSendAttemptServiceTests.swift — ordinary and restored payments are never released; a restore keeps the local release state and restores a pending attempt from its own backup
  • ran the release and refused cases on an iOS 26.5 simulator at 0aa5ea7 with a seeded attempt: release 5705f068… accepted and the next payment ab23b7b0… sent; a known candidate refused with mayStillConfirm.

@ovitrif ovitrif self-assigned this Oct 17, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-17T00:19:42.340176Z 0aa5ea7 PR opened
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread Bitkit/Services/OnchainSendAttemptService.swift Outdated
@greptile-apps

greptile-apps Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[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.

Findings

  1. P1 Small payments stay stuck ▶
  2. P2 Successful sync reports failure ▶

Summary

Adds cancellation for unresolved Paykit on-chain payments whose requests have ended.

  • Payment Pending offers cancellation only after a Paykit request has ended.
  • Bitkit sends the original coins back before clearing the pending send.

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
Loading

Reviews (1) · Last reviewed commit: "fix: restore a pending payment from its ..." · Reviewed by Greptile

Comment thread Bitkit/Services/OnchainSendAttemptService.swift Outdated
Comment thread Bitkit/Services/OnchainSendAttemptService.swift Outdated

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: restoreBackup now nils releaseApproved, releaseTxid, restoredFromBackup and backupCreatedAtMillis on both sides before comparing; covered by OnchainSendAttemptServiceTests.
  • Second cancel after an .unknown release: if the first sync of the second attempt is what discovers the release, prepareRelease finds 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.

@ovitrif

ovitrif commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

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 e37d0975… showed Payment Pending. Cancel, Continue, then the fee prompt (626 sats): the release ff701345… broadcast with Released on-chain payment e37d0975 through ff701345 after its request ended and confirmed at height 233120. e37d0975 was never broadcast. A new send a22071db went out and confirmed. After a wipe and a restore from backup, the next send went straight through.

rc3-b-ios-cancel-release-3x.mp4
rc3-b-ios-wipe-restore-3x.mp4
Restore while stuck, then cancel the restored payment (f0dabe8) ✅

The Shop payment 905c0a24 was stuck (order b7f71b83; its broadcast timed out). After a wipe and restore, the lock came back from the backup. The screen offered only Retry until the app saw the request expire, then offered Cancel. The release 4a766be2 broadcast with Released … after its request ended and confirmed at height 233363. 905c0a24 was never broadcast. A new send 43de333b went out and confirmed.

rc3-b2-ios-stick-2x.mp4
rc3-b2-ios-wipe-restore-cancel-4x.mp4

@ovitrif

ovitrif commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

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 restore
rc3-b-ios-rc2-wipe-restore-lock-returns-3x.mp4

So 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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: getTransactionDetails goes through BDK get_tx, which only searches canonical transactions in bdk_wallet 2.4.0.
  • originalCoinSpentElsewhere cannot be fooled by channel-funding change: ldk-node refuses funding-tx outpoints when the original send is built, and manual coin selection draws from listSpendableOutputs.
  • Write order: approveRelease, then recordRelease saved 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.
  • requireRecoveryPayer runs before the latch. Hardware wallets, transfers and Shop orderId attempts 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.

Comment thread Bitkit/Services/OnchainSendAttemptService.swift Outdated
Comment thread Bitkit/Services/PaykitPaymentProofService.swift
@ovitrif
ovitrif requested a review from jvsena42 October 17, 2026 14:08
@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): 1dd20a6. Setup is done and Payment Pending is reached. The cancel case runs after the request expires at about 14:23 local.

  • Debug build at 1dd20a6, installed on an iPhone 17 (payer) and an iPhone 17 Pro (payee), iOS 26.5, staging Electrum. Fresh wallets and Pubky profiles, linked as contacts. The payer has 600,000 sats, and its Electrum runs through a local bridge on 127.0.0.1:50098.
  • The payee sent a one-time 50,000-sat request (Savings only, 1 hour, the shortest expiry offered) at 13:22:53. The payer swiped at 13:24:43. The pre-send sync passed, then the broadcast was swallowed:
    [2026-10-17 16:25:00.475 UTC] INFOℹ️: Created new transaction 3800c63e1a9e1e0a47a605b689160fe97b496f64ed0334f21e083bd181a7514e sending 50000sats on-chain to address bcrt1qjkwd6lal3asuzrffxlw3xsx6q5spptk6kps8r5 - [LDK] [ldk_node::wallet:1962]
    [2026-10-17 16:25:32.061 UTC] ERROR❌: Failed to broadcast transaction due to timeout 3800c63e…514e: deadline has elapsed - [LDK] [ldk_node::chain::electrum:943]
    [2026-10-17 16:25:32.071 UTC] WARN⚠️: On-chain broadcast outcome unknown for 3800c63e…514e - SendConfirmation [SendConfirmationView.swift: performPayment(isAutomatic:) line: 1348]
    
  • Payment Pending shows "This transaction may have been sent. Its outcome is unknown." with Retry original payment, Details and Close. There is no cancel button yet, which is correct while the request is open. Staging Electrum reports 3800c63e… as a missing transaction. Broadcasts stay swallowed and all other traffic passes.

Environment note: the payee's first Contact › Send, about a minute after the contacts were saved, fell through to a direct payment with recovery_required (PrivatePaykitService+Payments.swift:186). After restarting both apps, the Request or Pay sheet appeared.

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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Bitkit/Services/OnchainSendAttemptService.swift Outdated
@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): 1dd20a6. The cancel case passes. The head has since moved to 370aabd (extra wallet coins when the original coins cannot fund the release). This run exercises the path where the original coins fund it on their own.

  • A new bitcoin: payment opened Payment Pending for 3800c63e…514e with Retry and no cancel button while the request was open. At 14:22:56, 3 s after the request expired, "Cancel expired payment" appeared on the open screen without any interaction, from the 30 s re-check.
  • Cancel → note → Continue → "Checking the network…" → "Send this payment's coins back to your wallet? Network fee: 422 sats." Two runs were dismissed with Cancel, and the button came back each time. The third was approved and showed "Payment cancelled. Its coins are back in your wallet."
    [2026-10-17 17:25:07.322 UTC] DEBUG: Syncing LDK... [LightningService.swift: sync() line: 493]
    [2026-10-17 17:25:09.682 UTC] INFOℹ️: LDK synced [LightningService.swift: sync() line: 498]
    [2026-10-17 17:25:09.910 UTC] INFOℹ️: Created new transaction 570fe66f1289acec1900679bf9234f81f463a8435af4a80a372aec8ab5e6b16f sending 1000sats on-chain to address bcrt1qwxn49hpjqvzz0dmh3a9m7rwnfd4qewr0rt642u - [LDK] [ldk_node::wallet:1962]
    [2026-10-17 17:25:12.749 UTC] DEBUG: Successfully broadcast transaction 570fe66f…b16f - [LDK] [ldk_node::chain::electrum:902]
    [2026-10-17 17:25:12.755 UTC] INFOℹ️: Released on-chain payment 3800c63e1a9e1e0a47a605b689160fe97b496f64ed0334f21e083bd181a7514e through 570fe66f1289acec1900679bf9234f81f463a8435af4a80a372aec8ab5e6b16f after its request ended - PaykitPaymentProof
    
  • After mining a block, staging Electrum has the release and reports the original as a missing transaction. The next on-chain payment was dispatched with no block.
  • As on Android, each Continue derives a new wallet address for the release, so dismissed fee prompts leave unused addresses behind.

Still running: the refused case and the timings.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Bitkit/Services/OnchainSendAttemptService.swift Outdated

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 original 3800c63e… 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.

Comment thread Bitkit/Views/Wallets/Send/SendPendingScreen.swift
Payment Pending also keeps its text whole when the cancel button shows; the hourglass shrinks instead.
@ovitrif
ovitrif requested a review from jvsena42 October 17, 2026 17:42

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta 370aabd..caeae98: no findings.

  • b0b17f9 adds only confirmed wallet coins as extras (confirmedTxids from listPayments()), with the original coins unfiltered. My LOW thread is fixed and resolved.
  • caeae98 adds coins one at a time, largest first, matching synonymdev/bitkit-android#1562. The utxos.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 with maxHeight: size and 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.

@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): caeae98. Setup is done and Payment Pending is reached. The cancel runs after the request expires at about 15:36 local.

  • Debug build on an iPhone 17e (payer, 390 pt wide) and an iPhone 17 Pro (payee). The payer's only coin is X, 1,200 sats (acd1ce75…f17b:0). The payee sent a one-time 1,000-sat request (Savings, 1 h), and the payer's broadcast was swallowed:
    [2026-10-18 17:41:55.991 UTC] INFOℹ️: Created new transaction 79874f15c4d155f160426cd285b601651a5ac0e08e42b5e1a5759a090fb64a79 sending 1000sats on-chain to address bcrt1qtt0z356aaem447s0vcyfqty2ghzertfz6umfuu - [LDK] [ldk_node::wallet:1962]
    [2026-10-18 17:42:15.424 UTC] ERROR❌: Failed to broadcast transaction due to timeout 79874f15…4a79: deadline has elapsed - [LDK] [ldk_node::chain::electrum:943]
    
    Decoded, the original spends X alone with no change (200-sat fee). Staging Electrum reports it as a missing transaction.
  • Payment Pending without the cancel button: the Transaction ID wraps in full over two lines on the iPhone 17e.
  • A confirmed 50,000-sat coin Y is in the wallet. An unconfirmed 90,000-sat coin Z goes in just before expiry. Expected: the release spends X and Y, and not Z, and nothing truncates once the button shows.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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). BodyMText with .textSecondary would 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.
With the cancel button: Android (hourglass clipped), iOS at 1dd20a6 before the truncation fix

Comment thread Bitkit/Views/Wallets/Send/SendPendingScreen.swift Outdated
Comment thread Bitkit/Views/Wallets/Send/SendPendingScreen.swift Outdated
@ovitrif

ovitrif commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator Author

@jvsena42 Thanks for the comparison. 634e0d1 covers the two inline threads and moves the "Checking the network…" line to BodyMText, as on Android.

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.

@ovitrif

ovitrif commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator Author

@jvsena42 Out of scope for this release: the on-chain description copy is tracked in #1000 for 3.0.5, because both wordings predate this PR and changing them needs new strings and translations.

@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): caeae98. The extra-coin release passes on the iPhone 17e.

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:

[18:44:05.243 UTC] Using 1 manually selected UTXOs with total value: 1200sats
[18:44:05.243 UTC] ERROR❌: Failed to create transaction: Insufficient funds: 0.00001200 BTC available of 0.00001329 BTC needed
[18:44:05.244 UTC] Using 2 manually selected UTXOs with total value: 91200sats
[18:44:05.252 UTC] Created new transaction c16e892c303707fca6b2000cdfec5840ae8598db1460536c5ac16d2f3a2b3d84 sending 1000sats on-chain
[18:44:34.205 UTC] INFOℹ️: Released on-chain payment 79874f15…4a79 through c16e892c…3d84 after its request ended - PaykitPaymentProof
[18:45:08.795 UTC] Onchain transaction bc0c8724…bf08 confirmed at height 235036
[18:45:09.057 UTC] Onchain transaction c16e892c…3d84 confirmed at height 235036

The release decoded from Electrum has inputs Z (559c53f5…dcdc:0, 90,000) and X (acd1ce75…f17b:0, 1,200), outputs 89,574 change + 1,000, and a 626-sat fee. Not spent: Z2 (150,000, largest but unconfirmed until the block that confirmed the release) and Y (50,000). Run 1 (dismissed, Z2 not yet seen by the wallet) picked the same set with the same 626-sat fee. The success toast showed.

Staging Electrum still reports the original as a missing transaction. The next payment was sent (eebe99bf…0b96). The payee never received the original 1,000.

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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 since 1dd20a6, 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.

@ovitrif
ovitrif enabled auto-merge October 18, 2026 19:25
@ovitrif
ovitrif merged commit 7b3f093 into master Oct 18, 2026
18 checks passed
@ovitrif
ovitrif deleted the fix/cancel-expired-onchain-payment branch October 18, 2026 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: expired unknown-outcome on-chain payment blocks all new on-chain sends

4 participants