[ovitrif]

Twin: synonymdev/bitkit-ios#998
Refs: #1559

Description

• "Cancel expired payment" now funds its release with other wallet coins when the original payment's coins cannot cover the 1,000 sat output and a fee above every candidate's. Before, a small Paykit or Shop on-chain payment that used one exact coin could not be cancelled, so every later on-chain send stayed blocked.
• The release tries the original's coins alone, then adds the wallet's other confirmed coins one at a time, largest first, and stops at the first set that pays. Unconfirmed coins are never added, because nodes holding a candidate reject a replacement with a new unconfirmed input (BIP125 rule 2). Each set holds every spendable original coin, and the release is refused unless it spends all of them, so no candidate can confirm. The change returns to the wallet; fee approval and PIN are unchanged.

Out of Scope

• The 1,000 sat release output: unchanged, since adding wallet coins removes the shortfall.

Design

N/A — no UI changes.

Preview

N/A

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

• Regtest: fund a wallet with a larger confirmed coin and one coin that exactly matches a Paykit or Shop on-chain payment of 1,000 sats or less plus its fee. Swallow the Electrum broadcast, let the request expire, open Payment Pending, tap Cancel expired payment, Continue, and approve the fee → the release spends the small coin with the larger one, and the next on-chain send is admitted — needs a swallowed Electrum broadcast, not in Capabilities

Unit: release adds wallet coins when the original coins fall short in OnchainSendAttemptStoreTest; release never adds an unconfirmed wallet coin, release adds confirmed wallet coins largest first until they fund it and release falls back to every confirmed wallet coin in LightningServiceTest.

[chatgpt-codex-connector]

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-17T17:03:39.782023Z	e9ed7c6	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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9ed7c68f2

ℹ️ 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".

[greptile-apps]

Confidence Score: 4/5

[Medium impact] The change reaches a narrow wallet cancellation and on-chain send workflow.

The PR appears safe to merge, with a non-blocking gap in tests for the release fallback.

Findings

1.  Release fallback remains untested ▶

Summary

This PR lets cancellation use other wallet coins when an expired payment's original coins cannot fund its release.

• Expired payment releases can use other wallet coins when needed.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Original coins] --> B{Preparation succeeds and fee outbids?}
    B -->|Yes| G[Return release for approval]
    B -->|No| C[Add largest other coin]
    C --> D{Preparation succeeds and fee outbids?}
    D -->|Yes| G
    D -->|No| E[Add all other spendable coins]
    E --> F{Preparation succeeds and fee outbids?}
    F -->|Yes| G
    F -->|No| H[Return error]

Loading

_{Reviews (1) · Last reviewed commit: "fix: add wallet coins to a release the o..." · Reviewed by Greptile}

[github-actions]

Regtest APK

Built from f497240 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

[talosmachina]

No findings. When the original payment's coins cannot pay the 1,000 sat output plus a fee above every candidate's, prepareRelease now retries with the wallet's largest other coin, then with every other spendable coin. Every set still has to spend each spendable original coin. Reviewed e9ed7c6, full tier (the change builds a transaction).

What I checked, and 5 candidates I ruled out

Read in full: prepareRelease, prepareOnchainSend and callOnchainSend in LightningService.kt; releaseOutbids, releaseInputSets, approveRelease/recordRelease in OnchainSendAttemptStore.kt; the caller abandonUnbroadcastOnchainPayment in PaykitPaymentProofRepo.kt and releaseExpired in SendPendingViewModel.kt; ldk-node v0.7.0-rc.76 prepare_send, validate_signed_inputs, build_transaction_psbt and get_spendable_utxos.
CI: CI / build green on this head, including testDevDebugUnitTest; lint and detekt green; the e2e-tests-local shards were still pending when I posted. Not built here.

Ruled out

• Wallet coins changing which coins the release spends: utxosToSpend is manually_selected_only in ldk-node, and validate_signed_inputs refuses any transaction whose inputs are not exactly the selected set. The new containsAll(originalCoins) check is stricter than the old any.
• Replacement rules once the release is larger: the fee rate starts at the highest candidate rate + 2 sat/vB, and releaseOutbids still requires an absolute fee above every candidate's. A larger release therefore clears both the absolute-fee rule and the incremental-relay rule.
• A swallowed error hiding a real failure: an error on one set falls through to the next. When every set fails, the last error is thrown, and releaseExpired maps it to MayStillConfirm exactly as before. The check and CancellationException still propagate (runSuspendCatching).
• Fee doubling carrying over between sets: feeRate resets at the start of each set, so a larger set starts at the lowest fee that can outbid.
• An unapproved fee: whichever set wins, its receipt goes through the unchanged Release fee dialog and the PIN before recordOnchainRelease and the broadcast.

Merge confidence: 4/5. This path builds a transaction, and no test drives the new fall-through in prepareRelease (only releaseInputSets is unit-tested).

[jvsena42]

No HIGH or MEDIUM findings. One LOW is inline, with a failing test.

I reviewed this as a funds change: the release now spends wallet coins beyond the payment's own.

Checked and clean:

• No original coin is dropped. prepareSendToAddress with utxosToSpend sets manually_selected_only, validate_signed_inputs refuses any other input set, and the check(containsAll(originalCoins)) at LightningService.kt:1095 sits outside runSuspendCatching, so a violation refuses the cancel instead of falling through to the next set.
• releaseOutbids is unchanged, and candidateMiningFees survives a restore.
• BIP125 rules 3 and 4: the rate restarts at the highest candidate rate + 2 for each set, and a larger release only widens the absolute-fee margin. Rule 2 is the LOW.
• The fee dialog still shows the whole cost: the 1,000-sat output and the change both return to the wallet.
• originalCoinSpentElsewhere looks only at original coins, so extras cannot fool it. approveRelease → recordOnchainRelease before broadcast is unchanged.
• Already raised and not repeated: Codex on cumulative subsets and dust, Greptile on a service-level fallback test, detekt on the unused feeAttempt.

perf: not relevant (up to 3 sets × 4 prepares behind the existing sync wait).

Device gate (partial): e9ed7c6 — not run yet. Pending: piotr's case, a payment of 1,000 sats or less whose broadcast is swallowed and whose request expires, then Cancel, which should release with the original coin plus a confirmed larger coin. My loop for today ends here, so this runs on the next pass. The cancel and refused cases of #1559 passed on device today at f22c596.

[piotr-iohk]

QA review

Scope: Full review of the complete pull request diff against merge base bf64c29, at f497240.

No new actionable code findings.

When an expired Paykit or Shop on-chain payment's own coins cannot pay the 1,000 sat release output plus a fee above every candidate, prepareRelease adds other confirmed wallet coins one at a time, largest first, and stops at the first set that pays. Each set still contains every spendable original coin. ldk-node v0.7.0-rc.76 signs exactly the selected set. Coins whose creating transaction is not ConfirmationStatus.Confirmed in listPayments() stay out, so the replacement does not add an unconfirmed input. The existing fee dialog shows that prepared mining fee, and the release is recorded only after approval and the PIN.

Release coin selection matches bitkit-ios#998 at caeae98. That comparison covers this flow.

CI build on this revision passed, including testDevDebugUnitTest. detekt passed. This review inspected the new unit tests in source. The current source includes the cumulative largest-first sets, the confirmed-coin filter, and the service tests requested on earlier threads. The pull request description still says the release tries the largest extra coin and then every other spendable coin; this revision adds confirmed coins one at a time. Mine the extra coin before the manual test.

Device testing: not performed in this review.

Ready for device testing.

Suggested additional test cases

• Android, regtest, Electrum broadcast swallowed. Fund one confirmed coin that exactly matches a Paykit or Shop on-chain payment of 1,000 sats or less plus its fee, and a larger coin with zero confirmations. Let the request expire, open Payment Pending, tap Cancel expired payment, then Continue. Expected: the release does not spend the unconfirmed coin. With no other confirmed coin large enough, cancellation is refused and the next on-chain send stays blocked.

[jvsena42]

Delta e9ed7c6..f497240: no findings.

• fac1464 adds an extra coin only when listPayments() reports its transaction as Confirmed. The original's own coins stay unfiltered, which BIP125 rule 2 allows. My LOW thread is fixed and resolved. release never adds an unconfirmed wallet coin in LightningServiceTest covers it.
• f497240 grows the sets one coin at a time, largest first, which answers the dust concern on the Codex thread. Every set still starts with all original coins, and check(containsAll(originalCoins)) sits outside the catch, so a set that drops one refuses the cancel instead of falling through. A set that cannot fund the release fails on its first prepare and breaks to the next set, so a persistent error costs one prepare per coin rather than four.

CI is green at this head, unit tests and all 7 local e2e suites included.

Device gate (partial): f497240 — not run yet. Pending: a 1,000-sat payment that spends one ~1,200-sat coin with its broadcast swallowed, then a request expiry, a confirmed 50k coin and an unconfirmed 90k coin, then Cancel. The release should spend the original coin plus the confirmed coin, and not the unconfirmed one. Continue → fee dialog timing too. The device result will follow.

[jvsena42]

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

