Skip to content

fix: defer automatic paykit work during payments - #1054

Open
ben-kaufman wants to merge 21 commits into
masterfrom
fix/paykit-background-publication
Open

ben-kaufman wants to merge 21 commits into
masterfrom
fix/paykit-background-publication

Conversation

@ben-kaufman

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

Copy link
Copy Markdown
Contributor

Related to contact payment latency #868.

This PR defers automatic Paykit contact preparation and private endpoint publication while a payment is being reviewed or submitted, then resumes the retained work safely. Backup observation startup and shutdown also serialize subscription storage.

Stacked on: app-lock recovery #1039, above payment latency #1025.

Description

  • Check payment activity when routine identity, peer and scheduling reads enter the SDK, after preparatory suspension, and before their existing bounded contention retry.
  • Defer automatic private endpoint publication before mutation while preserving explicit publication, withdrawals and recovery.
  • Retain deferred contact preparation, including forced refresh and link-only preparation, until payment review ends and the app is active; revalidate identity, session, generation and saved contacts before continuing.
  • Release publication and link coordination locks before waiting, and retain retry intent without advancing network backoff for payment admission deferral.
  • Wake parked Background retries when review ends or another contact’s Interactive retry becomes due, preserving ordinary transport-failure backoff.

Out of Scope

  • SDK transport and remote lease contention: #868. This change does not preempt admitted operations or alter SDK retries, payment authority or settlement.

Design

N/A — no UI changes.

Preview

N/A — no layout changes.

QA Notes

Deterministic tests exercise the background-admission and lifecycle races; no measured app latency improvement is claimed.

Setup

Use the journey guide and Payment Request setup at a161084d with disposable regtest wallets and a saved, linked fixture issuer. This source uses Paykit 0.1.1-rc1 and Core 0.8.0-rc10. Keep ordinary payment authorization enabled. The controlled transport/admission faults named below require additional fixture control; compatible issuer/homeserver access and staging merchant/Locks unlock must be established separately. Missing prerequisites leave the affected cases blocked.

Journeys

Manual Tests

  • 1. regression: defer automatic contact publication during payment review → background the app, end review, then return to the foreground → pending forced preparation resumes once with current identity and contact membership — deterministic SDK admission-race injection not in Capabilities.
  • 2. Defer automatic publication → explicitly disable contact payments or remove the contact → cleanup completes and deferred work does not republish it — deterministic publication-lock and admission-race injection not in Capabilities.

Automated Checks

  • added PaykitSdkIdentityCheckTests.swift cases — payment starting during the existing contention delay prevents a second Background SDK read.
  • added PaykitSdkOperationLockTests.swift cases — explicit publication and withdrawal remain available during payment activity.
  • added PrivatePaykitServiceTests.swift cases — the actual preparation owner retains deferred work without holding publication locks, waits for foreground in either lifecycle callback order, rejects stale ownership or removed contacts, and retries identity deferral when publication is unavailable. Additional cases preserve backoff through pre/post-write admission deferral and wake another contact’s Interactive retry at its deadline.
  • updated PaykitSdkOperationLockTests.swift — queued reads and automatic publication recheck admission after preparatory work without dirtying backup state.
  • updated PrivatePaykitServiceTests.swift — identity, linking and scheduling deferral preserve retry intent before missing-peer pruning; owner-level cases cover recovery, session changes, removed contacts, cancellation and ordinary missing-app errors.

Evidence

Final integration validation

Integration with master 4530ff7b at a161084d retains both USDT-warning cancellation and the payment-review callback. Bottom #1025: all-target build passed (19 targets), 378 focused tests passed, 0 failures/skips. One combined #1070 + #1081 + #1084 + #1085 composition, tree 8f1e72d3 from #1070 7cd096f4 plus #1081 8967dff8, #1084 b7561c08 and #1085 310e689a, covers the stack and those sibling changes: all-target build passed (19 targets) and 2,779 tests passed, 0 failures/skips using the six existing CI exclusions. These are not eight independent full-suite runs.

All eight heads passed SwiftFormat. Translation checks report 0 errors and 15 warnings, all matching master by key, language, file and occurrence count, including security__use_pin. Final toolchain/runtime and complete diagnostic disposition: Xcode 26.1 / iOS 27.0 simulator, serial tests; complete diagnostics inspected, with six SDK-construction QoS warnings and retained fixture/appearance messages matching baseline evidence; no introduced finding. Final-head CI after publication: pending.

Gaps

Earlier diagnostics included appearance warnings reproduced in earlier validation but not the immediate baseline, and simulator launch-telemetry messages not reproduced on either baseline. These remain disclosed historical findings, not verified baseline classifications for the final run.

The final Android 206 / iOS 215 comparison cohort was blocked before financial authorization, with no debit. It supplies no final payment-latency result or native benefit claim. Cases J1, 1, 2 remain unchecked and unexecuted on this integrated head. Native USDT-warning, PIN and biometric flows have not been verified by this integration validation.

October 26 master integration

Published integration head 5dbbc0c06435725b7025af9abebb0c8edd74bf90 incorporates actual master 2ce210252be8ae7ea130d2422a2fa704a191ef7d through ordinary signed merges, preserving the stacked base and prior child changes. The two parent conflict resolutions retain review-state ownership, root-safe subscription Back navigation, one outer USDT status task, prompt recipient-scoped proof reconciliation and the terminal-refresh exit. No local assertion/timeout weakening or new test suppression was introduced.

Validation: two 19-target builds and two canonical hosted unit suites passed: 2,910 on the parent and 2,937 on the combined tree, zero failures/skips with the exact six CI integration exclusions. This PR is covered by the combined stack/sibling tree f471e8a6 (2,937 units), including this branch; these are two suites, not eight independent runs. Full SwiftFormat0.64 passed on all eight final trees (0/844); translations had zero errors and 15 warnings matching execution on complete actual-master source. Independent frozen integration review found no actionable issue. Local environment: Xcode26.1/SDK26.1, iOS26.5 simulator.

Complete diagnostics retained: two App Intents notices per build; parent1,082/combined1,099 runtime scan records, including four/six SDK/Pubky/DHT QoS stacks, fault fixtures and hosted appearance/identity notices. Source comparisons are not exact-base runtime replay or warning-free evidence. New-head hosted CI follows publication. Master's #1130 retired Blocktank skips and E2E285 channel/migration omissions remain coverage gaps, not fixes for original #1109. Historical #1054 preparation/UI-launch causes remain unresolved. Existing QA case IDs and native gaps remain: no new payment, funded USDT, physical hardware, latency or merchant-unlock journey was run by this integration.

October 26 lifecycle integration and camera fixture

Published head 5a58bbdbdc44b878653bd5ebe6e9c23b0b733f77 includes approved and merged #1098 through master b8b7f3d9902de5669c025bb839232c1d7e98d2df. The overlapping lifecycle test retains one owned switch task per current context, holds resumed preparation until confirmation is visible, and keeps the exact-one-success assertion, cancellation/context guards and retained presentation/app-lock behavior. Seven ordinary signed merges preserve stacked bases and prior child changes; #1084/#1085 also retain merged #1081 through #1070.

The existing unfunded USDT UI case now resets camera permission before launch, explicitly waits for the camera alert, allows it and waits for dismissal before manual entry. This adds six test-only lines and two assertions; no test, assertion or timeout was removed or weakened. Production camera behavior and the merged Receive blank-band checks are preserved.

Final validation passed two 19-target builds, 2,913 parent canonical units and 2,940 units on combined tree db653be56e228dec55f4e6c0fd57e42e7d8475c2, with zero failures/skips and the exact six CI integration exclusions. This branch is included in that combined composition. These are two suites, not seven independent runs. The combined unfunded UI case passed, including both Receive blank-band swipes and explicit camera handling, using runner-only loopback endpoint settings. Full SwiftFormat 0.64 passed on all seven final trees (0/844); translations are unchanged. Independent final review found no actionable issue. All 151 distinct commits across local current/prospective base ranges verified cryptographically; all 129 distinct commits across the final published GitHub ranges report verified=true/reason=valid.

Complete diagnostics remain retained: parent 1,107/combined 1,125 broad records, including four/six SDK/Pubky/DHT QoS stacks, App Intents notices, hosted appearance and fault-fixture messages. The combined shared-keychain -25300 publication diagnostic has unchanged emitter/test source; its exact runtime cause remains unestablished. Source comparison is not exact-base runtime replay or warning-free evidence.

The old Receive-dismissal failure in #1070 run 40394372963 did not reproduce in the controlled original-head run; that run passed and used XCTest's default camera interruption handler. The camera-fixture comparison passed with an identical 157-file production app bundle. Neither result establishes the Receive failure's cause or repair. Local Xcode 26.1/SDK 26.1/iOS 26.5 differs from hosted CI; the completed hosted results are recorded below. The daemon launch issue #1110, original hardware-transfer issue #1109 and earlier coverage gaps remain. No new payment, funded USDT, physical-hardware, final latency or merchant-unlock result is claimed. João, Piotr and Ovi remain requested; existing pending requests are preserved without duplicate notification.

October 26 hosted CI confirmation

For published head 5a58bbdbdc44b878653bd5ebe6e9c23b0b733f77, unit/UI run 40412991053 passed on attempt 1: 2,928 unit tests, zero failures/skips; 43 UI executions, 39 passed and four existing prerequisite skips, zero failures. SwiftFormat 40412991095 passed: 0/844 files require formatting, 81 skipped. These workflows tested GitHub's PR merge commit using Xcode 16.4/SDK 18.5 on iOS 26.2.

Complete workflow diagnostics were inspected. The 571 duplicate-symbol records match the retained passing #1025 run 40376172874; workflow/cache-action sources match actual master. This is retained-log/source comparison, not an exact-base runtime replay or warning-free result. The remaining red aggregate contains cancelled duplicate jobs; the replacement unit/UI and formatting workflows passed.

No manual CI rerun was requested. Historical Receive-dismissal/daemon-launch causes and original hardware-transfer issue #1109 remain unresolved; existing skipped/deferred coverage and final Android 206/iOS 215 payment/unlock measurement remain incomplete. Existing human review requests are preserved.

October 29 inherited app-lock guard

Published ed755997 carries the four-file R1 correction from #1039 at aae380fc: a recreated confirmation uses the retained submission flag for its busy state and refuses another authorization while the original payment is in flight. This branch retains its existing Core 0.8.0-rc10, LDK 0.7.0-rc.77, Paykit 0.1.1-rc1 and VSS 0.5.23 pins, stacked base and prior history.

On this exact branch candidate, Xcode 27.1 / SDK 27.1 / iOS 26.5 passed the complete selected unit suite: 2,930 passed, 0 failed, 0 skipped, with the six existing unit-workflow integration exclusions. Both new confirmation regression tests passed. Full SwiftFormat 0.64.0 passed: 0/844 files require formatting; 81 existing exclusions. The first build stopped before tests because a copied package-cache record held old absolute framework paths; remapping only the private copy allowed the completed run. Source and dependency pins stayed unchanged.

Complete diagnostics were inspected: two AppIntents metadata notices, six native SDK/Pubky/mainline DHT construction QoS warnings, and retained fault-fixture/host output. Forty-six emitter/test files were compared with this branch's pre-fix head; the warning-emitting methods in the two changed files are unchanged. No introduced actionable finding was found. This is source and retained-evidence comparison, not an exact-base runtime replay or warning-free claim. The identical production patch reuses the two independent R1 source reviews and guard negative control from #1039, with the older child callers inspected separately.

All 19 commits in the actual current-parent PR range verified cryptographically locally and report GitHub verified=true / reason=valid after the normal push. New-head hosted checks remain pending and validate their own recorded PR-merge inputs separately from this local older-pin branch.

Existing cases J1, 1 and 2 and their documented fixture/admission prerequisites remain unchecked on this new head. The external native R1 report on #1039 reports two successful controlled regtest executions at aae380f; it is attributed parent evidence, not a device run on this child. No new financial journey, binary adoption or final Android206/iOS215 latency/unlock result is claimed.

October 29 backup observation repair

08db8d32 fixes a subscription-storage race exposed by unit/UI run 40862818689: the app crashed in Combine Set insertion during testStoppingRecoveryRetainsPendingBackupAndDoesNotUpload. The affected source and test were already present on master 031625d2. Subscription registration, cancellation and periodic-task ownership now share the synchronous backup queue; the existing generation rejects delayed setup and scheduling after stop. The original regression is unchanged, and one added test exercises eight observation stop/restart cycles followed by one successful pending-backup recovery.

Final local validation on Xcode 27.1 / SDK 27.1 / iOS 26.5 passed 22 focused tests and 2,848 full unit tests, with zero failures and one existing recovery-protocol fixture skip. SwiftFormat passed 0/823, with 95 existing exclusions. Two independent source reviews found no actionable issue. Complete build and runtime diagnostics were audited: zero compiler warnings/errors, five retained SDK/Pubky/mainline DHT priority-inversion stacks matching the earlier native roots, and disclosed simulator/intentional-fault output; the focused build also retained two AppIntents metadata notices. Source comparison supplies no exact-base runtime or warning-free claim.

The commit keeps 380da117's existing dependency lock (aa3182b3): Core 0.8.0-rc11, LDK 0.7.0-rc.78, Paykit 0.1.1-rc2 and VSS 0.5.26-rc1. All 21 commits in the actual master-based PR range verified locally and report GitHub verified=true / reason=valid. New-head hosted CI is pending. Earlier 380da117 integration passed 30 tests and E2E passed 13 cases with one explicit hardware-transfer skip; those reports precede this repair and remain attached to their own inputs. Existing J1, 1 and 2 and their controlled SDK/linked-issuer prerequisites remain unchecked; final Android206/iOS215 payment/unlock measurement and post-batching field latency remain incomplete.

Models used

  • Planning/scoping: gpt-6-astra (reasoning: xhigh), gpt-6.1-sol (reasoning: xhigh)
  • Implementation: gpt-6-astra (reasoning: xhigh), gpt-6.1-sol (reasoning: xhigh)
  • Review: gpt-6-astra (reasoning: xhigh), gpt-6.1-sol (reasoning: high)

@ben-kaufman
ben-kaufman marked this pull request as ready for review October 20, 2026 20:39
@greptile-apps

greptile-apps Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The changes are contained to Paykit payment and saved-contact workflows.

Retain missing-peer link preparation across payment deferral before merging.

Findings

  1. P1 Deferred contact linking is lost ▶

Summary

Defers automatic Paykit reads and endpoint publication during payment review or submission.

  • Background Paykit work waits when a payment is active.
  • Saved-contact preparation resumes after payment activity ends.
  • Payment-admission deferrals keep private-message retries available.

Tests were inspected, not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Automatic contact preparation] --> B[Wait for payment and review to end]
    B --> C[Wait for foreground]
    C --> D[Check session and publication availability]
    D --> E[Prepare links and publish endpoints]
    E -->|Admission deferred| B
    D -->|Publication unavailable| F[Prepare links only]
    F --> G[Read remaining work and schedule retries]
    G --> H[Finish preparation]
Loading

Reviews (1) · Last reviewed commit: "chore: rename changelog fragment" · Reviewed by Greptile

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift
@ovitrif ovitrif added this to the 3.1.0 milestone Oct 20, 2026
@ovitrif

ovitrif commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Triage: milestone 3.1.0, with its twin synonymdev/bitkit-android#1626. It closes no issue, so this PR carries the milestone itself. Not a release blocker.

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No findings. Defers automatic Paykit contact preparation and private endpoint publication while a payment is reviewed or submitted, and resumes it once review ends and the app is back in the foreground. Reviewed 4ddd593, full tier, against the stack base fix/paykit-review-app-lock. I reasoned from the code and did not build it: iOS does not build on this reviewer's Linux host.

What I checked, and 6 candidates I ruled out

Read in full: the diff in PrivatePaykitService+Contacts.swift, +Invoices.swift, +Payments.swift, PubkyService.swift, PaykitPaymentRequestService.swift. I also read their surroundings: withSdk, withStateRevisionTracking, withBackupStateRevisionTracking, checkRefreshAdmission, PaykitPaymentActivity, scheduleContactPreparation, syncLocalEndpointPublication, the drain retry loop and buildLocalEndpoints.

Call sites traced: refreshSavedContactEndpointsReturningError (contact preparation, pre-payment publication, reconcile), PubkyService.currentPublicKey(priority:), privateEndpointPublicationUnavailabilityReason, beginReview/begin in SendSheet, SendPendingScreen and the confirm views.

CI: SwiftFormat green. Run Tests was still pending when I reviewed.

Ruled out

  • Retry loop spins: checkRefreshAdmission throws only while shouldDeferRefresh holds (a payment or a review). waitUntilIdle(includingReviews: true) waits on that same condition, so each retry blocks until it clears.
  • Publication lock held across the wait: a deferral inside withPublicationLock (the identity re-read or syncPaymentLists) is caught outside the lock and returns to the loop, so the lock is released before waitUntilIdle.
  • Deferred write marks backup state dirty: in withBackupStateRevisionTracking the admission check runs in beforeOperation, before the do/catch that calls onChange, so an unattempted sync does not reset the snapshot.
  • Explicit paths deferred: checkRefreshAdmission acts only on .background. Withdrawals keep .ordered (withdraws ? .ordered : .background), and requireImmediatePublication reads use .ordered, so explicit publication and cleanup are not admission-gated.
  • Stale identity or removed contact republished after the wait: each pass re-checks the session, the generation, canPublish and the identity against the first pass's identity. Inside the lock it re-checks knownSavedContactKeys and cleanupPendingKey.
  • Pre-payment publication blocks the contact payment: the task is detached, and SendContactSelectView is not a review route (isReviewingPayment covers .confirm, .pin and .hardwareSign), so beginContactPayment retries run while it proceeds.

Merge confidence: 4/5, no findings, but the PR was not built or tested here and Run Tests is still pending.

@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 at 90e954c: no HIGH/MEDIUM. Two LOWs are inline, both reproduced with failing tests at this head. This PR is based on #1039, which has merge conflicts at the moment.

Checked and clean:

  • Every exit path ends deferral:
    • The wait and the admission check use the same condition, so there's no tight loop.
    • Back, cancel and disappear end the review, and payments end it in defer.
    • The generation bumps (identity switch, profile deletion, sharing disable) exit the loop.
  • Explicit publication, withdrawals and recovery run ordered and skip both the wait and admission.
  • No lock is held while waiting: the throw happens inside withPublicationLock, whose defer unlocks, and activeLinkPreparationKeys is cleared in defer.
  • No self-deadlock: presentation uses the ungated interactive prepareAndResolve.
  • On a deferral: the backup isn't marked changed, and recipient discovery keeps its targets.
  • 90e954c propagates the deferral from drainPendingPrivateMessages, which answers greptile's P1.
  • Parity with synonymdev/bitkit-android#1626: the four ops left unchecked in #1025 (identityStatus, linkedPeers, the scheduling snapshot, automatic sync) are now checked. Contact preparation waits on an event here, where Android polls every second.
  • Open latency: this PR adds no waits on the confirmation path.

Device gate (partial): 90e954c — not run yet. Pending:

  • review-refresh.xml;
  • contact publication deferred while Confirm is open, then resuming after Back, after paying, and after Home;
  • open → ready timing against the base.

Device results will follow in a separate review.

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift
Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift Outdated
@ben-kaufman
ben-kaufman requested a review from jvsena42 October 20, 2026 22:28

@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 — a cancelled retry worker can delete a replacement retry.

Findings:

  • MEDIUM — Recheck retry ownership after the MainActor hop — Bitkit/Services/PrivatePaykitService+Contacts.swift:1149. Invalidation can cancel the worker and queue a new generation's retry during the new shouldDeferRefresh await. The cancelled continuation then selects that new entry. Its isCurrent() check fails on cancellation, and the initial failure branch in drainPendingPrivateMessageRetry removes the replacement at lines 1259–1261. Contact linking or private-message recovery then waits for another trigger to queue it again. Recheck cancellation and retryGeneration immediately after the await, before reading the retry dictionary. Test 5 reproduces this against unchanged production source; the same test passes with that guard alone.

QA for 0077196: iOS 26.5 simulator. The lifecycle fixture invokes the actual preparation and cleanup owners, injects SDK operations, and reads real UIApplication activity. Its publication/withdrawal counters describe injected operations, not network delivery.

Test 1: payment review and PIN lifecycle ✅ Retained base-head device coverage: cancellation, later request arrival and app unlock preserved one authorized payment.

I relied on the base-head device report and recordings at e9b45c2. It covers direct Bitcoin and amount→Bitcoin review, 30-second confirmation/PIN waits, cancellation, later request arrival, unlock restoration, one 1,000-sat payment and an unpaid second request. This diff leaves those confirmation, authorization, restoration and settlement paths unchanged; I did not repeat that journey.

Test 2: deferred forced preparation across background ✅ Review ended in the background; the original forced preparation stayed parked and completed once after foreground return.

The fixture rejected automatic publication with operationInProgress, ended review before the background-pause callback, and retained the same saved contact and force-refresh operation. The recording and app trace show inactive-state waiting, then one publication and one owner completion after return. Both lifecycle callback orders, session/identity changes, contact removal, invalidation and cancellation also passed in the hosted tests.


Real simulator lifecycle with a controlled SDK rejection, at 2×.

Test 3: disable sharing or remove the deferred contact ✅ Explicit withdrawal completed during review, and ending review did not republish either contact.

I drove both variants through the actual cleanup owner. Each finished withdrawal while automatic preparation remained deferred. After review ended, the preparation owner completed with zero publications. The recording shows the counters and event order; app traces independently confirm it. These are injected SDK operations, not a Settings-screen or network-delivery test.


Sharing-disable and contact-removal variants, at 2×.

Test 4: existing admission and retry regressions ✅ The pristine head built and its focused hosted suites passed.

I verified 420 passed tests, with no failures or skips, in the Xcode result bundle. Coverage includes queued admission and contention retries, backup-state preservation, explicit publication/withdrawal, retained missing-peer work, foreground ordering, ordinary network backoff and Interactive retry deadlines. The recording shows hosted execution; assertions were checked in the test result.


Hosted test execution, at 2×.

Test 5: retry replacement during invalidation ❌ The cancelled worker deleted the next generation's retry in all three controlled interleavings.

I added testCancelledRetryWorkerMustNotDeleteReplacementRetryAfterMainActorHop without changing production service code. It holds MainActor, starts the old due retry, invalidates it, pauses background work, and inserts/schedules a replacement. After releasing MainActor and awaiting the old task, the replacement is gone despite its own worker remaining paused:

XCTAssertTrue failed - A cancelled worker removed the next generation's retry in iteration 0
XCTAssertTrue failed - A cancelled worker removed the next generation's retry in iteration 1
XCTAssertTrue failed - A cancelled worker removed the next generation's retry in iteration 2

The same test passes when the only production change is a cancellation/generation guard immediately after the new await. That control was local and is not part of this PR. The failure is established by XCTest assertions and source tracing; the recording only shows the hosted runner.


Hosted reproduction against the reviewed service code, at 2×.

Earlier findings verified fixed at this head:

Coverage: full e9b45c2..0077196, all nine changed files and reached admission, publication, retry, lifecycle, cancellation, identity/reset and backup-state paths. Native transport and measured latency improvement were not tested; the new device checks use injected SDK boundaries.

Twin: synonymdev/bitkit-android#1626 fix: defer automatic paykit work during payments matches the admission, retained-work and explicit-cleanup intent at 5bd39a8. iOS uses event-based waiting and has the retry-ownership race above.


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift Outdated
@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): deferral during payment review at 90e954c (A = iOS, simulator) with B = Android synonymdev/bitkit-android#1626 6cf67cd.

Trigger: B pays A 1,000 sats over Lightning by contact while A has a contact-payment Confirm open. That runs handleReceivedPayment → refreshSavedContactEndpoints → scheduleContactPreparation, which is automatic background work.

case hold length with no publication by A review ended A syncPrivatePaymentListsWithReservations finished B sees the new list
(a) Back 4 min 33 s (B: Waiting … newer than 303 ×4) 23:30:41 23:31:00.1 (work 13.1 s) v309 at B's next poll, 23:31:30
(b) paid 2 min 28 s PIN 23:35:27 23:35:51.9 (work 12.8 s) v310 (B's polls were blocked by its own "Received Bitcoin" sheet)
(c) Home 2 min 27 s background 23:47:33, back 23:47:52 23:48:36.4, after Invoice created at 23:48:22 v311 at 23:49:25

Every hold deferred the publication, and each way of ending the review published the retained update.

  • Case (c) and my LOW1: after Home and return, A created a new invoice and published, so the forced refresh wasn't lost in this run. The race window is narrow, and one run can't rule it out; the unit test in my inline comment reproduces it deterministically.
  • Explicit link while Confirm is open isn't reachable in the UI, because the confirmation sheet is modal. That control is left to the unit tests.

Still running: review-refresh.xml.

@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: None. I verified the retry-ownership finding is fixed: cancelled or superseded workers exit after admission suspends, before selecting a retry, and leave a replacement worker's handle intact.

QA for 7ed5b69: iOS 26.5 simulator. Tests 4 and 5 ran here; their assertions were checked in the Xcode results. Their recordings show the hosted runner, not internal retry state. All recordings are at 2×.

Test 1: payment review and PIN lifecycle ✅ Retained device coverage proves cancellation, later request arrival and app unlock preserve one authorized payment.

I retained the base-head report and recordings, run at e9b45c2. It covers direct Bitcoin and amount→Bitcoin review, confirmation/PIN waits, cancellation, later request arrival, unlock restoration, one 1,000-sat payment and an unpaid second request. Those payment and restoration paths remain unchanged; I did not repeat the journey.

Test 2: deferred forced preparation across background ✅ Retained lifecycle evidence proves forced preparation waits while inactive and completes once after foreground return.

I retained Test 2 of the previous report, run at 0077196. The actual preparation owner retained the original forced operation when review ended before the background-pause callback. Its preparation and foreground-wait paths are unchanged in this delta. SDK publication was injected; this establishes owner/lifecycle behavior, not network delivery.

foreground-retained.2x.mp4

Retained recording from 0077196.

Test 3: disable sharing or remove the deferred contact ✅ Retained cleanup evidence proves withdrawal completes during review and deferred work does not republish either contact.

I retained Test 3 of the previous report, run at 0077196. Both variants used the actual cleanup owner with injected SDK operations. The cleanup and publication-owner paths remain unchanged in this delta; this was not a Settings-screen or network-delivery test.

cleanup-retained.2x.mp4

Retained sharing-disable and contact-removal recording from 0077196.

Test 4: exact-head admission and retry regressions ✅ The pristine head built and its focused hosted suites passed, including cancellation with and without a replacement generation.

The private-contact, SDK identity/admission, operation-lock, payment-activity and request-service suites passed without failures or skips. The new regression verifies retained retry identity, cleanup of the cancelled worker's own handle, preservation of a replacement handle, and one completed intake under the current identity. Existing cases cover foreground ordering, admission deferral, explicit cleanup, backoff and Interactive retry deadlines.

focused.2x.mp4

Hosted focused-suite execution; assertions verified in the test result.

Test 5: original retry-replacement reproduction ✅ All three previously failing MainActor interleavings preserved the replacement retry.

I reran the independent reproduction from the previous review. Only the test file was extended; production source stayed at 7ed5b69. The test holds MainActor, starts the old due retry, invalidates it, pauses background work and schedules a replacement. After releasing MainActor and awaiting the old worker, the replacement remains in every iteration. XCTest assertions establish the result.

replacement.2x.mp4

Hosted execution of the original reproduction at the fixed head.

Coverage: contiguous delta 0077196..7ed5b69, both changed files and reached retry ownership, cancellation, invalidation and cleanup paths, plus the prior open finding. The changed race was exercised by hosted tests; no fresh payment journey or network-latency measurement was driven.

Twin: bitkit-android#1626 fix: defer automatic paykit work during payments at 5bd39a8 matches the automatic-work deferral, retained retry and explicit-cleanup intent. This worker-ownership fix is specific to the iOS retry implementation.


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

No new actionable code findings.

Automatic contact publication and background private-message retries wait out payment review and submission, then resume without holding the publication lock or advancing network backoff. Explicit publication, withdrawals, and recovery stay on the ordered path. A deferred recipient refresh keeps the previous targets. The same admission contract was compared with bitkit-android#1626 at 5bd39a88: that change requeues contact preparation after one second, and this change waits until payment and review activity is idle.

Validation: Run Tests and SwiftFormat passed on this revision. Those checks were not re-executed in this review.

Device testing: not performed in this review.

Ready for device testing. review-refresh.xml and the two manual publication and cleanup cases in the PR are still unchecked.

@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 7ed5b69: no issues found, and both my LOWs are fixed.

  • Deferred forced preparation (0077196): when publishing is unavailable and the app is inactive, preparation now waits for the foreground (Contacts.swift:299-313, :361-364). The waiter registers before the active check, so a resume in between isn't missed. Invalidation and cancellation release it.
  • Drain-retry backoff (0077196, 7ed5b69): a due Background retry during review now waits for idle or the next Interactive deadline without trying (:1164-1184). Deferral no longer advances backoff on the identity, scheduling-snapshot or drain paths (:1228, :1297-1304, :1335-1338).
  • No hot loop: the deferral check and the idle wait use the same condition (PaykitPaymentActivity.swift:51-52, :86-87). The only SDK operationInProgress comes from the Background admission check, and transport failures still back off.
  • Cancel re-check: the check after the activity await (:1151) covers ovi-reviewer's MEDIUM.

Device gate (partial): 7ed5b69 — from the 90e954c deferral gate, Back and pay still hold, since the app stays active on both. Home has to be re-run, because that path changed.

Pending:

  • the Home case;
  • link-contact-after-resume.xml with iOS first;
  • review-refresh.xml.

@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): d404f92 — Home case

Setup: both sides deleted the contact. iOS saved GateReq at 06:37:27. iOS opened on-chain Send Confirm (1,000 sats to its own address) at 06:38:43 (42). Android saved Gate1005 at 06:38:59.

phase iOS Paykit work Android Pay on Gate1005
Confirm open 06:38:43 → 06:41:10 no ensureLinkWithPeer/syncPrivatePaymentListsWithReservations; 10/11 receivePrivateMessagesFromLinkedPeers, 2/2 processPendingPrivateMessages, 2/4 identityStatus returned in under 1 ms —
Home 06:41:10 → reopen 06:42:11 backgrounded: 3 ops right at the transition, nothing after —
foreground 06:42:11.66 (lock screen), PIN 06:42:24 foreground sync ran (syncPaykitApp, syncPublicEndpoints); ensureLinkWithPeer started ~06:42:20 and finished 06:42:28.21 (7.9 s work); after unlock restored the Confirm, Background ops returned in under 1 ms again 06:43:12: direct Amount, no Request or Pay (list not yet published)
Confirm closed 06:43:36 ensureLinkWithPeer 06:43:43.8 (5.3 s), syncPrivatePaymentListsWithReservations 06:43:49.0 (4.4 s), processOutboundPrivateMessages 06:43:49.3 06:43:40 direct; 06:44:06 Request or Pay (+30 s after close)

Verdict: the hold applies while Confirm is open. On Home, the link work resumes on foreground (before unlock), and the hold comes back when unlock restores the same Confirm. Publication runs within 13 s of closing the review, and the counterparty sees Request or Pay 30 s after the close. That matches the PR's design, where the hold follows the Confirm's lifetime. Not a finding.

Still running: Confirm open→ready vs base, and the Failed to sync … → one more sync check.

jvsena42
jvsena42 previously approved these changes Oct 26, 2026

@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 at d404f92. My code scan has no open findings, and the device gate found none.

Device gate: d404f92 — iPhone 17 simulator, with an Android counterpart on an emulator. journeys: review-refresh.xml 1-18 passed, and 19-24 passed on an adapted live-rate USD request; link-contact-after-resume.xml 8/8 passed (iOS first); Home case held. perf: Confirm open → ready 3.3-5.6 s (median ~4.5 s) vs base 1806a0b 3.7-6.2 s (median ~4.5 s), so no regression. Background Paykit operations during review were rejected at admission in under 1 ms, and no private-list sync ran while Confirm was open. Figma: n/a; l10n: no UI text changed. Step tables and logs are in the three partial comments above.

  • The hold follows the Confirm's lifetime. On Home, link work resumes on foreground, the hold returns when unlock restores the same Confirm, the list publishes within 13 s of closing the review, and the counterparty sees Request or Pay 30 s after.
  • Linking needs a counterparty on the same paykit (0.1.1-rc1). An Android build on paykit 0.1.0 never completed the link (recovery_required), which is an environment constraint, not this PR.

⚠️ Not verified:

  • Failed to sync private Paykit endpoint publications followed by exactly one more syncPrivatePaymentListsWithReservations after endReview: no sync failed in any run, and injecting a homeserver failure on the simulator host needs root. That ordering rests on the code scan and its unit test.
  • review-refresh.xml 19-24 with fixed conversion terms and a USDT endpoint: these need a conversion-capable issuer fixture, which is not available on my setup.

CI: Run Tests is red at d404f92 on SendConfirmationLifecycleTests.testPendingWalletSwitchResumesAfterTags (a known flaky hosted timeout) and BitkitUITests.testExample, neither of which this PR touches. It needs a re-run before merge.

@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: Re-reviewed the full PR at 7c8c302 after the merge base changed since the saved assessment. No coverage baseline was supplied, so this pass covers the complete diff from merge base 69b41810 and inherits nothing.

1 actionable finding — resolve or provide an evidence-backed rebuttal.

A due background retry parks the only retry worker until payment review ends, so another contact's interactive link does not run at its deadline. Prior threads on dropped preparation, backoff consumption, retry ownership, and publication-failure identity reads were checked at this head and are not reopened.

Device testing: not performed in this review. This job has no Swift toolchain, so the cancellation behavior was not executed. testPaymentReviewDoesNotDelayAnotherContactsInteractiveRetryDeadline stubs sleep to return immediately, so a passing run does not keep the idle wait suspended across the deadline.

Compared the retry-wake contract with Android #1626 at 5dc9ee61. That was not a full review of the Android PR. Android uses a per-contact job and withTimeoutOrNull, so that contact's own deadline resumes its retry.

Suggested additional test cases

  • iOS, disposable regtest wallet, Paykit UI enabled, two saved contacts, and a payment review left open. One contact has a background private-message retry already due; the other has an explicit link due a few seconds later. Expected: that explicit link's intake runs while the review stays open. If the review stays open past the 20-second interactive window, link completion still refreshes that contact's payment-request eligibility.

Findings

  • [MEDIUM] Wake interactive contact retries at their deadline during review — inline at Bitkit/Services/PrivatePaykitService+Contacts.swift:1199.

Reviewer model: grok-4.7
Reasoning effort: high

Comment thread Bitkit/Services/PrivatePaykitService+Contacts.swift

@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: None in the delta. App-lock disappearance preserves the current nonretained request screen; actual dismissal still cancels setup and clears its state.

QA for 7c8c302: Xcode 27.0, iPhone 17 Pro simulator on iOS 26.5. I built the unmodified head and verified hosted assertion outcomes in xcresult. Retained device cases below name their tested revisions.

Test 1: app-lock and Paykit deferral integration ✅ The selected hosted suites passed without failures or skips, verified in xcresult.

Coverage includes Amount and the synthetic USDT exclusion surviving teardown/remount, retained Bitcoin confirmation and both dismissal orders, setup cancellation, payment-review ownership, forced preparation, publication-failure identity deferral, explicit cleanup and retry backoff. I ran the payment-request, send-sheet, confirmation-lifecycle, payment-activity and private-contact suites. The USDT case exercises its exclusion flag rather than a funded USDT payment. The result bundle records a hosted SDK QoS warning.

The recording shows the focused app-lock and actual-dismissal rerun; the result bundles establish the internal assertions.

app-lock.2x.mp4

Hosted app-lock and dismissal regression execution, at 2×.

Check 1: payment review and PIN lifecycle (J1) ✅ Retained native reports cover the new Amount-lock steps and the earlier confirmation/authorization journey.

The native report at 69b4181 proves that a pending fixed-price 1 USD request accepting Bitcoin and USDT preserves Savings and 1,000 sats through app-unlock PIN on Amount. Continue reaches Bitcoin confirmation with a prepared fee; dismissal leaves the request unpaid and reopenable. All four delta files, plus the reached app-lock and sheet owners, match that tested parent.

The native report at 1806a0b covers confirmation/PIN waits, cancellation, later request arrival, direct and fixed-conversion confirmation restoration, fresh authorization and one Bitcoin payment. The current parent review reconciles that evidence; the retained Bitcoin-review, authentication and payment paths remain unchanged. I did not repeat those native journeys.

Check 2: deferred forced preparation across background (manual 1) ✅ Retained owner/lifecycle evidence proves forced work waits while inactive and completes once after foreground return.

Test 2 at 0077196 uses the actual preparation owner, injected SDK rejection and real simulator lifecycle callbacks, including review ending before the background-pause callback. The previous review retains those unchanged owner and foreground-wait paths; this delta leaves them untouched. This proves retained ownership, rather than fresh network publication.

Check 3: disable sharing or remove a deferred contact (manual 2) ✅ Retained cleanup evidence proves withdrawal completes during review without deferred republication.

Test 3 at 0077196 covers both variants through actual cleanup owners with injected SDK operations. The previous review retains that evidence. This delta leaves cleanup, generation, saved-contact and publication-owner guards untouched. This is not a fresh Settings-screen or remote-withdrawal run.

Check 4: interactive retry deadline cancellation ✅ The reported stuck task group was not reproduced with the exact idle waiter and a real deadline.

I checked the new cancellation finding. Swift's AsyncStream implementation handles cancellation of its suspended iterator. The loop then exits, removes its waiter and reaches Task.checkCancellation(), allowing the task group to finish.

I compiled the unmodified PaykitPaymentActivity.swift with the same task-group race and a real 150 ms sleep using Swift 6.4 on macOS. In three runs, reflection confirmed one idle waiter was suspended when the deadline fired; the group returned in 153–162 ms with review still active. This checks the disputed cancellation boundary, rather than a fresh two-contact device journey. The existing owner regression also passed in Test 1, with its disclosed synchronous-sleep limitation.

The earlier requested publication-failure ordering is covered by Test 1's controlled owner regression and the passing report at 753a4c6; no fresh homeserver failure was injected. Run Tests and SwiftFormat passed at this head.

Coverage: contiguous delta d404f92..7c8c302, all four changed files and reached lifecycle callers, plus earlier findings. No planned case remains unrun. Hardware, in-flight submission and fee/UTXO/tag paths received source or hosted coverage rather than separate device journeys. Journey wording and the changelog received source review.

Twin: bitkit-android#1626 fix: defer automatic Paykit work during payments, checked at 5dc9ee6, matches background SDK admission, retained preparation and explicit publication/withdrawal. Android retries retained preparation after a delay; iOS waits for idle activity. The imported app-lock teardown/remount fix is iOS-specific.

Reviewer model: gpt-6.1-sol
Reasoning effort: high


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@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: None in the delta or earlier findings. The admission, retained-preparation and retry implementation is unchanged from the previous review; the imported confirmation switch owner preserves cancellation and resumes once after returning from Tags.

QA for 5a58bbd: Xcode 27.0, iPhone 17 Pro simulator on iOS 26.5. I built the unmodified head. Hosted assertions were checked in xcresult. Result bundles include SDK QoS warnings. Recordings play at 2×.

Test 1: payment lifecycle and Paykit integration ✅ The selected hosted suites passed without failures or skips, verified in xcresult.

Coverage includes Tags-return wallet switching, custom-fee navigation, teardown/remount, retained request review, expiration/history, blocked on-chain requests, USDT initial subscription authorization, admission deferral, retained preparation, publication-failure recovery, retry ownership and explicit cleanup. Profile and activity integration regressions also passed.

The recording shows the focused app-lock and Tags-return rerun; the result bundles establish the internal assertions.

lifecycle.2x.mp4

Hosted lifecycle regression execution.

Test 2: real retry deadline during review and submission ✅ Interactive intake and its link-completion callback ran at the real deadline while activity remained active, verified by assertions.

I extended only the test file to exercise the actual retry worker with a real one-second sleep. Production source stayed unchanged. Both variants confirmed one suspended idle waiter before the deadline, the Interactive intake and didLink callback before ending activity, and an unchanged Background retry index and deadline. This checks the reported cancellation boundary, without a two-wallet or remote-link-delivery claim.

boundary.2x.mp4

Hosted retry-owner execution; xcresult establishes the callback and backoff assertions.

Test 3: unfunded USDT amounts in a comma-decimal region ✅ Native UI assertions verified unavailable balances, dot decimal entry and recipient/Back navigation.

The unchanged UI test uses English with the German region and unavailable USDT endpoints. Home and wallet show an unknown balance rather than a false zero; entering 1.5 keeps a dot decimal, network navigation returns to Amount, and Back preserves the manual recipient. No payment was submitted.

T3-media.2x.mp4

Native unfunded amount and recipient navigation.

Test 4: USDT Receive gestures and camera permission ✅ Native UI assertions verified both blank-band swipes, QR/amount preservation, explicit camera permission and invalid-address rejection.

The unchanged UI test swiped the flexible band in both directions and returned to the same USDT QR, canceled amount edits without changing that QR, set the receive amount to 3 USDT and verified the details address. Dismissing Receive reached Send, the reset camera alert was allowed explicitly, a Bitcoin address was rejected, and an EVM payment URI opened Amount with 1.23; subsequent input retained six decimals without entering confirmation. This was unfunded and submitted no payment.

T4-media.2x.mp4

Native Receive, camera permission and payment-input regression.

Check 1: retained payment-review, PIN and app-lock journey (J1) ✅ Prior native reports cover the unchanged one-time request and authorization routes.

The native report at d404f92 covers review/PIN waits, cancellation, later request arrival, restored confirmation and one authorized Bitcoin payment. Its fixed-conversion limitation is covered by the native report at 69b4181, which verifies the fixed-USD Amount-lock, continuation and unpaid-reopening steps and retains the earlier confirmation/PIN evidence. The merged wallet-switch owner is freshly exercised in Test 1. I did not repeat a native settlement journey.

Check 2: retained forced preparation across background (manual 1) ✅ Passing owner/lifecycle evidence still covers forced work waiting while inactive and completing once after foreground return.

Test 2 at 0077196 uses the actual preparation owner, injected SDK rejection and real simulator callbacks, including review ending before the pause callback. The preparation and foreground-wait implementation remains unchanged from the previous review. This proves retained ownership, rather than fresh network publication.

Check 3: retained sharing-disable and removal cleanup (manual 2) ✅ Passing cleanup-owner evidence still covers withdrawal during review without deferred republication.

Test 3 at 0077196 covers both variants with injected SDK operations. Cleanup, generation, saved-contact and publication-owner paths remain unchanged since the previous review. This is not a fresh Settings-screen or remote-withdrawal run.

Coverage: contiguous delta 7c8c302..5a58bbd, all 87 changed files and reached callers, plus earlier findings. All planned cases completed. Other imported UI, documentation and locale changes received source or hosted coverage. No fresh funded USDT, hardware, settlement, homeserver-failure or latency measurement is claimed.

Twin: bitkit-android#1626 fix: defer automatic Paykit work during payments, checked at 5dc9ee6, matches automatic SDK admission, retained preparation and explicit publication/withdrawal. Android retries retained preparation after a delay; iOS waits for idle activity.

Reviewer model: gpt-6.1-sol
Reasoning effort: high


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest

@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 the pinned PR at 5a58bbd against merge-base 88f75ea. The comparison base moved to that commit and no coverage baseline was supplied, so nothing was inherited.

No new actionable code findings.

Background contact publication waits until payment review is idle and the app is active, releases the publication lock before that wait, and publishes only after the identity, session, generation, and saved-contact checks still match. Background message retries keep their current deadline when admission defers them, and an already-due interactive retry is selected again when its deadline arrives or the review ends. Explicit publication and withdrawals are not gated by that background admission check.

Hosted unit/UI run 40412991053 completed successfully for this head. This pass did not re-run the suite or inspect that log. Android parity was not compared: code search was rate-limited, and this review set has no Android twin.

Device testing: not performed in this review. Manual cases 1 and 2 still need the deterministic admission fixture described in the PR.

Earlier findings reconciled at this revision: 4 verified fixed, 1 not substantiated.

Reviewer model: grok-4.7
Reasoning effort: high

@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 of ed755997 since 5a58bbdb, with the same merge base 88f75ea. Inherited baseline: review 5664042058. This pass inspected the confirmation-recreation delta. Publication deferral, lock release, retry parking, SDK admission, explicit publish/withdraw, pre-payment handoff, and their unit tests are unchanged and inherited.

No new actionable code findings.

Recreating send confirmation while isSubmittingPayment is already true keeps the swipe loading and makes submitPayment return before PIN, biometrics, or a second payment. The return leaves the in-flight payment's own success or failure navigation in place. The flag remains on the send flow and clears when that payment finishes.

The new lifecycle host and swipe predicate were inspected against that code and were not run here. SwiftFormat passed on this head. Unit tests were still in progress. Android was not compared for this SwiftUI recreation path.

The open interactive-deadline thread remains the previously unsubstantiated concern. Its retry worker is unchanged in this delta.

Recommended before device testing

Wait for the unit-tests run on ed75599 to finish.

Suggested additional test cases

  • iOS, disposable regtest wallet, ordinary payment authorization enabled. Start an initial subscription payment or a manual Lightning send and remain on confirmation after authorization, while the payment is still in flight. Lock or background the app, then return to the foreground. Expected: no second PIN or biometric prompt, confirmation stays busy, and the original payment completes once with no second debit.

Device testing: not performed in this review.

Reviewer model: grok-4.7
Reasoning effort: high

@ovitrif
ovitrif changed the base branch from fix/paykit-review-app-lock to master October 29, 2026 09:26
@ovitrif
ovitrif dismissed stale reviews from jvsena42 and themself October 29, 2026 09:26

The base branch was changed.

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

Retargeted this PR to master before merging its base, #1039, so deleting that branch would not close it. If the base change dismissed the approval, it needs a re-approval.

@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 the complete PR diff at ed755997 against merge base ecc0fb86 (base master at 383491f2). The PR was retargeted from the #1039 branch to master, so the earlier 88f75ea baseline no longer applies and nothing was inherited. #1039 isn't yet in master at this base pin, so the reviewed diff also contains its app-lock review retention (AppScene, SendSheet, SendConfirmationView) alongside this PR's Paykit admission and deferral changes.

No new actionable code findings.

The new deferral can't spin. Background SDK reads (identity, linked peers, scheduling snapshot, pending outbound, ensureLinkWithPeer) and automatic, non-withdrawing publication recheck shouldDeferRefresh on entry and before the contention retry, and every waiter uses that same rule. Explicit (.ordered/.interactive) publication, withdrawals and reconciliation are not affected. The refresh releases the publication lock before it waits. After the wait it rechecks identity, session, generation and saved contacts before publishing, and it resumes from foreground waits on setBackgroundWorkPaused(false) or invalidation. Deferred background message retries keep their backoff step. The retry worker wakes when review ends, when the timer is rescheduled, or at another contact's interactive deadline. Pre-payment publication now runs through the same loop: before review starts, or after the payment ends. App-lock retention only re-mounts an unsubmitted, non-hardware, non-USDT Bitcoin confirmation that still owns the session, sheet and payment context. A recreated confirmation stays busy and refuses another authorization while isSubmittingPayment is set.

The open interactive-deadline thread doesn't hold at this head. group.cancelAll() ends the idle child's suspended AsyncStream wait, and waitUntilIdle then throws at Task.checkCancellation(), so the worker wakes at the deadline. This is consistent with the author's and Ovi's compiled probes on that thread. Basis: source analysis only, not run here. The thread can be resolved. The other earlier threads are fixed in the current source.

Validation: unit tests and SwiftFormat passed for this head. The retarget integration run merges this head with post-#1039 master 031625d2. It failed only in testUtxoSelection, which timed out after 120s waiting for regtest deposit confirmations; that is outside the changed paths. The added unit tests were read, not run, because no Swift toolchain was available. Android parity: on bitkit-android master 92628aa4, PaykitSdkService.checkRefreshAdmission also rejects Background work during payment review or submission. Android's retained-preparation internals were not compared.

Recommended before device testing

Rerun the retarget integration check so testUtxoSelection is green on this head.

Suggested additional test cases

  • iOS, two linked disposable regtest wallets. On the payer, start a payment to a saved contact whose private payment list hasn't arrived yet, and wait on confirmation. Expected: resolution completes, or the existing waiting state shows, without hanging. No contact publication runs while confirmation is open. After the payment finishes or review closes, exactly one background publication to that contact follows.
  • iOS, with contact preparation deferred by an open payment review: close the review, then pull down Control Center (app inactive, not backgrounded) and dismiss it. Expected: deferred preparation resumes once after returning to active, without needing to background and reopen the app.

Device testing: not performed in this review. Manual cases J1, 1 and 2 still need the deterministic admission fixture described in the PR.

Reviewer model: claude-opus-5-5
Reasoning effort: high

@ovitrif

ovitrif commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

Updated this branch with master so its approval sticks: the ruleset dismisses approvals on a branch behind its base. No change to the PR's own code.

@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: None. The earlier preparation, publication-failure and retry-ownership fixes remain intact.

QA for 380da11: Xcode 27.0, iPhone 17 Pro simulator on iOS 26.5. I built the unmodified head and verified hosted assertions in the Xcode result bundles. The two supplemental owner checks use a local Debug fixture with injected SDK boundaries and real application lifecycle events. Their publication counters represent injected operations, not network delivery. New recordings play at 2×.

Confirmation and Paykit integration ✅ The pristine head built and the selected hosted suites passed without failures or skips.

Coverage includes recreated confirmation refusing another authorization, retained busy state and submission ownership, request/app-lock lifecycle, SDK admission and contention retries, retained forced preparation, publication-failure recovery, retry backoff and explicit cleanup. The result bundle contains a hosted SDK QoS warning; the build also emitted a dependency sysroot warning and App Intents notices.

The recording is a short rerun of both new confirmation regressions using the original pristine test products. The result bundles establish the assertions; the video shows hosted execution.

guard.2x.mp4

Confirmation regression execution.

Deferred preparation after Control Center ✅ Forced work waited while inactive and completed once after Control Center closed, without entering background.

I started the actual preparation owner, rejected its first automatic publication for payment activity, and opened Control Center. Review ended while UIApplication.applicationState was inactive. The trace confirmed that the original forced operation stayed retained, then recorded one publication and one owner completion after returning active, with zero background transitions. The fixture forwards the active/background callbacks used by the app; SDK publication is injected.

inactive.2x.mp4

Control Center, inactive waiting and one resumed completion.

Request resolution with missing private details ✅ The request owner returned its waiting result during review, and retained publication completed once after review ended.

I injected RecoveryRequired at the SDK request-resolution boundary and called the actual request owner for the saved contact while review remained active. It returned privateLinkPending; automatic publication stayed deferred. Ending review produced one publication and one preparation completion. The recording and app trace establish that ordering.

This is a controlled caller/preparation-owner check. The SDK request-resolution call, native payment-request waiting UI and two-wallet network recovery were not exercised by this fixture.

pending-details.2x.mp4

Injected missing-details response and deferred publication ordering.

Check: payment review, PIN cancellation and app unlock (J1) ✅ Retained native evidence covers cancellation, request arrival and a single payment after app unlock.

The parent report at aae380f passed the authorized-send case twice: app unlock restored a busy control, another swipe did not request payment authorization, and releasing the original send completed one transaction and requester proof per request. Its linked recordings and chain evidence establish those results.

That report retains the unpaid review/PIN journey and fixed-conversion Amount-lock evidence. Confirmation, app-lock ownership, authorization, native send and proof-handling source, plus dependency pins and project/environment configuration, match the tested parent. I did not repeat settlement here.

Check: forced preparation across background (manual 1) ✅ Retained lifecycle evidence covers forced work surviving review completion before the background-pause callback.

Test 2 at 0077196 used the actual preparation owner, injected SDK rejection and real simulator lifecycle callbacks. Work waited while inactive and completed once after foreground return. The previous review at 5a58bbd retained those unchanged owner/foreground-wait implementations; this delta leaves them untouched. This proves retained ownership rather than fresh remote publication.

Check: disable sharing or remove a deferred contact (manual 2) ✅ Retained cleanup-owner evidence covers withdrawal during review without deferred republication.

Test 3 at 0077196 passed both sharing-disable and removal variants with injected SDK operations. The previous review retained the cleanup, generation, saved-contact and publication-owner guards, which remain unchanged here. Their hosted regressions also passed at this head. This is not a fresh Settings-screen or remote-withdrawal test.

Coverage: contiguous 5a58bbd..380da11, the confirmation correction and master merge, reached admission/preparation/publication/retry and payment-lifecycle callers, and earlier findings. The Paykit deferral implementation is unchanged since the previous review; the confirmation correction is now also in the base. The earlier real-delay retry-owner evidence remains applicable. Other merged master UI, hardware and USDT changes were not separately driven here. No new network-delivery or latency-improvement claim is made. The current-head integration check has passed.

Twin: synonymdev/bitkit-android#1626 fix: defer automatic Paykit work during payments, checked at cbdf76d, matches background SDK admission during review/submission. iOS retains its event-based preparation wait.


Reviewed by gpt-6.1-sol-high

@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

Register and cancel backup subscriptions on the backup queue, reject stale setup after stopping, and retain pending backups across observation restarts.
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