Repository navigation
fix: decouple wallet restore from profile refresh - #1686
ben-kaufman wants to merge 4 commits into
Conversation
|
Triage: 3.1.0. It keeps the shipped wallet restore from waiting on profile refresh, and 3.1.0 is due 28 Oct. |
 [Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryWallet restore now schedules optional identity activation and profile reads without waiting for them.
No actionable issues found. Review covered source and tests; tests were not run. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Required wallet and USDT recovery] --> B{Succeeded?}
B -->|No| C[Keep restore blocked]
B -->|Yes| D[Schedule identity work]
D --> E[Complete wallet restore]
D --> F{Identity revision still current?}
F -->|No| G[Skip obsolete work]
F -->|Yes| H[Activate identity and load profile]
I[Build backup] --> J{Identity data complete?}
J -->|No| K[Keep previous remote backup]
J -->|Yes| L[Upload backup]
Reviews (1) · Last reviewed commit: "fix: decouple wallet restore from profil..." · Reviewed by Greptile |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Moves Pubky identity activation after a wallet restore off the restore screen's critical path (scheduleInitializationAfterRestore, coalesced per stored-identity revision), and makes WALLET and METADATA backups fail rather than upload a payload with the identity part silently missing. Reviewed ac2a7c6, full tier, stacked on #1685 (fix/pending-wallet-restore-admission), reviewed as the delta against that base.
What I checked, and 6 candidates I ruled out
Read in full: PubkyRepo.kt (initialization, restore, adoption, sign-out and backup-state regions), BackupRepo.kt (scheduling, triggerBackup, markBackupFailed, wallet and metadata snapshots, full restore), WalletViewModel.kt restore paths, PrivatePaykitRepo.backupSnapshot, PaykitSdkService (currentPublicKey, session provider, clearSessionAccess), and paykit-rs v0.1.1-rc1 identity_status / initialize.
Call sites traced: scheduleInitializationAfterRestore (both WalletViewModel restore paths), hasIdentityForBackup (only captureWalletBackupDataBytes), initialize() (still only the init block, unchanged semantics with expectedRevision = null).
CI: lint, detekt and build-local green. build is red on one test, PubkyAuthManifestTest > signup and authorization links resolve only through their enabled aliases, at its first assertion (line 77, every alias still at its default). The diff touches neither that test nor the manifest; the base head 837aa1c passed build, and the same test failed the same way in two of the last eight failed ci runs on unrelated branches (fix/update-terms-of-use, feat/usdt-design-followup-20261015). All seven new tests in PubkyRepoTest, BackupRepoTest passed in that run. The e2e shards were still running when I read the checks.
Not built here: this review is reasoned from the code and CI; CI is the gate.
Ruled out
- Scheduled activation lost when the revision moves: the revision only moves through
startSignIn(non-restoring) or a non-preservingclearAuthenticatedState, both of which are a newer identity's own path, so dropping the stale job is the intent.restoreSessionBackupStatebumps it inside the restore, before the schedule reads it. - Something after
Completedneeding the identity: the public key is already set inside the required boundary (restoreSessionBackupStateLocalSeedstartSignIn), and the payment-requestactivateatBackupRepo.kt:1058runs during the restore, unchanged. - Coalescing reusing a finished job:
takeIf { !it.second.isCompleted }, and the completion handler clears the slot only when it still holds the same job. - Uncaught failure in the background job:
initializeSessioncatches its own failures,checkAdoptedSourcereturns aResult, andappScopecarries aSupervisorJobplus a logging handler. - WALLET backup blocked for good while a saved session exists: with a stored session
currentPublicKey()reads the bound identity state throughidentity_status, sostateis null only when the SDK has no identity, which is the case the check means to hold; an unreadable keychain fails the backup (covered bywallet backup cannot treat unreadable identity as absent). - METADATA
getOrThrowturning a benign miss into a failure: both snapshots returnnullfor absence insiderunSuspendCatching, so only a read error now fails, which is what the two new tests pin.
Merge confidence: 3/5, the build job is red on PubkyAuthManifestTest, which I traced to a flake outside this diff; a green re-run would move it to 4/5 (J1 is still unrun).
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: one LOW inline, reproduced with a failing unit test.
Checked and clean:
- No cross-identity leak on wipe and restore: the mutex, the revision and the
isCurrent(signIn)guards hold. - Identity isn't lost for good: the retry paths cover a failed background refresh. The WALLET backup that can stay blocked is the open P1 thread.
- No payment path starts before the identity exists: the key is set inside the required restore.
- Twin synonymdev/bitkit-ios#1121 keeps the header signed-out until the background activation lands, so it has no flip.
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
Full restore success→ Completed, head vs master, with the homeserver reachable and with it blocked; - checks that the header shows the identity and that profile and contacts arrive afterwards;
- counts
Restored paykit session forper restore (expected 1); - runs Settings → Backup for WALLET once the identity is active.
Device gate (partial): ac2a7c6 — 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:
Findings: No new findings. I confirmed that the redundant session import is fixed at 0fd5e42. The acknowledged terminal-restoration backup limitation still applies to Android: retained credentials with no SDK identity can block later complete WALLET backups. Temporary recovery below does not resolve that case.
QA for 0fd5e42: Android 16, API 36 emulator, staging regtest, an unfunded disposable wallet. I built the unmodified head and ran its focused native suites, then drove the app with local Debug controls over the real SDK and VSS paths. Installed APK bytes matched the built artifacts.
Test 1: J1 — complete optional identity outage journey ⏭️ Partial: activation first occurring after required recovery was not driven.
This LocalSeed backup activates its identity during required metadata restoration. Its later scheduled work enriches an already-active identity, so the first action's unauthenticated-completion expectation was not established. The subcases below cover held enrichment, unavailable SDK export, restart, stale profile rejection and required-recovery failure. Initialization with authentication unavailable was held separately across process restart.
Genuine post-required activation failure and funded payment replay remain unverified.
Test 2: J1 — restore with optional enrichment held ✅ Restore completed and ordinary wallet access remained available before release.
The success screen appeared while final enrichment was held. I left restore and opened Receive. App logs confirmed a running node, clear required-recovery markers and successful new-address allocation. Android kept the identity activated by required metadata restoration; the profile was still empty until enrichment resumed.
J1-restore2x.2x.mp4
Required restore, wallet access and receiving while enrichment is held; 2×. Node, marker and allocation state were checked in app logs.
Test 3: J1 — complete-backup protection and recovery ✅ An unavailable SDK identity preserved the remote WALLET record, then recovery permitted a complete backup.
The control removed live SDK session access, retaining the original identity secret for recovery. The real WALLET backup failed with Paykit identity is unavailable for backup; remote bytes and version stayed unchanged and backup work remained pending. Real sign-in with the original secret then allowed a fresh SDK export and complete upload, with pending status cleared. These assertions were checked in the control and app logs.
J1-backup-final2x.2x.mp4
Rejected backup, identity recovery and resumed profile display; 2×. Backup contents and pending status were checked in app logs.
Test 4: J1 — process restart during held work ✅ Saved credentials recovered the same identity and a single saved contact.
I killed the app during held optional work and held initialization again after relaunch. Authentication stayed unavailable while the ordinary wallet ran. Releasing initialization restored the identity and the same one contact. The contacts screens and app logs confirmed the result; no payment was submitted.
J1-restart2x.2x.mp4
One contact before and after process restart and initialization release; 2×. Authentication and node state were checked in app logs.
Test 5: J1 — delayed profile response after identity removal ✅ The late response did not restore the removed identity or profile.
I held a real profile response, removed the identity through the normal repository sign-out, then released the response. Its normal guard reported current=false; authentication and profile remained empty. The UI and app logs confirmed that the old profile was not applied.
J1-stale2x.2x.mp4
Identity removal while a profile response is held, then release; 2×. Stale-response rejection was checked in app logs.
Test 6: J1 — required recovery failure and retry ✅ Required markers blocked address and payment admission until the original payload was repaired and restored.
A malformed retained WALLET payload kept the restore-error screen with no Proceed action. Address allocation returned WalletBackupRestorePendingError. An invoice-admission probe returned the same error before invoice parsing or dispatch. After repairing the original retained WALLET and USDT-recovery payload copies, Retry completed required recovery while optional enrichment was still held. Markers cleared and address allocation succeeded. These states were checked in app logs.
J1-required-final2x.2x.mp4
Required-recovery error, repair, retry and success with enrichment held; 2×. Operation admission was checked in app logs.
Test 7: base comparison — held optional initialization ✅ The base waited for release; the reviewed head completed with the hold active.
I built and drove 55b31ee1 on the same emulator with the same disposable backup and a hold at the corresponding optional-initialization boundary. Required restore logged success, but the base remained at 100% throughout the 34.6-second hold. It completed after release. Test 2 shows the reviewed head completing before release. This is a controlled comparison, not a live homeserver-blackout benchmark.
C2-base2x.2x.mp4
Pinned base waiting after required recovery, then completing after release; 2×. The hold interval was measured from app logs.
Check 1: focused native regressions ✅ The unmodified-head build and selected suites passed without failures or skips.
./gradlew assembleDevDebug testDevDebugUnitTest \
--tests to.bitkit.repositories.PubkyRepoTest \
--tests to.bitkit.repositories.BackupRepoTest \
--tests to.bitkit.ui.WalletViewModelTestI inspected the test XML and complete build log. The suites cover coalescing, active-session preservation, replacement identities, unreadable credentials, complete-backup protection and required-recovery admission.
Local Debug controls and deciding log lines
Not pushed. Holds wait for explicit file removal. The backup control clears this disposable wallet's SDK session access and recovers using its original secret. The required-recovery control corrupts retained local payload input and repairs it from the captured original before Retry. SDK results and recovery decisions are not mocked.
Full restore success
QA1686 final enrichment held
identity=true, profile=null, contacts=1, pendingRestore=false, node=Running
QA1686 unavailable identity: failed=true, unchanged=true, pending=true
QA1686 qa1686BackupFault: backup protection and recovered export passed
QA1686 profile response held
QA1686 qa1686SignOut: identity removed
QA1686 profile released; current=false
QA1686 qa1686SpendingGate: payment admission blocked by required restore before invoice parsing
QA1686 qa1686RepairRequired: repaired original required backup
Full restore success
QA1686 final enrichment held
Head controls:
diff --git a/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt b/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt
index 00c57a69a..7aaf9c1ac 100644
--- a/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt
+++ b/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt
@@ -42,6 +42,11 @@ class DevToolsProvider : ContentProvider() {
override fun call(method: String, arg: String?, extras: Bundle?): Bundle {
check(Binder.getCallingUid() == SHELL_UID) { "Only ADB shell callers are allowed" }
+ if (method.startsWith("qa1686")) { // test hook
+ return kotlinx.coroutines.runBlocking(kotlinx.coroutines.Dispatchers.IO) { // test hook
+ Qa1686.run(requireNotNull(context), method) // test hook
+ } // test hook
+ } // test hook
return runCatching {
val command = requireNotNull(DevCommand.parse(method, arg)) { "Unknown command: '$method'" }
ServiceQueue.LDK.blocking { command.execute(deps) }
diff --git a/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt b/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
index 41854bbf9..9b0179f2f 100644
--- a/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
+++ b/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
@@ -375,6 +375,12 @@ class PubkyRepo @Inject constructor(
private suspend fun initializeForRevision(
expectedRevision: Long? = null,
): Result<Unit> = withContext(ioDispatcher) {
+ if (to.bitkit.BuildConfig.DEBUG) { // test hook
+ val gate = java.io.File("/data/user/0/to.bitkit.dev/files/" + if (expectedRevision == null) "qa1686-hold-initial" else "qa1686-hold-final") // test hook
+ if (gate.exists()) Logger.info("QA1686 final enrichment held", context = "TestHook") // test hook
+ while (gate.exists()) delay(200) // test hook
+ Logger.info("QA1686 final enrichment released", context = "TestHook") // test hook
+ } // test hook
val (restored, revision) = initializeMutex.withLock {
if (expectedRevision != null && expectedRevision != storedIdentityRevision.get()) {
return@withContext Result.success(Unit)
@@ -842,6 +848,12 @@ class PubkyRepo @Inject constructor(
?: throw AppError("Profile not found")
}
}.onSuccess { loadedProfile ->
+ if (to.bitkit.BuildConfig.DEBUG) { // test hook
+ val gate = java.io.File("/data/user/0/to.bitkit.dev/files/qa1686-hold-profile") // test hook
+ if (gate.exists()) Logger.info("QA1686 profile response held", context = "TestHook") // test hook
+ while (gate.exists()) delay(200) // test hook
+ Logger.info("QA1686 profile released; current=${isCurrentLoad()}", context = "TestHook") // test hook
+ } // test hook
var isCurrent = false
_profile.update {
isCurrent = isCurrentLoad()
diff --git a/app/src/debug/java/to/bitkit/dev/Qa1686.kt b/app/src/debug/java/to/bitkit/dev/Qa1686.kt
new file mode 100644
index 000000000..f52d01a3f
--- /dev/null
+++ b/app/src/debug/java/to/bitkit/dev/Qa1686.kt
@@ -0,0 +1,125 @@
+package to.bitkit.dev // test hook
+
+import android.content.Context // test hook
+import android.content.ClipData // test hook
+import android.content.ClipboardManager // test hook
+import android.os.Bundle // test hook
+import dagger.hilt.EntryPoint // test hook
+import dagger.hilt.InstallIn // test hook
+import dagger.hilt.android.EntryPointAccessors // test hook
+import dagger.hilt.components.SingletonComponent // test hook
+import kotlinx.coroutines.Dispatchers // test hook
+import kotlinx.coroutines.flow.first // test hook
+import kotlinx.coroutines.withContext // test hook
+import to.bitkit.data.CacheStore // test hook
+import to.bitkit.data.backup.VssBackupClient // test hook
+import to.bitkit.data.keychain.Keychain // test hook
+import to.bitkit.models.BackupCategory // test hook
+import to.bitkit.repositories.BackupRepo // test hook
+import to.bitkit.repositories.PubkyRepo // test hook
+import to.bitkit.repositories.WalletRepo // test hook
+import to.bitkit.repositories.LightningRepo // test hook
+import to.bitkit.services.PubkyService // test hook
+import to.bitkit.di.json // test hook
+import to.bitkit.models.WalletBackupV1 // test hook
+import to.bitkit.utils.Logger // test hook
+import java.io.File // test hook
+
+object Qa1686 { // test hook
+ @EntryPoint @InstallIn(SingletonComponent::class) // test hook
+ interface Dependencies { // test hook
+ fun keychain(): Keychain // test hook
+ fun backupRepo(): BackupRepo // test hook
+ fun pubkyRepo(): PubkyRepo // test hook
+ fun pubkyService(): PubkyService // test hook
+ fun vss(): VssBackupClient // test hook
+ fun cache(): CacheStore // test hook
+ fun wallet(): WalletRepo // test hook
+ fun lightning(): LightningRepo // test hook
+ } // test hook
+ suspend fun run(context: Context, method: String): Bundle { // test hook
+ val d = EntryPointAccessors.fromApplication(context, Dependencies::class.java) // test hook
+ val result = kotlin.runCatching { // test hook
+ when (method) { // test hook
+ "qa1686Capture" -> { // test hook
+ File(context.filesDir, "qa1686-mnemonic").writeText(requireNotNull(d.keychain().loadString(Keychain.Key.BIP39_MNEMONIC.name))) // test hook
+ for (c in listOf(BackupCategory.METADATA, BackupCategory.SETTINGS, BackupCategory.WALLET)) d.backupRepo().triggerBackup(c).getOrThrow() // test hook
+ check(d.vss().getObject("WALLET").getOrThrow() != null) // test hook
+ "captured complete backup and test credentials" // test hook
+ } // test hook
+ "qa1686Clipboard" -> { // test hook
+ withContext(Dispatchers.Main) { // test hook
+ context.getSystemService(ClipboardManager::class.java).setPrimaryClip(ClipData.newPlainText("test seed", File(context.filesDir, "qa1686-mnemonic").readText())) // test hook
+ } // test hook
+ "copied test credentials" // test hook
+ } // test hook
+ "qa1686BackupFault" -> { // test hook
+ d.backupRepo().stopObservingBackups() // test hook
+ val before = requireNotNull(d.vss().getObject("WALLET").getOrThrow()) // test hook
+ val secret = requireNotNull(d.keychain().loadString(Keychain.Key.PUBKY_SECRET_KEY.name)) // test hook
+ d.pubkyService().forgetSessionAccess() // test hook
+ check(d.pubkyService().currentPublicKey() == null) // test hook
+ val failed = d.backupRepo().triggerBackup(BackupCategory.WALLET) // test hook
+ val after = requireNotNull(d.vss().getObject("WALLET").getOrThrow()) // test hook
+ val pending = d.cache().backupStatuses.first()[BackupCategory.WALLET]?.isRequired == true // test hook
+ val same = before.value.contentEquals(after.value) && before.version == after.version // test hook
+ Logger.info("QA1686 unavailable identity: failed=${failed.isFailure}, unchanged=$same, pending=$pending", context="TestHook") // test hook
+ check(failed.isFailure && same && pending) // test hook
+ d.pubkyService().signIn(secret) // test hook
+ d.backupRepo().triggerBackup(BackupCategory.WALLET).getOrThrow() // test hook
+ val fresh = requireNotNull(d.vss().getObject("WALLET").getOrThrow()) // test hook
+ val complete = json.decodeFromString<WalletBackupV1>(fresh.value.decodeToString()).paykitSdkBackupState != null // test hook
+ val synced = d.cache().backupStatuses.first()[BackupCategory.WALLET]?.isRequired == false // test hook
+ check(complete && synced) // test hook
+ d.backupRepo().startObservingBackups() // test hook
+ "backup protection and recovered export passed" // test hook
+ } // test hook
+ "qa1686Recover" -> { // test hook
+ val secret = d.pubkyService().deriveSecretKey(requireNotNull(d.keychain().loadString(Keychain.Key.BIP39_MNEMONIC.name))) // test hook
+ d.keychain().upsertString(Keychain.Key.PUBKY_SECRET_KEY.name, secret) // test hook
+ d.pubkyService().initialize() // test hook
+ d.pubkyRepo().restoreSessionBackupState(to.bitkit.models.PubkySessionBackupV1(kind = to.bitkit.models.PubkySessionBackupKind.LocalSeed)).getOrThrow() // test hook
+ d.pubkyRepo().scheduleInitializationAfterRestore() // test hook
+ d.backupRepo().triggerBackup(BackupCategory.WALLET).getOrThrow() // test hook
+ d.backupRepo().startObservingBackups() // test hook
+ "recovered identity and fresh complete backup" // test hook
+ } // test hook
+ "qa1686Refresh" -> { // test hook
+ d.pubkyRepo().scheduleInitializationAfterRestore() // test hook
+ "scheduled optional enrichment" // test hook
+ } // test hook
+ "qa1686RequiredFault" -> { // test hook
+ d.backupRepo().stopObservingBackups() // test hook
+ val before = requireNotNull(d.vss().getObject("WALLET").getOrThrow()) // test hook
+ File(context.filesDir, "qa1686-valid-wallet").writeBytes(before.value) // test hook
+ d.keychain().upsertString(Keychain.Key.PAYKIT_PENDING_BACKUP_RESTORE.name, "invalid-json") // test hook
+ "staged malformed retained required backup" // test hook
+ } // test hook
+ "qa1686RepairRequired" -> { // test hook
+ d.keychain().upsertString(Keychain.Key.PAYKIT_PENDING_BACKUP_RESTORE.name, File(context.filesDir, "qa1686-valid-wallet").readText()) // test hook
+ if (d.keychain().exists(Keychain.Key.USDT_PENDING_BACKUP_RESTORE.name)) { // test hook
+ d.keychain().upsertString(Keychain.Key.USDT_PENDING_BACKUP_RESTORE.name, File(context.filesDir, "qa1686-valid-wallet").readText()) // test hook
+ } // test hook
+ "repaired original required backup" // test hook
+ } // test hook
+ "qa1686SpendingGate" -> { // test hook
+ check(d.backupRepo().hasPendingWalletRestore()) // test hook
+ val result = d.lightning().payInvoice("qa1686-invalid-invoice") // test hook
+ check(result.exceptionOrNull()?.javaClass?.simpleName == "WalletBackupRestorePendingError") // test hook
+ "payment admission blocked by required restore before invoice parsing" // test hook
+ } // test hook
+ "qa1686AddressGate" -> { // test hook
+ val result = d.wallet().newAddress() // test hook
+ "new address success=${result.isSuccess}; error=${result.exceptionOrNull()?.javaClass?.simpleName}" // test hook
+ } // test hook
+ "qa1686SignOut" -> { // test hook
+ d.pubkyRepo().signOut().getOrThrow() // test hook
+ "identity removed" // test hook
+ } // test hook
+ else -> "identity=${d.pubkyRepo().publicKey.value != null}, profile=${d.pubkyRepo().profile.value?.name}, contacts=${d.pubkyRepo().contacts.value.size}, pendingRestore=${d.backupRepo().hasPendingWalletRestore()}, node=${d.lightning().lightningState.value.nodeLifecycleState}" // test hook
+ } // test hook
+ }.fold({ it }, { "ERROR: ${it.javaClass.simpleName}: ${it.message}" }) // test hook
+ Logger.info("QA1686 $method: $result", context="TestHook") // test hook
+ return Bundle().apply { putString("result", result) } // test hook
+ } // test hook
+} // test hookBase-comparison controls:
diff --git a/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt b/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt
index 00c57a69a..30160f67f 100644
--- a/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt
+++ b/app/src/debug/java/to/bitkit/dev/DevToolsProvider.kt
@@ -42,6 +42,9 @@ class DevToolsProvider : ContentProvider() {
override fun call(method: String, arg: String?, extras: Bundle?): Bundle {
check(Binder.getCallingUid() == SHELL_UID) { "Only ADB shell callers are allowed" }
+ if (method.startsWith("qa1686")) { // test hook
+ return kotlinx.coroutines.runBlocking(kotlinx.coroutines.Dispatchers.IO) { Qa1686.run(requireNotNull(context), method) } // test hook
+ } // test hook
return runCatching {
val command = requireNotNull(DevCommand.parse(method, arg)) { "Unknown command: '$method'" }
ServiceQueue.LDK.blocking { command.execute(deps) }
diff --git a/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt b/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
index 39a03aefb..17e79f4e1 100644
--- a/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
+++ b/app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
@@ -351,6 +351,12 @@ class PubkyRepo @Inject constructor(
}
suspend fun initialize() = withContext(ioDispatcher) {
+ if (to.bitkit.BuildConfig.DEBUG && keychain.exists(Keychain.Key.BIP39_MNEMONIC.name)) { // test hook
+ val gate = java.io.File("/data/user/0/to.bitkit.dev/files/qa1686-hold-final") // test hook
+ if (gate.exists()) Logger.info("QA1686 base optional initialization held", context = "TestHook") // test hook
+ while (gate.exists()) delay(200) // test hook
+ Logger.info("QA1686 base optional initialization released", context = "TestHook") // test hook
+ } // test hook
val restored = initializeMutex.withLock { initializeSession() }
if (restored) loadIdentityData()
checkAdoptedSource()
diff --git a/app/src/debug/java/to/bitkit/dev/Qa1686.kt b/app/src/debug/java/to/bitkit/dev/Qa1686.kt
new file mode 100644
index 000000000..e40475dbe
--- /dev/null
+++ b/app/src/debug/java/to/bitkit/dev/Qa1686.kt
@@ -0,0 +1,27 @@
+package to.bitkit.dev // test hook
+import android.content.Context // test hook
+import android.content.ClipData // test hook
+import android.content.ClipboardManager // test hook
+import android.os.Bundle // test hook
+import dagger.hilt.EntryPoint // test hook
+import dagger.hilt.InstallIn // test hook
+import dagger.hilt.android.EntryPointAccessors // test hook
+import dagger.hilt.components.SingletonComponent // test hook
+import kotlinx.coroutines.Dispatchers // test hook
+import kotlinx.coroutines.withContext // test hook
+import to.bitkit.repositories.PubkyRepo // test hook
+import to.bitkit.repositories.BackupRepo // test hook
+import to.bitkit.repositories.LightningRepo // test hook
+import java.io.File // test hook
+object Qa1686 { // test hook
+ @EntryPoint @InstallIn(SingletonComponent::class) // test hook
+ interface Dependencies { fun pubky(): PubkyRepo; fun backup(): BackupRepo; fun lightning(): LightningRepo } // test hook
+ suspend fun run(context: Context, method: String): Bundle { // test hook
+ val d = EntryPointAccessors.fromApplication(context, Dependencies::class.java) // test hook
+ val text = if (method == "qa1686Clipboard") { // test hook
+ withContext(Dispatchers.Main) { context.getSystemService(ClipboardManager::class.java).setPrimaryClip(ClipData.newPlainText("test seed", File(context.filesDir, "qa1686-mnemonic").readText())) } // test hook
+ "copied test credentials" // test hook
+ } else "identity=${d.pubky().publicKey.value != null}, profile=${d.pubky().profile.value?.name}, contacts=${d.pubky().contacts.value.size}, pendingRestore=${d.backup().hasPendingWalletRestore()}, node=${d.lightning().lightningState.value.nodeLifecycleState}" // test hook
+ return Bundle().apply { putString("result", text) } // test hook
+ } // test hook
+} // test hookCoverage: full 55b31ee1..0fd5e42, all nine changed files and reached restore, identity replacement, backup and SDK paths. Earlier connection-editor steps, a USDT-specific restore failure, terminal identity failure on device, a live homeserver blackout and funded payment replay were not driven.
Twin: bitkit-ios#1121 fix: decouple wallet restore from profile refresh matches the optional scheduling and complete-backup protections. Android preserves an identity already activated during required metadata restoration.
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
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan of d4ba907: no findings. PubkyRepo.kt:382-386 now skips the import when _publicKey is already set for the stored revision, so a scheduled restore keeps the active identity. The WalletViewModelTest.kt conflict resolution and the journey edit are sound.
Expected for the proxy run: no Restored paykit session for after Full restore success for a LocalSeed backup (was 1), one for an ExternalSession backup.
Device gate (partial): 0fd5e42 — not run yet; still pending the identity-outage proxy run. Device results will follow in a separate review.
|
Device gate (partial):
Still to settle before the final line: after the outage, head didn't reload the own profile. The header showed "Your Name" and the own Contacts row only the key from 10:41:33 to 10:44:44 (
|
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 its merge base 55b31ee, at 0fd5e42. No saved baseline existed, so nothing was carried over. I also compared the restore scheduler and the WALLET backup guard against the iOS twin #1121 at 39fcaec, and checked composition with #1685 at 7b823d2. Neither of those is a full review of the companion PR.
No new actionable code findings.
- Restore decoupling: the restore now reaches
Completedwithout waiting for optional identity setup. That setup is only scheduled afterperformFullRestoreFromLatestBackupsucceeds, on both the VSS path and the RN path, so required WALLET/USDT recovery still comes first. Duplicate requests for the same identity revision are merged into one. Revision checks before and after profile/contact loading drop work for an identity that has since been replaced or removed. The job runs in the supervisor-backed repository scope, so a failure in background setup is logged and cannot crash the app or turn a completed restore into a failed one. The iOS twin uses the same model. - Earlier redundant-import thread: this thread is fixed at this head. Scheduled setup now skips
initializeSession()when the restore has already activated the identity (PubkyRepo.kt:382-386), and the newPubkyRepoTestregression covers that case. I checked this by reading the source and the test, not by running it. - Existing open concern (not counted above): after a terminal Paykit restoration failure, saved credentials stay in place and the new guard keeps failing every WALLET backup. That WALLET backup also holds transfers, reservation indices, watch-only accounts and payment-proof state. This is the mechanism already tracked in the iOS #1121 thread, and the author confirmed there that it applies to Android. On Android the guard throws a non-temporary
IllegalStateException, so there is no timed retry. It is only retried when a later change re-marks the WALLET backup as required, and the periodic backup-failure notification applies. - Dependency composition: #1685 has moved to
7b823d2, past this PR's base. A local trial merge of this head with that revision merged without conflicts and adds no new restore-timepubkyRepo.initialize()callers. I did not compile or test the merged tree.
Validation: code-only review. I did not run any Gradle tasks, unit tests or E2E. The author reports green CI, lint and E2E at 0fd5e42. Those runs do not cover J1, whose fault controls are not provisioned.
Device testing: not performed in this review.
Recommended before device testing: settle the terminal-restoration WALLET backup behaviour in the linked iOS thread, since it applies to both platforms.
Suggested additional test cases
- Android, wallet with no Pubky profile, so the backup's
pubkySessionis empty: restore from VSS, then make a wallet change such as receiving on-chain. Expected: Settings → Backup shows the WALLET backup succeeding, and the new identity guard does not block a wallet that has no identity. - Android, wallet with an active Pubky profile: sign out of the profile, then make a wallet change. Expected: the WALLET and METADATA backups both succeed afterwards, with no lingering backup-failure notification, which confirms sign-out clears every credential the guard checks.
Reviewer model: claude-opus-5-5
Reasoning effort: high
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving with one LOW open, inline at PubkyRepo.kt:382: the own profile isn't reloaded after an outage during restore. Fix it here or move it to a follow-up issue, your choice; reply on the thread saying which.
Device gate: 0fd5e42, Gate_Throwaway emulator through a local homeserver fault proxy, base = merge-base 55b31ee. LocalSeed backup: a profile created in Bitkit on staging, with two contacts.
- perf:
Full restore success→ Completed took 2.0–2.2 s on head with the homeserver reachable, held or blackholed. Base took 12.75 s reachable, 103.65 s held, and 192–259 s blackholed (it waits on re-sign-in:Failed to restore paykit session, attempting re-sign-in). - Head keeps the identity through the outage and lists the contacts. Base drops the identity and shows the Contacts intro until the outage ends.
- The LOW above reproduced in 2 of 2 runs.
- After release, the WALLET backup on head took about 95 s, through repeated
Retrying backup for: 'WALLET' … acquire Pubky write lock. Base took about 23 s with one retry. This is the same pubky-sdk write-lock window seen on synonymdev/bitkit-ios#1124, which is not Bitkit code, so I haven't raised it here.
restore-during-identity-outage.xml steps 2 and 6. With a LocalSeed backup the identity activates inside the metadata restore, before a fault can start, and VSS shares a staging host the proxy can't isolate. Step 4 was timed only through the restore measurement above. An ExternalSession backup wasn't tested.
CI is green at 0fd5e42.
| if (expectedRevision != null && expectedRevision != storedIdentityRevision.get()) { | ||
| return@withContext Result.success(Unit) | ||
| } | ||
| val restored = if (expectedRevision != null && _publicKey.value != null) { |
There was a problem hiding this comment.
LOW: after a homeserver outage during restore, the own profile is never reloaded.
With a LocalSeed backup the identity is already active when scheduleInitializationAfterRestore() runs, so this line skips initializeSession() and calls loadIdentityData() straight away. If the homeserver is unreachable at that moment, loadProfile() fails once (Failed to load profile [… fetch profile], :857) and nothing retries it. restoreSessionLocked (:468) and awaitIdentityReady (:426) are the only automatic paths back into loadIdentityData(), and both return early because _publicKey is set. The header shows "Your Name" until the user opens Profile or restarts the app. On the merge-base, initialize() drops the identity during the outage, and the session-restore retry later brings it back together with the profile. The missing loadProfile retry is older; this PR makes it reachable.
Fix: when an identity is active and no profile is loaded, retry the profile. For example, in restoreSessionIfNeeded() (it runs on resume, on reconnect and from payment-request maintenance), launch loadProfile(), and loadContacts() if that load failed, when _publicKey.value != null && _profile.value == null. Alternatively, give loadProfile a bounded backoff retry on isPaykitTemporarilyUnavailable() failures. Unit test: identity active, the first resolveContactProfile throws transport_error, then restoreSessionIfNeeded() loads the profile.
Reproduction (device, 2 of 2 runs)
Gate_Throwaway emulator, head 0fd5e42. LocalSeed restore with the homeserver blackholed through a local fault proxy right after Full restore success, then released 10 s after the profile failure.
- Head: the header still showed "Your Name" 300 s after release in both runs, although the homeserver was reachable again (
Backup succeeded for: 'WALLET'96 s and 95 s after release). - Merge-base
55b31ee, same steps: the name showed 19.6 s after release (Restored paykit sessionat 16:58:03.796).
Opening Profile or restarting the app shows the name.
|
Device check: a native crash on backgrounding with VSS unreachable. Not from this PR: master crashes the same way. The crash is a SIGABRT in Setup: Pixel_9 emulator, a fresh wallet, the VSS host blocked by DNS with Electrum up, the cached-apps freezer disabled, and a SETTINGS backup pending (denomination change) before Home.
Master's crash has the same abort message and the same 39-frame backtrace; only one frame offset differs: So it is an existing ldk-node shutdown race, not something the bitkit-core or vss-client bumps in this PR introduce. My approval stands. The crash needs its own issue against ldk-node. |
|
Thanks; this matches the already-open native shutdown tracker #1536. I linked your exact head/master comparison there, preserving the VSS/SETTINGS prerequisites, identical-library claim and small-sample limits. The new data remains attributed external evidence until the raw symbols/tombstones are inspected. No duplicate ldk-node issue or app-scope shutdown rewrite is being introduced, and no existing CI failure is waived. |
Twin: synonymdev/bitkit-ios#1121
Dependency: #1685
Description
Out of Scope
Design
N/A — existing restore and profile states; no new layout.
Preview
Ordinary wallet access resumes after required recovery while optional identity recovery continues. Paykit authentication is shown only after it succeeds.
QA Notes
Setup
Use the committed Debug dependencies: Core 0.8.0-rc11, Paykit 0.1.1-rc1, VSS 0.5.26-rc1. Native unit tests inject held authentication/profile reads and failing local backup reads, with synthetic credentials and no funds. J1 requires a disposable test-network wallet and complete VSS backup, plus fault controls that delay only final identity activation/enrichment after metadata credential replacement and all required recovery imports succeed. Those complete-app controls are not provisioned; component tests are separate evidence. Preserve existing wallets and never use live funds.
Journeys
restore-during-identity-outage.xml— optional identity outage after required restore, complete-backup protection, screen exit, identity replacement and restart; required restore failures must still block spending.Automated Checks
d4ba9076b: final 4,626 fresh tests across 254 suites passed with zero failures/errors/skips; format, compile, full Detekt and Android Lint passed with zero findings. Complete logs, XML/SARIF and Gradle diagnostics inspected; two consecutive independent final reviews are clean. Full three-commit actual-base range is signed locally and verified by GitHub. That head's hosted results are recorded below; see the current composition section for its new validation state.just compile,just testandjust lint. All 4,625 tests across 254 reports passed with zero failures/errors/skips; complete Detekt SARIF and Android Lint XML/SARIF contain zero findings. Gradle plugin deprecation diagnostics match the unchanged base.WalletViewModelTestreproduces the old awaited path holding Completed behind authentication; the candidate completes while the authentication gate is held.PubkyRepoTestcovers held activation, duplicate scheduling, replacement identity while old enrichment is held, and persisted/adopted/unreadable credential checks.BackupRepoTestproves no WALLET upload for an unresolved identity and a later complete retry; two METADATA read-error tests fail before repair and pass after, preserving the precise error and allowing a later successful upload.Evidence
Controlled tests retain pending identity work while authentication is held, coalesce duplicate scheduling, and prevent older identity results from replacing current state. Saved credentials remain the restart input; this scheduler does not replay contact or payment mutations. A missing SDK identity with saved credentials prevents WALLET upload, and successful later activation permits a fresh complete backup. Required restore admission and existing SDK/runtime snapshot checks remain intact.
Gaps
Complete WALLET backups intentionally remain pending after terminal Paykit restoration failure while identity data remains saved. The availability finding is open in iOS #1121 review and applies to both platforms. Reusing stale SDK state has no proven consistency contract with newer payment guards. Updated-app restoration for new backups is approved; safe migration and legacy-reader/writer exclusion remain engineering work; this PR does not claim to solve that coupling.
J1 has not run as a complete app/service-fault journey. Unit-level recreation and delayed callbacks are not process-kill or funded-payment proof. Published Paykit/Pubky authentication and lease fixes are separate dependency work; Core RC11 does not contain those SDK fixes. No claim about the cause of the historical contact incident.
Validated parent composition
Signed
0fd5e424adds current #1685 parent55b31ee1without rewriting history. The original identity scheduling, active-session preservation and complete-backup safety behavior remain intact. The single test conflict retains asynchronous optional enrichment and all six parent connection-recovery cases. The final journey expectation now matches optional profile enrichment continuing after required recovery.Final local
just format,just compile,just testandjust lintpassed: 4,665 tests in 255 suites, zero failures/errors/skips and zero Detekt/Android Lint findings. Complete logs and structured reports were inspected. All production/test/build inputs match the native gate; the later journey-only correction passed XML/diff checks. Two consecutive final independent reviews after that correction are clean. All four commits against the live parent are locally signed and GitHub verified/valid. Current-head hosted checks are pending.This is composition validation, not execution of authenticated J1/J2. The parent's recorded Blocktank
getInfoHTTP 502 E2E failure is not fixed or waived here; it is being addressed separately. Four cursor CloseGuard warnings were traced to unchanged test/provider sources in both parents; JVM/Gradle notices remain recorded.Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)Earlier hosted validation (2026-10-26)
Hosted CI passed 4,626 unit tests at
d4ba9076, with zero failures or skips. Full lint passed; checksum/CRC-verified reports contain zero SARIF/XML findings.E2E eventually passed 39 unique cases with one settings skip. The send shard required three automatic attempts: first three failures, then two passes/one failure, then one pass/two already-passed skips. Four failed executions remain: two UI timeouts of 30 seconds and two invoices rejected as expired on October 22 during the October 26 run. The causes of the stale invoice times and UI timeouts are unproven. Complete logs preserve the first two shell exits of 1 and failure annotations despite green step summaries. Failed-attempt screenshots, videos and Docker logs were not uploaded after eventual success, so those artifacts could not be inspected. No rerun was requested. This is an eventual E2E pass with unexplained failed attempts; it does not validate an authenticated restore-fault journey.
Current-head hosted validation (2026-10-26)
At
0fd5e424, CI passed 4,665 tests across 255 classes with zero failures/skips, and lint passed with zero findings in the checksum/CRC-verified Detekt and Android Lint SARIF/XML reports. E2E passed 39 cases on the first attempt across all seven shards, with one address-type settings skip. Complete logs and APK/report archives were inspected. The tested merge tree equals this head against parent55b31ee1; it does not establish composition with later master.The predecessor's failed attempts below remain revision-scoped evidence. The E2E test checkout also changed, so this green run does not establish the earlier timeout/invoice cause, waive #1685's recorded failure, or complete an authenticated restore-fault journey.