Repository navigation
fix: guard wallet operations until required restore completes - #1685
ben-kaufman wants to merge 2 commits into
Conversation
|
Triage: 3.1.0. It protects wallet operations during a shipped restore flow, and 3.1.0 is due 28 Oct. |
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryBlocks payments, new receive addresses, channel changes and fee changes while required wallet recovery remains pending.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Wallet operation requested] --> B{Required restore pending?}
B -->|Yes| C[Return failure]
B -->|No| D[Prepare or authorize operation]
D --> E[Native queue rechecks restore markers]
E -->|Pending| C
E -->|Clear| F[Run native operation]
G[Restore retry] --> H{Restore already running?}
H -->|Yes| I[Skip overlapping retry]
H -->|No| J[Restore wallet data]
J --> K{Required recovery complete?}
K -->|No| L[Keep Retry available]
K -->|Yes| M[Clear durable markers]
Reviews (1) · Last reviewed commit: "fix: guard wallet operations while requi..." · Reviewed by Greptile |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Blocks new receive data, payments, channel and fee mutations and prepared broadcasts while either durable restore marker (PAYKIT_PENDING_BACKUP_RESTORE, USDT_PENDING_BACKUP_RESTORE) exists, checked at repository admission and again inside the LDK queue, hides Proceed Without Backup while a marker remains, and coalesces overlapping backup retries. Reviewed 837aa1c, full tier, stacked on #1681 (reviewed as the delta against fix/usdt-recovery-parity), reasoned from the code: per the repo's review rules this branch is not built here.
What I checked, and 6 candidates I ruled out
Read in full: the diff; WalletRestoreState.kt; performFullRestoreFromLatestBackup, restoreUsdtRecovery, restoreWalletBackup, performRestore and captureWalletBackupDataBytes in BackupRepo.kt; executeWhenNodeRunning, sendOnChain, payInvoice, prepareRelease in LightningRepo.kt; newAddress*, prepareOnchainSend, prepareRelease, send, bumpFeeByRbf, accelerateByCpfp, reconcileWatchOnlyAccounts in LightningService.kt; broadcastPreparedCandidate, current, releaseInterruptedUnsubmittedPreparation in OnchainSendAttemptStore.kt; restoreFromBackup, onBackupRestoreRetry, onProceedWithoutRestore, collectStates, RestoreState in WalletViewModel.kt; the restore branch of ContentView; WalletRestoreErrorView.
Call sites traced: every caller of newAddress/newAddressInfoForType (PrivatePaykitAddressReservationRepo, BlocktankRepo refund allocation, WalletRepo.newAddress, TransferViewModel reverse swap claim), payInvoice (AppViewModel, QuickPayRepo, PaykitAllowanceExecutor, reverse swap), sendOnChain, openChannel, closeChannel, bumpFeeByRbf, accelerateByCpfp, retryOriginalOnchainSend.
CI: build, build-local, lint, detekt and three of the four local e2e shards green; e2e-tests-local - send still pending at review time.
Ruled out
- Restore deadlocking on its own gate:
restoreWalletBackupwrites the marker first, then runsreconcileWatchOnlyAccountsandreconcileReservedIndexesWithLdk, which reach LDK only throughrevealReceiveAddressesToAccountandrevealReceiveAddressesTo. Neither is gated, andPrivatePaykitRepo.restoreBackupandPaykitPaymentRequestRepo.activateallocate no address, so the marker cannot block the restore that clears it. The newLightningRepoTestcase assertsrevealReceiveAddressesandsyncstill succeed. - Receive address left empty after restore: the restored cache is cleared by
resetBip21()while the marker is set, and gatednewAddresscalls fail until it clears. Opening the Receive sheet callsrefreshReceiveState, andrefreshReusableReceiveAddressOrThrowregenerates an empty address, after the marker is gone, so nothing stays empty past the success screen. - Throwing after
beforeDispatchpersistedinitialDispatchAttempted = true: a marker that appears between the two checks leaves the attempt marked dispatched with no broadcast, andsendOnChainreturnsOnchainSendPendingError. That fails closed (the attempt keeps blocking the next send until recovery resolves it), and the window needs a restore to begin during an in-flight send while the restore UI blocks the wallet. - Proceed racing a new marker:
onProceedWithoutRestorerechecks after thestop()delay and an unreadable keychain reads as pending (getOrDefault(true)), so dismissal cannot outrun a marker written meanwhile. The defaultBackupFailed(count, hasPendingWalletRestore = true)also keeps the button hidden on thecollectStatescold-start path. - Retry coalescing dropping a request:
tryLockreturns early only while a restore is running, and that restore reads the same durable markers when it fails, so the dropped second tap has nothing the first did not do.unlocksits in the existingfinally, aftersetRestorePending(false). - Parity with iOS: iOS
BackupService.hasPendingWalletRestore()also treats an unreadable state as pending, matching the fail-closed reads here.
Merge confidence: 4/5, no findings, but the send e2e shard had not finished and J1 is unrun (its fixture is not provisioned, as the PR says).
Dependency: #1681
Description
A failed wallet backup restore could leave durable recovery pending while repeated-failure navigation allowed ordinary payments or new receive addresses. Block those operations at repository admission and native dispatch until required restore completes, including typed retries, channel/fee mutations and already prepared broadcasts. Keep node sync and reservation reveal available because restoration needs them, and coalesce overlapping backup retries.
The recovery screen offers Retry while a required marker remains. If wallet recovery completed and only later profile loading failed, existing Proceed behavior remains available; the action rechecks current durable state and fails closed on unreadable markers.
Out of Scope
No new backup format, payment replay or SDK import policy. Post-restore identity network latency and independent full-wallet snapshots remain separate investigations.
Design
N/A — no design available. Existing recovery screen; conditional visibility of Proceed Without Backup.
Preview
Required pending wallet recovery cannot be dismissed through the error screen.
QA Notes
Setup
Use committed Debug Core 0.8.0-rc11, Paykit 0.1.1-rc1 and VSS 0.5.23 dependencies. Unit tests provide persistent marker state and controlled node/backup callbacks; Compose tests run the actual recovery component on an API 36 emulator. J1 requires a disposable test-network fixture with a complete backup containing an unresolved payment and private address ceilings, VSS download/import fault controls and a post-restore profile-cache failure control. These complete app controls are not provisioned. Never reset a real wallet or use live funds.
Journeys
pending-wallet-recovery.xml— repeated failure, restart, operation admission, overlapping retry and safe profile-only failure. Unit/Compose component results are separate evidence, not execution of this complete journey.Automated Checks
just compile,just test,just lint: passed. Final full suite: 4,617 tests across 254 reports, zero failures/errors/skips. Complete Detekt SARIF and Android Lint XML/SARIF: zero findings; structured Gradle diagnostics match the unchanged base plugin deprecation.WalletRestoreErrorViewTest: both Compose tests passed on owned API 36 emulator. App/test APK installed bytes matched build outputs.Evidence
Pending required state prevents new operation admission and broadcast, including after preparation. Restore-only sync/reveal stay available. Proceed rechecks markers after asynchronous work; later marker changes prevent dismissal. Five independent review rounds, with two consecutive clean final rounds. The review-discovered native queue and post-callback broadcast gaps each fail before/pass after the shared boundary checks.
Gaps
Repository recreation is not a full process-kill VSS restore journey. Tests use controlled repositories/native adapters; no funded payment was submitted. iOS already gates normal startup on durable pending recovery and uses retry-only restore UI; its distinct post-restore identity wait remains separate work.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)