Repository navigation
fix: decouple wallet restore from profile refresh - #1121
ben-kaufman wants to merge 8 commits into
Conversation
|
Triage: 3.1.0, with its Android twin synonymdev/bitkit-android#1686. |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P1). Moves profile initialization off the wallet-restore critical path and blocks an identity-free WALLET backup while saved credentials exist. Reviewed ec20b26, full tier. Reasoned from the code and CI only: this review did not build the branch or run the iOS suite.
What I checked, and 4 candidates I ruled out
Read in full: the diff; PubkyProfileManager initialization, recovery and resolveSessionInitialization; BackupService wallet payload, runBackup, runScheduledBackup, isTemporaryBackupFailure; PaykitSdkService.currentPublicKey, importSessionLocked, signInLocked
Call sites traced: scheduleInitializationAfterRestore (AppScene restore path), backupSnapshot (WALLET payload), hasStoredIdentityForBackup
CI: SwiftFormat green, Run Tests still pending at review time
Ruled out
- A fresh backup missed after activation:
importSessionLockedandsignInLockedboth callmarkWalletBackupDataChanged(), and the BackupService listener re-marks WALLET as required, so a delayed activation gets its fresh upload. - A stale restore task reviving an old identity: both the scheduled task and
beginInitializationre-checksessionRevisionbefore and after awaiting, andinitializeSessionStatedrops a result from an older revision. - A second restore scheduled for the same revision: the
restoreInitialization?.revisionguard makes it a no-op, andbeginInitializationhands back the in-flight task. - Malformed UTF-8 credentials let through:
hasStoredIdentityForBackupreads rawKeychain.loadbytes, unlikehasStoredIdentity'sloadString, so a non-UTF-8 entry still counts.
Merge confidence: 3/5, one P1.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the runner does not report a reasoning setting)
| ) async throws -> (state: String?, sdkVersion: UInt64) { | ||
| let initialSdkVersion = await sdkService.backupStateVersion | ||
| guard try await sdkService.currentPublicKey() != nil else { | ||
| guard try !hasStoredIdentity() else { throw PubkyServiceError.sessionNotActive } |
There was a problem hiding this comment.
P1: a session that cannot be restored blocks every WALLET backup
The guard is right for the short window before activation, but it fires the same way once resolveSessionInitialization returns .restorationFailed. That happens with a saved session and no reachable secret key (PubkyProfileManager.swift:2056-2061), or when re-sign-in fails with an error that is not temporary (:2074). initializeSessionState handles that case with clearAuthenticatedState(clearCachedProfile: false) and leaves the keychain alone (:394-399). The credentials stay, the SDK has no identity, and from then on every WALLET payload throws sessionNotActive here.
sessionNotActive is not in BackupService.isTemporaryBackupFailure. runScheduledBackup therefore ends with .failed and does not reschedule. Each later data change marks the backup required again and fails again. Until the user signs in again or removes the profile, the remote WALLET record stays at its last upload, including the parts unrelated to Paykit: transfers, watch-only accounts, reservation indices and payment-proof state. Before this change, the same state produced a backup with paykitSdkBackupState: nil.
The tests cover an identity that later activates (testUnresolvedStoredIdentityCannotReplaceCompleteWalletBackup), not one that never will. One fix: fail closed only while restoration is pending or deferred. Once it has definitely failed, carry the previous paykitSdkBackupState forward. Either way, add a test with .restorationFailed and retained credentials that checks a later wallet change still uploads.
There was a problem hiding this comment.
Confirmed: after terminal restoration failure this guard leaves complete WALLET backups pending, including later ordinary wallet changes. The Android twin has the same availability limitation. I am keeping this thread open while addressing it.
I cannot safely carry an arbitrary cached paykitSdkBackupState forward yet. The current WALLET payload includes consumed/private receive state, reservation ceilings, payment requests/proofs and active payment attempts; a stale SDK snapshot has no established consistency contract with those newer records. Returning nil is also unsafe: the restore path treats absent SDK state as clearing Paykit state. The guard prevents a complete backup from being replaced by such a payload.
The ongoing cross-platform follow-up is an independently recoverable ordinary-wallet record, with explicit restore selection, identity/version binding, migration and concurrent-writer rules while preserving complete WALLET. Older apps currently ignore additional records and do not enforce the envelope version, so the old-reader compatibility policy must be settled before publishing this split. The existing independent USDT recovery record is unaffected. I will not mark this finding resolved or claim full-wallet backup independence until the replacement path has controlled outage/restart/restore tests.
There was a problem hiding this comment.
At ec20b26 the terminal-restoration limitation still holds: saved credentials remain, and an absent SDK identity still makes the WALLET snapshot throw sessionNotActive. The controlled app test confirmed that a temporary unavailable identity preserves the remote WALLET bytes and pending work, then permits a complete backup after credentials recover. That recovery result does not resolve this terminal-failure availability issue.
There was a problem hiding this comment.
At 754a899 I drove a terminal/no-secret case through the real resolver, then reserved a new ordinary watch-only account index. WALLET upload failed, leaving the prior remote bytes/version unchanged and newer work pending; restoring the original credentials allowed upload. On this composition that probe failed with the SDK Identity error before the sessionNotActive guard. J1 separately confirmed sessionNotActive protection and recovery from a temporary unavailable identity. The terminal availability limitation remains open.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- No cross-identity leak on wipe and restore: the static
clearLocalStatebumps the revision, and the state tree is recreated. - Identity isn't lost for good: the retry paths cover a failed background activation. The only lasting block is the WALLET backup already raised in the open P1 thread.
- No payment path starts before the identity exists: Paykit starts only when
authState == .authenticated, so the header stays signed-out or loading until the background activation lands. - Twin synonymdev/bitkit-android#1686 shows the restored identity at once and fills profile and contacts in afterwards.
Performance-relevant (the purpose is a faster restore). wallet/restore-during-identity-outage.xml needs fault controls that aren't provisioned, so the gate uses a proxy. It restores a regtest wallet whose backup carries a pubky session, then:
- times Restore →
restoreWalletBackupAndStartreturn, head vs master, with the homeserver reachable and with it blocked; - checks that profile and contacts arrive afterwards;
- counts
Paykit session restored forper restore (expected 1); - runs Settings → Backup once the identity is active.
Device gate (partial): ec20b26 — not run yet; pending that proxy run. Device results will follow in a separate review.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 💬 Comment — the acknowledged backup availability issue remains open.
Findings: No new findings. I confirmed that the existing HIGH finding still applies: complete WALLET backups remain blocked after terminal identity restoration failure while credentials remain stored. The successful temporary-fault recovery below does not resolve that case.
QA for ec20b26: iPhone 17 Pro, iOS 26.5 Simulator, staging regtest. I built the unmodified head, ran its focused native suites, then drove J1 with local Debug fault controls over the real SDK and VSS paths.
Test 1: J1 — restore, backup protection, restart and stale profile ✅ All six actions passed with the fault controls described below.
- Required recovery reached a running wallet while final identity initialization remained held and authentication stayed idle. I left restore and copied a regtest receive address, confirmed from the simulator clipboard. Releasing initialization restored the identity and allowed fresh complete backups.
- With SDK session access removed and malformed local session bytes, the backup failed with
sessionNotActive. The remote WALLET bytes and version stayed unchanged, and backup work remained pending. Restoring the original credentials allowed a complete backup with SDK state and cleared pending status. These assertions were checked in the app logs. - Killing the app during held initialization preserved saved credentials. After restart and release, the saved contact remained a single entry; the complete logs contained one contact-add event.
- Disconnecting while a real profile response was held invalidated that response. On release, its normal guard reported
can apply: false, and the identity-choice screen remained visible. - A malformed retained WALLET payload kept required recovery pending, the node stopped and wallet actions unavailable. Repairing that original payload and retrying reached the wallet while optional initialization was still held. Node and authentication states were checked in the app logs.
| Required recovery and optional initialization | Backup protection |
|---|---|
J1-required-focus-2x.2x.mp4 |
J1-backup-2x.2x.mp4 |
| Required failure, retry and wallet access while identity work is held; focused excerpt, 2×. | The real backup rejects unavailable identity data, then succeeds after credential recovery; assertions checked in logs; 2×. |
| Restart with a saved contact | Late profile response |
|---|---|
J1-contact-restart-focus-2x.2x.mp4 |
J1-stale-profile-2x.2x.mp4 |
| One contact before and after process restart and identity recovery; focused excerpt, 2×. | Disconnect while the profile read is held, then release it and retain the signed-out UI; 2×. |
The test wallet had no funds. No funded-payment replay, USDT-specific restore failure, live homeserver blackout or head-versus-base timing comparison was driven. The native suites cover coalescing and queued identity replacement separately.
Check 1: focused native regressions ✅ The selected suites passed with no failures or skips.
I ran xcodebuild test with -only-testing:BitkitTests/PubkyProfileManagerTests and -only-testing:BitkitTests/PrivatePaykitServiceTests on the unmodified head and inspected the finalized XCResult. These cover held initialization, coalescing, stale completion, replacement credentials, malformed stored bytes and backup retry. The simulator build and git diff --check also passed.
Local test hooks and deciding log lines
Not pushed. Holds wait for explicit file removal. The backup control temporarily replaces only this disposable wallet's local credential input and restores its original bytes afterward; it uses the actual SDK, backup writer and VSS record. The required-recovery control corrupts a local retained payload, then restores its original captured bytes before retry. SDK results and recovery decisions are not mocked.
required recovery complete; scheduling final identity
final identity initialization held
wallet start returned: running, authenticated: idle
backup fault staged; SDK identity absent: true
Backup failed for: 'WALLET': sessionNotActive
blocked backup: bytes unchanged: true, version unchanged: true, pending: true
recovered backup: SDK state present: true, pending: false
required restore failed; pending: true, node: stopped
original retained WALLET repaired before retry
profile response released; can apply: false
diff --git a/Bitkit/AppScene.swift b/Bitkit/AppScene.swift
index c39b57cf..8956b76f 100644
--- a/Bitkit/AppScene.swift
+++ b/Bitkit/AppScene.swift
@@ -1169,8 +1169,14 @@ struct AppScene: View {
widgets.loadSavedWidgets()
widgets.objectWillChange.send()
+ #if DEBUG // test hook
+ Logger.info("QA1121 required recovery complete; scheduling final identity", context: "TestHook") // test hook
+ #endif // test hook
pubkyProfile.scheduleInitializationAfterRestore()
await startWallet(completingBackupRestore: true)
+ #if DEBUG // test hook
+ Logger.info("QA1121 wallet start returned: \(wallet.nodeLifecycleState), authenticated: \(pubkyProfile.authState)", context: "TestHook") // test hook
+ #endif // test hook
}
private func retryWalletStart() async {
@@ -1244,9 +1250,21 @@ struct AppScene: View {
private func restoreVssBackup() async -> Bool {
do {
+ #if DEBUG // test hook
+ let repair = Env.appStorageUrl.appendingPathComponent("qa1121-repair-required") // test hook
+ if FileManager.default.fileExists(atPath: repair.path) { // test hook
+ let data = try Data(contentsOf: Env.appStorageUrl.appendingPathComponent("qa1121-valid-wallet")) // test hook
+ try Keychain.upsert(key: .paykitPendingBackupRestore, data: data) // test hook
+ try FileManager.default.removeItem(at: repair) // test hook
+ Logger.info("QA1121 original retained WALLET repaired before retry", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
try await BackupService.shared.performFullRestoreFromLatestBackup()
return true
} catch {
+ #if DEBUG // test hook
+ Logger.info("QA1121 required restore failed; pending: \(BackupService.shared.hasPendingWalletRestore()), node: \(wallet.nodeLifecycleState)", context: "TestHook") // test hook
+ #endif // test hook
app.toast(error)
return false
}
diff --git a/Bitkit/Managers/PubkyProfileManager.swift b/Bitkit/Managers/PubkyProfileManager.swift
index d29dd6b1..08a427bd 100644
--- a/Bitkit/Managers/PubkyProfileManager.swift
+++ b/Bitkit/Managers/PubkyProfileManager.swift
@@ -1208,6 +1208,16 @@ class PubkyProfileManager: ObservableObject {
let loadedProfile = try await Task.detached {
try await resolve(pk)
}.value
+ #if DEBUG // test hook
+ let gate = Env.appStorageUrl.appendingPathComponent("qa1121-hold-profile") // test hook
+ if FileManager.default.fileExists(atPath: gate.path) { // test hook
+ Logger.info("QA1121 profile response held", context: "TestHook") // test hook
+ while FileManager.default.fileExists(atPath: gate.path) { // test hook
+ try await Task.sleep(for: .milliseconds(200)) // test hook
+ } // test hook
+ Logger.info("QA1121 profile response released; can apply: \(publicKey == pk && profileWriteGeneration == read.generation)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
if publicKey == pk, profileWriteGeneration == read.generation {
profile = loadedProfile
readOnlyProfile = nil
@@ -1926,7 +1936,17 @@ class PubkyProfileManager: ObservableObject {
}
private nonisolated static func initializePersistedSession() async throws -> SessionInitializationResult {
- try await PaykitSdkService.shared.restorePersistedSession()
+ #if DEBUG // test hook
+ let gate = Env.appStorageUrl.appendingPathComponent("qa1121-hold-identity") // test hook
+ if FileManager.default.fileExists(atPath: gate.path) { // test hook
+ Logger.info("QA1121 final identity initialization held", context: "TestHook") // test hook
+ while FileManager.default.fileExists(atPath: gate.path) { // test hook
+ try await Task.sleep(for: .milliseconds(200)) // test hook
+ } // test hook
+ Logger.info("QA1121 final identity initialization released", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
+ return try await PaykitSdkService.shared.restorePersistedSession()
}
private nonisolated static func notifyAppStateBackupChanged() {
diff --git a/Bitkit/Views/Settings/Security/DataBackupsScreen.swift b/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
index 31b84c1a..34d6a147 100644
--- a/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
+++ b/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
@@ -50,6 +50,14 @@ struct DataBackupsScreen: View {
var body: some View {
VStack(alignment: .leading, spacing: 0) {
NavigationBar(title: t("settings__data_backups_nav_title"))
+ #if DEBUG // test hook
+ if FileManager.default.fileExists(atPath: Env.appStorageUrl.appendingPathComponent("qa1121-controls").path) { // test hook
+ Button("QA1121: unavailable identity backup") { Task { await qaUnavailableIdentityBackup() } } // test hook
+ .accessibilityIdentifier("QA1121BackupFault") // test hook
+ Button("QA1121: stage required restore error") { Task { await qaRequiredRestoreError() } } // test hook
+ .accessibilityIdentifier("QA1121RequiredFault") // test hook
+ } // test hook
+ #endif // test hook
GeometryReader { geometry in
ScrollView(showsIndicators: false) {
@@ -98,4 +106,48 @@ struct DataBackupsScreen: View {
.navigationBarHidden(true)
.padding(.horizontal, 16)
}
+
+ #if DEBUG // test hook
+ @MainActor private func qaUnavailableIdentityBackup() async { // test hook
+ do { // test hook
+ let before = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let keys: [KeychainEntryType] = [.paykitSession, .pubkySecretKey] // test hook
+ let values = try keys.map { try Keychain.load(key: $0) } // test hook
+ do { // test hook
+ defer { // test hook
+ for (key, value) in zip(keys, values) { // test hook
+ if let value { try? Keychain.upsert(key: key, data: value) } else { try? Keychain.delete(key: key) } // test hook
+ } // test hook
+ } // test hook
+ try await PaykitSdkService.shared.forgetSessionAccess() // test hook
+ try Keychain.upsert(key: .paykitSession, data: Data([0xFF])) // test hook
+ let active = try await PaykitSdkService.shared.currentPublicKey() // test hook
+ Logger.info("QA1121 backup fault staged; SDK identity absent: \(active == nil)", context: "TestHook") // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let after = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 blocked backup: bytes unchanged: \(before?.value == after?.value), version unchanged: \(before?.version == after?.version), pending: \(pending)", context: "TestHook") // test hook
+ } // test hook
+ _ = try await PaykitSdkService.shared.restorePersistedSession() // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let fresh = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let data = try JSONSerialization.jsonObject(with: fresh!.value) as! [String: Any] // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 recovered backup: SDK state present: \(data["paykitSdkBackupState"] is String), pending: \(pending)", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 backup control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+
+ @MainActor private func qaRequiredRestoreError() async { // test hook
+ do { // test hook
+ guard let item = try await VssBackupClient.shared.getObject(key: "WALLET") else { return } // test hook
+ try item.value.write(to: Env.appStorageUrl.appendingPathComponent("qa1121-valid-wallet")) // test hook
+ try Keychain.upsert(key: .paykitPendingBackupRestore, data: Data("invalid-json".utf8)) // test hook
+ Logger.info("QA1121 invalid retained WALLET staged; original recovery payload saved", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 required-restore control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+ #endif // test hook
}Coverage: full 34047db..ec20b26, all seven changed files and the reached restore, SDK activation, backup, identity replacement and reset paths. All six J1 actions were driven with the stated controls; genuine payment authorization was not exercised.
Twin: bitkit-android#1686 fix: decouple wallet restore from profile refresh matches the scheduling and backup protections. Android retains an already-active identity during optional enrichment; iOS waits for final initialization before authenticating its profile manager.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the PR diff against merge base 34047dbd, at ec20b264.
No new actionable code findings.
Wallet restore returns after required wallet and USDT recovery, and identity initialization continues in the background. A second request for the same session revision shares the in-flight initialization, and a result from an older revision is dropped. If the SDK has no active identity while saved, adopted, or unreadable credentials remain, the WALLET snapshot throws before upload, so the previous complete record stays in place. Required restore failure still returns before startWallet. Paykit work that uses automaticPaykitSession stays idle until authentication is applied for the current revision.
Terminal restoration still blocks later WALLET uploads remains open on this head. After .restorationFailed, the saved credentials stay in the keychain, backupSnapshot throws sessionNotActive, and that error is outside BackupService.isTemporaryBackupFailure, so a later ordinary wallet change is marked required and fails again. The author has left that thread open until a separate recoverable wallet record exists. This pass did not identify a further defect.
GitHub reports SwiftFormat and Run Tests successful for this head. I inspected the new scheduler and backup tests and did not run them. Device testing: not performed in this review. A separate simulator report attributes a J1 pass on this same head with local fault controls that are not in the tree. J1 cannot be driven from the committed app alone.
Source comparison with bitkit-android#1686 at 0fd5e424: both refuse a complete wallet backup while a saved identity cannot be exported, and journeys/wallet/restore-during-identity-outage.xml matches. Android keeps an already-active identity and loads profile data afterwards; iOS shows the profile manager authenticated only after final initialization. bitkit-ios#1120 at fd3ce015 was checked only for the receive-invoice commit above this base. That commit does not change the restore or backup contract reviewed here. That check is not a full review of #1120.
Suggested additional test cases
- iOS simulator. Disposable test-network wallet with a complete VSS backup and a saved session that initialization classifies as restoration failed (saved session, no usable secret key). After ordinary wallet access returns, make a non-Paykit wallet change and run backup. Expected result: the remote WALLET bytes remain the previous complete record and the WALLET backup stays pending.
Reviewer model: grok-4.7
Reasoning effort: high
|
Device gate (partial): iPhone 17 simulator, fresh regtest wallet, profile on staging with one saved contact, WALLET backup done; restore through a local homeserver proxy in pass-through (homeserver reachable). Times are UTC:
Still running: the same restore with the homeserver blocked, and both runs on the base branch ( |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving. The code scan found nothing (#1121 (comment) has the first device partial), and the device gate passed.
Device gate: 39fcaec (app code ec20b26; the head moved only by merges, the PR's own diff against its base is unchanged and the one conflict resolution is in PrivatePaykitServiceTests.swift) — iPhone 17 simulator, base 34047db (tip of fix/ordinary-wallet-paykit-waits, since this PR is stacked on #1120). perf, Full restore success → Node started / UI ready:
| Homeserver | Head | Base |
|---|---|---|
| Held (accepted, never answered) | 1.53 / 2.50 s | 31.44 / 31.48 s |
| Reachable | 1.56 / 1.58 / 2.24 s | 6.88 / 8.04 s |
| Refused | 1.33 s | 1.44 s |
Base waits on the 30 s Pubky re-sign-in timeout
[2026-10-27 09:30:35.594 UTC] Full restore success
[2026-10-27 09:31:05.704 UTC] WARN: Re-sign-in failed, keeping saved session for retry: Identity(... "complete Pubky auth flow")
[2026-10-27 09:31:07.038 UTC] Node started
- One
Paykit session restored forper restore. With the homeserver reachable, profile and contacts arrive about 3.4 s after the UI is ready; after an outage, about 97 s after release, the re-sign-in retry interval. - Settings › Security › Data backups once the identity is active: every category recent, no failures.
- journeys:
wallet/restore-during-identity-outage.xmlsteps 1, 2, 3 and 5 passed through a local homeserver fault proxy. Step 6 passed as an approximation: blocking all of VSS failed the restore to Try Again with the node stopped, and Try Again after unblocking restored the wallet and balance. During the outage the WALLET backup reportedtemporarily unavailablefour times and thensessionNotActive, with no upload, which is the case @talosmachina's open P1 thread covers. ⚠️ Not run: step 4. The proxy can't delay profile reads alone (TLS hides which request is which), and removing the identity needs the homeserver.- Both builds also spend 30 s inside the required restore under the outage (
Failed to restore pubky session backup state); that wait is inperformFullRestoreFromLatestBackup, outside this PR. - The merges since
ec20b26bring Paykit0.1.1-rc2and VSS client0.5.26-rc1from the base branch; they weren't on the build under test and belong to the PRs that introduce them.
l10n: no UI text changed. Run Tests is still pending at this head.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the entire PR at 39fcaec8 against merge base df5b0742, because the comparison base changed. No prior coverage was inherited.
No new actionable code findings.
The existing HIGH concern about terminal identity failure preventing later WALLET backups remains unresolved and applies at this head.
Validation: The current-composition Run Tests check failed. Its uploaded XCTest result confirms all six added regressions passed, but reports a SIGSEGV in BackupServiceTests.testTemporaryFailureRemainsPendingAcrossServiceRestart. Diagnose that crash and obtain a successful run; its cause is not established. Native tests were not run locally because this workspace lacks Xcode.
Targeted source comparisons covered restore scheduling and backup protection on Android 0fd5e424, plus startup/receive separation in dependency #1120 at df5b074. These were compatibility checks, not full reviews of those PRs.
Device testing: not performed in this review. J1 requires complete-app fault controls absent from the committed fixtures. Earlier simulator evidence used the older dependency build and does not validate this composition.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
|
@jvsena42 one correction on the device gate, re-checked at A blocked homeserver gives a temporary error, so restoration ends |
|
The crash requested in review 5666197929 is addressed in signed commit 754a899. Complete XCResult inspection symbolicated the prior SIGSEGV to cancellable Set insertion/resize inside the unjoined listener task. All insertions and removal now execute synchronously on MainActor; child #1123 retains its separate lifecycle/drain implementation. The final build and all 2,957 selected units passed, including the exact previously crashing test, with 1,675 unchanged input hashes, full formatting, complete nested diagnostics and two clean independent final reviews. All four live-parent commits are locally/GitHub signature-verified. The previous failure remains recorded; current-head hosted CI is running, so I am waiting for its result before requesting affected re-review. The terminal-identity/WALLET thread remains open and no full-app acceptance is implied by this unit result. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed the new commit 754a899 (serialize backup observer cancellable mutations). No findings.
- All
cancellablesmutations now run on the main actor, which removes the off-main insert racingstopObservingBackups. - No new deadlock path: the only synchronous hops are short
stateQueue.syncclosures and the serial backup queue, and nothing callsmain.sync. - No lost or doubled backup: the status observers prepend the current statuses, and
backupJobs/runningBackupTasksstill guard against a second upload.
This commit doesn't touch the open WALLET backup sessionNotActive thread.
Device gate: not needed for this delta. The earlier gate at 39fcaec still covers the restore path.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up at 754a8991, covering the delta since 39fcaec8 and inheriting the completed baseline review. Merge base remains df5b0742. The new commit only moves backup observer subscription storage and cancellation onto the main actor.
No new actionable code findings.
Settings, widgets, and app-state listeners are now registered in that same main-actor step, so a stop clears them with the rest of the set. Wallet snapshot validation, upload, and pending-retry behavior are unchanged.
The unresolved terminal identity-restoration failure that blocks later WALLET backups remains in this thread. PrivatePaykitService+Backup.swift is unchanged since the baseline, and sessionNotActive is still outside BackupService.isTemporaryBackupFailure. No matching GitHub issue was found.
Validation: unit tests succeeded on this head, including all 12 BackupServiceTests. testTemporaryFailureRemainsPendingAcrossServiceRestart passed; the SIGSEGV reported from run 40482860269 did not recur. SwiftFormat reported 0/849 files requiring formatting. Tests were not rerun locally because this workspace has no Xcode.
Device testing: not performed in this review. Android backup observation at 0fd5e424 remains synchronous and is outside this iOS isolation change. Dependency #1120 compatibility at df5b074 stays inherited.
Reviewer model: grok-4.7
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — the acknowledged terminal-identity WALLET backup availability failure remains reproducible.
Findings: No new findings in the delta. The existing HIGH availability finding remains open at Bitkit/Services/PrivatePaykitService+Backup.swift:11–12: later ordinary wallet state cannot be backed up while identity recovery remains unusable. T1 below adds device evidence to that thread.
QA for 754a899: iPhone 17 Pro, iOS 26.5 Simulator, Xcode 27.0, disposable unfunded staging-regtest wallet. Built the unmodified head with its committed Core 0.8.0-rc11, Paykit 0.1.1-rc2 and VSS 0.5.26-rc1 dependencies, then drove the app with the local Debug controls below.
Test 1: N1 — native identity, backup and observer regressions ✅ The selected suites passed without failures or skips, including the previously crashing backup restart test.
Ran xcodebuild test with these selections on the unmodified head:
-only-testing:BitkitTests/PubkyProfileManagerTests
-only-testing:BitkitTests/PrivatePaykitServiceTests
-only-testing:BitkitTests/BackupServiceTests
I inspected the finalized XCResult and complete log. The suites exercise held/coalesced initialization, stale identity completion, replacement credentials, malformed stored bytes, backup protection, pending retries and service restart. testTemporaryFailureRemainsPendingAcrossServiceRestart passed. The result bundle retains a QoS priority-inversion warning in the profile-manager tests; its cause is unproven.
N1-execution-2x.2x.mp4
Simulator execution excerpt, 2×; assertions were checked in XCResult.
Test 2: J1 — wallet restore during optional identity outage ✅ All six actions passed with the timing and input controls described below.
- A full VSS restore completed required recovery and reached a running node while final identity initialization remained held and authentication stayed idle. I left restore and copied a regtest receive address, checked from the clipboard. Releasing the hold restored the same identity and permitted fresh complete backups.
- Removing SDK session access and supplying malformed local credential input made WALLET backup fail with
sessionNotActive. App-log comparisons confirmed unchanged remote bytes and version, with required work retained. Restoring the original credentials permitted a fresh SDK export and complete upload, clearing pending status. - Killing the app while initialization was held preserved the saved credentials. Release after relaunch restored the identity and the same single contact. Before/after contact snapshots contained one entry, and the complete logs contained one contact-add event. No payment was submitted.
- A real fetched profile response was held before applying it. After normal UI disconnect, release logged
can apply: false; the accessibility snapshot confirmed the identity-choice screen remained visible. - Malformed retained WALLET data left required recovery pending, the node stopped and the UI on Try Again without wallet actions. Repairing the original captured payload and retrying reached wallet access while final identity initialization was still held. Node/authentication state and recovery markers were checked in app logs.
| Required failure, retry and held initialization | Complete-backup protection |
|---|---|
J1-required-2x.2x.mp4 |
J1-backup-2x.2x.mp4 |
| Retry excerpt showing required recovery and ordinary wallet access before identity release; 2×. | Unavailable identity, retained backup work and recovery; record contents checked in logs; 2×. |
| Process restart | Delayed profile after disconnect |
|---|---|
J1-restart-2x.2x.mp4 |
J1-stale-rerun-2x.2x.mp4 |
| Saved credentials and one contact recover after release; counts checked in snapshots/logs; 2×. | Disconnect, release and retain the identity-choice screen; rejection checked in logs; 2×. |
This used an unfunded wallet. Funded-payment replay, a USDT-specific required-restore failure and a live homeserver blackout were not exercised.
Test 3: T1 — ordinary WALLET backup after terminal identity failure ❌ A later ordinary allocation change stayed unbacked-up until manual credential repair.
After clearing live SDK session access, the control supplied an invalid saved session and no local secret. The real resolver logged No secret key to recover session, the branch that returns .restorationFailed. I then reserved a new ordinary watch-only account index through the normal store. WALLET backup failed; its previous remote bytes and version remained unchanged and work stayed pending. Restoring the original credentials allowed a complete backup and cleared pending status.
On these dependencies the backup attempt failed with the SDK Identity error while restoring a grant from the platform provider, before the sessionNotActive guard. This establishes the terminal/no-secret availability failure; J1 separately verifies the guard's protective behavior and temporary recovery. The restoration-failure alert flag is consumed by the UI, so its later printed value is not the resolver's result.
T1-terminal-rerun-2x.2x.mp4
Control execution and credential recovery, 2×; resolver branch, allocation, remote record and pending-state assertions were checked in app logs.
The existing thread remains unresolved. Preserving the previous complete record protects identity recovery but leaves the newer ordinary allocation state absent from it.
Reports considered:
- Earlier J1 report at ec20b26: historical control recipe; J1 was re-driven here on the new dependency composition.
- Current-head source and hosted-test review: confirms the observer repair and successful current-head CI.
- Merged-parent device report: receive, payment and reconciliation coverage belongs to that report; its complete service journey remains partial.
Coverage: contiguous delta ec20b264..754a899, all changed files and reached callers, plus the open terminal-backup issue. Merged receive/payment/USDT changes received source compatibility review; their complete device journeys were not repeated here. The current-head native run and J1 cover the restore/backup composition and observer restart regression.
Twin: bitkit-android#1686 fix: decouple wallet restore from profile refresh at 0fd5e424 matches optional scheduling and complete-backup protection. Android retains an identity already activated during required metadata recovery; iOS applies authentication after final initialization.
Local Debug controls and deciding evidence
Not pushed. Holds wait for explicit file removal. Backup controls operate on this disposable wallet's actual SDK, keychain inputs, backup writer and VSS records, restoring captured credentials afterwards. Required recovery uses a corrupted retained payload and repairs it from the captured original. Before the final stale-profile attempt, a one-use setup control reset this disposable saved grant; normal stored-secret sign-in fetched the held profile. SDK responses and recovery decisions were not mocked.
Full restore success
QA1121 required recovery complete; scheduling final identity
QA1121 final identity initialization held
QA1121 wallet start returned: running, authenticated: idle
QA1121 blocked backup: bytes unchanged: true, version unchanged: true, pending: true
QA1121 recovered backup: SDK state present: true, pending: false
QA1121 required restore failed; pending: true, node: stopped
QA1121 original retained WALLET repaired before retry
No secret key to recover session
Backup failed for: 'WALLET': Identity(code: "identity_error", context: "restore Pubky grant session from platform provider")
ordinary reservation index: 1, bytes unchanged: true, version unchanged: true, pending: true
QA1121 terminal manual credential recovery: pending: false
QA1121 profile response released; can apply: false
diff --git a/Bitkit/AppScene.swift b/Bitkit/AppScene.swift
index 0dc59e23..2d3387a9 100644
--- a/Bitkit/AppScene.swift
+++ b/Bitkit/AppScene.swift
@@ -1171,8 +1171,14 @@ struct AppScene: View {
widgets.loadSavedWidgets()
widgets.objectWillChange.send()
+ #if DEBUG // test hook
+ Logger.info("QA1121 required recovery complete; scheduling final identity", context: "TestHook") // test hook
+ #endif // test hook
pubkyProfile.scheduleInitializationAfterRestore()
await startWallet(completingBackupRestore: true)
+ #if DEBUG // test hook
+ Logger.info("QA1121 wallet start returned: \(wallet.nodeLifecycleState), authenticated: \(pubkyProfile.authState)", context: "TestHook") // test hook
+ #endif // test hook
}
private func retryWalletStart() async {
@@ -1246,9 +1252,21 @@ struct AppScene: View {
private func restoreVssBackup() async -> Bool {
do {
+ #if DEBUG // test hook
+ let repair = Env.appStorageUrl.appendingPathComponent("qa1121-repair-required") // test hook
+ if FileManager.default.fileExists(atPath: repair.path) { // test hook
+ let data = try Data(contentsOf: Env.appStorageUrl.appendingPathComponent("qa1121-valid-wallet")) // test hook
+ try Keychain.upsert(key: .paykitPendingBackupRestore, data: data) // test hook
+ try FileManager.default.removeItem(at: repair) // test hook
+ Logger.info("QA1121 original retained WALLET repaired before retry", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
try await BackupService.shared.performFullRestoreFromLatestBackup()
return true
} catch {
+ #if DEBUG // test hook
+ Logger.info("QA1121 required restore failed; pending: \(BackupService.shared.hasPendingWalletRestore()), node: \(wallet.nodeLifecycleState)", context: "TestHook") // test hook
+ #endif // test hook
app.toast(error)
return false
}
diff --git a/Bitkit/Managers/PubkyProfileManager.swift b/Bitkit/Managers/PubkyProfileManager.swift
index d29dd6b1..8fcc51b2 100644
--- a/Bitkit/Managers/PubkyProfileManager.swift
+++ b/Bitkit/Managers/PubkyProfileManager.swift
@@ -1208,6 +1208,16 @@ class PubkyProfileManager: ObservableObject {
let loadedProfile = try await Task.detached {
try await resolve(pk)
}.value
+ #if DEBUG // test hook
+ let gate = Env.appStorageUrl.appendingPathComponent("qa1121-hold-profile") // test hook
+ if FileManager.default.fileExists(atPath: gate.path) { // test hook
+ Logger.info("QA1121 profile response held", context: "TestHook") // test hook
+ while FileManager.default.fileExists(atPath: gate.path) { // test hook
+ try await Task.sleep(for: .milliseconds(200)) // test hook
+ } // test hook
+ Logger.info("QA1121 profile response released; can apply: \(publicKey == pk && profileWriteGeneration == read.generation)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
if publicKey == pk, profileWriteGeneration == read.generation {
profile = loadedProfile
readOnlyProfile = nil
@@ -1926,7 +1936,23 @@ class PubkyProfileManager: ObservableObject {
}
private nonisolated static func initializePersistedSession() async throws -> SessionInitializationResult {
- try await PaykitSdkService.shared.restorePersistedSession()
+ #if DEBUG // test hook
+ let gate = Env.appStorageUrl.appendingPathComponent("qa1121-hold-identity") // test hook
+ if FileManager.default.fileExists(atPath: gate.path) { // test hook
+ Logger.info("QA1121 final identity initialization held", context: "TestHook") // test hook
+ while FileManager.default.fileExists(atPath: gate.path) { // test hook
+ try await Task.sleep(for: .milliseconds(200)) // test hook
+ } // test hook
+ Logger.info("QA1121 final identity initialization released", context: "TestHook") // test hook
+ } // test hook
+ let reset = Env.appStorageUrl.appendingPathComponent("qa1121-reset-test-grant") // test hook
+ if FileManager.default.fileExists(atPath: reset.path) { // test hook
+ try Keychain.delete(key: .paykitSession) // test hook
+ try FileManager.default.removeItem(at: reset) // test hook
+ Logger.info("QA1121 disposable saved grant reset; keeping original local secret", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
+ return try await PaykitSdkService.shared.restorePersistedSession()
}
private nonisolated static func notifyAppStateBackupChanged() {
diff --git a/Bitkit/Views/Settings/Security/DataBackupsScreen.swift b/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
index 31b84c1a..9c0d7555 100644
--- a/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
+++ b/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
@@ -39,6 +39,7 @@ private struct StatusItemView: View {
struct DataBackupsScreen: View {
@StateObject private var viewModel = BackupViewModel()
+ @EnvironmentObject private var pubkyProfile: PubkyProfileManager // test hook
private var allSynced: Bool {
BackupCategory.allCases.allSatisfy { category in
@@ -50,6 +51,15 @@ struct DataBackupsScreen: View {
var body: some View {
VStack(alignment: .leading, spacing: 0) {
NavigationBar(title: t("settings__data_backups_nav_title"))
+ #if DEBUG // test hook
+ if FileManager.default.fileExists(atPath: Env.appStorageUrl.appendingPathComponent("qa1121-controls").path) { // test hook
+ Button("QA1121: terminal identity failure") { Task { await qaTerminalIdentity() } }.accessibilityIdentifier("QA1121TerminalFault") // test hook
+ Button("QA1121: unavailable identity backup") { Task { await qaUnavailableIdentityBackup() } } // test hook
+ .accessibilityIdentifier("QA1121BackupFault") // test hook
+ Button("QA1121: stage required restore error") { Task { await qaRequiredRestoreError() } } // test hook
+ .accessibilityIdentifier("QA1121RequiredFault") // test hook
+ } // test hook
+ #endif // test hook
GeometryReader { geometry in
ScrollView(showsIndicators: false) {
@@ -98,4 +108,80 @@ struct DataBackupsScreen: View {
.navigationBarHidden(true)
.padding(.horizontal, 16)
}
+
+ #if DEBUG // test hook
+ @MainActor private func qaTerminalIdentity() async { // test hook
+ do { // test hook
+ let before = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let keys: [KeychainEntryType] = [.paykitSession, .pubkySecretKey] // test hook
+ let values = try keys.map { try Keychain.load(key: $0) } // test hook
+ do { // test hook
+ defer { // test hook
+ for (key, value) in zip(keys, values) { // test hook
+ if let value { try? Keychain.upsert(key: key, data: value) } else { try? Keychain.delete(key: key) } // test hook
+ } // test hook
+ } // test hook
+ try await PaykitSdkService.shared.forgetSessionAccess() // test hook
+ try Keychain.delete(key: .pubkySecretKey) // test hook
+ try Keychain.upsert(key: .paykitSession, data: Data("invalid-session".utf8)) // test hook
+ await pubkyProfile.initialize() // test hook
+ let terminal = pubkyProfile.sessionRestorationFailed // test hook
+ let active = try? await PaykitSdkService.shared.currentPublicKey() // test hook
+ let index = try WatchOnlyAccountStore.reserveAccountIndex(walletIndex: 0, requestFingerprint: "qa1121-terminal-" + UUID().uuidString) // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let after = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 terminal: restorationFailed: \(terminal), sdk unavailable: \(active == nil), ordinary reservation index: \(index), bytes unchanged: \(before?.value == after?.value), version unchanged: \(before?.version == after?.version), pending: \(pending)", context: "TestHook") // test hook
+ } // test hook
+ await pubkyProfile.initialize() // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 terminal manual credential recovery: pending: \(pending)", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 terminal control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+
+ @MainActor private func qaUnavailableIdentityBackup() async { // test hook
+ do { // test hook
+ let before = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let keys: [KeychainEntryType] = [.paykitSession, .pubkySecretKey] // test hook
+ let values = try keys.map { try Keychain.load(key: $0) } // test hook
+ do { // test hook
+ defer { // test hook
+ for (key, value) in zip(keys, values) { // test hook
+ if let value { try? Keychain.upsert(key: key, data: value) } else { try? Keychain.delete(key: key) } // test hook
+ } // test hook
+ } // test hook
+ try await PaykitSdkService.shared.forgetSessionAccess() // test hook
+ try Keychain.upsert(key: .paykitSession, data: Data([0xFF])) // test hook
+ let active = try await PaykitSdkService.shared.currentPublicKey() // test hook
+ Logger.info("QA1121 backup fault staged; SDK identity absent: \(active == nil)", context: "TestHook") // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let after = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 blocked backup: bytes unchanged: \(before?.value == after?.value), version unchanged: \(before?.version == after?.version), pending: \(pending)", context: "TestHook") // test hook
+ } // test hook
+ _ = try await PaykitSdkService.shared.restorePersistedSession() // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let fresh = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let data = try JSONSerialization.jsonObject(with: fresh!.value) as! [String: Any] // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 recovered backup: SDK state present: \(data["paykitSdkBackupState"] is String), pending: \(pending)", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 backup control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+
+ @MainActor private func qaRequiredRestoreError() async { // test hook
+ do { // test hook
+ guard let item = try await VssBackupClient.shared.getObject(key: "WALLET") else { return } // test hook
+ try item.value.write(to: Env.appStorageUrl.appendingPathComponent("qa1121-valid-wallet")) // test hook
+ try Keychain.upsert(key: .paykitPendingBackupRestore, data: Data("invalid-json".utf8)) // test hook
+ Logger.info("QA1121 invalid retained WALLET staged; original recovery payload saved", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 required-restore control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+ #endif // test hook
}Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — the acknowledged terminal-identity backup failure remains reproducible.
Findings: No new findings in the delta. The existing HIGH finding at Bitkit/Services/PrivatePaykitService+Backup.swift:11 remains open: after terminal identity recovery failure, newer ordinary wallet state cannot be included in a complete WALLET backup until credentials are repaired.
QA for 73b7d58: Tested with a disposable, unfunded regtest wallet and controlled Debug timing and credential faults over the real SDK and VSS operations.
Test 1: Identity, backup and USDT regression suites ✅ The focused native suites passed on the unmodified head.
The finalized result bundle reports 288 passes with no failures or skips, including identity, backup observer, foreground recovery, cache and balance cases. A recorded repeat also passed. Xcode timed out collecting optional simulator diagnostics after the first suite finished; its test result still finalized successfully.
native-2x.2x.mp4
Simulator execution at 2×; assertions were checked in the result bundles and logs.
Test 2: J1 — restore during optional identity outage ✅ Required recovery completed and the wallet remained usable while final identity initialization was held.
The restored wallet reached Home and copied a regtest receive address while the node was running and authentication remained idle; these states and the copied address were checked in logs and the clipboard. An unavailable identity left the previous WALLET bytes and version unchanged with work pending; restoring credentials produced a complete backup. A corrupted required restore kept the node stopped and work pending; repairing the retained record and retrying started the wallet. A restart while initialization was held recovered the saved identity after release. A fetched profile held through disconnect was rejected on release, and the identity-choice screen remained visible; the guard and final screen were checked in logs and the accessibility snapshot.
restore-complete-2x.2x.mp4
backup-2x.2x.mp4
required-2x.2x.mp4
restart-2x.2x.mp4
Recordings at 2×. The late-profile recording stuttered and is omitted; the prior device report also covers that unchanged profile guard and checks that a contact was not duplicated across restart. No funded payment replay, USDT-specific required-restore failure or live homeserver blackout was driven.
Test 3: Ordinary backup after terminal identity failure ❌ A new ordinary allocation remained unbacked-up until credential repair.
With an invalid saved session and no local secret, the real resolver reached restoration failure. A subsequent watch-only allocation remained pending; the previous remote WALLET bytes and version stayed unchanged. Restoring the credentials allowed backup and cleared pending work. The resolver, allocation, remote record and pending status were checked in app logs.
terminal-2x.2x.mp4
Control execution and recovery at 2×.
Check 1: Merged USDT display and receive changes ✅ The earlier device evidence covers the display and receive behavior, with recovery integration checked in the native suites.
The cached balance review and receive transition report cover the merged display and receive paths. The receive view is unchanged since that report; the manager has additional recovery integration, covered here by focused native tests. The complete provider and funded-payment journeys were not repeated.
Coverage: contiguous delta 754a899..73b7d58, its reached callers and the open terminal-backup finding. The merged foreground notification scheduling and USDT recovery integration received source and native-test coverage; the complete notification timing and USDT service journeys were not driven on a device.
Twin: bitkit-android#1686 fix: decouple wallet restore from profile refresh matches the shared optional scheduling and complete-backup protection; Android retains an identity already activated during required metadata recovery.
Reviewer model: Unknown
Reasoning effort: Not exposed
Reviewed by gpt-6.1-terra-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the full PR diff against merge base 2512c46a, at 73b7d58a. No coverage baseline was supplied. The comparison base moved from df5b074 to 2512c46, including parent edits to Bitkit/AppScene.swift and Bitkit/Services/BackupService.swift.
No new actionable code findings.
No new actionable code findings. The existing terminal-restoration limitation remains open in discussion r4368930202: .restorationFailed keeps saved credentials, a missing SDK identity still rejects the WALLET snapshot, and that error is not treated as a temporary backup failure, so later ordinary wallet changes stay pending. The author has confirmed this. A temporary hold still preserves the previous WALLET object and can upload after activation.
Android twin bitkit-android#1686 at 0fd5e424 rejects the same identity-free WALLET replacement. It also skips a second session import when its in-memory public key is already set. This restore signs in through the SDK before scheduling profile initialization and does not assign PubkyProfileManager.publicKey on that path.
Dependency bitkit-ios#1120 was checked only for the required-restore gate, at 7f4d0296. That pin is one test-only commit ahead of this PR's base. Wallet start still waits until required recovery finishes. This was not a full review of #1120.
Validation: this review did not run unit tests or a simulator. The new backup and scheduling tests were inspected. Hosted Run Tests and SwiftFormat succeeded on this head. Device testing: not performed in this review.
The open terminal-backup thread remains unresolved, so this review does not treat the branch as ready for a new device pass.
Reviewer model: grok-4.7
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — terminal identity failure still prevents later ordinary WALLET backups.
Findings: No new findings in the delta. The existing HIGH finding remains open at Bitkit/Services/PrivatePaykitService+Backup.swift:12: newer ordinary wallet state cannot be backed up until the unusable identity credentials are repaired. Test T1 reproduces the availability failure on this head.
QA for c0b1e86: iPhone 17 Pro, iOS 26.5 Simulator; disposable, unfunded staging-regtest wallet. Built the unmodified head with its pinned dependencies, then used local Debug timing and credential controls over the real SDK and VSS operations.
Test 1: N1 — native restore and backup regressions ✅ The focused suites passed on the unmodified head.
Ran xcodebuild test for PubkyProfileManagerTests, PrivatePaykitServiceTests, BackupServiceTests, BackupForegroundRecoveryTests, AppCacheDataTests, ReceiveInvoiceRefreshTests and LightningServiceConfigTests. The finalized XCResult reports no failures or skips. Optional simulator diagnostic collection was stopped after the tests completed; Xcode finalized the result and exited successfully.
native-2x.2x.mp4
Execution excerpt at 2×; assertions checked in XCResult and the test log.
Test 2: J1 — restore, backup protection and restart ✅ Required recovery gated wallet access; after retry, ordinary receive worked while optional identity initialization remained held.
A corrupt retained WALLET payload left recovery pending, the node stopped and the app on Try Again. Repairing the captured payload and retrying completed required recovery, reached Home and copied a regtest receive address while final identity initialization was held and authentication was idle. Restarting during the hold retained recovery; releasing it restored the same identity and allowed a complete WALLET upload.
A separate unavailable-identity probe rejected backup with sessionNotActive, preserving the previous remote WALLET bytes/version and pending work. Restoring the original credentials produced a complete backup containing SDK state and cleared pending work. Recovery, authentication, record comparisons and pending status were checked in app logs; the receive address was checked in the clipboard.
backup-2x.2x.mp4
required-2x.2x.mp4
restart-2x.2x.mp4
Recordings at 2×. This repeat used retained recovery on the same wallet; a fresh mnemonic restore was not repeated.
Test 3: T1 — ordinary backup after terminal identity failure ❌ A new ordinary account allocation stayed unbacked-up until credential repair.
An invalid saved session with no local secret reached the real resolver's No secret key to recover session branch. A subsequent watch-only account allocation left the remote WALLET bytes/version unchanged and backup work pending. Restoring the original credentials allowed upload and cleared pending work. These assertions were checked in app logs.
On these dependencies the backup failed with the SDK Identity error before the sessionNotActive guard. This reproduces the terminal availability failure; the temporary recovery in J1 does not resolve it.
terminal-2x.2x.mp4
Control execution and credential recovery at 2×.
Check 1: Earlier J1 identity and contact evidence ✅ The prior passing report covers unchanged delayed-profile rejection and saved-contact duplication checks.
The earlier device report verifies rejection of a profile response released after disconnect and one saved contact across restart. The profile manager, SDK service and private-backup source are unchanged since that report; these subcases were not re-driven here. No funded-payment replay, USDT-specific restore failure or live homeserver blackout was driven.
Coverage: contiguous delta, the reached restore/backup callers and the open terminal-backup finding. Parent cache and startup integration received source and native-test coverage. The inherited Blocktank sunset and notification journeys were not repeated.
Twin: bitkit-android#1686 fix: decouple wallet restore from profile refresh at 0fd5e424 matches optional scheduling and complete-backup protection. Android preserves an identity already activated during required recovery; iOS applies profile-manager authentication after final initialization.
Reviewed by gpt-6.1-sol-high
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the entire pinned PR at c0b1e86f against merge base b93b3c02. The comparison base changed; no prior coverage was inherited.
No new actionable code findings.
The existing HIGH terminal-identity backup concern remains unresolved: later ordinary wallet changes can remain absent from complete WALLET backups until unusable credentials are repaired. Temporary identity recovery does not resolve that case. The current-head external simulator report also retains this failure.
Targeted comparisons with Android 0fd5e424 and dependency iOS #1120 at b93b3c02 checked optional initialization, required-recovery admission and backup contracts. Neither was a full companion review.
Validation: native CI and SwiftFormat succeeded. Their synthetic merge tree matches the reviewed head; the native log records all six added regressions and the previously crashing backup-restart test passing. Local diff and journey XML/parity checks passed. Native checks were not rerun locally because Swift/Xcode/SwiftFormat are unavailable.
Device testing: not performed in this review. J1 requires complete-app fault controls absent from the committed fixtures; external observations remain attributed evidence. Resolve the open backup concern before treating this branch as ready for further device testing.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — the terminal identity failure still prevents later ordinary WALLET backups.
Findings: No new findings in the delta. The existing HIGH finding remains open at Bitkit/Services/PrivatePaykitService+Backup.swift:12: newer ordinary wallet state stays absent from the remote backup until unusable identity credentials are repaired. T1 reproduces it on this head.
QA for 0b77f09: iPhone 17 Pro, iOS 26.5 Simulator; disposable, unfunded staging-regtest wallet. Built the unmodified head with its pinned dependencies, then used the local Debug controls below over real profile, SDK, USDT recovery and VSS operations.
Test 1: N1 — native profile and backup regressions ✅ The focused suites passed on the unmodified head.
Ran xcodebuild test for PubkyProfileManagerTests, PrivatePaykitServiceTests, BackupServiceTests and BackupForegroundRecoveryTests. Both new reconnect regressions passed, including retries during a held read without credential lookup and rejection after local state is cleared while the same key remains current. The finalized XCResult reports no failures or skips; it retains one QoS runtime warning in the profile-manager tests.
N1-2x.2x.mp4
Simulator execution at 2×; assertions checked in XCResult and the test log.
Test 2: J1R — missing own profile recovers on Home ✅ Foreground maintenance completed while a real profile response was held; releasing it restored the profile name on Home.
A Debug control cleared the local profile/cache and failed its public read while retaining authentication. After resuming on Home, the real profile response was held before its normal apply guard. Wallet maintenance returned during the hold, and additional resumes admitted no duplicate read. Release replaced “Your Name” with the saved profile name without opening Profile. A further resume started no new read. No persisted-session initialization was logged during these retries; coalescing, maintenance completion and authentication were checked in app logs, and the name was checked in the Home accessibility snapshot.
J1R-2x.2x.mp4
Failure, held retry and recovery at 2×.
Test 3: T1 — ordinary backup after terminal identity failure ❌ A later ordinary allocation stayed unbacked-up until credential repair.
An invalid saved session with no local secret reached the real resolver's No secret key to recover session branch. A subsequent watch-only account allocation left the remote WALLET bytes/version unchanged and backup work pending. Restoring the original credentials allowed upload and cleared pending work. These assertions were checked in app logs.
On these dependencies, capture throws the SDK Identity error before the sessionNotActive guard. This reproduces the terminal availability failure; successful temporary-fault recovery does not resolve it.
T1-2x.2x.mp4
Control execution and credential recovery at 2×; record comparisons checked in logs.
Test 4: J1U — required USDT recovery failure ✅ A malformed required USDT snapshot kept wallet access gated; repairing the retained payload and retrying completed recovery.
The control changed only the USDT snapshot in a captured local WALLET recovery payload. After restart, the real restore returned InvalidBackup, retained pending recovery, and left the node stopped with only Try Again available. Replacing that local payload with the captured original and tapping Try Again completed required recovery, started the node and returned to Home. The failure, pending marker and node state were checked in app logs; the gated screen and recovered Home were checked in accessibility snapshots. This was retained recovery on the same wallet, not a fresh mnemonic restore.
J1U-2x.2x.mp4
Required failure and retry at 2×.
Check 1: Earlier passing J1 evidence ✅ Earlier reports cover the unchanged restore, backup-protection, restart and contact paths.
- Previous-head device report: required wallet recovery, temporary identity backup protection and process restart. Their underlying restore, SDK and backup paths are unchanged in this delta.
- Earlier identity/contact report: rejection of delayed profile data after disconnect and one saved contact across restart. The read/apply and contact guards remain unchanged; N1 also exercises stale results from the new retry entry point.
These passing subcases were not re-driven. No funded-payment replay, USDT provider/payment journey or live homeserver blackout was driven.
Coverage: contiguous delta c0b1e86..0b77f09, all four changed files, reached foreground/reconnect callers, profile-write and identity guards, and the open terminal-backup finding. J1R drives the user-visible delta; N1 covers the new retry's coalescing and stale-result branches. No additional production behavior changed in the delta.
Twin: bitkit-android#1686 fix: decouple wallet restore from profile refresh at 9ce4302 matches the missing-profile retry through maintenance without waiting for profile networking. Android retains an already-activated identity during required recovery; iOS authenticates its profile manager after final initialization. The terminal-backup availability limitation remains on both platforms.
Local Debug controls and deciding evidence
Not pushed. The profile control stages a missing local profile, fails a read, then holds a real fetched response before the unchanged apply guard until explicit release. Terminal-backup inputs are restored after the probe. The USDT control corrupts only a local retained snapshot and repairs it from the captured original. SDK results and restore decisions are not mocked.
QA1121 missing staged; auth: authenticated, profile nil: true
QA1121 real profile response held
QA1121 foreground wallet maintenance returned; profile held: true, auth: authenticated
QA1121 profile response released; can apply: true
QA1121 profile applied: Reconnect Probe
No secret key to recover session
Backup failed for: 'WALLET': Identity(code: "identity_error", context: "restore Pubky grant session from platform provider")
ordinary reservation index: 1, bytes unchanged: true, version unchanged: true, pending: true
QA1121 terminal manual credential recovery: pending: false
Full restore error: InvalidBackup
QA1121 required recovery failed; pending: true, node: stopped
QA1121 retained WALLET repaired
Full restore success
Node started
diff --git a/Bitkit/AppScene.swift b/Bitkit/AppScene.swift
index af7fe12c..54b7bac8 100644
--- a/Bitkit/AppScene.swift
+++ b/Bitkit/AppScene.swift
@@ -499,6 +499,22 @@ struct AppScene: View {
) {
config in AppUpdateSheet(config: config)
}
+ .task { // test hook
+ #if DEBUG // test hook
+ while !Task.isCancelled { // test hook
+ let command = Env.appStorageUrl.appendingPathComponent("qa1121-command") // test hook
+ if let text = try? String(contentsOf: command, encoding: .utf8) { // test hook
+ try? FileManager.default.removeItem(at: command) // test hook
+ if text.trimmingCharacters(in: .whitespacesAndNewlines) == "missing" { // test hook
+ pubkyProfile.qaLoseProfile() // test hook
+ await pubkyProfile.loadProfile() // test hook
+ Logger.info("QA1121 missing staged; auth: \(pubkyProfile.authState), profile nil: \(pubkyProfile.profile == nil)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+ try? await Task.sleep(for: .milliseconds(250)) // test hook
+ } // test hook
+ #endif // test hook
+ } // test hook
.task(priority: .userInitiated, setupTask)
.task(id: [scenePhase == .active, wallet.walletExists == true, isWalletBackupRestoreRunning, network.isConnected]) {
guard scenePhase == .active, wallet.walletExists == true,
@@ -1235,9 +1251,21 @@ struct AppScene: View {
private func restoreVssBackup() async -> Bool {
do {
+ #if DEBUG // test hook
+ let repair = Env.appStorageUrl.appendingPathComponent("qa1121-repair-required") // test hook
+ if FileManager.default.fileExists(atPath: repair.path) { // test hook
+ let data = try Data(contentsOf: Env.appStorageUrl.appendingPathComponent("qa1121-valid-wallet")) // test hook
+ try Keychain.upsert(key: .paykitPendingBackupRestore, data: data) // test hook
+ try FileManager.default.removeItem(at: repair) // test hook
+ Logger.info("QA1121 retained WALLET repaired", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
try await BackupService.shared.performFullRestoreFromLatestBackup()
return true
} catch {
+ #if DEBUG // test hook
+ Logger.info("QA1121 required recovery failed; pending: \(BackupService.shared.hasPendingWalletRestore()), node: \(wallet.nodeLifecycleState)", context: "TestHook") // test hook
+ #endif // test hook
app.toast(error)
return false
}
@@ -1315,6 +1343,9 @@ struct AppScene: View {
await LightningService.shared.reconnectPeers()
try? await wallet.sync()
await sessionRecovery
+ #if DEBUG // test hook
+ Logger.info("QA1121 foreground wallet maintenance returned; profile held: \(pubkyProfile.isLoadingProfile), auth: \(pubkyProfile.authState)", context: "TestHook") // test hook
+ #endif // test hook
await retryPendingPaykitEndpointRemoval()
}
foregroundWalletSync = foregroundSync
diff --git a/Bitkit/Managers/PubkyProfileManager.swift b/Bitkit/Managers/PubkyProfileManager.swift
index 0e4e3ac6..5829e7d4 100644
--- a/Bitkit/Managers/PubkyProfileManager.swift
+++ b/Bitkit/Managers/PubkyProfileManager.swift
@@ -1199,6 +1199,14 @@ class PubkyProfileManager: ObservableObject {
}
#endif
+ #if DEBUG // test hook
+ func qaLoseProfile() { // test hook
+ invalidateProfileLoads() // test hook
+ profile = nil // test hook
+ clearCachedProfileMetadata() // test hook
+ } // test hook
+ #endif // test hook
+
// MARK: - Profile
func loadProfile() async {
@@ -1215,13 +1223,32 @@ class PubkyProfileManager: ObservableObject {
let resolve = remoteProfileResolver
do {
+ #if DEBUG // test hook
+ Logger.info("QA1121 admitted profile read", context: "TestHook") // test hook
+ if FileManager.default.fileExists(atPath: Env.appStorageUrl.appendingPathComponent("qa1121-fail-profile").path) { // test hook
+ throw URLError(.notConnectedToInternet) // test hook
+ } // test hook
+ #endif // test hook
let loadedProfile = try await Task.detached {
try await resolve(pk)
}.value
+ #if DEBUG // test hook
+ let gate = Env.appStorageUrl.appendingPathComponent("qa1121-hold-profile") // test hook
+ if FileManager.default.fileExists(atPath: gate.path) { // test hook
+ Logger.info("QA1121 real profile response held", context: "TestHook") // test hook
+ while FileManager.default.fileExists(atPath: gate.path) { // test hook
+ try await Task.sleep(for: .milliseconds(200)) // test hook
+ } // test hook
+ Logger.info("QA1121 profile response released; can apply: \(publicKey == pk && profileWriteGeneration == read.generation)", context: "TestHook") // test hook
+ } // test hook
+ #endif // test hook
if publicKey == pk, profileWriteGeneration == read.generation {
profile = loadedProfile
readOnlyProfile = nil
cacheProfileMetadata(loadedProfile)
+ #if DEBUG // test hook
+ Logger.info("QA1121 profile applied: \(loadedProfile.name)", context: "TestHook") // test hook
+ #endif // test hook
}
} catch {
Logger.error("Failed to load profile: \(error)", context: "PubkyProfileManager")
@@ -1936,7 +1963,10 @@ class PubkyProfileManager: ObservableObject {
}
private nonisolated static func initializePersistedSession() async throws -> SessionInitializationResult {
- try await PaykitSdkService.shared.restorePersistedSession()
+ #if DEBUG // test hook
+ Logger.info("QA1121 initialize persisted session invoked", context: "TestHook") // test hook
+ #endif // test hook
+ return try await PaykitSdkService.shared.restorePersistedSession()
}
private nonisolated static func notifyAppStateBackupChanged() {
diff --git a/Bitkit/Views/Settings/Security/DataBackupsScreen.swift b/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
index 31b84c1a..a4c5304d 100644
--- a/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
+++ b/Bitkit/Views/Settings/Security/DataBackupsScreen.swift
@@ -39,6 +39,7 @@ private struct StatusItemView: View {
struct DataBackupsScreen: View {
@StateObject private var viewModel = BackupViewModel()
+ @EnvironmentObject private var pubkyProfile: PubkyProfileManager // test hook
private var allSynced: Bool {
BackupCategory.allCases.allSatisfy { category in
@@ -50,6 +51,15 @@ struct DataBackupsScreen: View {
var body: some View {
VStack(alignment: .leading, spacing: 0) {
NavigationBar(title: t("settings__data_backups_nav_title"))
+ #if DEBUG // test hook
+ if FileManager.default.fileExists(atPath: Env.appStorageUrl.appendingPathComponent("qa1121-controls").path) { // test hook
+ Button("QA1121: terminal identity failure") { Task { await qaTerminalIdentity() } }.accessibilityIdentifier("QA1121TerminalFault") // test hook
+ Button("QA1121: unavailable identity backup") { Task { await qaUnavailableIdentityBackup() } } // test hook
+ .accessibilityIdentifier("QA1121BackupFault") // test hook
+ Button("QA1121: stage required restore error") { Task { await qaRequiredRestoreError() } } // test hook
+ .accessibilityIdentifier("QA1121RequiredFault") // test hook
+ } // test hook
+ #endif // test hook
GeometryReader { geometry in
ScrollView(showsIndicators: false) {
@@ -98,4 +108,84 @@ struct DataBackupsScreen: View {
.navigationBarHidden(true)
.padding(.horizontal, 16)
}
+
+ #if DEBUG // test hook
+ @MainActor private func qaTerminalIdentity() async { // test hook
+ do { // test hook
+ let before = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let keys: [KeychainEntryType] = [.paykitSession, .pubkySecretKey] // test hook
+ let values = try keys.map { try Keychain.load(key: $0) } // test hook
+ do { // test hook
+ defer { // test hook
+ for (key, value) in zip(keys, values) { // test hook
+ if let value { try? Keychain.upsert(key: key, data: value) } else { try? Keychain.delete(key: key) } // test hook
+ } // test hook
+ } // test hook
+ try await PaykitSdkService.shared.forgetSessionAccess() // test hook
+ try Keychain.delete(key: .pubkySecretKey) // test hook
+ try Keychain.upsert(key: .paykitSession, data: Data("invalid-session".utf8)) // test hook
+ await pubkyProfile.initialize() // test hook
+ let terminal = pubkyProfile.sessionRestorationFailed // test hook
+ let active = try? await PaykitSdkService.shared.currentPublicKey() // test hook
+ let index = try WatchOnlyAccountStore.reserveAccountIndex(walletIndex: 0, requestFingerprint: "qa1121-terminal-" + UUID().uuidString) // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let after = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 terminal: restorationFailed: \(terminal), sdk unavailable: \(active == nil), ordinary reservation index: \(index), bytes unchanged: \(before?.value == after?.value), version unchanged: \(before?.version == after?.version), pending: \(pending)", context: "TestHook") // test hook
+ } // test hook
+ await pubkyProfile.initialize() // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 terminal manual credential recovery: pending: \(pending)", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 terminal control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+
+ @MainActor private func qaUnavailableIdentityBackup() async { // test hook
+ do { // test hook
+ let before = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let keys: [KeychainEntryType] = [.paykitSession, .pubkySecretKey] // test hook
+ let values = try keys.map { try Keychain.load(key: $0) } // test hook
+ do { // test hook
+ defer { // test hook
+ for (key, value) in zip(keys, values) { // test hook
+ if let value { try? Keychain.upsert(key: key, data: value) } else { try? Keychain.delete(key: key) } // test hook
+ } // test hook
+ } // test hook
+ try await PaykitSdkService.shared.forgetSessionAccess() // test hook
+ try Keychain.upsert(key: .paykitSession, data: Data([0xFF])) // test hook
+ let active = try await PaykitSdkService.shared.currentPublicKey() // test hook
+ Logger.info("QA1121 backup fault staged; SDK identity absent: \(active == nil)", context: "TestHook") // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let after = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 blocked backup: bytes unchanged: \(before?.value == after?.value), version unchanged: \(before?.version == after?.version), pending: \(pending)", context: "TestHook") // test hook
+ } // test hook
+ _ = try await PaykitSdkService.shared.restorePersistedSession() // test hook
+ await BackupService.shared.triggerBackup(category: .wallet) // test hook
+ let fresh = try await VssBackupClient.shared.getObject(key: "WALLET") // test hook
+ let data = try JSONSerialization.jsonObject(with: fresh!.value) as! [String: Any] // test hook
+ let pending = BackupService.shared.getBackupStatus(category: .wallet).isRequired // test hook
+ Logger.info("QA1121 recovered backup: SDK state present: \(data["paykitSdkBackupState"] is String), pending: \(pending)", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 backup control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+
+ @MainActor private func qaRequiredRestoreError() async { // test hook
+ do { // test hook
+ guard let item = try await VssBackupClient.shared.getObject(key: "WALLET") else { return } // test hook
+ try item.value.write(to: Env.appStorageUrl.appendingPathComponent("qa1121-valid-wallet")) // test hook
+ try Keychain.upsert(key: .paykitPendingBackupRestore, data: try JSONSerialization.data(withJSONObject: { () -> [String: Any] in // test hook
+ var object = try! JSONSerialization.jsonObject(with: item.value) as! [String: Any] // test hook
+ object["usdtWallet"] = "invalid-usdt-recovery-json" // test hook
+ return object // test hook
+ }())) // test hook
+ Logger.info("QA1121 invalid USDT recovery staged in retained WALLET; original payload saved", context: "TestHook") // test hook
+ } catch { // test hook
+ Logger.error("QA1121 required-restore control error: \(error)", context: "TestHook") // test hook
+ } // test hook
+ } // test hook
+ #endif // test hook
}Reviewed by gpt-6.1-sol-high
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up re-review of changes since c0b1e86f, including the missing-profile retry and its callers, at 0b77f099. Inherited the completed baseline review for unchanged restore admission, identity and backup protection, workflow and Appium coverage, and the recorded Android, dependency, and Paykit comparisons.
No new actionable code findings.
An active, initialized session with no profile now schedules one coalesced loadProfile from restoreSessionIfNeeded and returns while that read continues. Foreground and network recovery in AppScene still proceed with wallet maintenance. The same session and an empty profile are checked again before the read, and clearing authenticated state still drops a late result for the same key. The new unit tests and the added J1 action describe that behavior. Android 9ce43027 has the same non-blocking reload; that comparison is not a full review of bitkit-android#1686.
The existing terminal-identity backup concern remains unresolved: after restoration has failed while saved credentials remain, later ordinary wallet changes stay out of complete WALLET backups until those credentials can be used again. That backup path is unchanged in this follow-up. Unit tests and SwiftFormat passed on this revision. Device testing: not performed in this review. J1 still depends on complete-app fault controls that the journey marks as not provisioned.
Reviewer model: grok-4.7
Reasoning effort: high
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the entire PR diff against merge base f31c259b, at 2d4fc719. The comparison base moved from b93b3c02 to f31c259b, and that parent range changes AppScene, profile initialization, backup observation, and the Paykit backup snapshot. Inherited coverage is empty.
No new actionable code findings.
Restore returns after required wallet and USDT recovery. Identity initialization continues in the background, same-revision initialization is shared, and a result from an older session revision is dropped. Foreground and reconnect maintenance loads a missing profile for the current authenticated session and returns before that read finishes, keeping the existing session. A WALLET snapshot with no active SDK identity still fails closed while saved, adopted, or unreadable credentials remain, so the previous complete WALLET record stays in place.
The existing terminal identity backup limitation is unchanged: after restoration has failed and credentials remain, later ordinary wallet changes stay out of complete WALLET backups until those credentials are repaired. A repository issue search found no separate issue for that mechanism.
Validation: SwiftFormat passed on this head. Unit tests were still in progress, so this review does not use their result. The new profile and backup tests were inspected only. Device testing: not performed in this review. Device notes on ec20b264, 754a8991, and 0b77f099 stay tied to those builds.
Compared bitkit-android#1686 at 217b42ef for restore scheduling, missing-profile retry, and the null Paykit snapshot refusal. journeys/wallet/restore-during-identity-outage.xml matches. Android also treats a live in-memory public key as identity; iOS uses the adopted reference and stored credential bytes. iOS clears in-memory authentication before background initialization.
Recommended before device testing: wait for the in-progress unit-tests run on 2d4fc719. J1 still depends on the fault controls named in the journey.
Reviewer model: grok-4.7
Reasoning effort: high
Description
Ordinary wallet restore finishes after required wallet and USDT recovery, while optional Pubky activation and profile enrichment continue afterward. Same-revision initialization coalesces, and obsolete identity results cannot restore an earlier sign-in.
The reconnect follow-up in
0b77f099retries a missing own profile through existing foreground/reconnection maintenance. It retains the original authenticated session, returns without waiting for the public profile read, and uses the existing overlapping-read and profile-write guards. It does not re-import credentials, add a polling loop, or delay Home.Complete WALLET and METADATA capture still rejects unavailable or unreadable identity data. Terminal identity restoration can therefore leave newer ordinary WALLET state pending; the existing availability finding remains open. No stale SDK snapshot or partial backup is used.
Twin: synonymdev/bitkit-android#1686. This remains stacked on #1120, branch
fix/ordinary-wallet-paykit-waits.Current tree and validation
Head
2d4fc7199b476f41ce56ae05c786f3ee1981bed5, live parentf31c259bd496ba74a25f51959e4e33ee488b5814, tree5faf02db8bb0040676575b66ec4e71b03c9bf873. The signed additive merge incorporates the current ordinary-wallet parent; the complete-WALLET availability finding below remains open.Final combined-tree validation passed 2,853 unit and 10 UI logical cases with zero failures and five prerequisite skips. All 1,659 frozen inputs match. The 2,897 physical XCResult records reconcile to 2,868 logical cases including skips, plus 29 additional successful dynamic launch configurations. A passed SegmentedTabIdentifier case omitted by the wrapper was verified in raw and nested results. SwiftFormat reports 0/823 changes (96 exclusions); translations report zero errors and 15 actual-base warnings. Complete logs and all nested diagnostics were inspected and sealed. Two AppIntents notices, 16 physical QoS observations across 10 logical tests and five raw thread-performance notices remain recorded; timing causation is unproven. Workflow integration exclusions, native recovery fixture and Trezor/local-fork skips are not passes. The complete eight-commit live-parent range is verified locally and on GitHub. Current-head native CI 40876605714 passed 2,853 unit and 39 UI executions, with five fixture skips and no failures/retries; formatting 40876605759 passed 0/823. Synthetic merge
17377a2chas the exact head tree. Complete logs, all 2,897 nested summaries, exported diagnostics and the checksum/CRC-verified artifact were inspected. All 286 distinct linker warnings match actual parentf31c259b. A keyboard queue timeout in a passing navigation test and 24 CoreGraphics NaN notices in the passing comma-region USDT case remain disclosed without causal claims. Four integration classes and all nine migration jobs did not execute; they are not passes. The authenticated J1 and terminal complete-WALLET gaps remain open.The prior reconnect-specific validation below remains revision-scoped evidence for
0b77f099; it is not substituted for the final combined-tree run.verified=true,reason=valid). Published history was preserved with additive integration.Prior-head native CI 40781277921 passed 2,820 unit and 39 UI executions, with one unit and four UI skips, zero failures or failed-test retries. The complete 5,758-line job log, all 2,864 nested execution summaries and exported raw diagnostics were inspected. Thirty successful dynamic launch executions account for repeated identifiers. Synthetic merge
98995349againstb93b3c02has the exact head tree. Its 286 distinct linker warnings match verified actual-base diagnostics; two AppIntents notices and nonfailing platform/runtime observations remain recorded without a blanket timing-cause claim. Hosted formatting 40781277928 passed 0/821, with 90 exclusions. Four integration classes were excluded and all nine migration jobs skipped; none count as passes. The ProfileRecovery UI pass is controlled UI evidence, not authenticated J1 or closure of terminal WALLET availability.Design / Preview
No layout change. Home remains usable while optional identity recovery continues. A temporarily missing own profile can recover during ordinary maintenance without navigating to Profile or re-authenticating.
QA Notes
Setup
Use the committed Debug dependencies: Core 0.8.0-rc11, Paykit 0.1.1-rc2 and VSS 0.5.26-rc1. J1 needs a disposable test-network wallet, a complete VSS backup and saved identity. Delay only final optional activation/enrichment after metadata credential replacement and required recovery imports succeed. Preserve existing wallets and use no live funds.
Journeys
restore-during-identity-outage.xml: optional identity outage after required restore, complete-backup protection, screen exit, missing-profile reconnect, stale enrichment after identity replacement, restart, and required recovery failures retaining spending/address gates.The reconnect action holds a real profile read while maintenance continues, then expects Home to recover after release without manual Profile navigation. Native tests use controlled service resources and are not a complete authenticated app journey. The author's isolated complete-app J1 fault controls remain unprovisioned.
External simulator review 5658584985 reports six J1 actions at historical head
ec20b264using real SDK/VSS paths and uncommitted Debug fault controls. Its raw artifacts were not independently verified by the author. It does not establish this follow-up's new case, terminal unrecoverable identity, USDT-specific restore failure or a live homeserver blackout. Piotr's subsequent review retained the terminal-backup finding.Remaining findings and historical evidence
The terminal identity/full-WALLET finding remains unresolved on both platforms. Updated-reader compatibility was approved, but the released conditional VSS API alone cannot exclude legacy unconditional writers or version-reset ABA. This PR does not activate a split backup writer, infer historical incident causation, or claim that full-wallet redesign/contact CAS is acceptance evidence for J1. Pubky #741 merged externally; deployed server revision/configuration remains unverified.
The earlier backup-observer Set race repair and its failed run 40482860269 remain revision-scoped evidence; the exact competing schedule was not reproduced. Prior combined head
c0b1e86fpassed native CI 40719305858: 2,818 unit and 39 UI executions, one unit/four UI skips, zero failures. Its complete logs/nested results were inspected, and its 286 distinct linker warnings matched actual-parent evidence. Earlier failures, skips and runtime observations are retained, not erased or carried forward as new-head passes.Models used
gpt-6-astra(xhigh).gpt-6-astra(xhigh) andgpt-6.1-sol(high). Historical reviews remain recorded; the focused follow-up used a fresh Sol High combined review with verified execution metadata.