Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
 [Medium impact] The change affects one wallet payment workflow for eligible Shop on-chain sends. Fix the broken device-test constructor call before merging; refreshing the cancel option would also improve the waiting flow. FindingsSummaryAdds cancellation for unresolved Shop on-chain payments after their request ends.
The new constructor argument breaks an existing device test. The cancel option also does not refresh when a request ends while the screen remains open. Tests were not run. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Open Payment Pending] --> B{Shop request ended?}
B -->|Yes| C[Offer cancel]
C --> D[User confirms]
D --> E[Sync wallet]
E --> F{No candidate known and input parents known?}
F -->|No| G[Keep payment pending]
F -->|Yes| H{Attempt unchanged under lock?}
H -->|No| G
H -->|Yes| I[Remove original proof]
I --> J[Delete attempt]
J --> K[Show success and close]
Reviews (1) · Last reviewed commit: "fix: let an expired payment that never r..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 059d2569e2
ℹ️ 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".
| PaymentRequestLifecycleState.ACCEPTED, | ||
| PaymentRequestLifecycleState.ACTIVE_RECURRING -> { | ||
| val now = Clock.System.now() | ||
| val request = runCatching { record.toPaykitPaymentRequestHistory(now) }.getOrNull() | ||
| ?: return@withContext false |
There was a problem hiding this comment.
For an unresolved subscription payment, the record state is ACTIVE_RECURRING, but toPaykitPaymentRequestHistory() delegates to a parser that explicitly rejects that state as a recurring request. This branch therefore always returns false, so an expired recurring-period attempt remains guarded and continues blocking every subsequent on-chain send. Resolve the billing period identified by requestId.billingPeriodStartsAt from the subscription record before checking its payment or quote deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e6cc735: a subscription payment is now checked against its own billing period. It counts as ended when the subscription is canceled, rejected or expired, or when that period's payment deadline (from the subscription's pricing and the started proof's period) has passed; only a proof for that period refuses it. Covered by recurring period counts as ended only without its own proof.
There was a problem hiding this comment.
@ben-kaufman fixed in 51ede25: an active subscription period also counts as ended once the stored proof's quote deadline has passed, through submissionDeadline on the materialized period request. expired quote ends an active subscription period covers a live and an expired quote.
| val requestedAt = (System.currentTimeMillis() / 1000).toULong() | ||
| withTimeoutOrNull(timeout) { | ||
| sync() | ||
| while ((lightningService.status?.latestOnchainWalletSyncTimestamp ?: 0uL) <= requestedAt) { |
There was a problem hiding this comment.
When an up-to-date wallet sync completes within the same wall-clock second as the tap, latestOnchainWalletSyncTimestamp equals requestedAt; the <= condition then waits for an unrelated later sync and can hit the 30-second timeout even though sync() already succeeded. This makes cancellation repeatedly report that syncing is unavailable on fast syncs, leaving all new on-chain sends blocked. Track actual sync completion or a pre-sync baseline instead of requiring a later timestamp second, and use the repository's injectable time helper rather than the direct system clock.
AGENTS.md reference: AGENTS.md:L285-L285
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e6cc735: a sync that finishes in the same second as the tap now counts, and the tap time comes from nowMillis().
There was a problem hiding this comment.
@ben-kaufman fixed in 51ede25: the check reads the last on-chain sync timestamp before calling sync() and waits, up to 30 s, for one strictly later. A sync that finished earlier in the same second no longer counts, and a failed or skipped sync never moves the timestamp, so cancellation reports that it could not sync instead. only a sync finished after the request counts as fresh covers the same-second case.
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. |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Adds a "Cancel expired payment" path on Payment Pending for a Shop on-chain payment whose request ended and whose candidates the wallet never saw after a fresh sync, removing the original proof under the proof lock before deleting the attempt under the store lock. Reviewed 717e4f9, full tier, reasoned from the code and CI (Bitkit Android branches are not built here).
What I checked, and 5 candidates I ruled out
Read in full: OnchainSendAttemptStore.kt (abandonUnbroadcast, releaseInterruptedShopPreparation, broadcastPreparedCandidate), PaykitPaymentProofRepo.kt (onchainRequestHasEnded, quoteDeadlinePassed, abandonUnbroadcastOnchainPayment, releaseDeniedPrivateConsumption, reconcile), LightningRepo.sync and the new wait, LightningService.walletKnowsNoCandidate, SendPendingViewModel.kt, SendPendingScreen.kt
Call sites traced: abandonUnbroadcastOnchainSend, onchainCandidatesNeverReachedNetwork, SendPendingViewModel constructor (unit and device tests updated)
CI: lint, detekt, build-local green; build (unit tests) and the e2e jobs still running at review time
Ruled out
- Attempt changes between the network check and the release (a retry broadcasts a new candidate):
abandonUnbroadcastreloads under the store mutex and requirescurrent == expected, and a retry rewritestxid/candidateTxids, so the release is refused;inFlightPreparationscovers a preparation still holding the attempt. - Lock-order inversion: the store mutex is taken first and the proof
operationMutexinside the callback; no path inPaykitPaymentProofRepocalls into the store while holdingoperationMutex(the onelightningRepocall under it isgetPayments). - Proof removed but attempt left behind on a crash: proof removal runs first and the attempt still satisfies
canBeAbandonedwith no matching proof, sooriginalis empty and a later call deletes it. - Stale sync accepted: the wait requires
latestOnchainWalletSyncTimestampstrictly after the tap, and a timeout raisesOnchainSendAbandonSyncError, shown as its own message rather than a refusal. - Pruned wallet history read as "never broadcast":
walletKnowsNoCandidatealso requires every original input's parent transaction to be known, so a wallet that lost history refuses instead of releasing.
Merge confidence: 4/5, walletKnowsNoCandidate and the post-tap sync wait, the checks that decide whether coins are released, have no unit test of their own; the repo and view-model tests stub them.
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
ben-kaufman
left a comment
ben-kaufman
left a comment
There was a problem hiding this comment.
Reviewed e6cc735. The 134 focused store, proof-repository, and pending-ViewModel tests pass. An additional regression against the unchanged repository fails after simulating proof removal with the attempt still present, confirming the new inline finding.
Two existing findings are still incomplete. In #1553 (comment), the recurring path now materializes the period but still ignores the original quote expiry, so retry can reject the quote while cancellation stays unavailable. In #1553 (comment), the comparison now accepts a cached sync from earlier in the same second. The sync result is ignored and can return before an ongoing sync completes, so fresh wallet evidence is not established. Please address those in the existing threads. No device or funded-payment tests were run in this review.
| } | ||
| if (!safe) return@withLock false | ||
| for (proof in original) releaseDeniedPrivateConsumption(proof) | ||
| if (original.isNotEmpty()) persist(proofs - original.toSet()) |
There was a problem hiding this comment.
If the process stops after removing this proof and before deleting the attempt, an expired recurring payment cannot finish cancellation after restart. activeRecurringPeriodPassed gets its period only from the removed proof, so a subscription that is still active now returns false and the attempt keeps blocking new sends. Could we retain enough release evidence with the attempt to resume after this intermediate state, and cover it with a restart test?
There was a problem hiding this comment.
Fixed in 51ede25: once the request is found ended, the attempt is marked releaseApproved before the proof is removed, so after a restart onchainRequestHasEnded returns true from the attempt alone and the release finishes after a fresh network check. approved release finishes after its proof was removed covers an active subscription with no local proof left.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at e3688a6: one MEDIUM inline. This round was reviewed on Opus because Fable credits ran out.
Checked and clean:
approveReleaseandabandonUnbroadcastre-load the attempt under the store mutex and requirecurrent == expected, so a retry that retains or broadcasts a candidate in between blocks the release. Lock order is store mutex, then the proofoperationMutex.- Retry candidates join
candidateTxidsbefore dispatch. A candidate the wallet once saw stays in the BDK graph after eviction, and missing input parents refuse the release. - Restored attempts are excluded.
releaseApprovedis persisted before the proof is removed, so a kill mid-release resumes with the network check re-run. removeOriginalShopProofsnever strips a submitted, verified, non-candidate or hardware-wallet proof. The recurring path refuses when a proof exists for the same period.- #1548 (Back on the recovery PIN screen) is present but not worsened. A cancel against a stale attempt is refused rather than hanging.
The same mechanism is in the iOS twin, synonymdev/bitkit-ios#971 (deeec53d).
Performance-relevant: Payment Pending polls onchainRequestHasEnded every 30 s, reading all payment requests each time, and Cancel waits up to 30 s for a fresh sync.
The device gate waits for the MEDIUM fix.
| return@withLock false | ||
| } | ||
| if (!removeOriginalProof(current)) return@withLock false | ||
| keychain.delete(KEY, current.walletIndex) |
There was a problem hiding this comment.
MEDIUM: cancelling an expired payment frees the original inputs without invalidating the signed original, so a later payment with other coins and the original can both confirm.
Cancel trusts a single Electrum server's history, which ldk-node documents it should not. walletKnowsNoCandidate passes when the configured Electrum server's history has no candidate txid but has every input's parent. That shows only that this server never reported the signed original, not that its bytes reached nobody. ldk-node rc.76 payment/onchain.rs:209 says: "neither Rejected nor Unknown authorizes clearing the original payment guard."
Nothing keeps the original inputs reserved after cancel. prepare_send_to_address does not add the signed tx to the BDK graph or lock its outpoints. Once the guard is gone, the next send's coin selection can pick disjoint UTXOs. On master, the only way out of an Unknown attempt was a retry, and retainPreparedReceipt required receipt.inputs.toSet() == originalInputs.toSet(), so every later spend conflicted with the original.
The button can show within seconds of an Unknown broadcast. The payee controls CANCELED, or the quote deadline may already have passed, and there is no minimum age.
Failure path:
- The broadcast outcome is Unknown.
- The request ends.
- The user taps Cancel and reads "Its coins are available again".
- The user checks out again and pays with other coins.
- The original surfaces later, from a withholding server, a plaintext-TCP custom server that is MITM'd, or a late delivery.
- The payee is paid twice, and Bitkit has already deleted the original's proof.
Against an honest server, the forced fresh sync would see a tx that reached a mempool, so the exposure needs a dishonest or interposed server, or a narrow overload race. But the cost is a second full payment in a common flow, and the cancel copy promises more than the check proves.
Fix: make cancel conflict with the original rather than forget it. Either spend the original inputs to a wallet-owned output and delete the guard only after that replacement is Accepted or Observed, or keep a persisted tombstone of the original inputs and force the next send to include one of them until they are seen spent. At minimum, word the dialog and toast as "may still confirm" until then. The retry path cannot be reused as-is, because retainPreparedReceipt(isRecovery=true) requires the same address and amount.
Reproduction
./gradlew :app:testDevDebugUnitTest --tests 'to.bitkit.repositories.OnchainSendAttemptStoreTest'
Fails on e3688a6 (32 tests, 1 failed):
java.lang.AssertionError: disjoint send admitted while abandoned original is still valid. Expected an exception of class java.lang.IllegalArgumentException to be thrown, but was completed successfully with the result: <OnchainSendAttempt(walletId=wallet0, ... requestId=PaykitPaymentRequestId(paymentRequestId=request-2 ...), evidence=Pending, txid=cdcd...
An illustrative store-side change that requires the next receipt to spend one of the abandoned inputs makes all 32 pass. It is in memory only, so it is not a production fix.
diff --git a/app/src/test/java/to/bitkit/repositories/OnchainSendAttemptStoreTest.kt b/app/src/test/java/to/bitkit/repositories/OnchainSendAttemptStoreTest.kt
index d5ccfcc0b..fd4c17b6c 100644
--- a/app/src/test/java/to/bitkit/repositories/OnchainSendAttemptStoreTest.kt
+++ b/app/src/test/java/to/bitkit/repositories/OnchainSendAttemptStoreTest.kt
@@ -780,6 +780,50 @@ class OnchainSendAttemptStoreTest : BaseUnitTest() {
assertTrue(store.admitForTest(walletId = WalletScope.default).attemptId != attempt.attemptId)
}
+ @Test
+ fun `payment after an abandon cannot spend coins disjoint from the abandoned original`() = test {
+ var saved: String? = null
+ val keychain = mock<Keychain>()
+ whenever(keychain.loadString(key, 0)).thenAnswer { saved }
+ whenever(keychain.upsertString(eq(key), any(), eq(0))).doSuspendableAnswer { saved = it.getArgument(1) }
+ whenever(keychain.delete(key, 0)).doSuspendableAnswer { saved = null }
+ val store = OnchainSendAttemptStore(testDispatcher, keychain, mock(), kotlin.time.Clock.System)
+ val attempt = store.admit(
+ walletId = WalletScope.default, requestId = PaykitPaymentRequestId("request", "payer"),
+ orderId = "order", address = "bcrt1qrecipient", amountSats = 1000uL,
+ isMaxAmount = false, feeRateSatsPerVByte = 1uL, isTransfer = false,
+ channelId = null, tags = emptyList(), beforeSendAttempt = {}, payerIdentity = "payer",
+ )
+ val original = OnchainPreparedReceipt(
+ txid = "ab".repeat(32),
+ inputs = listOf(OnchainSendInput("11".repeat(32), 0u)),
+ address = attempt.address,
+ amountSats = attempt.amountSats,
+ )
+ store.retainPreparedReceipt(attempt.attemptId, 0, original, false)
+ store.broadcastPreparedCandidate(attempt.attemptId, 0, original.txid) { beforeDispatch ->
+ beforeDispatch()
+ OnchainSendOutcome.Unknown(original.txid)
+ }.also { store.recordOutcome(attempt.attemptId, it, 0) }
+ val approved = requireNotNull(store.approveRelease(requireNotNull(store.current())))
+ assertTrue(store.abandonUnbroadcast(approved) { true })
+
+ // Same payee re-issues the request after the cancel; coin selection picks a different UTXO.
+ val next = store.admit(
+ walletId = WalletScope.default, requestId = PaykitPaymentRequestId("request-2", "payer"),
+ orderId = "order-2", address = "bcrt1qrecipient", amountSats = 1000uL,
+ isMaxAmount = false, feeRateSatsPerVByte = 1uL, isTransfer = false,
+ channelId = null, tags = emptyList(), beforeSendAttempt = {}, payerIdentity = "payer",
+ )
+ val disjoint = original.copy(txid = "cd".repeat(32), inputs = listOf(OnchainSendInput("22".repeat(32), 1u)))
+ // The signed original still spends 11..:0 and can surface later; a disjoint new payment lets both confirm.
+ assertFailsWith<IllegalArgumentException>("disjoint send admitted while abandoned original is still valid") {
+ store.retainPreparedReceipt(next.attemptId, 0, disjoint, false)
+ }
+ val conflicting = disjoint.copy(inputs = original.inputs + disjoint.inputs)
+ assertEquals(conflicting.txid, store.retainPreparedReceipt(next.attemptId, 0, conflicting, false).txid)
+ }
+
@Test
fun `ordinary payment is never abandoned`() = test {
var saved: String? = null
There was a problem hiding this comment.
HIGH: A timed-out local broadcast can still arrive after cancellation
This overlaps @jvsena42's finding above. Adding the concrete timeout evidence from my independently saved source review.
The pinned native implementation can return Unknown while its spawn_blocking(transaction_broadcast) continues; electrum_timeout_returns_unknown_while_blocking_rpc_remains_in_flight explicitly demonstrates that lifetime. A newer sync can observe no candidate before that delayed broadcast is processed.
At app/src/main/java/to/bitkit/services/LightningService.kt:1041–1042, absence from wallet history still permits deleting the started proof and send guard without stopping the old broadcast or invalidating its signed transaction. A later payment using different UTXOs can therefore confirm alongside it.
Retain the guard until dispatch is definitively ruled out or the original candidate is invalidated. Cover a broadcast held past timeout and released after the cancellation check.
Evidence basis: source tracing and the existing native timeout regression test; I have not run a funded/device reproduction of this cancellation path.
There was a problem hiding this comment.
Agreed: cancelling must make the original unconfirmable rather than forget it. This PR moved to 3.0.5, and the RC3 part split out to synonymdev/bitkit-ios#990, so the fix lands here for 3.0.5. The planned approach: after the fresh-sync check, cancel spends at least one of the original's coins back to the wallet at a fee above every candidate's, behind the same fee approval and PIN as Retry, and keeps the guard and the started proof until that release is accepted or the wallet sees it. A Rejected or Unknown release keeps both. The dialog will say that cancelling sends the coins back to the wallet and costs a network fee.
There was a problem hiding this comment.
The release transaction planned in the reply to the double-pay finding covers this too: it spends one of the original's coins at a higher fee, so a broadcast still in flight can no longer confirm, and the guard stays until that release is accepted or the wallet sees it. It lands here for 3.0.5.
|
Closing: ldk-node rc.76 alone fixes the dropped-connection bug, shown on device in synonymdev/bitkit-ios#990 (synonymdev/bitkit-ios#990 (comment)) and on synonymdev/bitkit-android master. The recovery features here aren't needed for 3.0.0. The branch stays, so this can be reopened if a real case shows up. |
6 participants
Closes #1551
Twin: synonymdev/bitkit-ios#971
Description
Out of Scope
OnchainSendAttemptStore: ordinary sends and transfers with an unknown outcome keep their existing Retry; only Paykit Shop payments whose request ended get the cancel.Design
N/A — no design available; adds a secondary "Cancel expired payment" button and a confirmation dialog to the existing Payment Pending screen.
Preview
Regtest staging, Android emulator, 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 refused: transaction already known
A Shop payment with an unknown outcome whose request has ended blocks a new
bitcoin:payment. Its transaction is already known to the wallet, so the cancel is refused and the payment stays pending.and-refused2x.mp4
Cancel released, next payment sent
The same blocked state, with a transaction that never reached the network: after a fresh sync the payment is cancelled, and a new on-chain payment is sent.
and-released2x.mp4
QA Notes
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
Cancel a Shop payment whose request has ended while its outcome is unknown → it is cancelled only if none of its transactions reached the network, and new on-chain sends work again — needs an Electrum server that swallows requests (synonymdev/bitkit-ios
scripts/electrum-bridge.py).Steps
python3 scripts/electrum-bridge.py --port 50099),adb reverse tcp:50099 tcp:50099, and set Settings › Advanced › Electrum Server to127.0.0.1, port50099, TCP.touch /tmp/electrum-bridge/blackhole, swipe, and wait for Payment Pending.rm /tmp/electrum-bridge/blackhole.Released on-chain payment <txid> after its request ended without it reaching the network, and the next on-chain payment is sent. The Shop order stays unpaid.lose-replyinstead ofblackholein step 2, so the transaction reaches the mempool: Payment Pending shows "This payment may still reach the network, so it cannot be cancelled.", or the next sync recognizes the transaction and no cancel is offered.Automated Checks
OnchainSendAttemptStoreTest.kt— an unresolved Shop payment is released only after its proof is removed and only if unchanged; an ordinary payment is never releasedPaykitPaymentProofRepoTest.kt— a canceled, rejected or expired request releases a payment that never reached the network and removes its proof; an open request, a submitted proof or a possibly broadcast payment keeps itSendPendingViewModelTest.kt— the cancel is offered only for an ended Shop request, reports a failed sync, and clears the pending state on successbitcoin:payment8cd9116c…was broadcast.