Repository navigation
fix: deliver completed Paykit proofs promptly - #1591
ben-kaufman wants to merge 13 commits into
Conversation
|
Triage: milestone 3.1.0. It makes Paykit proof delivery start right after the payment is persisted, an improvement on 3.0.x but not a release blocker. It closes no issue, so this PR carries the milestone itself. |
[Medium impact] The PR appears safe to merge; no actionable regression was found. SummaryStarts proof delivery after completed payment state is saved, without waiting for reconciliation or keeping the payment screen open.
No actionable issues found. Tests and payment journeys were not run during this review. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Payment completes] --> B[Save completed proof]
B --> C[Return to caller]
B --> D[Start app-scoped delivery]
D --> E[Lock proof operations]
E --> F{Proof still stored?}
F -->|No| G[Skip stale work]
F -->|Yes| H[Check identity and queued proof]
H --> I[Queue proof if needed]
I --> J[Drain intended recipient queue]
A -. Recovery after failure or restart .-> K[Reconcile stored proofs]
K --> H
Reviews (1) · Last reviewed commit: "fix: deliver completed Paykit proofs pro..." · Reviewed by Greptile |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Starts recipient-scoped proof delivery right after the durable proof write, on the repo's own app scope, and adds duration-only SDK diagnostics. Reviewed 53f704c, full tier, reasoned from the code and CI (the Android branch is not built on my side).
What I checked, and 5 candidates I ruled out
Read in full: the PaykitPaymentProofRepo.kt diff with completeLightningPayment, completeOnchainPayment, completeHardwareOnchainPayment, reconcileHardwareOnchainProof, persistAndSubmit, submitReady and reconcile; PaykitSdkService.processOutboundPrivateMessages, measuredOperation, withPaykitKey, withStateRevisionTracking; PaykitSdkOperationLock.withLock; appScope.
Call sites traced: all three awaitDelivery = false callers (lines 349, 673, 967) and the remaining processPendingPrivateMessages caller in PaykitPaymentRequestRepo.
CI: build (which runs testDevDebugUnitTest), build-local, lint and detekt green; the local e2e shards were still running.
Ruled out
- Self-deadlock on
operationMutex: everyawaitDelivery = falsecaller holds the mutex, but the delivery is a separate coroutine ondeliveryScopethat waits for it, so the caller returns and releases first. - Delivery after a reset or a removal: the launched job re-reads the store under the mutex and only submits a proof still present (
proof in loadProofs()), andsubmitReadyre-checks the identity, the capability and the record before writing. - Delivery under a switched identity:
processOutboundPrivateMessagesnow receivesexpectedIdentityand checks it inside the SDK lock, so a switch between queueing and flush throws instead of sending. - Interactive priority spinning: the
do ... while (report == null)loop only yields null on theBackgrounddeferral path;Interactiveruns once. - Double submit from reconcile racing the launched job: both run under
operationMutex, and the first one through removes the local proof (non-USDT) or finds it already queued in the SDK record.
Merge confidence: 4/5, no findings, but the e2e shards had not finished when I read it, and the delivery paths are covered by unit tests only.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- No double-pay or wrong attribution. The launched delivery holds
operationMutex, submits only a proof still byte-equal inloadProofs(), andsubmitReadyre-checks capability and identity. The interactive flush checksexpectedIdentityinside the SDK lock. No payment-sending path changed. - No skipped evidence gate. Lightning requires the preimage to match the hash; on-chain runs
finishAcceptedShopActivitybefore persisting; hardware waits for the exact transaction. - Crash between persist and delivery. Reconcile recovers it, and the already-queued branch flushes without a second submit.
success-dismissal.xml matches synonymdev/bitkit-ios#1025.
Device gate (partial): not run yet. Perf-relevant (repeated work): measuredOperation, withPaykitKey and readRevision add a Logger.debug line to every SDK operation, Background observeState polling included, and debug lines reach the release log file. To measure against master: "Finished Paykit operation" and "Prepared Paykit key" lines per minute idle on Payment Requests, and the time from "Queued a Paykit payment proof" to the processOutboundPrivateMessages finish. Also pending: payment-requests/success-dismissal.xml. The device result will follow.
d6ea2ac to
c1ba9db
Compare
|
@jvsena42 The current head guards these diagnostics with The latest change also removes the redundant early claim for new inline requests, using the existing atomic claim-and-accept operation. Early claims remain before versioned endpoint consumption, hardware receipt preparation and accepted retries. All 4,377 unit tests pass. Staging timing is recorded in the description with its limitations; matched final-change measurements remain in progress. |
There was a problem hiding this comment.
Verdict:
Findings: None. I found no actionable defect in the reviewed change.
QA for 449f512:
Test 1: Payment Request success dismissal ✅ Closing a 6,007-sat Lightning success preserved proof delivery; reopening retained one outgoing payment, and dismissing a new 5,000-sat request sent nothing.
The requester displayed “Proof submitted.” I inspected both dismissal intervals for error toasts and reopened sheets. Recording at 2×, payer left and requester right.
success-dismissal.2x.mp4
Test 2: Recipient transport isolation and retry ⏭️ I could not establish the proof-delivery-only overlap: an unrelated admitted SDK send also blocked target-request preparation.
A selective private-stream PUT hold was confirmed in the proxy. SDK diagnostics recorded an identity check waiting 72.175 seconds for admission; preparation resumed after release. The failed unrelated send retried, and the subsequent 7,017-sat payment reached “Proof submitted.” This does not verify proof delivery while the unrelated transport remains held. Recipient-specific proof retry passed in the repository tests.
Recording at 2× shows the preparation wait and recovery, payer left and requester right; the hold and queue timing were checked in proxy snapshots and SDK logs.
recipient-isolation.2x.mp4
Test 3: Staging payment through merchant unlock ✅ A 7,013-sat regtest checkout reached merchant completion, credential issuance and the expected 43-byte guarded-content read.
In the correlated debug trace, proof enqueue waited 14.875 μs and spent 2.535 seconds in SDK work; recipient delivery waited 1.745 seconds and spent 7.976 seconds in SDK work. The duration lines contained operation names, trace IDs and durations without payment or contact contents. The recording shows merchant completion and content access. This is one checkout sample, without a matched before/after measurement.
Recording at 2×, payer left and merchant reader right.
staging-unlock.2x.mp4
Test 4: Proof, request, SDK and send-flow regression checks ✅ All four affected unit-test classes passed at the reviewed head.
Checked the Gradle result and JUnit XML for PaykitPaymentProofRepoTest, PaykitPaymentRequestRepoTest, PaykitSdkServiceTest and AppViewModelSendFlowTest, including suspended enqueue/transport, recipient retry, restart, identity isolation, atomic acceptance and refresh admission.
I reviewed the full PR diff and reached callers. Hardware, USDT, reset and injected persistence-failure paths were checked through code and unit tests only. Test 2 remains unverified for the reason above.
Twin: bitkit-ios#1025 fix: deliver Paykit payment proofs promptly at 0634b16 matches the delivery approach and dismissal journey.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 537d35e.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Proof handoff, recipient-scoped background delivery, atomic acceptance, and the bounded identity and peer-list retry match the payment rules on this revision. Versioned private-list consumption still claims before consume, and final authorization still rejects a blocked peer. Passive refresh is deferred on confirmation, the initial payment PIN, hardware signing, and USDT review. Recovery authorization is outside that set. bitkit-ios#1025 at 2142997 was compared for this flow only; its single pin route includes recovery.
The pinned CI build and lint runs succeeded. The added unit tests were inspected and not executed here. Device testing was not performed in this review, and the new journeys are not run by CI.
Suggested additional test cases
- Android, payment PIN enabled, biometrics disabled, two linked disposable regtest wallets, and an unresolved Paykit on-chain payment already on the pending screen. Leave recovery PIN open for 30 seconds while the payer is online, then authorize the original retry. Expected: passive inbox refresh stays deferred during that PIN, the retry broadcasts one payment, and reopening the app does not send another.
Findings
- [LOW] Defer passive refresh during Paykit recovery authorization — inline at
app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt:187.
There was a problem hiding this comment.
Verdict:
The PR moved to a28a077 after this run; these results are for 537d35e.
Findings: No new findings. The existing recovery authorization finding applies to this reviewed head; its follow-up in a28a077 is outside this review.
QA for 537d35e: Android 11 emulators, staging regtest; locally built from the pinned head. Recordings play at 2×.
Test 1: Payment review and PIN refresh deferral ✅ The original request survived confirmation/PIN holds, cancellation and backgrounding, then paid once.
Held the 1,000-sat on-chain confirmation and payment PIN for 30 seconds each. Cancelling added no outgoing activity. A second 2,000-sat request arrived without restarting; reopening the original preserved its amount and Savings selection. Authorizing it produced one 1,141-sat outgoing activity including fee; the requester showed Proof submitted. The second request remained unpaid during this case.
review-refresh-final.2x.mp4
Test 2: Payment Request success dismissal ✅ Closing success preserved proof delivery and did not reopen the payment sheet.
Paid a 6,007-sat Lightning request, closed success and restarted the payer. One 6,008-sat outgoing activity remained, including fee; the requester showed Proof submitted. Opened a fresh 5,000-sat request with Spending selected and closed it without paying.
success-dismissal.2x.mp4
Test 3: Staging payment through merchant unlock ✅ Paying and dismissing success completed merchant verification and opened the protected content.
Paid the merchant's 7,013-sat on-chain request. One 7,154-sat outgoing activity remained, including fee. The merchant completed verification, issued access and read the protected 43-byte content. This used disposable merchant services on staging regtest because the deployed staging Locks endpoint rejected paid unlocks with HTTP 451.
App logs recorded proof enqueue 6.45 seconds after the send amount was logged: SDK queue wait 14 microseconds and work 3.09 seconds. Background outbound work finished at 09:11:20 UTC; merchant logs recorded completed verification at 09:11:36 UTC after regtest confirmation. The video aligns the app and merchant reader.
staging-unlock-final.2x.mp4
Test 4: Proof, request, SDK and send-flow regressions ✅ All four affected unit-test classes passed.
Ran PaykitPaymentProofRepoTest, PaykitPaymentRequestRepoTest, PaykitSdkServiceTest and AppViewModelSendFlowTest at 537d35e. They cover durable proof handoff, admission deferral, nested review holds, contention retry/cancellation and final authorization. Checked the Gradle result and XML reports; this is automated coverage, with no device recording.
Test 5: Recipient transport isolation and retry ⏭️ Unrun: the PR was pushed before the controlled overlap case began.
The controlled peers and local build were prepared. The head-moved signal arrived during Test 3, so this case remains unproven at 537d35e, as it was in the previous review.
Test 6: Pending payment recovery authorization refresh ⏭️ Unrun: the staging attempt did not reach pending recovery before the PR was pushed.
The attempted connection-loss setup completed its 2,000-sat payment normally. It did not exercise recovery PIN or pending-screen authorization. The video shows that setup attempt, not recovery authorization. No device verification is claimed for the existing recovery finding.
recovery-refresh.2x.mp4
Reviewed the contiguous delta 449f512..537d35e and its reached callers, including the existing review thread. USDT review, hardware signing and biometric recovery were not driven on a device. The PR moved to a28a077 during testing; these results cover 537d35e only.
Twin: synonymdev/bitkit-ios#1025 fix: deliver Paykit payment proofs promptly, compared at 2142997, matches the proof-delivery and review-deferral approach; its shared PIN route also covers recovery.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
jvsena42
left a comment
There was a problem hiding this comment.
One MEDIUM inline: a contact link opened during a payment review fails, and the deferred contacts load isn't retried. No fund impact. The branch was rebased, so I reviewed the full diff against b2acca1 with a focus on what's new. The debug log volume I noted last time is behind BuildConfig.DEBUG (aa41e4b).
Checked and clean, reviewed as a funds change:
- Delivery: the launched delivery takes
operationMutex, submits only a proof still inloadProofs(), andsubmitReadyre-checks identity and capability.- The
awaitDelivery = falsecallers (PaykitPaymentProofRepo.kt:351, 676, 970) all hold the mutex. - A failed persist falls back to synchronous delivery.
- The
- Inline claim:
requiresAcceptanceexcludes billing periods, so the subscription paid-period check stays.- A versioned list still claims before consumption.
accept()runs before dispatch.
- Backup:
backupSnapshotruns underpersistenceMutex, and thebackupStateVersionrecheck catches a snapshot taken between enqueue and removal. - Cancellation:
withPaymentReviewreleases infinally.retryContendedReadusesrunSuspendCatching.checkRefreshAdmissionthrows before any SDK call.
- Cross-identity: transport passes
expectedIdentity = proof.identity, checked inside the SDK lock. - Other reviewers' threads: @piotr-iohk's RecoveryPinCheck / Pending hold LOW is fixed in
a28a077(SendSheet.kt:642-644, SendPendingScreen.kt:574-580).
Perf-relevant: proof transport re-timed to fire-and-forget, Background refresh deferred during review, contended reads retried. I'll measure on the device gate after the fix.
Device gate: waits for the fix.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of a28a077 since 537d35e, using the inherited baseline review. This pass inspected async proof admission priority, the recovery-review hold for fee entry, prepared-fee and warning approval, abandon confirmation, biometrics, and recovery PIN, and the shared lifecycle helper used by confirmation, payment PIN, and hardware signing. Inherited coverage remains proof persistence, per-recipient background transport, and post-restart retention; inline acceptance and final pre-dispatch authorization; USDT and submission refresh deferral; identity and linked-peer retry; the unchanged payment-request tests and journeys; and the prior shared-flow comparison with bitkit-ios pull 1025 at 2142997f.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
Completed Lightning and default-wallet on-chain proofs that are queued without waiting now take interactive admission and can pass queued background work, while an ordered sign-out barrier still runs first. Restart reconciliation, hardware proofs, and the immediate submit used when persistence fails stay ordered. Recovery UI holds passive refresh while those dialogs or the recovery PIN are visible, including across resume, and releases it when the review is dismissed or the app is stopped. That split matches bitkit-ios a95f85a for this delta.
Validation: the changed unit tests and send journeys were inspected, not executed. Device testing: not performed in this review. The iOS comparison is source-only.
Earlier findings reconciled at this revision: 1 still present, 1 verified fixed.
Findings
- [MEDIUM] Contact link opened during payment review fails as a decoding error — inline at
app/src/main/java/to/bitkit/repositories/PubkyRepo.kt:1245. Still present from an earlier unpublished review.
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — system Back leaves recovery authorization active after the PIN screen closes.
Findings:
- MEDIUM — Release recovery review when system Back cancels PIN (
app/src/main/java/to/bitkit/ui/screens/wallets/send/SendPendingScreen.kt:248). After fee approval, choose Use PIN code on the recovery biometric prompt, then press Android system Back. The PIN destination pops without its cancellation result, leavingrecoveryApproval=Pin. The new review condition then keeps passive refresh deferred on idle Pending; retry and abandon also remain disabled. Route system Back through the same cancellation callback as header Back. Test 7 records the failure; logs showed the hold persisting for over a minute. - The existing MEDIUM contact-link decoding error remains at
PubkyRepo.kt:1244. I have not duplicated that finding.
QA for a28a077:
Test 1: recovery fee and PIN cancellation ✅ The original survived cancellation and foreground re-entry; fresh approval completed one payment.
A debug-only pause immediately before native broadcast created the unresolved 3,003-sat payment. I cancelled fee entry and exact-fee approval, and used header Back to cancel recovery PIN after waits, rejected an invalid fee rate, and reopened after backgrounding. The video shows the retained Pending payment and final completion. Logs confirmed refresh deferral during review and admission while idle; an independent transaction lookup confirmed the original funding input, 3,003-sat recipient output and 141-sat fee. The requester showed Proof submitted.
recovery-refresh.2x.mp4
Test 2: recipient transport isolation and retry ⏭️ Retry recovered, but completed-proof delivery while another peer was held remains unverified.
Two established local peers and a prepared recipient request were used. Proxy records confirmed repeated 503 responses and a held PUT on only the unrelated peer's private stream. After foreground re-entry the held SDK work prevented the request list from loading, so I never reached the required proof-only overlap. Releasing the fault restored the list; peer inspection confirmed the previously failed rejection was delivered. A later request completed after release, as shown in the video. This is partial retry evidence, not an isolation pass.
recipient-isolation.2x.mp4
Test 3: payment review and PIN refresh deferral ✅ Waits, cancellation and app re-entry preserved the request; final approval paid it once.
On the plain build, I held the 1,000-sat Savings confirmation and PIN across refresh intervals, cancelled, received another request without restarting, then reopened and paid the original. The video shows the later request, retained original and requester Proof submitted. Activity inspection showed one 1,141-sat outgoing including the fee; the later 2,000-sat request remained unpaid.
review-refresh.2x.mp4
Test 4: Lightning success dismissal ✅ Closing success delivered proof; restarting and dismissing a later confirmation did not pay again.
The plain build paid the 6,007-sat request from Spending. The video shows success closing, requester Proof submitted, restart and a later unpaid 5,000-sat confirmation closing. Activity and balance inspection confirmed the same single 6,008-sat outgoing including fee.
success-dismissal.2x.mp4
Test 5: staging merchant unlock ✅ Checkout completed and issued readable access after success was closed.
The plain build paid 7,013 sats on-chain. The video shows the app payment and merchant completion/access issuance; browser DOM inspection confirmed the guarded read returned 45 bytes. Activity showed one 7,154-sat outgoing including fee. The staging merchant fixture used the matching Locks demo because the deployed proof-bundle endpoint returned HTTP 451.
Logs measured proof enqueue at 5.303 s queue / 3.404 s work, and targeted background outbound delivery at 1.887 s queue / 5.451 s work. Merchant verification completed before proof enqueue, so this sample establishes checkout correctness, not a proof-delivery speedup or a before/after improvement.
staging-unlock.2x.mp4
Test 6: automated regressions ✅ The focused proof, request, SDK, lifecycle, navigation and recovery suites passed.
Checked Gradle completion and JUnit results for admission priority, cancellation, recovery review holds, lifecycle cleanup and proof persistence behavior at the pinned head. No device media applies.
Test 7: recovery abandon and biometric cancellation ❌ System Back from recovery PIN left authorization active and recovery controls disabled.
A real pre-broadcast interruption retained the 5,005-sat original. After its deadline, cancelling the abandon confirmation and the 422-sat release-fee approval preserved it. Biometric fallback followed by header Back restored the controls; refresh-admission logs returned to idle.
Using Android system Back from the retry's recovery PIN instead returned to Pending with disabled controls, and logs reported an active review hold for over a minute. I then repeated the system-Back failure on the plain APK through abandon's release-fee approval and biometric fallback; Pending remained on “Checking the network” with disabled recovery controls. The video shows both paths. An independent Bitcoin RPC lookup confirmed the original input was still unspent; no replacement was sent.
recovery-abandon.2x.mp4
I reviewed the contiguous delta 537d35e..a28a077 and its recovery, proof and refresh callers. Recipient proof delivery under an already-held unrelated transport remains undriven for the reason in Test 2. Hardware signing and proof persistence fallback were checked in source and automated regressions, without a hardware device journey. I did not carry forward earlier unproved device cases as passes.
Twin: synonymdev/bitkit-ios#1025 matches the recovery-review protection intent; its deferred contact loading also retries, unlike the remaining Android contact-link failure.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of 6d09d83 since a28a077 on pull 1591. Base 42b1de36abd73265bad36755f2fea70488faa977 and merge base b2acca1989470829148bb88a2c36bb84022ac11d match the inherited baseline previous QA review. This pass covered that one commit: a deferred contact load retries instead of completing, contact Pay no longer joins a non-cancellable eligibility read after two seconds, and automatic proof reconciliation waits until payment submission is idle. USDT review deferral was rechecked and is unchanged. Proof persistence, inline acceptance, the identity read retry, and the earlier payment-request tests and journeys stay on that baseline. Shared-flow comparison used bitkit-ios#1025 at a50cf69.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
A core-queue OperationInProgress no longer marks the contact load complete. The load retries after 500ms, does not hold the SDK lock during that wait, and stops on cancellation or a sign-in change. Pay keeps the two-second eligibility bound when the native read cannot be cancelled; a target that is not ready falls through to payment, and a late read does not open the request sheet by itself. Automatic full refresh that reconciles proofs captures the sign-in, waits until submission is idle, and skips reconciliation if that sign-in, Paykit, the wallet, polling, or profile deletion no longer holds. Explicit recovery that is not limited to active polling still reconciles without that wait. iOS automatic full refresh at a50cf69 uses the same idle-then-recheck shape.
Validation was source and test inspection at the pinned revision. The new cases in PubkyRepoTest, ContactDetailViewModelTest, and AppViewModelSendFlowTest describe the retry, timeout, polling cancellation, and replaced-session paths; they were not executed here. Device testing: not performed in this review.
Non-blocking consistency suggestions
- iOS
ContactsManager.loadContactsrethrowsoperationInProgress. Android now retries that deferred observation until the hold ends, so the same in-flight contact refresh can fail on iOS and complete on Android. - iOS
journeys/send/prepared-fee-recovery.mdadds recovery case 4 for Android biometric Use PIN code, system Back, and header Back. The Android note updated here does not include that step.
Suggested additional test cases
- Android, signed-in wallet with saved contacts. Trigger a contact refresh, then keep a payment review open so the SDK defers background reads. Leave the review. Expected: that same refresh updates the contact list, and contacts are not left loading.
- Android, saved contact, with the payment-request eligibility read already inside the SDK and unable to cancel. Tap Pay. Expected: within about two seconds the app leaves the contact loading state for payment or a pay error, and the request-or-pay sheet appears only when a target was already known.
Earlier findings reconciled at this revision: 1 still present, 1 verified fixed.
Findings
- [MEDIUM] System Back from recovery PIN leaves the review hold and recovery controls stuck — inline at
app/src/main/java/to/bitkit/ui/screens/wallets/send/SendPendingScreen.kt:248. Still present from an earlier unpublished review.
jvsena42
left a comment
There was a problem hiding this comment.
Delta at 548380e: the MEDIUM is fixed (trace on the thread), and there are no new issues. Reviewed as a funds change.
6d09d83:PubkyRepoTestnow holdsbeginPaymentReviewthrough the realPaykitSdkService/PubkyService. It assertsobserveStateis never called, the SDK lock stays free during the retry, and the load succeeds afterendReview. Cancellation and sign-out are covered too.- Reconcile wait (
AppViewModel.kt:1133-1143): automatic FULL refreshes wait for the submission to end, then re-check sign-in, Paykit, wallet, polling and profile deletion. The work is deferred rather than dropped, and INBOX polls aren't gated. All three paths are tested inAppViewModelSendFlowTest. retainedHardwareOnchainRequestreturns early only when the existing filter would find nothing. It skips anidentityStatus()read; no proof write path changed.- System Back on PIN (
SendPinCheckScreen.kt:52) now runs the header-Back callback, which clearsrecoveryApprovaland releases the hold. That fixes @ovitrif's and @piotr-iohk's threads.
Perf-relevant: a 500 ms retry of the observeState admission check while a hold is up, and the FULL reconcile gated on the submission.
Device gate (partial): 548380e — not run yet. Pending:
review-refresh.xml,success-dismissal.xmlandonchain-original-payment-retry.xml(including system Back on PIN), cross-device with synonymdev/bitkit-ios#1025;- a contact link opened during a review;
- proof-delivery timing against master.
Device results will follow in a separate review.
Adopt published Paykit 0.1.1-rc1 and Core 0.8.0-rc10. Combine adjacent identity and request reads for proof preparation and submission, and read drain scheduling state in one SDK operation. Preserve acquisition-only retry, cancellation, identity and exact-proof guards, with actual service and scheduling regression coverage.
548380e to
5ced847
Compare
|
Updated to published Paykit Canonical compile, all 4,414 JVM tests and canonical lint pass. Detekt and Android Lint report zero findings without a baseline; all 12 PR commits are verified. I re-requested review. The earlier diagnostic-device measurements remain separate from this official-package update. Repeated native payment/recovery testing is continuing; I’m not claiming a fixed latency or a completed final device pass yet. The prior reviewed head remains available at |
jvsena42
left a comment
There was a problem hiding this comment.
Delta at 5ced847: no HIGH/MEDIUM. One LOW (parity) is inline. The rebase changed only import context (range-diff: all = except an import in SendPendingScreen.kt).
Checked and clean, reviewed as a funds change:
5ced847:prepareandsubmitReadyeach do one combinedreadPaymentRequests(counterparty, expectedIdentity)read instead of two, at the same Ordered priority. The capability, identity, already-paid and exact-proof checks are kept. Review holds don't defer it, and the retry fires only onSharedStateBusy, which the SDK documents as safe.- The scheduling snapshot keeps its
priority()invalidation, generation check and retry intent (tested). - Paykit
0.1.0→0.1.1-rc1(upstream #201-#203): 8 commits. They wrap claim, claim-and-accept and submit in one storage operation and add the two read APIs, all additive at the FFI level.paykit-lib, storage, backup and domain code are unchanged, so there's no persisted-state or wire change against 0.1.0 in either direction, and no double-pay or lost-proof path. One side effect is longerSharedStateBusywindows, because claim now fetches the counterparty registry while holding the write lock. That's responsiveness only. - bitkit-core rc9 → rc10: only upstream #177, which runs the USDT send preflight reads concurrently. The guards are unchanged, and only which error shows first can differ (InsufficientBalance vs QuoteExpired).
Device gate (partial): 5ced847 — not run yet at this head. The device results from 548380e stand for system Back on PIN and for a contact link during review. Pending at this head: success-dismissal.xml, onchain-original-payment-retry.xml, review-refresh.xml and proof timing against master, cross-device with the iOS twin. Device results will follow in a separate review.
|
Device gate (partial):
The admission path from the app log: There's one Still running: |
|
Device gate (partial 2): a contact link opened during a payment review at
No decoding error is logged, and the screenshots show no error toast. uiautomator can't see toasts, so that part rests on the log and the screenshots. Still running: |
|
Device gate (partial 3):
Still running: |
|
Device gate (partial 4): 15/15 passed. Step 7 used 1 sat/vB, because the original spends two inputs exactly, with no change.
Backend check: Proof: broadcast 19:12:38.3 →
Still running: timing against an Android master build. |
There was a problem hiding this comment.
Verdict: 💬 Comment — 5ced847 passed review and QA; the newer 766f4a8 is outside this review.
The PR moved to 766f4a8 after this run; these results are for 5ced847.
Findings: None at 5ced847.
QA for 5ced847:
Test 1: payment review and PIN refresh deferral ✅ Waiting, cancelling and reopening preserved the original request, which completed once.
The plain build held the 1,000-sat confirmation and PIN across refresh intervals, cancelled without paying, received a later 2,000-sat request without restart, and reopened the original after backgrounding. Final approval produced one 1,141-sat outgoing; the requester showed Proof submitted. Backend transaction inspection confirmed the 1,000-sat output and 141-sat fee. The later request stayed unpaid.
J1-2x.2x.mp4
Test 2: Lightning success dismissal ✅ Closing success delivered proof; restart and dismissal of a later confirmation caused no extra payment.
The plain build paid 6,007 sats from Spending. The video shows success closing, requester Proof submitted, restart and a later unpaid 5,000-sat confirmation closing. Activity and balance inspection confirmed one 6,008-sat outgoing including fee and no send-error toast.
J2-final-2x.2x.mp4
Test 3: original on-chain retry ✅ Cancellation retained the original, and fresh approval completed one payment.
A disclosed debug pause followed by process termination immediately before real native broadcast created the unresolved payment; no native result was fabricated. Fee entry, exact-fee approval and recovery PIN remained open across 30-second waits. Invalid 1000 sat/vB disabled Continue. Fee cancellation, Android system Back, header Back and background/reopen retained the original. Admission logs confirmed the review hold cleared on cancellation. Fresh approval paid the original 30,003 sats once; backend inspection confirmed its funding outpoint and 141-sat fee, and the requester showed Proof submitted.
J3-2x.2x.mp4
Test 4: abandon and biometric cancellation ✅ System and header Back restored recovery controls without spending the original input.
With emulator fingerprint enrollment and the same disclosed pre-broadcast interruption, the 5,005-sat original survived fee cancellation, biometric fallback to PIN, system Back and background/reopen. After the actual request deadline, I cancelled the abandon confirmation and its prepared 422-sat release fee. Both system and header Back after abandon's biometric fallback restored enabled Retry and Cancel expired payment actions. Admission logs returned to idle; Bitcoin RPC confirmed the original input remained unspent. No replacement or release was broadcast.
M1-2x.2x.mp4
Test 5: recipient isolation and durable delivery retry ✅ A failed recipient stream did not block another proof, and restart preserved the failed delivery.
The pristine local E2E build paid recipient A 1,101 sats while a native transport proxy returned HTTP 503 only for that recipient's encrypted outbound stream. Recipient inspection showed no proof. Recipient B then reached ProofSubmitted for 1,202 sats while A's failure rule remained active. After payer restart, Activity still held exactly two outgoing payments and the same balance; the proxy recorded another A failure. Restoring only A's stream delivered its original proof without another payment. Recipient SDK records and Bitcoin RPC verified both proofs, transactions and 141-sat fees.
M2-2x.2x.mp4
Test 6: merchant payment through unlock ✅ One payment completed merchant verification, issued access and read the expected guarded file.
The plain build paid 7,013 sats through the repository Locks/Paykit merchant flow using real staging Pubky identities. Backend inspection confirmed one payment and a 141-sat fee; browser inspection confirmed the expected 45-byte file. The second clip shows completed verification, issued access and the actual content.
Debug traces separated proof enqueue (16 µs queue, 1.713 s work) from recipient transport (0.269 s queue, 8.761 s work). Merchant verification completed 19.8 s after enqueue, approximately 11 s after transport finished. This is one correctness and latency sample, without a before/after speed claim. The deployed Shop UI was not exercised.
M3-2x.2x.mp4
M3-unlock-2x.2x.mp4
Test 7: deferred saved-contact link ✅ Closing confirmation resumed the waiting link and opened the saved contact.
On the plain build, a contact link opened while the 5,000-sat confirmation remained visible. Closing confirmation within the existing bounded link wait opened the correct saved contact with working Request Or Pay. Screen and log inspection found no decoding error or payment. The automated adapter tests separately covered wrapped deferral, cancellation and sign-out.
D1-2x.2x.mp4
Test 8: automated regressions ✅ The focused proof, request, SDK, scheduling, contact, lifecycle, PIN Back and navigation suites passed.
Gradle completion and JUnit XML confirmed no failures, errors or skips. These checks include combined-read authorization guards, admission priority, cancellation, recovery state and durable proof handling. No device media applies.
All planned cases passed at 5ced847. Hardware signing, storage-write failure and USDT variants were checked in source or automated coverage rather than separate device journeys. The newly pushed 766f4a8 was not reviewed or tested here.
Coverage: full fe2c28c..5ced847 review after the rebase, with focused review of 9055e1d..5ced847 and the prior open findings. The previous review supplied the recovery regression to retest; no earlier failed or undriven case was carried forward as a pass. The independent cross-platform device report also confirms original-input retry and recovery system Back at 5ced847.
Twin: synonymdev/bitkit-ios#1025 fix: deliver Paykit payment proofs promptly now pins the same Paykit 0.1.1-rc1 and Core 0.8.0-rc10 at bc2fb60 and describes matching fresh-read APIs. That closes the dependency split; its new-head device matrix is not established by this Android run.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
jvsena42
left a comment
There was a problem hiding this comment.
Delta at 766f4a8: no HIGH/MEDIUM. One LOW (log cause) is inline. The commit fixes a crash: before it, a contended currentIdentity() in completeOnchainPayment escaped viewModelScope.launch right after an accepted broadcast. Master has the same crash at PaykitPaymentProofRepo.kt:596.
Checked and clean, reviewed as a funds change:
- No double pay: the proof persisted with
paymentStarted = truebefore broadcast stays untouched (the repo test asserts it's unchanged). A second Pay is refused inprepareby therequestIdandisStartedForguards.failOnchainPayment,cancelPreparationandreleasePrivatePaymentListare never called. - The proof still completes:
reconcile()runs on every FULL refresh and on backup restore, and the new test shows exactly onesubmitPaymentProofand one shop followup. A kill between payment and proof leaves the same durable state, which reconciles on the next launch. - Cancellation:
runSuspendCatchingrethrows, so the proof stays for reconcile. - Parity: the parity LOW is closed. synonymdev/bitkit-ios#1025 now pins Paykit
0.1.1-rc1and Core0.8.0-rc10, the same as here. iOS already returns false instead of throwing here (PaykitPaymentProofService.swift:1137), so there's nothing to port.
The device results from 5ced847 stand, because this commit changes only the failure path:
review-refresh.xml17/17;success-dismissal.xml13/13;onchain-original-payment-retry.xml15/15;- the contact link during review;
- system Back on the recovery PIN.
Device gate (partial): 766f4a8 (journeys from 5ced847, success path unchanged) — pending: proof and admission timing against an Android master build. Device results will follow in a separate review.
jvsena42
left a comment
There was a problem hiding this comment.
Approving. My MEDIUM is fixed and verified on the device. The log-cause LOW from my last review stays open inline; it can be fixed here or in a follow-up, your choice.
Device gate: 766f4a8 (journeys run at 5ced847; 766f4a8 changes only the failure path) — journeys: 45 passed, 0 failed (review-refresh.xml 17/17, success-dismissal.xml 13/13, onchain-original-payment-retry.xml 15/15), plus a contact link during Confirm and PIN, and system Back on the recovery PIN; perf: broadcast → proof queued median 5.3 s (master 52.3 s), broadcast → requester paid median 49.8 s (master 86.6 s), one readPaymentRequests per submit; Figma: n/a (no new UI); l10n: no UI text changed; iOS: cross-device with synonymdev/bitkit-ios#1025 (Paykit 0.1.0 ↔ 0.1.1-rc1, no interop issue).
Ran on Pixel_9 (emulator), paired with the iOS simulator, PIN and "Require PIN for payments" on.
-
The on-chain retry used a real dropped broadcast: a local Electrum proxy swallowed
blockchain.transaction.broadcast.- The retry spent the same two inputs and paid exactly 3 000 sats with the approved 200-sat fee, and the requester was paid once.
- At 1000 sat/vB it shows "Enter a valid positive fee rate" with Confirm disabled.
- System Back on PIN keeps Pending and re-enables Retry, with no broadcast.
-
Proof timing, 3 runs each:
broadcast → Queued Queued → requester paid PR 4.8, 5.3, 5.8 s 39-47 s master 12.5, 52.3, 59.2 s 27-52 s -
Observed, not blocking: after the retry succeeded, the app logged
Activity already exists for txid 2385e7fa…plus anONCHAIN_SEND_ATTEMPTupsert and a wallet backup about once a minute for 19 minutes, until the transaction settled. It looks like reconcile of the retained attempt, and I haven't compared it with master. -
⚠️ Not run: the hardware-wallet variant of the retry journey, and theprepared-fee-recovery.mdcases that need history and persistence faults.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. The diagnostic LOW remains deferred to synonymdev/bitkit-android#1629 chore: clarify background proof failure diagnostics.
QA for 766f4a8:
Head: 766f4a8.
Test 1: accepted payment with unavailable proof identity ✅ Success survived the fault and restart; restoring availability delivered the original proof without another debit.
A disclosed debug-only hook made SDK identity lookup throw SharedStateBusy immediately after a real accepted 1,601-sat on-chain broadcast. The native payment outcome was unchanged. The app showed Bitcoin Sent and retained one started, incomplete proof. Closing success and restarting while the fault remained active preserved one 1,742-sat outgoing and a 198,258-sat balance.
Removing only the identity fault allowed ordinary reconciliation to deliver the original transaction proof. Requester SDK inspection showed no proof during the fault, then exactly one matching proof after recovery and another refresh. Bitcoin RPC independently confirmed the 1,601-sat recipient output and 141-sat fee. The recording shows the payer; the proof and transaction assertions were checked through the requester SDK and Bitcoin RPC. Video plays at 2×.
M4-2x.2x.mp4
Test 2: automated accepted-payment regressions ✅ The current-head proof and send-flow suites passed.
Ran the pristine head's PaykitPaymentProofRepoTest and AppViewModelSendFlowTest, including both new identity-contention cases. Gradle completion and JUnit XML confirmed no failures, errors or skips. These checks include original-proof retention, recreation, one submission across repeated reconciliation and accepted send success without another financial dispatch. No device media applies.
The following passes are retained from the 5ced847 review and recordings. The delta leaves their successful paths unchanged; they were not re-driven here.
Test 3: payment review and PIN refresh deferral ✅ Previously passed: waits, cancellation and reopening retained the original request, which paid once.
The original 1,000-sat request completed once; the later 2,000-sat request stayed unpaid. Evidence is in Test 1 of the linked report.
Test 4: Lightning success dismissal ✅ Previously passed: closing success preserved delivery, and restart caused no extra payment.
The 6,007-sat payment produced one outgoing activity; a later confirmation was dismissed unpaid. Evidence is in Test 2 of the linked report.
Test 5: deferred saved-contact link ✅ Previously passed: closing confirmation resumed the waiting link.
The correct saved contact opened with Request Or Pay and no payment or decoding error. Evidence is in Test 7 of the linked report.
Test 6: original on-chain retry ✅ Previously passed: cancellation retained the original, and fresh approval paid it once.
Fee/PIN waits, system and header Back, invalid fees and background/reopen were exercised. Evidence is in Test 3 of the linked report.
Test 7: merchant payment through unlock ✅ Previously passed: one payment completed verification and granted readable access.
The repository Locks/Paykit flow issued access and returned the expected guarded file. Evidence is in Test 6 of the linked report. The deployed Shop UI was not exercised.
Test 8: recipient isolation and durable delivery retry ✅ Previously passed: one recipient's failed transport did not prevent another proof, and restart preserved retry.
Restoring the failed recipient's stream delivered its original proof without another payment. Evidence is in Test 5 of the linked report.
Test 9: abandon and biometric cancellation ✅ Previously passed: system and header Back restored recovery controls without spending the original input.
The expired original survived fee and abandon cancellation, biometric fallback and background/reopen. Evidence is in Test 4 of the linked report.
No planned case remains unrun. Hardware signing, storage-write failure and USDT variants retain source/automated coverage rather than separate device journeys. The independent cross-platform approval and timing report also covers the unchanged journeys and master comparison; no new performance claim is made here.
Reviewed the contiguous delta 5ced847..766f4a8 (three files) and its accepted-send, cancellation and durable reconciliation paths. The changed failure branch was driven in Test 1; subsequent subscription-refresh failure was inspected in source, without a separate device fault case.
Twin: synonymdev/bitkit-ios#1025 fix: deliver Paykit payment proofs promptly at bc2fb60 matches the dependency pair and recovery intent. Its completion method already returns false when identity lookup throws; no additional iOS change is needed for this failure case.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of bitkit-android#1591 at 766f4a89, because the comparison base changed since the saved assessment. This pass covers the full diff from merge base fe2c28c6. Inherited coverage is empty.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
An accepted on-chain payment stays successful when background proof completion throws, and the started local proof remains for reconciliation. Review holds cover confirm, PIN, hardware sign, recovery fee, abandon, and spend authentication, plus USDT quote review. They release on backgrounding, dismissal, and cancellation, and system Back uses the existing PIN cancellation path. Claim-and-accept checks the peer inside the Paykit 0.1.1-rc1 transaction (f384694), and the combined identity and request read matches that SDK snapshot. The shared proof, claim, and review behavior matches bitkit-ios#1025 at bc2fb605.
Validation: build and lint succeeded on this revision. The new unit tests were inspected and were not executed in this review. Device testing: not performed in this review. The device pass is the author's review-refresh, success-dismissal, and onchain-original-payment-retry journeys, plus the manual SDK, recovery, and transport cases. Ready for device testing.
Earlier findings reconciled at this revision: 1 still present, 2 verified fixed.
Findings
- [LOW] Background proof-failure log drops the throwable — inline at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:6134. Still present from an earlier unpublished review.
Description
0.1.1-rc1and Bitkit Core0.8.0-rc10. Read current identity and request records together when preparing/submitting proofs, and use a fresh private-message scheduling snapshot to avoid redundant shared-state acquisitions. Preserve fresh authorization checks and post-mutation validation.Related to #1407 and #1419. Authorization, quoted amounts and expiry, identity and billing-period isolation, durable recovery and duplicate-payment protection remain enforced.
Out of Scope
AppViewModel background proof completion: distinguish completion failures from later subscription-refresh failures in privacy-safe diagnostics: #1629, deferred from review under the repository LOW-finding policy. The repeated review comment is tracked in the same issue.
Deployed server/Locks latency and remaining end-to-end investigation: bug: paykit pay sheet and confirmation are slow to appear #1407 and Contact payment latency on staging with the shared Paykit runtime: pin paykit-rs#171 and re-measure #1419. No settlement or polling-policy changes.
Design
N/A — no UI changes.
Preview
N/A — no layout changes.
QA Notes
Journeys
review-refresh.xml— review and PIN cancellation, background/reopen and later request arrival preserve one-payment behavior.success-dismissal.xml— closing payment success preserves proof delivery and reopening does not send another payment.onchain-original-payment-retry.xml— fee and PIN waiting/cancellation retain the original Pending payment; explicit retry still needs fresh approval.Manual Tests
Automated Checks
LifecycleTest.kt— verifies review lifecycle release and reacquisition without holding idle Pending.PaykitPaymentProofRepoTest.kt— verifies retained accepted proofs across identity contention and repository recreation, with one submission across repeated reconciliation; also verifies durable native enqueue priority, reconciliation overlap, Ordered restart/hardware/failure paths, Background transport and existing identity, period and recovery guards.PaykitSdkServiceTest.kt— verifies actual service admission, default priority, sign-out barriers, bounded standalone-read retries and cancellation before admission and after native entry.SendPendingViewModelTest.kt,SendSheetNavigationTest.ktandSendPendingObservationDeviceTest.kt— cover recovery review prompts, routed PIN holds and idle Pending admission.SendPinCheckScreenTest.kt— exercises the real Compose PIN screen and Back dispatcher for ordinary and recovery routes, header cancellation and a later successful PIN. The regression fails without the Back handler. Recovery view-model tests cover cancellation followed by a single authorized retry or expired-payment release.AppViewModelSendFlowTest.ktandPaykitPaymentRequestRepoTest.kt— preserve accepted success and one financial send when background proof completion throws (the regression fails without the fix), and retain inline claim/accept, versioned consumption, nested review, fresh authority and retry coverage, including deferred reconciliation across cancellation and session replacement.PubkyRepoTest.kt— exercises wrapped deferral through the actual SDK/service adapters, retry without holding the SDK lock, cancellation and sign-out.ContactDetailViewModelTest.kt— verifies the eligibility timeout with an already-admitted non-cancellable native call.Final validation on the rebased branch with the published dependencies: canonical compile, all 4,416 JVM tests across 244 suites, and canonical lint pass. Full Detekt SARIF and Android Lint XML/SARIF contain zero findings, with no Lint baseline. Complete compiler/test diagnostics were inspected; remaining tooling notices were verified against the current base. A fresh independent source review found no actionable issue in the dependency and combined-read changes. The actual system-Back regression previously failed when its production fix was removed. Native device validation of this official dependency pair is still in progress; diagnostic-build timing samples are reported separately.
Disposable-wallet tests use Bitcoin regtest and real staging Pubky transport. Previous fixed BTC/USD payments, PIN authorization, interrupted proof-delivery recovery and merchant unlock checks succeeded. Those earlier samples do not establish the performance or runtime coverage of the final priority/recovery-review changes.
A native trace showed routine inbox work starting during PIN entry and blocking payment preparation. This change prevents new passive refresh work during that review. Already-running operations, explicit recovery and payment authority checks remain intact; it does not promise a fixed latency reduction.