Skip to content

feat: share paykit state across apps - #1401

Open
ben-kaufman wants to merge 34 commits into
masterfrom
codex/paykit-shared-runtime-local-20260930
Open

ben-kaufman wants to merge 34 commits into
masterfrom
codex/paykit-shared-runtime-local-20260930

Conversation

@ben-kaufman

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

Copy link
Copy Markdown
Contributor

Closes #1356

This PR moves Bitkit to Paykit's identity-wide shared state using the published 0.1.0-rc62 SDK.

SDK: https://github.com/pubky/paykit-rs/releases/tag/v0.1.0-rc62

Companions: iOS #856, Paykit Server #33.

Description

  • Uses encrypted homeserver state and one Encrypted Link per contact identity, shared by authorized Paykit apps.

  • Stores wallet reservations, pending payment proofs, and recovery backups locally while following shared request state and execution claims.

  • Retains broadcast transaction IDs before remote identity reads, defers publication for unavailable contact links, and preserves manually detached activity contacts through sync.

  • Saves one-time acceptance intent before the remote call and includes it in wallet backups, so interrupted acceptance and wallet restore can resume safely, including shared-state lock conflicts. Execution rechecks subscription state after asynchronous lookups, and acceptance IDs are removed only when shared state confirms payment, cancellation, or rejection.

  • Lets Bitkit authorize Paykit access, a watch-only account, or both as independently requested claims, without sharing wallet spending keys.

  • Shows the requested access in the authorization sheet. Server reconnect requests Paykit access without allocating another watch-only account.

  • Pays the exact endpoint supplied by a Payment Request, permits a fixed on-chain destination for unpaid recurring periods, and attributes received activity through its matching receiving address, including companion accounts used by Paykit Server. Unchanged history backfills are skipped, while missing transaction details and failed address derivation remain retryable.

  • Handles contact publication, requests, and subscriptions using app-owned endpoints inside the shared identity; resolves Paykit from GitHub Packages rather than mavenLocal().

  • Keeps local contact-sharing settings off when cleanup fails, retries withdrawals and registry updates, and discovers established publication recipients from shared state without adding unrelated unfinished links. Serializes private/public cleanup with sharing changes and coalesces foreground retries.

  • Includes app ownership fields when validating subscription proposals against the transport size limit. One-time and subscription proposals deliver to the selected recipient without draining unrelated peers.

  • Reuses validated Paykit keys and backup fingerprints for unchanged state, refreshes keys after identity errors, and retries session restoration during foreground maintenance.

  • Checks incoming private messages during the ten-second foreground poll without draining outbound work. Full synchronization runs on startup, explicit refreshes, and elapsed-time maintenance after 30 seconds, then every 60 seconds. Notification handling can reload saved requests without network intake. Temporary session-restoration transport and lock failures retain the saved session for retry instead of starting another auth flow.

  • Completes public payment setup before preparing private contacts in the background. Coalesces repeated preparation, reuses established links, backs off unavailable lookups, and invalidates pending publication during cleanup. Recipient-discovery timeouts apply to the lookup itself, not time queued behind other SDK work.

  • Reads public request capabilities up to eight contacts at a time outside the SDK mutation queue, without waiting for all-contact preparation. Private-message retries drain only the affected peers; idle cleanup skips unrelated work. Full private cleanup refreshes the registry once and retains retry state until that refresh succeeds.

Out of Scope

  • Guaranteed private-list withdrawal on contact deletion. Deletion blocks immediately even if withdrawal fails. The old list can remain at the peer, and registry cleanup can remain pending until the contact is explicitly re-added.
  • Migration from receiver-folder data.
  • Homeserver lock-finalization safety: the SDK cooldown is a mitigation, not a fix for a write completing after lock expiry.

Design

N/A — no design available.

Preview

QA Notes

