Skip to content

fix: deliver completed Paykit proofs promptly - #1591

Open
ben-kaufman wants to merge 13 commits into
masterfrom
fix/paykit-proof-latency
Open

ben-kaufman wants to merge 13 commits into
masterfrom
fix/paykit-proof-latency

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Description

  • Adopt published Paykit 0.1.1-rc1 and Bitkit Core 0.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.
  • Keep accepted on-chain payments successful when background proof completion encounters a temporary SDK error; retain the durable proof for ordinary reconciliation and preserve cancellation.
  • Start completed Bitcoin proof submission from the application scope after durable persistence, so closing the payment screen does not interrupt the handoff.
  • Deliver through the intended recipient’s ordered queue and release proof-state serialization after durable SDK enqueue. Local snapshots wait only for local writes; recipient transport continues at background priority.
  • Claim new inline requests atomically during acceptance, avoiding a redundant shared-state operation. Keep early claims before versioned endpoint consumption, hardware receipt preparation and accepted-request retries.
  • Defer routine inbox and display refreshes while preparing and reviewing a foreground payment, during PIN entry and submission, checking again at SDK admission. Release review deferral on cancellation, dismissal or backgrounding. Preserve explicit recovery, fresh payment checks and retryable snapshots.
  • Retry standalone identity and peer-list reads once after a cancellable 250 ms delay when shared-state lock acquisition reports busy. Preserve the captured SDK identity and serialized operation; never retry payment writes or other errors.
  • Use the SDK’s atomic peer authorization for fresh proposal acceptance instead of an additional preflight read. Keep final pre-dispatch authorization and early endpoint-consumption checks.
  • Retry contact loads deferred by a payment hold without completing or dropping their waiting contact link; cancellation and identity changes stop the retry. Keep automatic proof reconciliation outside an active submission, and bound the optional contact-eligibility wait even when its native call cannot be cancelled.
  • Skip remote identity lookup when local proof storage has no signed hardware receipt for the requested payment. Positive matches still revalidate the current identity and reload the receipt; unreadable storage remains an error.
  • Route Android system Back through the existing PIN cancellation callback, restoring Pending recovery actions and releasing the review hold.
  • Add debug-only duration diagnostics for queue waits and SDK work without payment or contact contents.
  • Prioritize SDK enqueue after durable native Bitcoin/LN completion, while keeping passive recovery and persistence-failure fallbacks ordered and recipient transport in the background. Existing identity/cleanup barriers and bounded queue fairness still apply.
  • Protect Pending recovery fee, abandon and spend-authentication review from passive refresh. Release the hold on backgrounding, dismissal or cancellation; idle Pending remains available for recovery observation.

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

Design

N/A — no UI changes.

Preview

N/A — no layout changes.

QA Notes

Journeys

  • new review-refresh.xml — review and PIN cancellation, background/reopen and later request arrival preserve one-payment behavior.
  • updated success-dismissal.xml — closing payment success preserves proof delivery and reopening does not send another payment.
  • updated onchain-original-payment-retry.xml — fee and PIN waiting/cancellation retain the original Pending payment; explicit retry still needs fresh approval.

Manual Tests

  • Make SDK identity lookup unavailable after an accepted on-chain payment → keep the app open and payment successful, then restore availability → reconcile the original proof without another debit — SDK fault injection not in Capabilities.
  • Create a controlled unresolved original send → cancel fee, abandon and available biometric review, then background/reopen → retain the original payment without authorizing replacement or holding idle Pending — controlled native broadcast/receipt faults not in Capabilities.
  • Hold an unrelated recipient’s transport → pay an established recipient → its ordered proof delivery proceeds independently and failed delivery stays retryable — recipient transport fault injection not in Capabilities.
  • Collect a staging payment through unlock → distinguish local queue wait, SDK work and merchant completion — correlated staging latency capture not in Capabilities.

Automated Checks

  • added LifecycleTest.kt — verifies review lifecycle release and reacquisition without holding idle Pending.
  • updated 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.
  • updated PaykitSdkServiceTest.kt — verifies actual service admission, default priority, sign-out barriers, bounded standalone-read retries and cancellation before admission and after native entry.
  • updated SendPendingViewModelTest.kt, SendSheetNavigationTest.kt and SendPendingObservationDeviceTest.kt — cover recovery review prompts, routed PIN holds and idle Pending admission.
  • added 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.
  • updated AppViewModelSendFlowTest.kt and PaykitPaymentRequestRepoTest.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.
  • updated PubkyRepoTest.kt — exercises wrapped deferral through the actual SDK/service adapters, retry without holding the SDK lock, cancellation and sign-out.
  • updated 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.

@ovitrif ovitrif added this to the 3.1.0 milestone Oct 19, 2026
@ovitrif

ovitrif commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

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.

@greptile-apps

greptile-apps Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable regression was found.

Summary

Starts proof delivery after completed payment state is saved, without waiting for reconciliation or keeping the payment screen open.

  • Completed payment proofs start delivery in the background.
  • Paykit logs show where selected SDK calls spend time.

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
Loading