• CI APK on a Pixel 9 emulator: fresh wallet whose only coin is X, 1,200 sats (02c56b7c…dc5f:0). The payee is an iOS simulator on fix: let an expired payment that never reached the network be cancelled bitkit-ios#998 at caeae98, linked as a contact. Electrum runs through a local bridge.
• The payee sent a one-time 1,000-sat request (Savings, 1 h). The payer's broadcast of the original was swallowed:

  14:39:46.279 INFO  [ldk_node::wallet:1962] Created new transaction 226426e7d402e6f963c28dc4ad80c22768651b2b292921d1e76cf8fba4b431b0 sending 1000sats on-chain to address bcrt1qep7z370xrknmrk89z9dh897juggmv69ta6rpdu
  14:40:02.042 ERROR [ldk_node::chain::electrum:1078] Failed to broadcast transaction due to timeout 226426e7…31b0: deadline has elapsed
  

  Decoded, the original spends X alone: one input, one 1,000-sat output, 200-sat fee, no change. X therefore cannot fund the release by itself. Staging Electrum reports it as a missing transaction.
• Payment Pending shows Retry, with no cancel button while the request is open.
• 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.

[jvsena42]

Device gate (partial): f497240. The extra-coin release passes on the Pixel 9 emulator.

The request expired at about 15:34:50, and "Cancel expired payment" appeared on the open Payment Pending at 15:35:04. The shared staging chain kept mining, so the 90,000-sat coin Z confirmed before the cancel. To keep an unconfirmed largest coin in play, I deposited 150,000 (Z2) just before run 1 and 200,000 (Z3) just before run 2. Each was still unconfirmed when its release was built.

Run 1 (dismissed). X alone fails, and the next set adds the largest confirmed coin (Z, 90k). The larger unconfirmed Z2 is skipped:

15:36:37.251 Using 1 manually selected UTXOs with total value: 1200sats
15:36:37.457 ERROR Failed to create transaction: Insufficient funds: 0.00001200 BTC available of 0.00001329 BTC needed
15:36:37.581 Using 2 manually selected UTXOs with total value: 91200sats
15:36:37.714 Created new transaction 5b87f332e3be2c09d726a1bced707dd20311a810dfdb026219f974f7d5bcc969 sending 1000sats on-chain

The fee dialog showed "Network fee: 626 sats".

Run 2 (approved). Z2 had confirmed by then and Z3 was unconfirmed:

15:40:07.695 Using 1 manually selected UTXOs with total value: 1200sats
15:40:07.723 ERROR Failed to create transaction: Insufficient funds: 0.00001200 BTC available of 0.00001329 BTC needed
15:40:07.726 Using 2 manually selected UTXOs with total value: 151200sats
15:40:08.124 Created new transaction 8d34be228a350b672af0ce7d5096319d87ac74c706793f331cfec943d06ef826 sending 1000sats on-chain
15:41:03.260 INFO [PaykitPaymentProofRepo.kt:566] Released on-chain payment '226426e7…31b0' through '8d34be22…f826'
15:45:05.857 Onchain transaction 8d34be22…f826 confirmed

The release decoded from Electrum has inputs X (02c56b7c…dc5f:0, 1,200) and Z2 (e0e00d48…c852:0, 150,000, the largest confirmed coin), outputs 149,574 change + 1,000, and a 626-sat fee. Not spent: Z3 (200,000, largest but unconfirmed), and Z and Y (smaller confirmed coins, not needed once one coin was enough).

Staging Electrum still reports the original as a missing transaction. The next payment was sent (7775ab3f…f52c). The payee received only the two follow-up payments, never the original 1,000. Continue → fee dialog timings are next.

[jvsena42]

Approving at f49724063.

The code scan of e9ed7c6..f497240 is clean (partial review above). Both fixes are in: extra coins are confirmed-only, and they are added one at a time, largest first. CI is green at this head.

Device gate: f497240 — CI APK on a Pixel 9 emulator. The payee was an iOS simulator on synonymdev/bitkit-ios#998. Details in the partial comments above.

• Extra-coin release: passed. The original spent only a 1,200-sat coin X with its broadcast swallowed, and the request expired. On both runs, X alone failed with Insufficient funds: 0.00001200 BTC available of 0.00001329 BTC needed, and the next set added exactly one coin: the largest confirmed one. The larger unconfirmed coin was skipped each time (150,000 in run 1, 200,000 in run 2). The approved release 8d34be22… spent X + the 150,000 confirmed coin with a 626-sat fee, and confirmed. The original never reached the network, the next payment was sent, and the payee never received the original.
• perf: the two extra coin sets cost 0.4 s (X-only failure to release built). ⚠️ Continue → fee dialog took 21.5 s and 15.6 s here, against 4.8–6.0 s on f22c596 yesterday. The time went before and during the explicit sync (Syncing LDK… 7.5–11.3 s after the tap, sync 4.6–5.7 s), while another party was mining blocks every 30–60 s on the shared staging chain, I was depositing a coin right before each Continue, and a background incremental sync was finishing at the same moment. None of that time is in prepareRelease, which this PR changes. It stayed under the 30 s limit. A run on a quiet chain would show whether the pre-sync gap is environmental.
• Figma: N/A, no UI change. l10n: no strings changed.

Each Continue still derives a new wallet address for the release, as on #1559. This is an observation, not a finding.