Skip to content

fix: pause paykit work during profile deletion - #954

Open
ben-kaufman wants to merge 11 commits into
masterfrom
codex/ios-rc77-recovery
Open

ben-kaufman wants to merge 11 commits into
masterfrom
codex/ios-rc77-recovery

Conversation

@ben-kaufman

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

Copy link
Copy Markdown
Contributor

Closes #955

Related to #908 and #861; recovery and broader latency investigation remain open. Android counterpart: synonymdev/bitkit-android#1510.

This PR pauses automatic Paykit refresh and publication during profile deletion so they do not compete with explicit cleanup.

Description

  • Defers automatic contact refresh, Shop imports, request intake/presentation and endpoint publication during deletion, then resumes for a retained identity.

  • Receive/channel publication captures the starting session before preparation and rechecks cancellation, identity, deletion and sharing inside the existing shared endpoint lock. A delayed preparation cannot republish endpoints after deletion removes them. Already-admitted writes finish before removal; explicit cleanup is unchanged.

  • Matching paid-invoice invalidation and payment-proof bookkeeping remain enabled. Replacement stays pending after transient errors, and an older admitted write cannot acknowledge newer invalidation. Public refresh resumes without waiting for contact refresh.

  • Retains contact deep links during deletion and retries once preparation is available. Only confirmed absence of a stored identity permits signed-out public contact navigation. Running, deferred or failed restoration and unreadable credentials keep the URL pending without private reads or a scan error; restoration completion re-enters existing preparation. Signed-in contact refresh retains identity and cancellation checks.

  • Incorporates the merged backup fix and translations without rewriting history.

  • Retains a forced public Lightning refresh before channel work can suspend, including during deletion. Before authentication, the request is bound to the stored-identity revision and profile object; existing publication consumes it only after that same identity restores. Sign-out and identity replacement invalidate it, without a new recovery loop or authentication grant. An invoice revision plus session/sharing/cancellation checks prevent older preparation from restoring the invalidated cache.

  • Both wallet-backed publication paths re-arm pending reconciliation if an older public snapshot lands after an acknowledged replacement. Enabling sharing and rollback preserve that publisher-owned result; explicit removal acknowledgement is unchanged. This repairs eventual endpoint state; it does not prevent every transient stale write.

  • Each public publication prepares one wallet endpoint snapshot. Public invoice preparation retries once when only the invoice revision changes under the same eligible session. Repeated invalidation ends with a publication failure rather than false session loss. Session changes, disabled sharing/UI, cancellation, and deletion before or during the retry remain rejected; creation and decoding errors still propagate. Existing admitted writes and conservative publication acknowledgement are unchanged.

  • Both wallet-backed publishers acknowledge the invoice revision captured with the final Bitcoin endpoint snapshot, before later USDT preparation. A successful retry no longer schedules a redundant republish; newer invalidation still stays pending, including when the invoice is empty.

Out of Scope

  • SDK, storage/backup formats, dependencies, pending-payment protection and safety waits: unchanged; uses published RC77. Explicit sharing-setting transitions are not redesigned.
  • Recovery after interruption: remote contention, bounded initialization attempts and retry spacing remain separate. The original remote lock holder is unproven.
  • Overall latency and a maximum deletion duration: not guaranteed. Simulator samples are not a benchmark or complete resolution of either issue.

Design

N/A - no UI changes.

Preview

N/A - no UI changes.

QA Notes

Journeys

  • delete-profile.xml: reviewer 5549389182 reported 7/7 actions passed on preceding aac9d0b with zero contacts, persistent progress feedback, and no deleted profile returning on reopen. That pass and earlier d7e0e1f 62-contact runs do not exercise the current retry correction.

Manual Tests

  • Hold an admitted SDK operation, begin deletion and trigger contact/payment/maintenance events. Release it and verify admitted work and explicit cleanup finish without new automatic intake/publication. Hold receive/channel endpoint preparation, remove endpoints during deletion while revocation is held, then release preparation: endpoints must remain absent. A write already inside the shared endpoint lock must finish before cleanup removes it. These gates are outside journey Capabilities.

  • Hold invoice preparation or identity lookup before deletion and verify automatic public/private rotation stays deferred while matching invoice invalidation and receipt attribution complete. Native lock tests are not a payment/device pass.

  • Hold contact deep-link preparation and make cleanup fail before preparation returns. Verify the retained identity resumes automatic work and the link retries once without an error loop. Also verify successful deletion opens a retained public contact signed out without a QR error or private-contact read.

  • After deferred invoice replacement, fail deletion and the first resumed publication. Verify maintenance retries without another scene activation/payment. Older writes must not acknowledge newer invalidation; stale, cancelled or disabled-sharing failures must not mark new work pending.

  • With a saved identity, hold or fail restoration and open a saved-contact link: no Add Contact navigation, private-contact read or scan error; the URL remains pending. Complete recovery and verify existing-contact navigation once. A genuinely absent identity, including successful deletion, must still open the public contact. This requires controlled restoration beyond journey Capabilities.

  • Hold initial restoration or SDK admission with public contact payments enabled, disable sharing, then release: no new automatic publication. An admitted write may finish without consuming pending cleanup. No new device execution is claimed for this counterpart case.

  • Hold deletion, make a channel usable/ready, then fail deletion retaining the identity: the public invoice refresh survives, including an older held invoice creation. Skipped/failed publication stays pending; newer invalidation is not consumed by an older write, and cancelled/replaced/disabled-sharing work cannot refill the cache. Release old public snapshot X after replacement Y is acknowledged: normal reconciliation restores Y. Repeat through enabling sharing and rollback, including successful current publication clearing earlier pending work. Controlled gates are outside journey Capabilities; private channel-refresh behavior is unchanged.

  • Hold initial restoration before authentication, make a channel usable, then let restoration fail or publication remain blocked. Restore the same saved identity and retry normal publication without another channel event: the old invoice must be refreshed. Replacement identity, sign-out, cancellation and disabled sharing must not authorize the pending refresh. Controlled restore/publication gates are outside journey Capabilities.

  • Enable contact payments while the public invoice is invalidated during address preparation or invoice creation. One invalidation should re-prepare successfully without rollback or a redundant maintenance republish; repeated invalidation should stop after one retry. Invalidate again after the Bitcoin snapshot while later endpoint preparation is held, including an empty Lightning invoice: the newer refresh must remain pending. Hold the invoice-only retry, begin deletion, then release: no replacement publication or cache refill. Identity/session changes, cancellation, disabled sharing/UI and genuine creation/decode errors must still reject. These require controlled gates beyond journey Capabilities.

Automated Checks

  • PubkyProfileManagerTests.swift: deletion lifecycle and session/identity checks.
  • PaykitPaymentRequestServiceTests.swift: queued automatic intake versus explicit proof completion.
  • ReceiveInvoiceRefreshTests.swift: paid invalidation, deferred publication, retries, acknowledgement ownership, and receive/channel preparation racing deletion, cancellation, sharing and session replacement. The existing production endpoint lock orders admitted writes before cleanup.
  • WalletViewModelReceiveTests.swift: local receive details continue without authentication while automatic public publication stays blocked.
  • PubkyContactLinkTests.swift: retained URLs and real navigation-model routing without private reads when signed out, plus stale/cancelled preparation.