Journeys

  • updated import-all-contacts.xml - Continue completes public setup without waiting for every contact to link; retest with 61 contacts and unavailable profiles.

  • new cancellation-during-confirmation.xml - a subscription canceled while confirmation is open cannot be paid after its cancellation is received.

  • new fixed-onchain-destination.xml - later unpaid subscription periods can use the request's fixed on-chain address, while paid periods remain unavailable.

  • new contact-payment-sharing.xml - disabling contact payments stays off after leaving and returning to Settings.

  • updated automatic-presentation.xml - linked contacts on separate identities automatically present new requests and defer them while another sheet is open.

  • new paykit-only-approval.xml - approves Paykit access without creating a watch-only account.

  • new paykit-reconnect.xml - renews server access without replacing its account or invoices.

  • new accepted-device-ownership.xml - only the accepting install can resume a one-time request after restart.

  • updated contact-request-or-pay.xml - contact payments and requests use identity-wide state.

  • updated delete-and-readd-contact.xml - deletion blocks private requests until the contact is explicitly re-added.

  • updated issuer-interoperability.xml - requests from another app retain their exact endpoint and request context.

  • updated payment-deadline-history.xml - shared request expiry and history remain consistent.

  • updated request-summary.xml - request details show the shared request and endpoint correctly.

  • updated wallet-leg.xml - authorizes the server, pays its exact request destination, attributes the received payment, and preserves manual detachment after sync and restart.

  • updated create-and-propose.xml - oversized proposals are rejected before sending, and shorter proposals use the shared identity and app ownership.

Manual Tests

  • Hold sharing withdrawal in progress, foreground the app, then request sharing on again. Cleanup must not overlap, and publication must wait for it to finish. Repeat with foreground cleanup already active; this requires fault injection.
  • Force a lock conflict after acceptance commits but before its response read, restart Bitkit, then refresh and retry the accepted one-time request. Lock fault injection is not a journey capability.
  • Inject private withdrawal and public/app-registry update failures, then disable contact payments. Both sharing settings must stay off and cleanup must remain pending until recovery, without re-sharing cleared endpoints. Repeat with only the public/app update failing, and with a recipient removed by another authorized app while Bitkit has no local contact cache.
  • Force-stop while a shared-state lock is held, relaunch, and keep the app foregrounded and connected. Session setup must recover after the lock expires without another resume or connectivity event.
  • Back up an accepted but unpaid one-time request, stop the original wallet, then restore on a replacement install and retry. Automated wallet backup/restore is not a journey capability. Running the same wallet on multiple devices concurrently is unsupported.

Automated Checks

  • added PaykitReceivedPaymentContactsTest.kt and ActivityServicePaykitContactsTest.kt - receiving-address attribution, rejection of unrelated outputs, companion-account lookup, and backfill cache invalidation.
  • updated PrivatePaykitContactResolverTest.kt and LightningServiceTest.kt - reservation/request ambiguity checks and account-specific derivation.
  • added RefreshContactPaykitLinkUseCaseTest.kt - refreshes an identity link without receiver selection.
  • added PaykitKeyGenerationTest.kt - initial generation selection, cached key reuse, remote rotation, rollback rejection, and invalidation after identity errors.
  • updated PaykitBackupStateTrackingTest.kt, ContactPaymentSettingsRepoTest.kt, and AppViewModelSendFlowTest.kt - cached backup fingerprints, uncertain-write checks, disabled sharing after cleanup failure, and foreground session-restoration retries.
  • updated PaykitSdkServiceTest.kt, PubkyRepoTest.kt, and PubkyAuthApprovalViewModelTest.kt - identity setup, authorizer access, separate or combined claims, and discovery timeouts that exclude SDK queue waits.
  • updated PaykitPaymentRequestRepoTest.kt, PaykitPaymentProofRepoTest.kt, and PrivatePaykitRepoTest.kt - exact destinations, execution ownership, publication, and cleanup failure reporting with retained retry state.
  • updated PaykitPaymentRequestPresentationStoreTest.kt, PaykitPaymentRequestRepoSubscriptionTest.kt, and AppViewModelSendFlowTest.kt - durable acceptance intent and wallet restore, identity-switch guards, and acceptance before LNURL invoice lookup.
  • removed RefreshContactPaykitReceiversUseCaseTest.kt - contact refresh targets the identity instead of discovering receiver folders.
  • ran the iOS/Android/server regtest flow on published rc58: Android paid a fresh 17,000-sat request to its exact address, the server confirmed it, and iOS attributed it to Buyer. All 37 simultaneous-sync readiness samples stayed healthy.
  • verified rc62 resolves from GitHub Maven without a local override.

