Skip to content

fix: export wallet backup without paykit registry - #1784

Closed
jvsena42 wants to merge 2 commits into
masterfrom
fix/wallet-backup-registry
Closed

jvsena42 wants to merge 2 commits into
masterfrom
fix/wallet-backup-registry

Conversation

@jvsena42

@jvsena42 jvsena42 commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Status: does not fix the issue yet. A device run (homeserver blocked) and review show the Paykit backup export takes a homeserver write lock in PubkySharedStateStorage, so WALLET still retries with acquire Pubky write lock (request timeout). Removing the registry fetch is not enough. Kept as a draft until the approach is decided.

Fixes #1746
Twin: synonymdev/bitkit-ios#1219

This PR stops the WALLET backup from fetching the Paykit App Registry, so it completes while the Pubky homeserver is unreachable.

Description

  • Exports the Paykit SDK backup state without first deriving the Paykit identity key. Deriving the key fetches the App Registry from the homeserver, so a homeserver outage failed every WALLET backup while METADATA, ACTIVITY and WIDGETS kept backing up. The SDK export (export_backup_state in paykit-rs v0.1.1-rc2, the pinned version) is a single local storage transaction and needs neither the key nor the network. The runtime-generation check, priority handling and payment deferral around the export are unchanged.

Out of Scope

  • Paykit setup on cold start: setup still derives the key from the registry, and the backup export waits for setup. If the app starts while the homeserver is down, the WALLET backup is still expected to fail until setup succeeds. Found by reading the code, not reproduced; changing how setup derives the key offline is a separate change.
  • Backup retry backoff: fix: retry a backup that fails on a busy paykit lock #1552 (merged before 1337f12) already doubles the temporary-failure retry delay from 10 s up to 5 min, and open fix: preserve pending backups across restart #1689 reworks retry ownership and clears the retry counters on reset while keeping that schedule. The issue's "every 10 s" was not reproduced here and is left to those PRs.

Design

N/A — no UI changes.

Preview

N/A

QA Notes

Setup

  • Dev flavor (regtest) against staging, a wallet with a Pubky identity set up, and a way to make the homeserver unreachable without cutting other traffic, for example an HTTP proxy on the emulator that blackholes the homeserver host.

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

  • 1. Wallet with a Pubky identity, app already running → block the homeserver host → change wallet state so a WALLET backup is scheduled (receive or send) → the log shows Backup succeeded for: 'WALLET' and no get_paykit_app_registry retry warning — blocking a single host not in Capabilities

Automated Checks

  • added PaykitSdkServiceTest.kt — the backup export does not load the Pubky secret key or call the App Registry; fails on master, passes with the fix

Evidence

  • just compile, just test and just lint passed locally on 99b23c7.
  • The new test was run against master's PaykitSdkService.kt and failed as expected.

Gaps

  • Case 1 was not run on a device: no homeserver-blocking setup was run for this PR.

Models used

  • Planning/scoping: claude-opus-5-5 (reasoning: Not exposed)
  • Implementation: claude-opus-5-5 (reasoning: Not exposed)
  • Review: Not performed

Metadata note: the session exposes a numeric reasoning budget, not a named effort level.

🤖 Generated with Claude Code

jvsena42 and others added 2 commits November 5, 2026 07:04
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Removes Paykit key derivation before exporting the wallet backup.

  • Wallet backups export Paykit state without contacting the App Registry.

Cold-start backups during a homeserver outage remain explicitly out of scope, as acknowledged by jvsena42. Tests were inspected but not run during this review.

Reviews (1) · Last reviewed commit: "fix: export wallet backup without fetchi..." · Reviewed by Greptile

@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 17ef309 (run).

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

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 finding (1 P1). Drops withPaykitKey around the Paykit SDK backup export so the WALLET backup no longer fetches the App Registry. Reviewed 17ef309, light tier (3 changed lines of code plus a test), with the paykit-rs side read at the pinned tag.

The App Registry fetch does go away, but the export still needs the homeserver: with the storage adapter Bitkit uses, every SDK storage transaction takes a remote write lock and reads the encrypted state blob. So #1746 is not fixed by this change. See the inline comment.

What I checked, and 2 candidates I ruled out

