Repository navigation
fix: show ordinary Lightning results without Paykit waits - #1684
ben-kaufman wants to merge 2 commits into
Conversation
|
Triage: 3.1.0. It fixes the shipped Lightning payment result screen, and 3.1.0 is due 28 Oct. |
 [Medium impact] The PR appears safe to merge; ordinary results skip unrelated work without changing request-specific proof handling. SummaryOrdinary Lightning failures and pending results now skip unrelated Paykit proof work. Payments tied to an incoming request keep their existing checks and proof handling.
Reviews (1) · Last reviewed commit: "fix: show ordinary Lightning results wit..." · Reviewed by Greptile |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. For a Lightning send that has no incoming Paykit request, the result screen no longer waits on proof cleanup or reconciliation: a failure skips failLightningPayment, and a pending result skips refreshIncomingPaykitPaymentRequests(). Reviewed 3b95acd, light tier (7 changed production lines), stacked on #1681 and reviewed as the delta against fix/usdt-recovery-parity.
What I checked, and 4 candidates I ruled out
Read in full: proceedWithLightningPayment, clearFailedLightningPayment, associateLightningPaymentProof, preparePaymentProof, refreshIncomingPaykitPaymentRequests, handlePaymentFailed, handlePaymentSuccessful in AppViewModel.kt; failLightningPayment (both overloads), associateLightningPayment and reconcile in PaykitPaymentProofRepo.kt; the two new tests in AppViewModelSendFlowTest.kt.
Call sites traced: every failLightningPayment( and associateLightningPayment( caller in app/src/main, and every refreshIncomingPaykitPaymentRequests() call.
CI when read: build-local and lint green; build and the local e2e shards were still running. Not built here: on this track the branch's Gradle code would run with a write token in its environment, so CI is the gate.
Ruled out
- An ordinary failure leaves a stale Lightning proof:
associateLightningPaymentProofreturns before writing anything whenrequest == null, andpreparePaymentProofdoes too, so this flow never creates a proof keyed to that hash. The node'sPaymentFailedevent still runsfailLightningPayment(paymentHash)off the send path inhandlePaymentFailed. - An allowance payment on the same hash loses its cleanup:
PaykitAllowanceExecutorassociates its proof and fails it itself, throughpayer.failLightningPayment(paymentHash, it), so it does not depend on this branch. - A pending ordinary payment no longer brings up the next request: the refresh dropped from this path also runs after
PaymentSuccessfulandPaymentFailedand on the polling loop, and nothing on the ordinary pending path changes request state.trackandpreserveContactPaymentContextstill run first, so the pending hash and the contact context are kept. - Reordering the
whenchanges how request payments are handled: whenisPaymentRequestis true, the guard is skipped andLightningPaymentFailedErrorand the two-argumentfailLightningPaymentrun as before. The guard now comes beforeLightningPaymentFailedError, but that arm and the old!isPaymentRequestarm both returnedtrue, so the only difference is the dropped cleanup call.
Merge confidence: 4/5, build and e2e were still running when this was read and the branch was not built here; the change itself is covered by the two new AppViewModelSendFlowTest.kt cases.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the runtime does not report it to the reviewer)
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- "Ordinary" is
contactPaymentContext?.incomingPaymentRequest == null, the same value that gates proof preparation and association. So a Paykit payment can't be classified as ordinary, and an ordinary send never has a proof to clean up. - An ordinary Lightning failure no longer awaits
failLightningPayment(AppViewModel.kt:6264), and an ordinary pending result no longer awaitsrefreshIncomingPaykitPaymentRequests(AppViewModel.kt:6231). Paykit payments keep both. - Receive and startup were already independent of publication on Android (the debounced address observer,
AppViewModel.kt:836-850). - Twin synonymdev/bitkit-ios#1120 covers receive publication and startup cleanup instead. iOS still awaits
failLightningPaymenton an ordinary Lightning failure, which this PR's body acknowledges.
Performance-relevant (removes a user-visible wait). The gate times Pay → error/pending screen with the homeserver unreachable, head vs master, and counts Failed to clear a pending Paykit payment proof during an ordinary failure (expected 0).
Device gate (partial): 3b95acd — not run yet; pending that measurement and wallet/ordinary-during-paykit-outage.xml. Device results will follow in a separate review.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 3b95acd. My code scan stands with no findings, and the device gate found none.
Device gate: 3b95acd (base dc5a47d) — Pixel_Tablet with a 60,000-sat spending channel, staging. journeys: wallet/ordinary-during-paykit-outage.xml step 4 passed: an ordinary Lightning payment that fails with a real PaymentFailed(RouteNotFound) reaches the error screen once per swipe, with no automatic second payment. perf: Pay → "Payment Failed" 2.8 s (4 runs), base 2.8 s; 0 Failed to clear a pending Paykit payment proof on both. Figma: no design; l10n: no UI text changed.
-dns-server) and a filtering CONNECT proxy (-http-proxy); both carried only system traffic, and the app's traffic bypassed them. The emulator image has no root and no hosts file, and airplane mode would also cut the LSP and Electrum. Step 5 needs a hold invoice, which the staging LSP doesn't expose.
Skipped — covered by CI at 3b95acd: unit tests, lint, detekt, e2e (all green).
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict:
Findings: None. I reviewed the full diff and reached callers at 3b95acd; ordinary sends create no request proof, and genuine requests retain their authorization, claim and proof paths.
QA for 3b95acd: Built with the committed dependencies. Device cases used Android 15 on a Pixel 7 emulator, disposable local regtest funds and an authenticated local Pubky identity. The payment-result cases used the debug-only lock/observation hook below; no SDK result was fabricated.
Test 1: J1.4 — ordinary failure with proof maintenance held ✅ A real recipient-rejected invoice reached Payment Failed while the proof lock remained held, with one submission.
The 101-sat canceled invoice produced RECIPIENT_REJECTED. Device logs recorded the send result 0.97 seconds after submission with the gate still held, and no automatic second submission. The recording shows confirmation through the error screen.
failure2x.2x.mp4
Ordinary failed payment at 2×.
Test 2: J1.5 — ordinary pending result with proof maintenance held ✅ A real hold invoice reached Payment Pending with its original hash and 200 sats before releasing the proof lock.
Paykit was enabled and the wallet was authenticated. LND reported one accepted 200,000-msat HTLC; device logs recorded one submission and the same hash at pending navigation after the existing 10-second node-event timeout. The recording shows the pending screen. Hash, submission count and the held gate were checked in logs and LND state.
pending2x.2x.mp4
Real hold-invoice pending result at 2×.
Test 3: J1.1–2 — receive during public publication delay ✅ Receive returned a current local QR while an authenticated public endpoint PUT remained held, and releasing the fault reconciled the current public invoice.
An actual synthetic payment triggered public-invoice replacement. In the full edit run, the displayed QR decoded to amount=0.00002 and message=Held-publication; the proxy confirmed an outstanding endpoint PUT while that QR was usable. Close/reopen was checked through the UI. After release, an independent public read matched the wallet's current public invoice, differed from the paid invoice and the cleanup flag cleared. Identity sign-out during reconciliation was not driven. The clip shows a repeat QR presentation under the same publication fault; QR payload and reconciliation claims were checked separately from the video.
receive2x.2x.mp4
Receive QR presentation during a held publication PUT at 2×.
Test 4: J1.3 — recovery startup during disabled-Paykit cleanup ⏭️ The pending order/swap plus retained endpoint-cleanup scenario was not established.
Direct authenticated Pubky and regtest Lightning routing worked, but the fixture does not seed that combined recovery state. Startup was inspected in code; this is not a device pass.
Test 5: J1.6 — retained proof restart and genuine requests ⏭️ Public endpoint recovery passed above; the complete retained-proof restart and genuine-request fault scenario remains unrun.
The repository's compatibility check found controlled peers on an older Paykit revision and no Paykit Server revision pinned to this head's released Paykit 0.1.1-rc1. Direct Pubky signup/publication provided the receive subset, but not the matching request/proof peer. The proof-lock cases above do not establish retained-proof service recovery. Request-specific authorization and proof regressions passed in the focused component suite.
I ran assembleDevDebug and the focused AppViewModelSendFlowTest suite successfully at the exact head, including both new held-proof regressions. The existing same-head device report already covered an ordinary routing failure without an isolated Paykit outage; I relied on that result rather than repeating it.
Debug test hook and deciding device logs
Not pushed. The hook holds the real proof-operation mutex until explicitly released, logs submission/navigation, and connects the real regtest peer for setup. It does not change the result decision, authorization or SDK outcome.
diff --git a/app/src/main/java/to/bitkit/repositories/PaykitPaymentProofRepo.kt b/app/src/main/java/to/bitkit/repositories/PaykitPaymentProofRepo.kt
index 4a5b66f36..c21f20a36 100644
--- a/app/src/main/java/to/bitkit/repositories/PaykitPaymentProofRepo.kt
+++ b/app/src/main/java/to/bitkit/repositories/PaykitPaymentProofRepo.kt
@@ -18,6 +18,7 @@ import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.launch
+import kotlinx.coroutines.isActive // test hook
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import kotlinx.coroutines.withContext
@@ -135,6 +136,20 @@ class PaykitPaymentProofRepo @Inject constructor(
private val persistenceMutex = Mutex()
private val deliveryScope = appScope(ioDispatcher, TAG)
+ init { // test hook: explicit file-controlled contention on the real proof-operation lock
+ if (to.bitkit.BuildConfig.DEBUG) deliveryScope.launch { // test hook
+ val gate = java.io.File("/data/user/0/${to.bitkit.BuildConfig.APPLICATION_ID}/files/v4-proof-hold") // test hook
+ while (kotlinx.coroutines.currentCoroutineContext().isActive) { // test hook
+ if (gate.exists()) operationMutex.withLock { // test hook
+ android.util.Log.i("V41684", "PROOF_LOCK_HELD") // test hook
+ while (gate.exists()) kotlinx.coroutines.delay(100) // test hook
+ android.util.Log.i("V41684", "PROOF_LOCK_RELEASED") // test hook
+ } // test hook
+ kotlinx.coroutines.delay(100) // test hook
+ } // test hook
+ } // test hook
+ } // test hook
+
// Process-local activity handoffs only; restored proofs must reconstruct their activity.
private val observedHardwareProofs = mutableSetOf<PendingPaykitPaymentProof>()
diff --git a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
index 94648b955..ed2f9c393 100644
--- a/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
+++ b/app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt
@@ -71,6 +71,7 @@ import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.merge
import kotlinx.coroutines.flow.stateIn
import kotlinx.coroutines.flow.update
+import kotlinx.coroutines.isActive // test hook
import kotlinx.coroutines.launch
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
@@ -86,6 +87,7 @@ import org.lightningdevkit.ldknode.PaymentId
import org.lightningdevkit.ldknode.SpendableUtxo
import org.lightningdevkit.ldknode.SyncType
import org.lightningdevkit.ldknode.Txid
+import to.bitkit.ext.of // test hook
import to.bitkit.BuildConfig
import to.bitkit.R
import to.bitkit.data.CacheStore
@@ -6229,6 +6231,7 @@ class AppViewModel @Inject constructor(
pendingPaymentRepo.track(pendingHash)
preserveContactPaymentContext(pendingHash)
if (incomingPaymentRequest != null) refreshIncomingPaykitPaymentRequests()
+ if (to.bitkit.BuildConfig.DEBUG) android.util.Log.i("V41684", "SHOW_PENDING hash=$pendingHash amount=$displayAmountSats gate=" + java.io.File(context.filesDir, "v4-proof-hold").exists()) // test hook
setSendEffect(SendEffect.NavigateToPending(pendingHash, displayAmountSats.toLong()))
lnurlComment?.let { activityRepo.setLightningMessageIfEmpty(paymentHash, it) }
return@onFailure
@@ -6243,6 +6246,7 @@ class AppViewModel @Inject constructor(
is LightningPaymentFailedError -> error.reason.toSendFailureDetails(context, error.paymentRequest)
else -> error.toSendFailureDetails(context, _sendUiState.value.currentLightningPaymentRequest())
}
+ if (to.bitkit.BuildConfig.DEBUG) android.util.Log.i("V41684", "SHOW_ERROR hash=$paymentHash gate=" + java.io.File(context.filesDir, "v4-proof-hold").exists()) // test hook
setSendEffect(SendEffect.NavigateToError(failure))
}
}
@@ -6616,12 +6620,33 @@ class AppViewModel @Inject constructor(
onBroadcast = onBroadcast,
)
+ init { // test hook: fixture setup only; payment execution is still the real production path
+ if (to.bitkit.BuildConfig.DEBUG) viewModelScope.launch(bgDispatcher) { // test hook
+ val command = java.io.File(context.filesDir, "v4-command") // test hook
+ while (kotlinx.coroutines.currentCoroutineContext().isActive) { // test hook
+ if (command.exists()) { // test hook
+ val action = command.readText().trim() // test hook
+ command.delete() // test hook
+ runSuspendCatching { // test hook
+ if (action == "node") android.util.Log.i("V41684", "NODE_ID=" + lightningRepo.lightningState.value.nodeId) // test hook
+ if (action.startsWith("peer ")) { // test hook
+ lightningRepo.connectPeer(org.lightningdevkit.ldknode.PeerDetails.of(action.removePrefix("peer "))).getOrThrow() // test hook
+ android.util.Log.i("V41684", "PEER_CONNECTED") // test hook
+ } // test hook
+ }.onFailure { android.util.Log.e("V41684", "SETUP_FAILED", it) } // test hook
+ } // test hook
+ kotlinx.coroutines.delay(200) // test hook
+ } // test hook
+ } // test hook
+ } // test hook
+
private suspend fun sendLightning(
bolt11: String,
amount: ULong? = null,
paymentDeadlineAt: Instant? = null,
onBeforeSend: suspend () -> Boolean,
): Result<PaymentId> {
+ if (to.bitkit.BuildConfig.DEBUG) android.util.Log.i("V41684", "SUBMIT_INVOICE") // test hook
return lightningRepo.payInvoice(
bolt11 = bolt11,
sats = amount,
Failure:
--------- beginning of main
10-26 23:15:45.903 9876 9926 I V41684 : PROOF_LOCK_HELD
10-26 23:15:57.527 9876 9876 I V41684 : SUBMIT_INVOICE
10-26 23:15:58.496 9876 9876 I V41684 : SHOW_ERROR hash=ccaa3e8a12d4c673329e862c273b1f17e2240d462fcc5d7d381df121ad63efc9 gate=true
Pending:
--------- beginning of main
10-26 23:14:51.658 9876 9925 I V41684 : PROOF_LOCK_HELD
10-26 23:14:57.744 9876 9876 I V41684 : SUBMIT_INVOICE
10-26 23:15:07.982 9876 9876 I V41684 : SHOW_PENDING hash=8556a73c5df1149f70307ea83764bc09feff634a5ad6eceba401e5472ad83677 amount=200 gate=true
Full review: dc5a47d..3b95acd, including payment preparation, result handling, node events, private-contact and allowance siblings. The full startup/restart/genuine-request service journey and a base-versus-head latency comparison were not driven.
Twin: bitkit-ios#1120 fix: decouple ordinary receive and startup from Paykit publication at fd3ce01 matches the shared intent and journey. It removes different receive/startup waits; Android already prepares receive locally, while this patch removes ordinary Lightning result waits. This is a source comparison, not a cross-platform device parity claim.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
Dependency: #1681
Companion: synonymdev/bitkit-ios#1120
Description
An ordinary Lightning payment could already be failed or pending in the node while its result screen waited for unrelated Paykit proof maintenance. Return the node-derived result without that unrelated work when there is no incoming payment request. Pending payments retain their original hash and contact context; actual Paykit requests keep their authorization, claim and proof handling.
Out of Scope
No payment is retried by this change. Full-wallet backup separation, pending-restore admission and the separate iOS receive/startup waits are follow-up work. No historical contact incident attribution.
Design
N/A — no design available. Existing result screens and copy.
Preview
Existing error and pending screens appear when the node result is known.
QA Notes
Setup
Use the committed Debug dependencies (Core 0.8.0-rc11, Paykit 0.1.1-rc1, VSS 0.5.23). The deterministic repository tests inject an unrelated held proof cleanup/reconciliation and controlled ordinary node failure/pending results. No funds or services are needed for those tests. J1 requires disposable test-network wallets with synthetic funds, a usable Lightning channel, a controlled failing/hold invoice and a service fixture that delays only an unrelated retained proof. This full service fixture is not provisioned; never use live funds.
Journeys
ordinary-during-paykit-outage.xml— ordinary receive/startup/payment-result behavior while unrelated Paykit services wait. Native component tests cover the two changed Android payment-result steps; the complete journey remains unrun.Manual Tests
N/A — use J1 when its controlled service prerequisites are available.
Automated Checks
AppViewModelSendFlowTest; unchanged production fails both. Candidate focused suite: 504 passed.just compile,just test,just lint: passed. Complete unit suite: 4,606 tests across 254 reports, zero failures/errors/skips.Evidence
Controlled failure and pending results appear before the unrelated proof gate is released, with one invoice submission. Existing request-specific tests continue to pass. Two independent reviews inspected request ownership, proof cleanup, pending tracking, call paths and parity. iOS ordinary pending presentation already returns independently; iOS local proof mutation has a different locking boundary and no matching remote wait was confirmed.
Gaps
No full-app proof-fault/restart journey was run. These component tests mock the node/proof repository; they are not deployed Lightning or Pubky verification. Compatible released Paykit/Pubky authentication/lease fixes remain separate dependencies of the broader recovery work.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)