All 3,314 unit tests passed against published rc62, and Kotlin compilation passed. Formatting and the complete Detekt report were checked against the base: no introduced findings. Coverage includes eight-way discovery with 61 contacts and a stalled peer, cancellation, wallet-wipe isolation, scoped retries, cleanup failure/retry and the existing identity, publication, payment and recovery tests.

Staging performance remains open. The last published rc61 one-contact run sent a request in 23-26 seconds, but Android's open request list showed it only after 4.9-6.4 minutes. Android's private withdrawal phase took about 49 seconds in an earlier rc61 run. rc62 fixes the separately reproduced multi-peer lease failure; it does not establish the remaining latency targets. These unfunded-wallet runs do not verify payment execution, complete Marketplace unlock, or the 61-contact, cold-start and backup-stall scenarios. The separate broadcast-outcome dependencies remain iOS #844 and Android #1384. The unchecked journeys above still need device QA.

@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 0/5

[High risk] Restructures Paykit payment state and authentication across the app.

The PR is not safe to merge until inbound attribution, private-sharing failure handling, and existing backup restoration are addressed.

Findings

  1. P1 Security Unrelated output misattributes payment ▶
  2. P1 Failed cleanup leaves sharing advertised ▶
  3. P1 Existing Paykit backups cannot restore ▶

Summary

The PR moves Paykit contact links, payment requests, authorization claims, and app publication to identity-wide shared state, and updates payment proofs and received-payment attribution. It also changes persisted Paykit backup formats. Review findings concern inbound contact attribution, private-sharing cleanup failures, and restoring existing backups.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Shared Paykit requests] --> B[Contact endpoint index]
  B --> C[Inbound activity attribution]
  A --> D[Bitkit request and payment flow]
  D --> E[Local pending proofs and wallet backup]
  F[Sharing settings] --> G[Private-list cleanup]
  G --> H[Published app capabilities]
Loading

Reviews (1) · Last reviewed commit: "docs: clarify paykit integration contrac..."

Comment thread app/src/main/java/to/bitkit/services/CoreService.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/models/PaykitPaymentStateBackup.kt
@greptile-apps

greptile-apps Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff

These findings could not be posted inline.

  • P1 Failed cleanup reports success app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt:315 ▶

    When private-list removal fails, this branch records a pending retry but exits with a successful Result. Callers disabling sharing therefore cannot detect that cleanup is incomplete and may report success while previously published private endpoints remain available.

@github-actions

github-actions Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 172cb30 (run).

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

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

Advice: ✅ Approve

Review: diff 91 files.
Pair PR synonymdev/bitkit-ios#856: equivalent.

Findings:
9 inline (1 MEDIUM, 8 LOW)

QA:
Tests queued.


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

Comment thread app/src/test/java/to/bitkit/repositories/PaykitPaymentRequestRepoTest.kt Outdated
Comment thread app/src/test/java/to/bitkit/repositories/PaykitPaymentProofRepoTest.kt Outdated
Comment thread app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt Outdated
Comment thread app/src/test/java/to/bitkit/repositories/PaykitPaymentRequestRepoTest.kt Outdated
Comment thread app/src/test/java/to/bitkit/repositories/PrivatePaykitRepoTest.kt Outdated
Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
Comment thread app/src/test/java/to/bitkit/repositories/PaykitReceivedPaymentContactsTest.kt Outdated
Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Fixed the cleanup reporting issue from this comment in e0ade20. Failed private-list withdrawal now returns a failure to callers while keeping the pending marker and cached publication for retry. The sharing preference stays disabled.

ovi-reviewer[bot]

This comment was marked as resolved.

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

No new actionable code findings.

Device testing needs a fresh wallet. Paykit presentation state, in-flight proofs, and wallet backups written with the previous receiver-path format do not decode. The author treats that migration as out of scope because Paykit has not launched; upgrading a development install that already stored that state leaves payment-request activation unfinished, and restoring one of those backups can leave later wallet backups blocked.

Android leaves a received transaction unassigned when its outputs match more than one contact. iOS #856 still assigns the reserved receiving-address contact in that case. See the open parity thread. This pass did not review the iOS PR.

CI build, lint, and the local e2e suites succeeded at this revision. This review inspected the changed payment, authorization, attribution, and persistence paths and their tests; it did not run those checks or a device. The Paykit journeys listed in the PR were not executed.

Suggested additional test cases

  • Android and iOS. Fresh wallets. Receive one transaction whose receiving output is a registered companion-account address for contact A and whose other output matches a request or reservation for contact B. Android should store the companion address and leave the contact empty. Record the iOS result before treating the apps as matched.

Device testing: not performed in this review.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One MEDIUM (two installs can pay one request twice) and two LOWs inline. Paykit is ungated on master, so these are user-facing from the next release.

Checked and clean:

  • Auth approval: the claim is parsed the same way at display, in the repo and in the service. authorize() re-checks the claim, clientId and permissions against the displayed state, approvalBootstrap pins the clientId, and a Paykit-only approval sends an empty account payload that validateAccountPayload enforces. Only derivePaykitIdentitySecretKey(generation) leaves the device, and the root key never does. Signup URLs reject companion claims.
  • Amount pinning is unchanged (acceptsPaymentAmount / acceptsLightningInvoice). Endpoints are filtered by acceptedPaymentEndpointIdentifiers, and ensurePaymentAllowed runs before both rails.
  • Received-payment attribution uses PAYEE records only, with network-checked addresses. Ambiguous matches stay unlabelled and an existing contact is never overwritten.
  • The key-generation floor is keyed per public key and cleared by Keychain.wipe(). withStateRevisionTracking finalises under NonCancellable, and restorePrivateContact re-blocks on failure.
  • The App Registry fetch per SDK call adds latency but no new offline failure: every shared-state transaction already reads the homeserver.
  • Not raised, because nothing reaches them today: execution claims are never released on abandon (releasePaymentRequestExecutionClaim has no call site), and a missing registry counts as generation 1 against the saved floor. Both start to matter once a second executor app, or key rotation, exists.

Non-blocking: is there a Figma frame for the new PaykitAccessSection in the auth approval sheet? Link it and I'll diff the implementation against it on the next pass.

Comment thread app/src/main/java/to/bitkit/services/CoreService.kt Outdated
Comment thread app/src/main/java/to/bitkit/data/keychain/Keychain.kt
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

The payment and backfill fixes are pushed in 2c05d91, with matching changes in iOS #856. All 3,110 Android unit tests passed, along with Kotlin compilation. The two-install journey is added on both platforms but has not been run on devices yet.

For the design question, no Figma frame was supplied for PaykitAccessSection. The PR keeps N/A — no design available.

@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 from the full review at deac999 (review), at 2c05d919. This pass inspected the execution-ownership and contact-backfill delta, including payment and history callers. Coverage of the rest of the PR is inherited from that baseline.

No new actionable code findings.

The two-install payment and full-history backfill comments are addressed at this revision. An accepted one-time request stays payable only on the install that stored that acceptance, including after restart, and the other install keeps it in history without pay controls. A finished contact backfill is skipped until the identity, activity, transaction details, or reservations change; missing details and failed address derivation still retry. Recurring periods are outside that guard: an install that has the active payer subscription can still pay one. This pass did not review iOS #856; the attribution parity thread is unchanged on Android in this delta.

Build and lint passed at this revision. The new ownership and backfill tests were inspected, not executed here. Local e2e still had pending suites at check time. The new accepted-device-ownership journey is the device check for this delta.

Device testing: not performed in this review.

Ready for device testing.

@piotr-iohk

piotr-iohk commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Staging e2e pass, including paykit suite: https://github.com/synonymdev/bitkit-android/actions/runs/36861644059 ✅

@ben-kaufman
ben-kaufman requested a review from piotr-iohk October 1, 2026 12:33

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

Follow-up at 2c05d91. One MEDIUM is still open: recurring periods are double-payable across installs. I replied on the existing two-install thread rather than opening a new one.

Resolved:

  • Two-install double pay for one-time requests. Every entry needs local ownership: auto-present, the list, notifications, retry, the hardware path through prepareContactPayment and authorizeHardwareContactPayment, and the software path through the pre-send ensurePaymentAllowed(forExecution=true), including LNURL-pay. The SDK accept runs first, the local id is persisted next, the identity is re-checked, and the final gate requires the persisted id. A stale PROPOSED snapshot on the second install fails at the SDK accept.
  • Backfill: PaykitReceivedPaymentContacts now has value equality, and the cache key includes the identity generation, activity revision and reservation attributionVersion. A mid-scan key change aborts the scan.
  • The ledger is keyed by normalized identity, cleared by Keychain.wipe(), and kept out of backups.

Not raised:

  • An accepted one-time request that no install owns, after a kill between accept and save or a restore to a new device, stays blocked until the payee cancels. That is the stated trade-off, and a payer-side cancel would reopen the race.
  • Activation failing closed on an unreadable ledger matches the existing subscription-store behaviour.
  • The orphaned-keychain thread is settled.

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 1, 2026 13:05
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-shared-runtime-local-20260930 branch from 098464f to ae0764a Compare October 1, 2026 13:30
@ovitrif

ovitrif commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

@ben-kaufman build check red

ovi-reviewer[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

@ovitrif I checked the red build. APK compilation passed. LightningNodeServiceTest failed during setup because Robolectric could not download android-all-instrumented:14-robolectric-10818077-i7 (Connection reset by peer), not because of a Paykit assertion or compile error.

The updated branch passes compilation and all 3,124 unit tests locally. The new push starts a fresh CI run.

Comment thread app/src/main/java/to/bitkit/services/CoreService.kt Fixed
@ben-kaufman
ben-kaufman requested a review from piotr-iohk October 2, 2026 15:53

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Advice: ✅ Approve

Reaudit: diff 21 files.
No new findings; the rest is in the review.
Retest suggested: Tests 1-2, J1, J2, J4, J8, J14 (Lock conflicts, cleanup, cancellation during confirmation, fixed addresses, message wait, proposal size, and contact-pay files changed).
synonymdev/bitkit-ios#856 at c5ae314 matches the 840/1001 proposal fixtures, fixed on-chain reuse, paid-period block, cancellation recheck, LINKED-only cleanup discovery, 30/60/120 private-message sync, and startup retries that skip inbox sync.

QA:
Tests queued.


Reviewed by grok-4.7-xhigh via gh-pr-review-loop skill

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

Review at the new head (2f42424).

New HIGH inline, found on device: a first-time private link can't finish inside the 60 s link lease, so every attempt fails and holds operationLock for 60–90 s.

Lock status for the four asks is on the existing thread. Burst exit is partial; retry keys, drain gating and initial-sync intake are fixed. About 3–5 min of continuous lock work remain after each foreground.

The cleanup and cancelled-subscription LOWs are fixed; the threads are confirmed.

Device gate (partial): iOS c5ae3143d against Android 8823181 on the staging homeserver.

  • The first link took about 9–16 min. Both sides saved each other at ~16:20 UTC. iOS logged about 8 lease-expired failures, the last at 16:28:51; Android's last failure was at 16:30:19. By 16:36 Contact Pay took the linked path.
  • Each failed call held operationLock for 60–90 s.
  • iOS cold launch with one saved contact: profile usable in 37 s, Contacts list in 82 s.
  • create-and-propose in steady state passes all steps; Propose took 52 s under the lock.
  • The other linked journeys and cancellation-during-confirmation.xml are next, at the maxAdvanceSteps = 1 heads.

Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggestion: 👍 Approve

Reaudit: diff 3 files.
No new findings; the rest is in the review.
synonymdev/bitkit-ios#856 at f8f45ca advances one handshake step per call, in PubkyService.swift at lines 755, 925 and 940.

Note

Retest Suggested J9

@ovi-reviewer retest J9

QA:
Tests queued.


Reviewed by grok-4.7-medium via gh-pr-review-loop skill

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggestion: 👍 Approve

Reaudit: diff 7 files.
No new findings; the rest is in the review.
Android and synonymdev/bitkit-ios#856 at d78aaa both drop the 14 by 2 second startup burst, take one handshake step per call, and resume the same 1, 3, 8, 20, 45, and 90 second drain ladder without restarting an active job.

Note

Retest Suggested J9

@ovi-reviewer retest J9

QA:
Tests queued.


Reviewed by grok-4.7-xhigh via gh-pr-review-loop skill

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Private messages are now received on every foreground inbox refresh, not just the slower maintenance rounds. Outbound processing and endpoint discovery keep their existing backoff. The matching fix is in iOS #856.

Tested against local Pubky 0.14, merged Paykit Server, and Bitcoin regtest: private payments and requests in both directions, a server invoice paid to its exact address, fresh server authorization, and Paykit-only reconnection retaining the existing watch-only account. The slow request sample was 67.5 seconds before the fix; a fresh request took 23.2 seconds afterward with the server stopped. These are individual debug-build measurements, not production percentiles.

All 3,181 unit tests pass and the E2E build succeeds. I inspected the Detekt diagnostics: the same 22 existing findings remain, with none added by this change.

Remaining test limits: no live Lightning payment, full content-unlock UI, or physical-device performance run. An invalid regtest spend exposed a pre-existing wallet/Core recovery problem: a failed outgoing transaction can leave its address marked used and block a later request to that unpaid address. I have not bypassed that safety check; reliable transaction reconciliation needs separate follow-up.

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 2, 2026 20:36

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

Review at the newest head. The lock-saturation thread stays open; the recount for these heads is on that thread and a staging rerun is in progress.

New LOWs, inline:

  • the 5-min Transport cooldown also stalls a handshake that is in progress

The wall-clock maintenance deadline is a refinement reply on the lock thread.

Comment thread app/src/main/java/to/bitkit/repositories/PrivatePaykitRepo.kt 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.

Device run at the newest heads found a HIGH regression, inline: on a fresh app start, a linked contact never becomes a payment-request target, so Contact Pay never offers Request. The deferred contact preparation now races target discovery for the SDK lock. The lock-load thread stays open; this head's numbers are on it.

}

privatePaykitRepo.prepareSavedContacts(state.contactKeys)
privatePaykitRepo.scheduleSavedContactPreparation(state.contactKeys)

@jvsena42 jvsena42 Oct 2, 2026 •

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.

HIGH (found on device, regression in eaf6a09): on a freshly started app a linked contact takes ~48 min to become a payment-request target. Until then Contact Pay never offers Request, and the other request entry points stay empty.

On staging at this head, 4 Contact Pay taps over 13+ min on the tablet and iOS never showed the Request or Pay sheet. Each went straight to the amount screen after 71–180 s, and the Payments tab had no Request button. The tablet logged Timed out inspecting payment request support for the iOS contact four or more times in 15 min. At 10a3a64, once warmed up, the sheet showed in 1–5 s.

Trace:

  • scheduleSavedContactPreparation (this line) and refreshKnownSavedContactEndpoints (PrivatePaykitRepo.kt:253-262) now return before the coalesced job publishes (:194-225).
  • So contacts activation (:764-765) and every maintenance round (:899-907) run refreshIncomingPaykitPaymentRequests and refreshPaymentRequestTargets(force = true) while that job holds the SDK operation lock for linkedPeers, syncPrivatePaymentListsWithReservations and the drain retries.
  • eligibleTargets gives each contact 5 s including the lock wait (PaykitPaymentRequestRepo.kt:1168-1169). On timeout it keeps previousTargets[publicKey], which is empty on a fresh process, so the counterparty is only added once a round happens to win the race. On the tablet the first Request or Pay sheet appeared 48 min after launch (22:07:59 UTC).
  • Each round recreates the same race. Contact Pay waits only 2 s (ContactDetailViewModel.kt:63), then falls through to Pay.
  • At 10a3a64 the publication was awaited before the inbox sync and the target refresh, so discovery ran on an idle lock.

Fix: expose the preparation Job and await it, or refresh targets when the job completes, before refreshPaymentRequestTargets(force = true) in the activation and maintenance paths. The Continue/import path can stay non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed the ordering in 0fdff06. Contact activation, polling startup, foreground refresh and maintenance now wait for the existing background preparation job before inbox and target discovery. Continue/import still returns without waiting for peer preparation. Added coverage that pauses preparation and checks that discovery starts only after it finishes. This needs a fresh staging retest before calling the missing Request option resolved.

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.

The ordering is fixed in 0fdff06: activation (:763), foreground (:791), polling start (:891) and maintenance (:907) now join the preparation job before intake and discovery. Continue stays non-blocking, and there is no deadlock: join releases serializedDispatcher, and the job only needs publicationMutex and the SDK lock.

Keeping this open for the staging retest. Discovery still has the 5 s timeout under the SDK lock. On a cold launch, the activation, polling-start and foreground paths each run their own intake after the join, and any LINKING key gets a 1 s drain retry. So the first round can still miss. It should land by the first maintenance round rather than 48 min later.

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.

Staging retest at 0fdff06, with iOS at 778d9f2c. The tablet still hadn't offered Request or Pay 11 min after a cold launch.

  • Probes at +5:56, +7:00 and +9:01 all went to the plain amount screen, after 117 s, 93 s and 129 s.
  • The contact deeplinks sent at +56 s and +3:56 were dropped: the contact screen never opened within 60 s.
  • On the same run, iOS offered the sheet at +6:39.
  • Tablet load is unchanged at ~171–178 homeserver requests/min.

Still probing; I'll update with the first success.

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.

Retest result at 0fdff06: the tablet never offered Request or Pay in 23 min after a cold launch.

  • Probes at +5:56, +7:00 and +9:01 went to the plain amount screen after 93–129 s.
  • Probes at +11:43 and +19:42 produced neither the sheet nor the amount screen within 152 s.
  • Over that stretch the log shows:
    • 22:48:00 UTC: Failed to refresh incoming Paykit payment requests [ConcurrentUpdate … Pubky resource is locked or changed]
    • 22:48:51: Failed to refresh private Paykit endpoints before payment … concurrent_update
    • 22:50:22: Timed out inspecting payment request support for both contacts
  • Some of the later probes overlap a slow contact-payments turn-off on the iOS side, which was still running after 10.5 min.

The ordering fix didn't get Android discovery through on staging. The 5 s discovery timeout under the lock still loses, and shared-state conflicts now also fail the refresh. Still HIGH.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed one concrete cause in bb4637d: the five-second discovery timeout included time queued behind other SDK operations, so a lookup could time out before fetching the registry. It now starts after acquiring the SDK lock. An actual lookup timeout still preserves a previously known target and marks discovery incomplete; cancellation still propagates.

The regression tests cover both queue contention and a stalled registry read. All 3,193 Android tests pass. This does not establish that the 23-minute staging delay is resolved; please keep this open for a retest on this head.

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.

Last sample at 0fdff06 (tablet → iOS at 778d9f2c). Send Request on the tablet failed twice with RequestUnavailable.

  • The Request or Pay sheet opened in 3 s, then Send Request (2,000 sats) was tapped at 23:34:34 UTC.
  • After 2 min 37 s: Timed out inspecting payment request support and Failed to create Paykit payment request [RequestUnavailable] (PaykitPaymentRequestRepo.kt:616).
  • A retry failed the same way 40 s later. Nothing reached iOS.

So the tablet listed iOS as a target but couldn't create the request, because the propose-time support check timed out. bb4637d's change, which starts the 5 s timeout only after the lock is acquired, targets exactly this. I'm retesting at bb4637d / 4dd4d6047 now.

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.

Staging retest at bb4637d / 4dd4d6047, cold launch at 23:49:40 UTC (tablet) and 23:50:12 (iOS):

  • iOS first offered Request or Pay at ~12 min 19 s after launch, 3.0 s from tap. That is slower than 778d9f2c (+6:39). Earlier probes fell back to plain Pay after 100–136 s, or produced nothing within 200 s.
  • Tablet still hadn't offered it at +13:47. Probes gave nothing within 151 s, or plain Pay after 90 s.
  • Most contact deeplinks sent to the tablet after a cold start were never handled; logcat shows only one Received deeplink.
  • Tablet load: 169/min in minutes 0–5 and 189/min in minutes 5–10, the same at every head so far.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tested with Paykit #171 locally: pubky/paykit-rs#171 (comment). Both directions completed request/payment flows, and a warm Android contact-pay check showed Request or Pay within 2.8s. This was a fresh one-contact pair on a local homeserver, not the staging contact set. #171 reduces idle receive from 12-13 shared-state transactions to one without rewriting encrypted state, but does not remove Bitkit waits behind full contact preparation or unrelated outbound work. Those staging timings remain open; the SDK improvement is separate and can be consumed after merge/release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in 4951920. Recipient discovery no longer waits for all-contact preparation. Public capability lookups run eight at a time outside the mutation queue while retaining cancellation, original contact order and per-peer failure handling.

The 61-contact regression lets 60 responsive peers finish while one remains stalled; it verifies the scheduling behavior, not staging latency. All 3,208 unit tests pass with no introduced lint findings. Keeping the staging eligibility report open for a comparable retest.

@ben-kaufman
ben-kaufman requested a review from jvsena42 October 2, 2026 22:23
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Pushed 75e5429. Sharing changes and foreground cleanup now use the same gate across both private and public withdrawal, so cleanup cannot overlap a later enable. New one-time and subscription requests also drain only the selected recipient, not every contact with queued messages.

All 3,200 unit tests passed; compile and lint checks passed with no introduced findings. These changes use rc59. The separate SDK #171 test results are here: pubky/paykit-rs#171 (comment). That reduces idle shared-state work, but neither the one-contact test nor these app fixes closes the staging performance issue. Please retest the same larger contact set and sharing-OFF flow on this head.

@jvsena42

jvsena42 commented Oct 3, 2026

Copy link
Copy Markdown
Member

@ben-kaufman conflicts

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Updated in dc73b29: conflicts with master are resolved, including its profile-cache, import-identity and prioritized public-read changes. The branch now uses published Paykit rc60, including paykit-rs#169, #170 and #171, and publishes the signed Noise-key authorization during authorizer setup.

Compilation and all 3,313 unit tests pass using the GitHub Maven artifact with no local override. The full lint report has no introduced findings. I am retesting staging request/sharing latency on this build; the earlier rc59 performance measurements should not be treated as results for rc60.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Updated to published rc62 in 172cb30. It includes the SDK fix for outbound batches exhausting peer leases while waiting on shared storage; the earlier batching and lightweight polling changes remain. All 3,314 Android unit tests passed against GitHub Maven without a local override. The staging APK contains the exact native library from that package, with no new lint or compiler findings.

This is not a staging performance sign-off. The latest rc61 run sent in 23-26 seconds but took 4.9-6.4 minutes to appear in Android’s open request list. rc62 fixes a separate multi-peer failure; the one-peer delay remains open, with results in #1406: #1406 (comment).

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: update paykit to the pubky 0.14 release

5 participants