Final author validation at 417bccab: 2,643 native tests passed with exactly the six existing CI live/integration-suite exclusions, plus 314 focused tests. xcresult independently confirms all unique passing cases. Three full-native CLI per-test listings were omitted by interleaved output; xcresult and complete raw totals reconcile them. One test added, one strengthened, two existing snapshot gates adapted, none removed. The unchanged 21a8f59f production source failed both new boundary regressions. The endpoint builder now prepares one snapshot, retaining forced Lightning refresh and a single invoice-only retry budget. Repeated invalidation during that sole retry still rejects publication, retains pending work and preserves enable rollback. The second address-time invalidation from the old duplicate-call reproducer is no longer reached; it is not claimed as a post-fix executed rejection.

Full SwiftFormat lint retains 57 exact baseline findings against actual master bd099662; translations are byte-identical at zero warnings/errors. The complete compiler audit verified all 16 production-rebuild warnings and the one final-focused warning against the existing 77-warning reference: all 22 referenced source files are unchanged on actual master. This is verified reference reuse, not a fresh master compiler run. The incremental native run emitted no diagnostics. Independent correctness/native/polish review is clean on the exact committed patch. The full eleven-commit PR range is cryptographically signed and verified locally and on GitHub. No dependency, binding, persisted format, SDK operation lock or safety wait changed.

Fresh 417bccab hosted CI is pending. Previous 21a8f59f unit, build and translations passed; the unit job reported 2,645 native passes and 39 UI passes with four UI skips on its GitHub merge checkout, not this local branch's test inventory. Its E2E job117150149762 was queued before the push. Its integration job117141829556 was cancelled at the one-hour job limit after two real internal attempts each failed one of 34 tests with LDKNode.WalletOperationTimeout; the third attempt has no final result. Both annotations identify refund-lifecycle test line138, lightning.sync() after VSS restore/start and before refund deposit/mining. The exact operation inside sync and root cause remain unknown. Test/service/workflow/dependency sources match actual master, but this does not establish an unrelated/base failure. Actual master bd099662 passed all 34 integration tests on its first attempt, refund lifecycle64.273s; previous master afdc534b also passed34/34, refund49.697s. The timeout is not base-reproduced. No blind rerun, timeout/assertion/exclusion change or funded local live test was made.

Earlier aac9d0b passed all required CI, including actual E2E117022271919 and e2e-status117090359909; its integration passed34 tests first attempt, refund46.819s. Earlier d7e0e1f1 had two same-named refund-lifecycle timeouts before cancellation. Later passes do not establish a root-cause fix. Preceding 21a8f59f author validation remains historical:2,642 native/313 focused,57 baseline format findings and zero translation errors/warnings.

No new author-driven UI/device/payment test was run for this correction. Reviewer5551080932 approved preceding aac9d0b and reported seven controlled simulator cases using local hold/failure/logging hooks: force during failed deletion, force before failed restoration, eventual stale-snapshot repair, paid-invoice replacement after failed deletion/publication, sharing-toggle admission, refusal of preparation after deletion removal, and finish-before-removal of admitted writes. It also reproduced the enable/invoice LOW fixed here. Those results are historical for 417bccab, not a current device pass. Late-snapshot enable/rollback, held-restore sharing-toggle, replacement/sign-out/cancellation device ownership, and some pending-marker enable/rollback cases were not driven.

Reviewer5549389182's ordinary zero-contact journey was on aac9d0b; retained-link cases were carried from earlier d7e0e1f reviews, not re-driven here. Current manual checkboxes remain open. Native fixtures are not physical-device/mainnet or controlled remote acceptance. Private channel-refresh implementation is unchanged from master. Existing routing tests do not mount MainNavView or establish restoration-observer timing. Prior aac9d0b author evidence (2,638 native and 309 focused) remains historical. Initial fixture/toolchain failures and the superseded overbroad test command, interrupted after unit tests while entering extra UI configurations, are not counted as final passes.

The pre-USDT revision capture was source-inspected, not separately tested with a suspended USDT operation. Native tests hold the admitted endpoint write; neither those tests nor the code review establish device timing, remote acceptance or a measured speedup. The preceding b8a30780 correction passed 2,641 native and 312 focused tests and remains historical evidence.

The earlier publication-admission follow-up passed 2,627 native tests and focused receive/service checks. That evidence predates the current follow-ups and is not substituted for final validation above.

Historical simulator evidence at 1ae24c1b: reviewer 5544014702 completed ordinary deletion with one and 62 contacts and held-operation admission. Successful-deletion retained-link routing failed and was subsequently fixed. The failed-deletion run did not achieve cleanup failing before preparation returned, and no payment during deletion was tested. Earlier five Profile/contact-import UI smoke passes predate the follow-ups.

Earlier author timing-only samples on a preserved unfunded one-contact simulator recorded deletion at 33.13s versus 43.02/43.16s in two baselines, with zero new saved-contact-observer SDK admissions versus eight before; an admitted read completed. These are historical samples, not a final-head benchmark. Recovery still took 73.00s and recreation 43.58s. Timing hooks are absent. No payment, funding, broadcast or wallet reset was performed.

An earlier hosted navigation assertion passed on an unchanged rerun. Its original cause remains unproven; that is not evidence of a root-cause fix or complete manual acceptance.

The startup regression reproduced reuse of the old invoice across nine restoration/preparation/publication combinations on the preceding production source. Failure cases also lost pending repair. The final tests exercise real gated profile initialization with an adopted-reference fixture and narrow endpoint/preparation fakes; they do not establish mounted SwiftUI lifecycle timing or remote device behavior.

Reviewer comment 6216622041 reports historical d7e0e1f 62-contact deletions in 58.9 and 59.6 seconds versus 70.8 and 70.0 seconds on master eb3156a. Both builds left one private withdrawal pending and logged a capability-disable policy refusal, so these warnings predate this PR. Review5549389182 additionally reports historical aac9d0b zero-contact deletion at 30.4-31.4 seconds versus master 33.4-34.6 seconds. These are bounded simulator samples, not a general speedup or maximum-delay guarantee. No safety check or wait was bypassed.

@greptile-apps

greptile-apps Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

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

Summary

Pauses automatic Paykit work while profile deletion runs, then resumes it if the profile remains signed in.

  • Profile deletion pauses automatic Paykit work until cleanup ends.
  • Contact links retry after profile deletion ends.

Acknowledged scope: ben-kaufman explicitly leaves interrupted recovery, remote contention, retry spacing, and overall deletion latency open. The PR description also states that controlled failed-deletion/deep-link device acceptance remains unchecked.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Begin profile deletion] --> B[Pause automatic Paykit work]
    B --> C[Run explicit cleanup]
    C --> D{Profile still signed in?}
    D -->|Yes| E[Resume automatic work and retry retained contact link]
    D -->|No| F[Keep automatic work stopped]
    B --> G[Allow admitted work and payment proofs to finish]
Loading

Reviews (1) · Last reviewed commit: "fix: pause paykit work during profile de..." · Reviewed by Greptile

@ben-kaufman
ben-kaufman requested a review from ovitrif October 15, 2026 23:59

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No findings. Gates automatic Paykit work (contact refresh, Shop import, request intake and presentation, endpoint publication, reconciliation) on a new automaticPaykitSession that is nil while a profile deletion runs. The same gates hold for awaited work rechecked through canRefresh. Explicit proof refresh and the deletion's own cleanup keep currentSession. Reviewed 1ae24c1, full tier.

What I checked, and 6 candidates I ruled out