Reviews (1) · Last reviewed commit: "fix: deliver completed Paykit proofs pro..." · Reviewed by Greptile

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 766f4a8 (run).

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

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No findings. 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: every awaitDelivery = false caller holds the mutex, but the delivery is a separate coroutine on deliveryScope that 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()), and submitReady re-checks the identity, the capability and the record before writing.
  • Delivery under a switched identity: processOutboundPrivateMessages now receives expectedIdentity and 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 the Background deferral path; Interactive runs 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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 in loadProofs(), and submitReady re-checks capability and identity. The interactive flush checks expectedIdentity inside the SDK lock. No payment-sending path changed.
  • No skipped evidence gate. Lightning requires the preimage to match the hash; on-chain runs finishAcceptedShopActivity before 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.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

@jvsena42 The current head guards these diagnostics with BuildConfig.DEBUG, so they do not reach release logs. Recipient transport now runs outside the proof operation mutex, and backup snapshots serialize only with local proof writes. Tests separately suspend enqueue, transport and local persistence to check those boundaries.

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.

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 20, 2026 00:24

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: ⚠️ Not verified — recipient transport overlap remains unverified.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Reviewed the full PR diff against its merge base, at 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.

Comment thread app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt Outdated

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verdict: ⚠️ Not verified — recipient transport isolation and recovery authorization remain unverified at 537d35e.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 in loadProofs(), and submitReady re-checks identity and capability.
    • The awaitDelivery = false callers (PaykitPaymentProofRepo.kt:351, 676, 970) all hold the mutex.
    • A failed persist falls back to synchronous delivery.
  • Inline claim:
    • requiresAcceptance excludes billing periods, so the subscription paid-period check stays.
    • A versioned list still claims before consumption.
    • accept() runs before dispatch.
  • Backup: backupSnapshot runs under persistenceMutex, and the backupStateVersion recheck catches a snapshot taken between enqueue and removal.
  • Cancellation:
    • withPaymentReview releases in finally.
    • retryContendedRead uses runSuspendCatching.
    • checkRefreshAdmission throws 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.

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Outdated

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Follow-up 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.

Comment thread app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Outdated

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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, leaving recoveryApproval=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 piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Follow-up 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.loadContacts rethrows operationInProgress. 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.md adds 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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delta at 548380e: the MEDIUM is fixed (trace on the thread), and there are no new issues. Reviewed as a funds change.

  • 6d09d83: PubkyRepoTest now holds beginPaymentReview through the real PaykitSdkService / PubkyService. It asserts observeState is never called, the SDK lock stays free during the retry, and the load succeeds after endReview. 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 in AppViewModelSendFlowTest.
  • retainedHardwareOnchainRequest returns early only when the existing filter would find nothing. It skips an identityStatus() read; no proof write path changed.
  • System Back on PIN (SendPinCheckScreen.kt:52) now runs the header-Back callback, which clears recoveryApproval and 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.xml and onchain-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.
@ben-kaufman
ben-kaufman force-pushed the fix/paykit-proof-latency branch from 548380e to 5ced847 Compare October 20, 2026 17:27
@ben-kaufman
ben-kaufman requested a review from jvsena42 October 20, 2026 17:29
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Updated to published Paykit 0.1.1-rc1 and Core 0.8.0-rc10, and rebased onto current master. Proof preparation/submission now read identity and request records together; message scheduling uses one fresh SDK snapshot. Existing authorization, exact-proof, cancellation and durable-delivery checks remain in place.

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 548380e7 for comparison.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delta 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: prepare and submitReady each do one combined readPaymentRequests(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 on SharedStateBusy, 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 longer SharedStateBusy windows, 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.

Comment thread gradle/libs.versions.toml
@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): review-refresh.xml at 5ced847 on Pixel_9 (emulator), PIN and "Require PIN for payments" on. Android is the payer and iOS synonymdev/bitkit-ios#1025 at 4022aa7 is the requester (Paykit 0.1.1-rc1 ↔ 0.1.0). 17/17 passed.

  • The requested amount stays ₿1 000 with a ₿141 fee across 30 s idle, PIN, system Back and close.
  • A second request arrives without a restart.
  • After Home, relaunch and PIN, the confirmation is still ₿1 000 and swipeable. There's no stuck spinner on Android.
  • After paying there's exactly one outgoing activity, the requester row shows +₿1 000, and the 2 000 request stays pending on both sides.
  • No interop issue between the SDK versions.

The admission path from the app log:

18:46:22    PIN entered
18:46:50.2  claimPaymentRequestForExecution finished (queue 3.3 s, work 5.5 s)
18:46:58.9  acceptPaymentRequest finished
18:47:28.1  send activity created (txid 9d375a4d…bd2bc)
18:47:30.3  readPaymentRequests (1 call)
18:47:32.8  submitPaymentProof (work 2.5 s) → Queued a Paykit payment proof
18:47:55.4  processOutboundPrivateMessages finished (work 17.0 s)

There's one readPaymentRequests per submit, as 5ced847 intends. A 29 s gap between accept and broadcast is filled by allPaymentRequests (queue 19.9 s) and seven back-to-back identityStatus reads (≈1.07 s each). It's unverified whether master behaves the same; a master baseline will settle it.