Read in full: PaykitSdkService.kt (exportBackupState, withPaykitKey, refreshPaykitKey, paykitKey, handle, completeSdkCall, PaykitSdkSessionProvider), PrivatePaykitRepo.backupSnapshot, BackupRepo retry path; paykit-rs v0.1.1-rc2 paykit-ffi/src/sdk.rs, paykit-ffi/src/storage.rs, paykit-sdk/src/backup/mod.rs, paykit-sdk/src/storage/{mod,pubky_shared}.rs, paykit-lib/src/pubky_routing/locks.rs
Call sites traced: exportBackupState has one production caller, PrivatePaykitRepo.backupSnapshot (via BackupRepo WALLET snapshot)
Build: not built here. On the Bitkit track a review does not run the PR's Gradle build, so this is reasoned from the code and from CI.
CI: build-and-test, build-local, lint green at review time; e2e still running.

Ruled out

  • Export without a fresh key after a key-generation change: dropping refreshPaykitKey means the export uses whatever Paykit key the session provider holds. A stale or missing key fails with PaykitException.Identity rather than writing wrong data, and I could not build a normal sequence where the export runs between a generation change and the next keyed call, so not posted.
  • Same behaviour in later rc tags: v0.1.1-rc5 has the same transaction > run_operation_erased path, so bumping the pin alone does not change this.

Merge confidence: 3/5, one P1: the change does not deliver the fix it claims.

Reviewer model: claude-opus-5-5, cold reader xiaomi/mimo-v2.6-pro
Reasoning effort: Not exposed (the review session reports no effort setting)

if (priority == Priority.Background && isPaymentSubmissionActive.value) return@withLock null
val state = withPaykitKey { completeSdkCall { it.exportBackupString() } }
// Export reads local SDK storage only; deriving the Paykit key would fetch the App Registry.
val state = completeSdkCall { handle().exportBackupString() }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Export still takes a homeserver write lock, so WALLET backup still fails offline

Bitkit builds the SDK with withPaymentAdapterAndPubkySharedStateAndClientConfig, which wires PubkySharedStateStorage (paykit-ffi sdk.rs:250 at v0.1.1-rc2, the pinned version). With that adapter export_backup_string is not a local read: backup::export_backup_state calls storage.transaction, which with no active operation goes through run_operation_erased (pubky_shared.rs:381). That needs the Paykit identity secret in the session access, takes the remote lock with with_write_lock (storage.lock(...), an HTTP call to the homeserver, locks.rs:170), then reads the pending markers and the encrypted state blob from the homeserver. With the homeserver blackholed (manual test 1), the get_paykit_app_registry error goes away but the export fails at acquire Pubky write lock, and the WALLET backup retries as before. The changelog line is not delivered. The new test passes only because PaykitSdk is a mock, so it cannot see this path. Making the backup survive an outage needs a change below this call, either in the SDK (an export that can serve the last observed state) or in how BackupRepo treats a Paykit transport failure.

Verified by: traced exportBackupString through paykit-rs v0.1.1-rc2 (sdk.rs:349 > runtime/backup.rs:11 > backup/mod.rs:240 > storage/mod.rs:170 > pubky_shared.rs:517-523 > run_operation_erased > with_shared_state_lock > locks.rs:170). A cold reader from another model family checked the same trace and found it holds.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Confirmed. This change removes the App Registry fetch but does not fix #1746, and the test passes only because PaykitSdk is a mock.

Device run (Android, 17ef309, rooted google_apis emulator, homeserver 34.65.231.81 dropped with iptables at 12:36:43): before the block the WALLET backup succeeded (12:34:07). After it, WALLET retried at 10 s, 20 s and 40 s, each failing with acquire Pubky write lock (request timeout), while ACTIVITY, METADATA and WIDGETS kept succeeding. The backoff from #1552 works; the export itself still needs the homeserver.

I'm leaving this thread open and the PR as a draft until we decide where the fix belongs: below this call (an SDK export from a local snapshot, or a separate backup category for Paykit state), or closing #1746 as working by design.

@jvsena42
jvsena42 marked this pull request as draft November 5, 2026 10:38
@jvsena42 jvsena42 self-assigned this Nov 5, 2026
@jvsena42

jvsena42 commented Nov 5, 2026

Copy link
Copy Markdown
Member Author

Closing: this doesn't fix #1746. The export still needs the homeserver write lock (see review thread).

@jvsena42 jvsena42 closed this Nov 5, 2026
@jvsena42
jvsena42 deleted the fix/wallet-backup-registry branch November 5, 2026 14:29
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: wallet backup retries every 10s while the homeserver is down

2 participants