Read: the full diff, plus PubkyProfileManager session and sign-out paths, PrivatePaykitService deletion and reconciliation, PublicPaykitService.syncCurrentPublishedEndpoints, PaykitPaymentProofRefreshState, MainNavView deep-link preparation, and the three new tests
Call sites traced: retryPendingPaykitEndpointRemoval, refreshIncomingPaykitPaymentRequests, prepareSavedPaykitContacts, handlePendingPaykitSubscriptionNotification, syncReceivePublicPaykitEndpoints
CI: validate and Greptile green. Run Tests, Run Integration Tests and build-local were still pending when I reviewed. iOS does not build on this reviewer's Linux host, so this review is reasoned from the code and CI.

Ruled out

  • Resume burst after a successful deletion: the isDeletingProfile onChange handlers fire then too. Every path they call gates on automaticPaykitSession, which is nil once sign-out has cleared the auth state.
  • A rejected refresh leaving a stale refreshTask: the task now clears itself in a defer when canRefresh refuses it. testAutomaticRefreshPausesWithoutCancellingAdmittedWorkAndResumes covers both admitted and queued work.
  • Polling republish using pubkyProfile.publicKey instead of session.publicKey (AppScene.swift, full-mode round): the only call between the top-of-loop automaticPaykitSession guard and the republish is the synchronous startShopContactImport(), so the identity is the same.
  • Proof refresh stalled by deletion: automatic: false compares against currentSession, which deletion keeps until sign-out's session mutation.
  • Endpoint reconciliation skipped at launch while session restore runs: retryPendingPaykitEndpointRemoval now returns until the profile is authenticated. The authState onChange reruns it when the UI flag is off, and the UI-on paths run after foreground sync.
  • Deep-link retry loop after deletion: retryPendingContactAfterDeletion is cleared when handlePendingDeepLink starts. It survives only when that call bailed on an in-flight preparation, so the isPreparingPendingContactDeepLink handler fires at most one retry.

Merge confidence: 4/5, no findings, but the unit-test and build jobs had not finished and the branch was not built here.

@ben-kaufman

ben-kaufman commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Historical 1ae24c1b: the first Run Tests attempt had one failure: SendConfirmationLifecycleTests.testPendingWalletSwitchResumesAfterTags. Its visibility tracker still contained Tags and Confirmation after the coin picker appeared. The later navigation-path and preparation assertions did not fail, and the deletion regression tests passed.

The failing test and SendConfirmationView are unchanged by this PR. The exact cause is still unproven, and this specific failure has not been reproduced on the base.

Update: Ben's attempt 2 passed on unchanged head 1ae24c1b: 2,607 native tests and 39 UI tests passed, with four UI tests skipped. The previously failing testPendingWalletSwitchResumesAfterTags passed in 6.412s. No code or assertion change was made. E2E and e2e-status also passed, so all required CI on that historical head was green. The controlled manual cases remain unchecked.

Update on 21a8f59f (October16): integration38950937886/job117141829556 hit the one-hour job limit, but cancellation was preceded by two actual internal failures:34 tests/1 failure each, both BlocktankRefundAddressLiveIntegrationTests.testRefundAddressLifecycleAcrossOrdersRestartAndVssRestore with LDKNode.WalletOperationTimeout (77.911s/78.438s). Attempt3 was cancelled without a final result. Full log and annotations were inspected; both point to line138, the lightning.sync() after VSS restore/start and before refund deposit/mining. No test artifact was uploaded. The exact internal sync failure remains unknown.

The test, Lightning/Blocktank services, workflow and dependency lock match actual master. That is not proof the failure is unrelated: current master bd099662 passed34/34 on its first attempt, refund64.273s; previous master afdc534b also passed34/34, refund49.697s. This refund failure is not reproduced on base. It recurs from historical d7e0e1f1; intervening passes did not establish a root-cause fix.

The 21a8f59f unit job passed2,645 native tests and39 UI tests with4 UI skips on its GitHub merge checkout; the old navigation case passed6.669s. Build/translations passed, while actual E2E117150149762 was queued before the subsequent push. These are not new-head passes.

Current 417bccab fixes the separately reviewed duplicate endpoint preparation; it is not a refund-timeout fix. Its local314 focused/2,643 native tests passed with the six normal CI live/integration exclusions. Fresh hosted CI is pending. No blind rerun, weakened assertion/timeout/exclusion, new funded test or device reset was performed; controlled manual cases remain unchecked.

@piotr-iohk piotr-iohk mentioned this pull request Oct 16, 2026
5 of 8 tasks

@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 1ae24c1.

No new actionable code findings.

Automatic contact refresh, Shop import, request intake and presentation, and endpoint publication stay paused for the whole profile deletion, then resume only while that signed-in session remains. Admitted refreshes, explicit cleanup, and payment-proof completion keep running. A local receive invoice still commits when public publication is deferred. A contact link that arrives during deletion stays pending and is retried once preparation is free, including read-only routing after sign-out.

Compared with bitkit-android#1510 at bc00345 for the shared pause and resume contract. Android fails incoming request refresh closed for the entire deletion; this PR still allows explicit proof refresh and adds the contact-link retry. Journey actions match. Interrupted recovery and a maximum deletion duration remain out of scope, as stated on the PR.

Validation: the new tests in PubkyProfileManagerTests, PaykitPaymentRequestServiceTests, and ReceiveInvoiceRefreshTests were inspected and not executed here. On this revision, Run Tests, Run Integration Tests, e2e-tests, and Validate Translations passed. ovi-reviewer was still pending.

Device testing: not performed in this review.

Ready for device testing

Suggested additional test cases

  • iOS, Paykit UI enabled, disposable signed-in profile. Receive a contact deep link and complete profile deletion successfully before that link is routed. Expected: onboarding is shown, the link is kept, and it then opens through read-only contact routing once preparation finishes, with no repeated error.

@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 contact link opened during a deletion that succeeds ends in an error toast (Test 6).

Findings:

  • MEDIUM Bitkit/MainNavView.swift:437: when the deletion succeeds, this retry runs signed out. loadContactsIfNeeded(for: contactPublicKey) fails with Pubky shared state requires an active session, and showScanError() shows "Unable To Read QR" on the profile onboarding screen. The contact never opens, though the description promises read-only profile routing. On master the same link opened during a deletion shows the contact page with no error. The signed-out load fails on master for any contact link too; this retry is the new path into it.
  • LOW Bitkit/ViewModels/WalletViewModel.swift:1590: a paid-invoice replacement skipped during deletion gets one later attempt and no retry. That attempt is the resume Task in AppScene.swift:519-526. It runs once, after a contacts refresh, and only logs a failure. When a deletion fails on a homeserver error and that attempt fails too, the paid invoice stays published until the next foreground, channel event or payment.

QA for 1ae24c1:

Tested on an iPhone 17 simulator (iOS 26.5) with regtest wallets and staging Pubky profiles.

Test 1: delete-profile journey, one contact ✅ Deletion showed progress, reached profile onboarding in 20 s, and the profile stayed gone after reopening Profile and after a relaunch (UI snapshot).

While deletion ran, the screen showed a spinner and had no tappable control. Afterwards the Home header read "Profile" with no name.

t1-delete-profile-journey.2x.mp4
Test 2: delete-profile journey, 62 contacts after preparation settled ✅ Deletion reached profile onboarding 18 s after the confirm, and reopening Profile did not bring the profile back.

The 62 contacts were saved by link: 61 keys with no homeserver plus one real staging profile, not a Ring import. Deletion was confirmed nearly 5 min after the last save. The spinner showed while it ran, and the app log has no safety-wait line.

t5-62-contacts-settled.2x.mp4
Test 3: delete-profile journey, 61 contacts right after saving ✅ Deletion reached profile onboarding 15 s after the confirm, and reopening Profile did not bring the profile back.

The contacts were saved the same way on a recreated profile; one of the 62 keys timed out in the profile lookup before Save. Deletion was confirmed 1 min 43 s after the last save, while contact preparation was still running, and the app log has no safety-wait line.

t3-62-contacts-immediate.2x.mp4
Test 4: automatic Paykit work paused during deletion ✅ While the deletion's first request was held, a background and foreground and a contact link started no new Paykit work; admitted work finished and deletion reached onboarding.

The homeserver was held at the network for the first 14 s of the deletion, with open connections closed first so any new request had to reconnect.

  • No new homeserver connection opened (connection log).
  • The app log shows no contact load until the deletion ended.
  • A reconcile admitted before the confirm finished after the release.

On master, the same foreground during a held deletion started a contact load within a second, and a later foreground started a Shop import (master's app log).

t2-automatic-work-paused-during-deletion.2x.mp4
Test 5: failed deletion resumes work and retries the contact link ✅ After a failed deletion the identity stayed signed in, automatic work restarted at once, and the contact link was retried once and later opened the contact.

The contact link's preparation was held, deletion started, and the homeserver was then made unreachable.

  • Cleanup waited for the admitted preparation and failed only after the preparation timed out at 30 s. So the order in this PR's manual test (cleanup fails first) did not occur.
  • "Unable to Delete Profile" showed.
  • Contact refresh, Shop import, endpoint refresh and request intake restarted within 110 ms (app log).
  • The link was retried once, with one toast and no loop.
  • With the network back, the kept link opened the contact at the next foreground (second video).
t3-failed-deletion-resumes.2x.mp4
t3-retained-link-routes.2x.mp4
Test 6: contact link opened during a deletion that succeeds ❌ The link was kept, but after the deletion reached onboarding its retry showed "Unable To Read QR" and the contact never opened.

This is the case @piotr-iohk suggested in their QA review. The link was a saved contact's, opened 9 s into the deletion (Test 4's run, at its end). The log shows Failed to load contacts before routing contact link: ... Pubky shared state requires an active session. On master, the same link opened during a deletion showed the contact page with no error (second video), and the deletion then reached onboarding (UI snapshot).

t4-master-link-during-deletion.2x.mp4

Full review of 0ea3fcc..1ae24c1. No device case drove a payment during deletion, so the receive-invoice publish deferral (the LOW finding) was checked in code only.

Pair PR synonymdev/bitkit-android#1510 matches on the pause and resume: both defer automatic work during deletion, invalidate a paid public invoice locally and publish its replacement on resume. It differs in two places. Android also refuses incoming-request refresh for the whole deletion, where this PR still allows explicit proof refresh, as @piotr-iohk's review notes. The contact-link retry is iOS-only.

Comment thread Bitkit/MainNavView.swift
Comment thread Bitkit/ViewModels/WalletViewModel.swift

@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 at f6cc033, descendant of 1ae24c1, with the same comparison base and merge base 0ea3fcc. Inherited the completed baseline review for deletion pause and begin/end pairing, Shop import, polling, request intake and presentation, payment-proof refresh and receive-invoice deferral, unchanged tests and journey notes, and Android pause/resume parity from bc00345. This pass reviewed the follow-up delta: signed-out contact routing, cleanup-pending acknowledgement, resume ordering, the new tests and notes, and Android publication fences at eacdc265.

No new actionable code findings.

The two concerns in the changes-requested review are addressed in this source. After a successful deletion, a retained contact link skips the private contact read and opens Add Contact. A paid public invoice cleared during deletion stays pending, resumed publication runs before contact refresh, and a failed foreground publish remains pending for the existing maintenance poll. An admitted write leaves that flag set when its Lightning invoice is no longer current. That is source analysis; the simulator timing from the earlier review was not repeated on this commit.

Device testing: not performed in this review. Simulator evidence on the changes-requested review is for 1ae24c1 only.

Recommended before device testing: Run Tests and Run Integration Tests were still pending on this commit. validate had passed. This review did not execute the Swift tests. The author reports a local pass on the fix thread; that report is not execution by this review.

Android #1510 at eacdc265 uses the same acknowledgement rule, and both journey notes treat contact-link retry as iOS-only. Android also stops an in-flight public publish inside its mutex once deletion has started. iOS foreground refresh and reconciliation pass a session fence; receive and channel refresh still check deletion before the call. That pre-call check predates this follow-up.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Cross-platform follow-up in b368a3b: receive and both channel-publication routes now retain the starting session and recheck identity, deletion, cancellation and sharing after preparation, inside the existing endpoint lock. This closes the pre-lock gap where deletion could remove endpoints before delayed preparation republished them. Already-admitted writes still finish before cleanup; proof/bookkeeping and safety waits are unchanged. iOS already serializes SDK-queued publication with removal through that outer lock.

Three table-driven regressions reproduced the stale-publication/admission failures before the fence. Final 2,627 native tests and 39 focused tests pass. Full formatting matches the 57-finding master baseline; translations are 0 warnings/errors; no introduced compiler diagnostics. Independent review is clean, and all four PR commits are signed and GitHub-verified. Current master, including the merged backup fix, is included.

Fresh CI is pending. No new device/payment acceptance or general recovery-latency result is claimed; the controlled manual cases remain unchecked in the body.

@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 saved contact's link opened while the session restore has failed opens Add Contact, and Save then fails (Test 1).

The PR moved to b368a3b after this run; these results are for f6cc033.

Findings:

  • MEDIUM Bitkit/Utilities/MainNavRouting.swift:20: preparePubkyContactLink and routePublicContactIfSignedOut (:41) treat !isAuthenticated as signed out. A stored identity whose session restore failed or was deferred also has no publicKey.
    • Its saved contact's link then opens Add Contact as a new contact. The link is already consumed, so nothing re-routes after the restore.
    • Save fails while offline, and after the restore it shows "Invalid pubky key format".
    • On master the same link shows "Unable To Read QR", stays pending and opens the contact page once the session restores.
    • testSignedOutContactLinkDoesNotWaitForPrivateContacts asserts .addContact while a stored identity restores.
    • b368a3b leaves this routing and the session restore unchanged, so this still applies there.

Both findings of the earlier review are fixed in code, as @piotr-iohk's review also reads them:

  • After a successful deletion, the kept contact link skips the private contact load and opens Add Contact.
  • A paid public invoice sets the pending flag, and the maintenance round publishes a new invoice and acknowledges only a current write.

QA for f6cc033:

Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet and a staging Pubky profile.

Test 1: saved contact's link while the session restore has failed ❌ The link opened Add Contact for a contact already saved, and Save failed before and after the session restored.

The wallet had one saved contact. The app was cold-launched with the homeserver unreachable, so the restore failed (app log) while Home still showed the profile name (second video, same state on master).

  • Opening the contact's link showed Add Contact with Save.
  • Save showed "Failed to add contact" while offline.
  • With the network back, the session restored and contacts loaded (app log). The screen stayed on Add Contact, and Save showed "Invalid pubky key format".
  • On master, installed over the same wallet, the same launch and link showed "Unable To Read QR". The link then opened the contact's page once the session restored (second video).
t4-head.2x.mp4
t4-master.2x.mp4

Not driven at f6cc033, because the PR moved to b368a3b first:

  • The contact link kept during a deletion that succeeds (the earlier Test 6).
  • A failed deletion resuming work and retrying the kept link (the earlier Test 5). This push changed the resume order and the signed-in link preparation.
  • A payment on the public invoice during a deletion that fails, and its republish by maintenance (the PR's manual tests 1 and 3).

Tests 1-4 of the earlier review passed at 1ae24c1: the delete-profile journey with one and with 62 contacts, and the pause of automatic work during deletion. This push leaves the deletion and its pause gates unchanged.

Delta review: diff 9 files. No device case drove the publication acknowledgement in PublicPaykitService.swift or the paid-invoice pending flag in WalletViewModel.swift.

Pair PR synonymdev/bitkit-android#1510 at e8ade7d keeps a paid public invoice's replacement pending the same way. It has no contact-link retry or signed-out contact route, so the finding is iOS-only.

Comment thread Bitkit/Utilities/MainNavRouting.swift Outdated

@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 b368a3b: two LOWs inline. I found no HIGH or MEDIUM.

I reviewed this as a funds, auth and deep-link change (paid-invoice rotation, publication fences, contact link routing).

Checked and clean:

  • Paid invoices: rotatePublicPaykitInvoiceIfNeeded clears the local invoice and sets cleanup pending whatever the deletion state (WalletViewModel.swift:1502-1509). Only a write whose desired bolt11 equals the current publicPaykitBolt11, with the session still current, clears pending. Receipt attribution is not gated on automaticPaykitSession. Incoming requests cannot be presented mid-deletion: three checks require the session.
  • Identity: publication fences compare SignedInSession (key + revision). Sign-out bumps the revision before taking the endpoint lock. Signed-out routing only pushes .addContact with a validated key. saveContact and the endpoint reads need an SDK session, so nothing private is read or granted.
  • Lifecycle: performProfileDeletion resets with defer and calls endProfileDeletion() on both branches. The deletion Task is unstructured, so Back or dismiss cannot cancel it. Try again and Disconnect are fenced by the session revision. A link retained during deletion retries at most once.
  • Concurrency: every check, then await, then recheck site rechecks after the await. The PaykitPaymentRequestManager.refresh generation guard is correct. No nested endpoint-lock acquisition was added.

Performance-relevant: pause and resume of the poll and publication around the shared endpoint lock. To measure: confirm → onboarding with 1 and 62 contacts against master, SDK admissions during deletion, and the resume burst after a failed deletion.

⚠️ Not verified: the suggested fixes for both LOWs have not been run green. The reproductions fail on head as shown.

Device gate (partial): b368a3b — not run yet. Pending: journeys/profile/delete-profile.xml (1 and 62 contacts) timed against master, the failed-deletion resume count, and green runs of both suggested fixes. The device result will follow.

Each LOW can be fixed here or moved to a follow-up issue. Reply on the thread with which one.

Comment thread Bitkit/Utilities/MainNavRouting.swift
Comment thread Bitkit/ViewModels/WalletViewModel.swift
@jvsena42

Copy link
Copy Markdown
Member

Device gate (partial): d7e0e1f — profile deletion timing, 0 contacts. 62-contact runs still going.

iPhone 17 Pro simulator (iOS 26.5), Debug builds of master eb3156a and this PR, staging Pubky homeserver, same Wi-Fi. Each sample deletes a fresh profile created through Bitkit after ~60 s idle. Timed from tapping "Yes, Delete" to profile onboarding appearing (accessibility poll every ~0.25 s).

Build Sample Contacts deleted Sign-out Onboarding Total
master 1 +7.3 s +33.6 s +34.0 s 34.0 s *
master 2 +6.8 s +33.1 s +33.4 s 33.4 s
PR 1 +7.1 s +30.1 s +30.4 s 30.4 s
PR 2 +6.5 s +30.2 s +30.5 s 30.5 s

* Master sample 1 idled ~40 s instead of 60 s and was the wallet's first profile.

The PR is about 3 s faster than master. Most of the remaining time is the ~23 s between "Deleted all contacts" and sign-out, where the app logs nothing. That is the request count tracked in pubky/paykit-rs#197. Automatic Paykit work lines (publication, refresh, intake, endpoint, maintenance) between the tap and onboarding: 0 on all four runs. paykit_session loaded from keychain lines: master 52 / 49, PR 43 / 43.

Loading state, same on both builds:

  • The alert fades out.
  • For ~0.3 s the plain Edit Profile screen shows with no indicator.
  • Then a full-screen dim overlay with a centred spinner covers the screen; snapshot-ui reports 0 tap targets.
  • Profile onboarding replaces it. There is no completion toast, unlike Android's "Profile deleted".

The only WARN/ERROR in each window, on both builds: ERROR❌: Backup failed for: 'WALLET': identityChanged - BackupService, at sign-out.

@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 as a full reassessment at d7e0e1f1 because the comparison base changed since the saved assessment. No earlier coverage was inherited. The merge base is eb3156ab.

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

The current source still pauses automatic Paykit intake and publication during profile deletion and resumes it for a retained identity. The resolved contact-link and paid-invoice threads match that source (signed-out routing, paid-invoice pending, stored-identity routing). The open channel-refresh thread still matches this revision: discussion. The shared resume contract was compared with Android #1510 at 71d9ef7.

Run Tests succeeded on this commit. The new unit tests were inspected and not re-run here. They do not cover a forced channel refresh skipped during a failed deletion. Device testing was not performed. The author's manual checklist remains unchecked.

Suggested additional test cases

  • iOS unit: public sharing is on and a reusable public bolt11 is already published. Begin profile deletion, call refreshPaykitEndpointsAfterChannelAvailabilityChanged with forceRefreshLightning: true, then fail deletion so the same session remains. Resume publication with refreshIfNeeded: false. Expected: the published Lightning invoice is a new invoice, and the cached bolt11 is left unpublished.

Device testing: not performed in this review.

Findings

  • [LOW] Retain a forced public Lightning refresh skipped during failed deletion — inline at Bitkit/ViewModels/WalletViewModel.swift:1452.

Comment thread Bitkit/ViewModels/WalletViewModel.swift
@jvsena42

jvsena42 commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Device gate (partial): d7e0e1f — profile deletion with 62 contacts.

Run Total
62 contacts, delete right after import 58.9 s
62 contacts, after ~2 min idle 59.6 s
master eb3156a, 62 contacts, right after import 70.8 s
master eb3156a, 62 contacts, after idle 70.0 s

Both runs show the same overlay with spinner throughout and reach profile onboarding. On reopening Profile, the deleted profile does not reappear. paykit_session loaded from keychain lines: 94 and 95 (43 at 0 contacts). There was no uncertain-write cooldown; the Android twin hit a 300 s one on its idle run.

Both runs leave one contact's private endpoint withdrawal pending, and public cleanup is refused by policy at sign-out:

Log lines (UTC, idle run)
12:53:43.971 tap "Yes, Delete"
12:54:13.700 WARN⚠️: Private Paykit endpoint withdrawal remains pending for ["pubkygujx6qd..."] - PrivatePaykit [PrivatePaykitService+Contacts.swift: removePublishedEndpointsLocked(...) line: 428]
12:54:13.701 WARN⚠️: Failed to remove private Paykit endpoints before clearing session: privateUnavailable - EditProfileView.deleteProfile [PubkyProfileManager.swift: removePrivatePaykitEndpoints(context:) line: 1419]
12:54:40.107 WARN⚠️: Failed to remove public Paykit state before clearing session: Policy(code: "policy_error", context: "cannot disable Paykit app capability private_payments while app-owned work is still active") - PubkyProfileManager.signOut
12:54:43.577 onboarding shown

The run right after import logs the same three warnings, plus Failed to inspect private Paykit identity during contact link retry: unknown/Swift.CancellationError at +4 s.

Master logs the same three warnings in both 62-contact runs, so they predate this PR. With 62 contacts the PR is ~11 s faster than master; most of the gain is in private endpoint removal (PR fails it at +28–30 s, master at +33.5–37 s). Further deletion timing is tracked in synonymdev/bitkit-android#1522 and pubky/paykit-rs#197.

@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. No findings in this push, and all seven device cases pass at d7e0e1f.

The PR moved to aac9d0b after this run; these results are for d7e0e1f.

The PR moved to aac9d0b after this run, so these results are for d7e0e1f.

Findings: none.

The MEDIUM from our review of f6cc033 is fixed. A saved contact's link now waits through a failed session restore and opens the contact once the session restores (Test 1).

QA for d7e0e1f:

Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet, a staging Pubky profile and a channel bought from staging Blocktank. Tests 5 to 7 add local wait hooks to the head build, which are not part of this PR. The hooks hold endpoint preparation, a write inside the endpoint lock and the session revocation. That stages the holds your manual tests describe.

Test 1: saved contact's link while the session restore has failed ✅ The link waited through four failed restores with no Add Contact and no toast. It opened Bob's contact page once the session restored.
  • After a cold launch with the homeserver unreachable, the restore failed four times. Home still showed the profile name.
  • For 66 s after the link opened, Home stayed, and the app log shows no contact load or routing.
  • With the network back, the session restored and contacts loaded. The link was then routed once, to Bob's contact page, and Back returned Home.
t1.2x.mp4
Test 2: failed deletion resumes automatic work and retries the kept link ✅ The profile stayed signed in, the link was retried once, and the kept link opened Bob's contact page at the next foreground.
  • Bob's link preparation was held, the deletion was confirmed, and the homeserver was then made unreachable.
  • Cleanup waited about 35 s for the held preparation, then failed. "Unable to Delete Profile" showed.
  • The link was retried once, with one "Unable To Read QR" toast and no loop. The public endpoint refresh ran before the contact refresh (app log).
  • As at 1ae24c1, cleanup did not fail before the preparation returned, because cleanup waits for admitted preparation.
t2.2x.mp4
Test 3: paid public invoice during a deletion that fails ✅ A later maintenance round replaced the paid invoice on the homeserver, with no foreground and no new payment.
  • The homeserver's Lightning endpoint held invoice A. The deletion was confirmed while the homeserver was held, and the LSP paid A while the deletion ran (app log).
  • The homeserver was then made unreachable. The deletion failed, and so did the first resumed publication.
  • The app already held a new invoice B with the cleanup flag set, while the homeserver still served A.
  • With the network back, the first maintenance round failed on "acquire Pubky write lock". The next round published B, about 86 s after the network returned, and the flag cleared.
  • Checked by reading the endpoint from the homeserver, the app's stored invoice and flag, and the app log. The recording shows the deletion and its failure.
t3.2x.mp4
Test 4: contact link opened during a deletion that succeeds ✅ After sign-out, the kept link opened Bob's public profile (Add Contact) once, with no toast.
  • Bob's link opened 2 s after the deletion was confirmed. It was routed once, right after sign-out.
  • The app log shows no routing failure and no contact load after the deletion. Back showed profile onboarding.
t4.2x.mp4
Test 5: contact payments turned off while publication waits ✅ No endpoints were published after the change, and the cleanup flag stayed set.
  • The held-restore variant cannot be staged. While the restore is running or has failed, the "Enable payments with contacts" toggle is hidden, because it renders only while pubkyProfile.isAuthenticated (GeneralSettingsView.swift:136).
  • A receive publication and a maintenance round were held before admission, and contact payments were then turned off. The change waited for the held maintenance round.
  • After the release, the homeserver had no endpoints, public sharing was off and the cleanup flag was still set. All three were unchanged 75 s later.
  • The change ended with an error toast: "cannot disable Paykit app capability private_payments while app-owned work is still active". On this wallet, the successful deletion in Test 4 had logged Bob's private payment list withdrawal as still pending. The SDK refuses to disable private payments while a contact still holds a shared list. Master does the same (see below).
t5.2x.mp4
Test 6: delayed preparation cannot republish endpoints a deletion removed ✅ Both held publications were refused after the deletion removed the endpoints, and the homeserver stayed empty.
  • A payment started a receive publication. Its preparation was held, and so was the maintenance round's preparation.
  • The deletion then removed the endpoints, and its session revocation was held.
  • After the release, both publications were refused with sessionNotActive (app log). The homeserver had no endpoints 12 s later or after the deletion finished.
t6.2x.mp4
Test 7: a write already inside the endpoint lock finishes before the deletion removes it ✅ The deletion waited for the admitted write, then removed the endpoints and finished.
  • A receive publication was admitted and held inside the endpoint lock, and the deletion was then confirmed.
  • For 25 s, the endpoints stayed on the homeserver and the deletion did not reach revocation.
  • After the release, the write finished with no publication failure. The deletion then removed the endpoints and reached onboarding.
t7.2x.mp4

A deletion with a saved contact leaves that contact's private payment list withdrawal pending on master too:

  • @jvsena42's 62-contact runs show it at d7e0e1f.
  • On master eb3156a, a fresh profile with one saved contact logged the same pending withdrawal and the same private_payments refusal at sign-out. Its deletion left app-registry.json and shared-state.bin on the homeserver, the same as this PR's head on a fresh profile.

Carried, not re-driven:

  • Our review of 1ae24c1 covered the delete-profile journey with one and with 62 contacts, and the pause of automatic work during deletion (Tests 1 to 4). Later pushes leave the deletion and its pause gates unchanged.
  • @jvsena42 timed the journey at d7e0e1f against master.

Delta review of f6cc033..d7e0e1f: diff, this PR's own changes in 9 files plus two master merges. No device case drove the channel-availability publication route. The session fence that Test 6 exercised on the receive and maintenance routes covers it in code.

Pair PR synonymdev/bitkit-android#1510 at 71d9ef7 keeps a forced Lightning refresh that a deletion deferred. At d7e0e1f this PR still dropped it (@jvsena42's LOW). aae3dff and aac9d0b address that after this run, and no case here drove them. The contact-link routing is iOS-only.

jvsena42
jvsena42 previously approved these changes Oct 16, 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 aac9d0b. No HIGH, MEDIUM or LOW left on my side.

Delta since my code scan at b368a3b:

  • Contact link during a deferred restore: fixed in d7e0e1f. MainNavRouting.swift:19-21,47-48 keeps the link pending while a restore runs or credentials are still stored. It routes signed-out only when hasStoredIdentity() is false; a Keychain error counts as stored. The renamed testContactLinkDistinguishesSignedOutFromRestoringIdentity and the new testStoredIdentityContactLinkRoutesSavedContactAfterRecovery cover .restorationDeferred and .restorationFailed.
  • Forced channel refresh during deletion: fixed in aae3dff and aac9d0b. When sharing and the Lightning option are on, WalletViewModel.swift:1459-1471 invalidates the cached invoice (revision bump) and sets cleanup pending. A stored identity that is not signed in yet gets a refresh behind storedIdentityGuard(). Nothing is cleared when sharing or Lightning is off. The guard is dropped for a replacement identity (createIdentity, adoptRingIdentity, completeSignupAuthentication, clearLocalState and signOut bump the revision). The new state is @MainActor.
  • acknowledgePublishedEndpoints (PublicPaykitService.swift:419-434) re-arms pending when the written bolt11 is not the current one. The revision fence (:1404-1410, :1421, :1521) stops an older invoice creation from refilling an invalidated cache.
  • The adaa830 master merge has an empty --remerge-diff.

Device gate: aac9d0b — delete-profile.xml: 7/7 actions pass (0 contacts at aac9d0b; 0 and 62 contacts at d7e0e1f). The deleted profile does not return on reopen, and the overlay with spinner stays up for the whole wait. perf: delete profile 30.4–31.4 s at 0 contacts (master 33.4–34.6 s) and 58.9–59.6 s at 62 contacts (master 70.0–70.8 s), no automatic Paykit work in the deletion window. Further timing is tracked in synonymdev/bitkit-android#1522 and pubky/paykit-rs#197. l10n: no UI text changed.

⚠️ Not verified on device: the README's manual checks (holding an admitted SDK operation or the endpoint preparation during deletion). They need operation gates the journey Capabilities table does not list; unit tests cover them.

CI at aac9d0b: unit, integration, build and validate are green. e2e-tests-local was still running when I posted.

@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 at aac9d0bd, covering aae3dff9 and aac9d0bd since d7e0e1f1 (baseline review). Inherited baseline coverage is profile-deletion pause and resume of automatic Paykit refresh, contact import, request intake, and endpoint publication; contact deep-link retention during deletion, restoration, and signed-out routing; and payment-request refresh gating versus explicit payment-proof completion. This pass rechecked public publication acknowledgement, contact-payment cleanup, and forced public Lightning refresh through failed deletion and session restoration.

No new actionable code findings.

The prior forced-refresh concern (discussion) is addressed at this revision. An authenticated channel refresh invalidates the cached public invoice and leaves cleanup pending before the deletion guard returns. After a failed deletion, the next current publication builds a replacement invoice. An older admitted write leaves cleanup pending when its snapshot revision no longer matches the current invoice. Enabling contact payments, and restoring a previous sharing preference, keeps that publisher-owned result. A channel refresh before sign-in stays with the stored identity across a held, deferred, or failed restoration, including a skipped or failed publication, and is discarded after sign-out, identity replacement, or a new session for the same key. Android #1510 at 9fddab92 requests that startup refresh once restoration yields a sign-in. The iOS journey text keeps private endpoint refresh on its existing path.

Run Tests passed on this commit. The new unit tests were inspected and not re-run locally. The device pass reported by ovi-reviewer is for d7e0e1f1.

Ready for device testing

Device testing: not performed in this review.

@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. One LOW in this push, and all seven device cases pass at aac9d0b.

Findings:

  • LOW Bitkit/ViewModels/WalletViewModel.swift:1421: the new invoice revision check reports a replaced invoice as a lost session.
    • Turning on "Enable payments with contacts" runs refreshPublicPaykitEndpoints. If the public invoice is invalidated while it runs (a payment lands on the cached public invoice, or a channel becomes usable or ready), :1421 or :1521 throws PubkyServiceError.sessionNotActive.
    • ContactPaymentsService.setEnabled rethrows it because the change is still current. Settings shows "Error: No active Pubky session" and the setting turns back off, though the session is fine. The rollback also removes the endpoints the payment had just republished.
    • The foreground, maintenance and channel callers return quietly on sessionNotActive; the enable path does not.
    • Reproduced with a payment on the cached public invoice and a local hold that only widens the window between the revision capture and the check (video and toast below). The channel trigger was not driven. Retrying the preparation once when only the invoice revision changed would avoid it.
t8.2x.mp4

The forced-refresh LOWs from @jvsena42 and @piotr-iohk at d7e0e1f are fixed at this head: a forced refresh during a failed deletion and one before sign-in both publish a new invoice (Tests 1 and 2).

QA for aac9d0b:

Tested on an iOS 26.5 simulator (iPhone 17) with a regtest wallet, a staging Pubky profile and channels bought from staging Blocktank. The head build adds local hooks that are not part of this PR. They hold or fail the deletion, hold endpoint preparation, a snapshot before the endpoint lock, a write inside the lock and the session revocation, and log which path consumed a kept refresh. Invoices are named by a short hash of the bolt11 read from the homeserver.

Test 1: forced channel refresh during a deletion that fails ✅ A channel that became ready during the deletion cleared the public invoice, and after the deletion failed the homeserver got a new invoice with no further channel event.
  • The homeserver served invoice 7ff43b48, and the deletion was held.
  • A new zero-conf channel from Blocktank became ready during the hold. After the delayed forced refresh, the app's stored invoice was empty and cleanup was pending, while the homeserver still served 7ff43b48.
  • The deletion then failed ("Unable to Delete Profile") and the profile stayed signed in.
  • Within 10 s the homeserver served a new invoice 3113f569, and cleanup pending cleared.
  • Checked by reading the homeserver endpoint, the app's stored invoice and flag, and the app log.
t1.2x.mp4
Test 2: forced channel refresh before sign-in ✅ The channel became usable while the session restore was failing, and after the restore the publication replaced invoice bdb0d6e5 instead of reusing it.
  • Cold launch with the homeserver unreachable: the restore failed three times while the channels reconnected. The app logged that it kept the refresh for the stored identity.
  • With the network back, the session restored. The publication that reuses a valid invoice consumed the kept refresh, and the homeserver then served a new invoice 1c106302 with cleanup pending false.
  • No channel event happened after the launch. A run without the log lines showed the same replacement.
t2.2x.mp4
Test 3: an older endpoint snapshot landing after a paid-invoice replacement ✅ The paid invoice came back on the homeserver for about 24 s, then maintenance republished the replacement with no new payment or foreground.
  • A foreground publication's snapshot with invoice eca3ff3d was held before the endpoint lock.
  • The LSP paid eca3ff3d, and the replacement 7fbc5152 was published.
  • Releasing the snapshot wrote the paid eca3ff3d back, and cleanup became pending again.
  • 24 s later the homeserver served 7fbc5152 again and pending cleared. That window is the transient stale write the description of this PR allows for.
t3.2x.mp4
Test 4: paid public invoice during a deletion that fails ✅ The first resumed publication failed, and maintenance published a new invoice 58 s after the network returned.
  • The LSP paid the published invoice 7fbc5152 while the deletion was held. The app cleared its stored invoice and set cleanup pending, and the homeserver kept 7fbc5152 while the deletion ran.
  • With the homeserver unreachable, the deletion failed, and so did the first resumed publication and the next maintenance round.
  • With the network back and no foreground or payment, the homeserver served a new invoice 4c6ac3ea and pending cleared.
t4.2x.mp4
Test 5: contact payments turned off while publications wait, then on again ✅ Once the change applied, the endpoints were removed and nothing republished them. Turning it back on published new endpoints with cleanup pending false.
  • A payment left the receive publication and a maintenance round held before admission. Contact payments were then turned off, and the setting waited for the held maintenance round.
  • After the release, the held receive publication, admitted while sharing was still on, published first. The change then removed the endpoints and turned sharing off, and nothing republished them over the next 80 s.
  • Turning the setting on again published new endpoints, and cleanup pending stayed false.
t5.2x.mp4
Test 6: delayed preparation cannot republish endpoints a deletion removed ✅ The held publication was refused after the deletion removed the endpoints, and the homeserver stayed empty.
  • A payment's receive publication was held in preparation. The deletion removed the endpoints, and its session revocation was held.
  • After the release, the publication was refused with sessionNotActive (app log).
  • The homeserver had no endpoints 14 s later, and still none after the deletion reached profile onboarding.
t6.2x.mp4
Test 7: a write already inside the endpoint lock finishes before deletion removes it ✅ The deletion waited 42 s for the held write, then removed the endpoints and finished.
  • A payment's publication was admitted and held inside the endpoint lock, and the deletion was then confirmed.
  • While the write was held, the homeserver kept its endpoints and the deletion did not finish.
  • After the release, the write finished with no publication failure. The deletion then removed the endpoints and reached profile onboarding.
t7.2x.mp4

Not driven:

  • A late snapshot while contact payments are being enabled, and during a failed enable's rollback (the second half of the seventh manual test). Both go through the same acknowledgement Test 3 drove on the maintenance path; checked in code only.
  • The held-restore variant of the sixth manual test: the contact payments setting is hidden until the session restores (GeneralSettingsView.swift:136).

Carried, not re-driven:

Delta review of d7e0e1f..aac9d0b: diff, 4 production files and 2 test files. No device case drove:

  • A kept refresh dropped after sign-out, identity replacement, a new session for the same key or cancellation. testStartupChannelRefreshCannotTransferToAnotherIdentityOrIneligibleCaller covers it, and it passed on the test simulator.
  • A successful enable clearing a cleanup flag that was already set, and a rollback that restores sharing on. Checked in code only.

Pair PR synonymdev/bitkit-android#1510 at d46bd6c keeps the forced refresh as a request that the next current publication consumes, for a deletion and for the first sign-in, matching this PR. I found no invoice revision check in its endpoint preparation that would fail an enable.

Comment thread Bitkit/ViewModels/WalletViewModel.swift Outdated
jvsena42
jvsena42 previously approved these changes Oct 16, 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.

Re-approving at b8a3078 ("retry superseded public invoice preparation"), with one LOW inline.

  • ovi-reviewer's LOW (WalletViewModel.swift:1421, the enable failing with "No active Pubky session" on invoice invalidation) is fixed. The retry is bounded (for attempt in 0 ..< 2, :1415) and retries only an invoice-revision mismatch. Session, sharing, UI and cancel changes still throw sessionNotActive, and attempt 1 rejects both deletion flags (:1419, :1422).
  • No paid invoice is re-advertised. The retry re-reads hasReusablePublicPaykitInvoice and canCommitInvoice after every suspension, and the acknowledgement still re-arms pending on a revision or bolt11 mismatch. The cross-identity fences are unchanged.

Device gate: b8a3078 — no re-run needed. This delta changes publication logic only, with no UI or journey change. delete-profile.xml passed 7/7 at aac9d0b and d7e0e1f. perf: at most one extra invoice creation per publication (bounded retry), plus the redundant republish in the LOW.

The LOW can be fixed here or moved to a follow-up issue. Reply on the thread with which one.

Comment thread Bitkit/Services/PublicPaykitService.swift Outdated

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Follow-up at 21a8f59f, covering changes since aac9d0bd against the prior QA review. Comparison base and merge base are unchanged at eb3156ab. This pass reviewed the one-shot public-invoice retry, its rejection for deletion, cancellation, identity or session changes, and disabled sharing or Paykit UI, contact-payment enable and restore, and acknowledgement of the revision captured with the Bitcoin/Lightning snapshot before USDT preparation. Inherited coverage is profile-deletion gating, contact-link retention, payment-request gating, forced public-refresh retention, and the startup comparison with Android PR 1510 at 9fddab92.

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

A revision-only supersession retries once inside a single refreshPublicPaykitEndpoints call and no longer fails enable as a lost session. Both publishers acknowledge the revision taken after that snapshot returns and before USDT preparation, so a successful retry does not schedule a redundant republish, and a newer invalidation stays pending, including an empty Lightning invoice. That is the capture point requested on b8a30780.

Android PR 1510 was compared only at 98cab12b for this flow. It keeps a pending Lightning refresh across publication attempts and does not retry invoice creation on a revision change. That is not an iOS defect, and it is not a review of that PR.

Validation is source inspection at this pin. I did not run tests or a device. Hosted Run Tests passed on this commit. Integration was still running and end-to-end was queued. Device testing: not performed in this review. Simulator results cited on the PR remain historical for aac9d0b and earlier commits.

Findings

  • [LOW] Enable accepts a second invoice supersession — inline at Bitkit/ViewModels/WalletViewModel.swift:1415.

Comment thread Bitkit/ViewModels/WalletViewModel.swift

@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.

Re-approving at 417bcca. No findings. This round was reviewed on Opus because Fable credits ran out.

  • My LOW is fixed in 21a8f59. Thread replied.
  • 417bcca makes one refreshPublicPaykitEndpoints call per buildWalletEndpoints, forced when refreshIfNeeded && includeLightning (:582-583), instead of a forced call followed by a second snapshot call with its own retry budget. That fixes @piotr-iohk's "enable accepts a second supersession": a second invalidation inside the single retry throws publicationFailed with bolt11 cleared and pending set, and enable rolls back (testForcedPublicationUsesOneInvoiceRetryBudget, testEnablingContactPaymentsRetriesInvoiceSupersessionOnce).
  • No paid invoice is reused. Attempt 1 still forces a new one, and a payment after the commit bumps the revision so the ack keeps pending. No shared state across sessions was added, the canCommitInvoice guards are unchanged, and no locks were added.
  • Parity: Android #1510 also prepares once per publication. It acknowledges by comparing bolt11 rather than by revision.

Device gate: 417bcca — no re-run needed for this delta (publication logic only, no UI or journey change). delete-profile.xml passed at aac9d0b and d7e0e1f. perf: one fewer refreshPublicPaykitEndpoints per forced publication (read from code).

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

QA review

Scope: Follow-up review of changes since 21a8f59f, at 417bccab, against the completed baseline review. Comparison base and merge base remain eb3156ab. This pass covered the single public-endpoint snapshot, its one invoice-revision retry, failure when that same preparation changes again, rejection for deletion, cancellation, identity or session replacement, and disabled sharing or Paykit UI, contact-payment enable and restore, revision acknowledgement before USDT preparation, the updated ReceiveInvoiceRefreshTests, and the unchanged profile journey paragraph. Android preparation was compared at 98cab12b. Inherited baseline coverage is profile-deletion pause and resume, contact deep-link retention, payment-request gating, forced public-refresh retention, and the startup comparison with Android PR 1510 at 9fddab92.

No new actionable code findings.

buildWalletEndpoints now prepares one wallet snapshot and forces a Lightning refresh on that call when a refresh is requested and Lightning publication is enabled. A revision change during address preparation or invoice creation retries once. A further change in that call fails publication, keeps cleanup pending, and leaves the session in place. Enabling contact payments stays enabled after one successful retry and restores the previous preference when publication fails. The acknowledged revision is the one taken with that snapshot before USDT preparation, so a newer invalidation stays pending, including when the Lightning invoice is empty. Android PR 1510 at 98cab12b also prepares the address and Lightning invoice once per publication. iOS still retries once when only the invoice revision changes.

Validation: source inspection at 417bcca. The updated tests were read and not executed. Run Tests, Run Integration Tests, and build-local were still in progress on this commit.

Device testing: not performed in this review.

Recommended before device testing: wait for Run Tests and Run Integration Tests on 417bcca.

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.

bug: profile deletion competes with background paykit work

4 participants