Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryRemoves Paykit key derivation before exporting the wallet backup.
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 |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
There was a problem hiding this comment.
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
refreshPaykitKeymeans the export uses whatever Paykit key the session provider holds. A stale or missing key fails withPaykitException.Identityrather 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_erasedpath, 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() } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Closing: this doesn't fix #1746. The export still needs the homeserver write lock (see review thread). |
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
export_backup_statein 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
Design
N/A — no UI changes.
Preview
N/A
QA Notes
Setup
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
Backup succeeded for: 'WALLET'and noget_paykit_app_registryretry warning — blocking a single host not in CapabilitiesAutomated Checks
PaykitSdkServiceTest.kt— the backup export does not load the Pubky secret key or call the App Registry; fails on master, passes with the fixEvidence
just compile,just testandjust lintpassed locally on 99b23c7.PaykitSdkService.ktand failed as expected.Gaps
Models used
claude-opus-5-5(reasoning: Not exposed)claude-opus-5-5(reasoning: Not exposed)Metadata note: the session exposes a numeric reasoning budget, not a named effort level.
🤖 Generated with Claude Code