Still running: success-dismissal.xml, onchain-original-payment-retry.xml (including system Back on PIN), and proof and admission timing against an Android master build.

@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial 2): a contact link opened during a payment review at 5ced847, which is my earlier MEDIUM, re-checked on the device. Passes.

open when the link fires result
Confirm (adb … bitkit://contact?pubky=… at 18:50:31) The sheet stays on Confirm. After Back (18:50:38) the Contact screen opens.
PIN (link at 18:54:23) PIN stays. Back → Confirm → Back, and the Contact screen opens by 18:54:37.
18:50:30.176 Received deeplink 'bitkit://contact'
18:50:30.179 Starting scan from 'deeplink'

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: success-dismissal.xml, onchain-original-payment-retry.xml (including system Back on the recovery PIN), and timing against master.

@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial 3): success-dismissal.xml at 5ced847. Android pays an iOS synonymdev/bitkit-ios#1025 requester over Lightning. 13/13 passed.

  • The 6 007 request reaches SendSuccess. Close dismisses it with no toast, per the screen recording.
  • There is exactly one outgoing activity (−6 008), and the iOS row turned paid at 18:58:41.
  • Force-stop and relaunch add nothing new (Spending stays 149 998).
  • A second request dismissed with system Back isn't paid and shows no toast.
  • Proof (Lightning): PaymentSuccessful 18:57:10.4 → readPaymentRequests 18:57:26.5 (queue 14.6 s) → Queued 18:57:31.5 → processOutboundPrivateMessages finished 18:57:43.2 (work 9.4 s) → requester paid by 18:58:41. That's one readPaymentRequests per submit.

Still running: onchain-original-payment-retry.xml (including system Back on the recovery PIN) and timing against an Android master build.

@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial 4): onchain-original-payment-retry.xml at 5ced847, including system Back on the recovery PIN. Android pays an iOS-issued 3 000 sat on-chain request, with a local Electrum proxy dropping blockchain.transaction.broadcast so the original never reaches the node:

19:07:36.285 ERROR [ldk_node::chain::electrum:1051] Failed to broadcast transaction 946d33c7…62c8: … EOF while parsing a value at line 1 column 0
19:07:36.287 INFO  [ldk_node::chain::electrum:1012] Retrying broadcast of 946d33c7…62c8 on a new connection
19:07:44.242 ERROR [ldk_node::chain::electrum:1078] Failed to broadcast transaction due to timeout 946d33c7…62c8: deadline has elapsed

15/15 passed. Step 7 used 1 sat/vB, because the original spends two inputs exactly, with no change.

  • Pending shows "outcome is unresolved", and the retry dialog prefills the original rate.
  • At 1000 sat/vB it shows "Enter a valid positive fee rate in sat/vB." and Confirm is disabled.
  • 5 sat/vB is refused with "could not be retried… funds remain protected" (Insufficient funds: 0.00003200 BTC available of 0.00003888 BTC needed).
  • The approval shows ₿200. Authorize goes to PIN with no broadcast, and 30 s on PIN still broadcasts nothing.
  • System Back on the recovery PIN (548380e): Payment Pending with the same txid, Retry original payment enabled again, no broadcast.
  • The final retry broadcasts at 19:12:38.3, and "Bitcoin Sent" follows.

Backend check: 2385e7fa… spends the same two original inputs (d1811597…:1, 70f5f428…:1) and pays exactly 3 000 sats, with the approved 200-sat fee. The original never reached the node. The requester row shows +₿3 000 once.

Proof: broadcast 19:12:38.3 → readPaymentRequests 19:12:43.4 (one call) → Queued 19:12:50.9 → processOutboundPrivateMessages 19:13:03.8.

⚠️ Not run:

  • the hardware-wallet variant;
  • prepared-fee-recovery.md cases 1-4, which need history and persistence faults;
  • a direct observation of the review hold on PIN. The log shows identityStatus / allPaymentRequests reads during PIN, and those are allowed by design.

Still running: timing against an Android master build.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delta 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 = true before broadcast stays untouched (the repo test asserts it's unchanged). A second Pay is refused in prepare by the requestId and isStartedFor guards. failOnchainPayment, cancelPreparation and releasePrivatePaymentList are never called.
  • The proof still completes: reconcile() runs on every FULL refresh and on backup restore, and the new test shows exactly one submitPaymentProof and one shop followup. A kill between payment and proof leaves the same durable state, which reconciles on the next launch.
  • Cancellation: runSuspendCatching rethrows, so the proof stays for reconcile.
  • Parity: the parity LOW is closed. synonymdev/bitkit-ios#1025 now pins Paykit 0.1.1-rc1 and Core 0.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.xml 17/17;
  • success-dismissal.xml 13/13;
  • onchain-original-payment-retry.xml 15/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.

Comment thread app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving. 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 an ONCHAIN_SEND_ATTEMPT upsert 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 the prepared-fee-recovery.md cases that need history and persistence faults.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

utAck

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: 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.

Comment thread app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants