Repository navigation
fix: defer automatic paykit work during payments - #1054
ben-kaufman wants to merge 7 commits into
Conversation
[Medium impact] The changes are contained to Paykit payment and saved-contact workflows. Retain missing-peer link preparation across payment deferral before merging. Findings
SummaryDefers automatic Paykit reads and endpoint publication during payment review or submission.
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]
Reviews (1) · Last reviewed commit: "chore: rename changelog fragment" · Reviewed by Greptile |
|
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
left a comment
There was a problem hiding this comment.
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:
checkRefreshAdmissionthrows only whileshouldDeferRefreshholds (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 orsyncPaymentLists) is caught outside the lock and returns to the loop, so the lock is released beforewaitUntilIdle. - Deferred write marks backup state dirty: in
withBackupStateRevisionTrackingthe admission check runs inbeforeOperation, before thedo/catchthat callsonChange, so an unattempted sync does not reset the snapshot. - Explicit paths deferred:
checkRefreshAdmissionacts only on.background. Withdrawals keep.ordered(withdraws ? .ordered : .background), andrequireImmediatePublicationreads 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,
canPublishand the identity against the first pass's identity. Inside the lock it re-checksknownSavedContactKeysandcleanupPendingKey. - Pre-payment publication blocks the contact payment: the task is detached, and
SendContactSelectViewis not a review route (isReviewingPaymentcovers.confirm,.pinand.hardwareSign), sobeginContactPaymentretries 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
left a comment
There was a problem hiding this comment.
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, whosedeferunlocks, andactiveLinkPreparationKeysis cleared indefer. - No self-deadlock: presentation uses the ungated interactive
prepareAndResolve. - On a deferral: the backup isn't marked changed, and recipient discovery keeps its targets.
90e954cpropagates the deferral fromdrainPendingPrivateMessages, 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.
There was a problem hiding this comment.
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 newshouldDeferRefreshawait. The cancelled continuation then selects that new entry. ItsisCurrent()check fails on cancellation, and the initial failure branch indrainPendingPrivateMessageRetryremoves the replacement at lines 1259–1261. Contact linking or private-message recovery then waits for another trigger to queue it again. Recheck cancellation andretryGenerationimmediately 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
|
Device gate (partial): deferral during payment review at Trigger: B pays A 1,000 sats over Lightning by contact while A has a contact-payment Confirm open. That runs
Every hold deferred the publication, and each way of ending the review published the retained update.
Still running: |
There was a problem hiding this comment.
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
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 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
left a comment
There was a problem hiding this comment.
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 SDKoperationInProgresscomes 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.xmlwith iOS first;review-refresh.xml.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at 2bbd2d2e after the merge base changed since the saved assessment. Coverage is the complete diff from merge base c1e06c20, with no inherited coverage.
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, withdrawal, and recovery stay available. 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 the first private-message retry delay, 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.
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None. The integration preserves the earlier preparation, backoff and retry-ownership fixes.
QA for 2bbd2d2: Three cases ran on iOS 26.5 simulators. Four passing cases are retained at their original tested revisions because their covered paths remain unchanged. Recordings are at 2×.
Test 1: payment review and PIN lifecycle ✅ Retained native coverage proves cancellation, later request arrival, unlock restoration and one authorized Bitcoin payment.
The device report at e9b45c2 covers all review-refresh.xml steps, including direct and amount→Bitcoin entry, confirmation/PIN waits, cancellation, later request arrival and app-lock restoration. The requester and regtest backend confirmed one 1,000-sat payment and an unpaid second request. Those Bitcoin paths remain unchanged.
The linked report contains both paired recordings.
Test 2: deferred forced preparation across background ✅ Retained lifecycle evidence proves the original forced operation waits while inactive and completes once after foreground return.
Test 2 at 0077196 exercises the actual preparation owner, ending review before the background-pause callback. Publication is injected; this proves ownership and real application lifecycle behavior, rather than network delivery. The covered owner and foreground-wait paths remain unchanged.
foreground-retained.2x.mp4
Retained lifecycle recording at
0077196.
Test 3: disable sharing or remove a deferred contact ✅ Retained cleanup evidence proves withdrawal completes during review without deferred republication.
Test 3 at 0077196 covers both variants through the actual cleanup owner with injected SDK operations. Cleanup and publication-owner paths remain unchanged. This does not claim a fresh Settings-screen or remote-withdrawal test.
cleanup-retained.2x.mp4
Retained sharing-disable and contact-removal recording at
0077196.
Test 4: admission and retry ownership ✅ Retained passing regressions preserve replacement retries across all three controlled MainActor interleavings.
The passing report at 7ed5b69 supersedes the earlier failed reproduction. It verifies cancellation after admission, retained retry identity and preservation of a replacement worker's handle. All covered admission/retry service and test files are identical in this delta. Its hosted recordings and Xcode assertions establish the result.
Test 5: link contact after resume ✅ A newly saved contact became request-capable after Home/resume and remained listed once.
I drove link-contact-after-resume.xml with two fresh native iOS identities and reciprocal sharing. After Contact Saved showed the peer's name, I pressed Home, waited five seconds and resumed. Request or Pay with enabled Request was first observed 119 seconds after Save, including tooling and sampled retries. Initial attempts preceded eligibility; this is an observed bound, not a link-completion benchmark. Request opened Amount. I closed without sending and verified one contact entry.
link-resume.2x.mp4
Save, Home/resume, sampled retries, Request Amount and final contact list.
Test 6: integration regressions ✅ The pristine head built and its focused hosted suites passed without failures or skips.
I checked the Xcode result bundle for scanner routing, migrations, USDT payment links, confirmation lifecycle, swipe guards and payment requests. The recording shows hosted execution; the assertions are established by the result bundle.
integration-regressions.2x.mp4
Hosted execution of the focused integration suites.
Test 7: hosted USDT address navigation ✅ Manual and clipboard inputs stayed in the Send sheet, reached USDT Amount and returned through the correct Back route.
Manual EVM input changed the heading to Send USDT; Continue opened Amount, and Back restored the entered address. Clipboard input opened Amount and returned to scanner/options on Back. Dismissing the sheet returned Home. This unfunded check did not exercise quote review, authorization or USDT submission.
usdt-route.2x.mp4
Manual-entry and clipboard navigation, Back and sheet dismissal.
Coverage: contiguous delta 7ed5b69..2bbd2d2, including the merged send routes and reached lifecycle, scanner, migration, receive and workflow changes, plus earlier findings. Receive wraparound, activity-copy UI and CI workflows were reviewed without a new device case. No listed case remains unrun; retained injected-operation cases do not establish fresh network delivery or a latency improvement.
Twin: synonymdev/bitkit-android#1626 fix: defer automatic Paykit work during payments at 5bd39a8 matches automatic-work deferral, retained retry intent and explicit cleanup. iOS retains its event-based waiting implementation.
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
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.
Stacked on: app-lock recovery #1039, above payment latency #1025.
Description
Out of Scope
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
a161084dwith disposable regtest wallets and a saved, linked fixture issuer. This source uses Paykit0.1.1-rc1and Core0.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
review-refresh.xml(inherited from fix: deliver Paykit payment proofs promptly #1025, unchanged) — review and PIN cancellation, background/reopen and later request arrival preserve one-payment behavior.Manual Tests
Automated Checks
PaykitSdkIdentityCheckTests.swiftcases — payment starting during the existing contention delay prevents a second Background SDK read.PaykitSdkOperationLockTests.swiftcases — explicit publication and withdrawal remain available during payment activity.PrivatePaykitServiceTests.swiftcases — 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.PaykitSdkOperationLockTests.swift— queued reads and automatic publication recheck admission after preparatory work without dirtying backup state.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
4530ff7bata161084dretains 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, tree8f1e72d3from #10707cd096f4plus #10818967dff8, #1084b7561c08and #1085310e689a, 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.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)