Skip to content

fix: guard wallet operations until required restore completes - #1685

Open
ben-kaufman wants to merge 2 commits into
fix/usdt-recovery-parityfrom
fix/pending-wallet-restore-admission
Open

ben-kaufman wants to merge 2 commits into
fix/usdt-recovery-parityfrom
fix/pending-wallet-restore-admission

Conversation

@ben-kaufman

Copy link
Copy Markdown
Contributor

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

  • J1: new 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

  • Unchanged admission permits address allocation/payment authorization in two failing regressions. The original Proceed handler settles despite a required marker; overlapping retries invoke restore twice. Each candidate behavior has pass-after coverage.
  • New coverage checks both marker types, unreadable state, repository recreation, callback races, explicit retries/channel/fee paths, prepared release broadcast, retry coalescing and profile-only failure after markers clear.
  • Required 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

  • Planning/scoping: gpt-6-astra (reasoning: xhigh)
  • Implementation: gpt-6-astra (reasoning: xhigh)
  • Review: gpt-6-astra (reasoning: xhigh)

@ovitrif ovitrif added this to the 3.1.0 milestone Oct 24, 2026
@ovitrif

ovitrif commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator

Triage: 3.1.0. It protects wallet operations during a shipped restore flow, and 3.1.0 is due 28 Oct.

@greptile-apps

greptile-apps Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

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

Summary

Blocks payments, new receive addresses, channel changes and fee changes while required wallet recovery remains pending.

  • Wallet operations wait until required backup recovery finishes.
  • The recovery screen keeps retrying until wallet recovery is complete.

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]
Loading

Reviews (1) · Last reviewed commit: "fix: guard wallet operations while requi..." · Reviewed by Greptile

@github-actions

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 837aa1c (run).

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

@talosmachina talosmachina left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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: restoreWalletBackup writes the marker first, then runs reconcileWatchOnlyAccounts and reconcileReservedIndexesWithLdk, which reach LDK only through revealReceiveAddressesToAccount and revealReceiveAddressesTo. Neither is gated, and PrivatePaykitRepo.restoreBackup and PaykitPaymentRequestRepo.activate allocate no address, so the marker cannot block the restore that clears it. The new LightningRepoTest case asserts revealReceiveAddresses and sync still succeed.
  • Receive address left empty after restore: the restored cache is cleared by resetBip21() while the marker is set, and gated newAddress calls fail until it clears. Opening the Receive sheet calls refreshReceiveState, and refreshReusableReceiveAddressOrThrow regenerates an empty address, after the marker is gone, so nothing stays empty past the success screen.
  • Throwing after beforeDispatch persisted initialDispatchAttempted = true: a marker that appears between the two checks leaves the attempt marked dispatched with no broadcast, and sendOnChain returns OnchainSendPendingError. 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: onProceedWithoutRestore rechecks after the stop() delay and an unreadable keychain reads as pending (getOrDefault(true)), so dismissal cannot outrun a marker written meanwhile. The default BackupFailed(count, hasPendingWalletRestore = true) also keeps the button hidden on the collectStates cold-start path.
  • Retry coalescing dropping a request: tryLock returns 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. unlock sits in the existing finally, after setRestorePending(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).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants