Skip to content

btcwallet: persist named account recovery bounds - #11189

Open
bhandras wants to merge 2 commits into
lightningnetwork:masterfrom
bhandras:account-recovery-backup
Open

bhandras wants to merge 2 commits into
lightningnetwork:masterfrom
bhandras:account-recovery-backup

Conversation

@bhandras

@bhandras bhandras commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Named wallet accounts cannot be recovered from a seed alone. A periodic account snapshot can miss internal change issued and broadcast before the next snapshot. After wallet loss, a seed restore can then miss spendable outputs.

This adds an opt-in --wallet-account-backup file. The btcwallet backend records account identity and both branch bounds before named address, account creation/import, and PSBT funding calls return success. The recorder merges branch maxima, rejects identity changes, serializes writers with a lifetime lock, and persists with file fsync, atomic rename, and directory fsync. Normal startup rejects missing evidence or incompletely reconstructed named accounts. One-time enrollment is explicit and refuses an existing file.

The file must live on storage independent of the wallet database. The guarantee covers successfully issued keys through the documented methods. Default-account operations retain existing behavior. Entirely watch-only/remote-signing wallets, arbitrary offline xpub derivation, and unacknowledged keys observed through inspection are outside this first implementation. Imported account metadata still requires its original signing source.

The recovery guide documents enrollment, immutable account index/scope/schema checks, reconstruction of both branches before rescan, failure handling, and rollback. The optional secondary Kubernetes exporter is lndinit #100; it is not the synchronous authority and is not required for this change.

Validation:

  • The new wallet-account_backup_recovery integration case passes with both bitcoind and btcd. It covers real RPC failure on unavailable storage, immediate durable counts, restart, seed restoration at account index 2, an omitted-internal-branch negative control, recovery of exactly 799,225 sat and a confirmed spend. The test also recreates raw key family 300 after seed restoration and verifies the same public key. Added as separate signed commit 47834ee. Native Windows is explicitly skipped because this mode requires directory fsync; the platform limit and reproduction commands are documented.
  • Fable review follow-up: replaced integration helpers defined only in _test.go, fixing ordinary package and coverage compilation. go build ./itest/, go vet ./itest/, and coverage-package compilation pass. Recovery documentation now explains raw key families above 255 and retries after account creation/import commits before a backup failure.
  • Full race suites for ./lnwallet/accountbackup ./lnwallet/btcwallet passed.
  • Real wallet allocation/funding tests cover persistence failure, failed results, stale snapshots, identity conflicts, and unchanged default-account availability.
  • The isolated Bitcoin Core/LND regtest drill creates account index 2, publishes internal change, deletes the wallet, restores the seed and verifies a confirmed spend. Omitting the internal branch is rejected at protected startup and recovers zero of 798,570 sat in the unprotected negative control. Correct reconstruction recovers all 798,570 sat. The drill passed twice.
  • Repository-pinned custom golangci-lint v2.4.0 with the repository plugin passed for the changed wallet packages and root package. The custom builder's git invocation fails locally; the same pinned linter/plugin was built directly.
  • Both binaries build with walletrpc signrpc; sample configuration validation checks 367 options. Formatting, all-module tidiness, Go documentation audit and diff whitespace checks passed.

Independent code review is READY, including the final compatibility fix. Hosted static checks caught a direct bbolt import breaking the WebAssembly RPC build. The final head uses the existing kvdb Bolt wrapper; make rpc-js-compile, lock/wallet race regressions and custom lint now pass.

Gateway run 34508412499 exceeded its 15-minute job limit and was cancelled without a verdict. This remains an external review blocker. The lint blank line and release-note PR link are also fixed. CI is rerunning on signed head 47834ee; this PR is not yet declared merge-ready. No runtime rollout is included.

@bhandras

Copy link
Copy Markdown
Collaborator Author

/gateway review

Review PR #11189 at 7fd3dad against master. Independently verify the invariant: before a supported named-account address or funded PSBT returns success, independent durable storage contains that account's immutable identity and both branch bounds covering the issued keys. Normal startup must reject missing evidence and incompletely reconstructed named accounts.

Trace NewAddress, LastUnusedAddress, CreateAccount, ImportAccount, FundPsbt, PsbtCoinSelect, automatic change paths, storage failure after allocation, concurrent stale snapshots, process restart, and seed restoration. Check atomic replace/fsync/locking and identity matching for imported xpubs sharing a derivation index. Default-account availability is deliberately unchanged; entirely watch-only wallets, offline xpub derivation, and inspection of unacknowledged keys are explicitly outside this feature's scope.

Validation: full accountbackup/btcwallet race suites; real wallet issuance and failure regression tests; a Bitcoin Core/LND disaster-recovery drill passed twice, including omitted-internal-branch negative control (zero of 798,570 sat), protected-startup rejection, complete reconstruction/rescan and confirmed spend. Pinned custom lint, formatting, module tidiness, sample config and builds pass. Independent review is READY after fixing imported-account identity matching.

For each finding give a reachable trigger/execution path, consequence, change attribution, existing guards/recovery, likelihood, smallest useful fix and regression risk. Classify blocker, follow-up, or not worth changing. Do not demand arbitrary offline derivation recovery, remote-signing support, or default-account redesign in this opt-in named-account change. Require BLOCKED, READY WITH FOLLOW-UPS, or READY. Stop once the stated invariant is proven and no concrete introduced blocker remains.

@github-actions github-actions Bot added the severity-critical Requires expert review - security/consensus critical label Sep 10, 2026
@github-actions

Copy link
Copy Markdown

🔴 PR Severity: CRITICAL

file classification | 12 files (excl. tests) | 898 lines changed (excl. tests)

🔴 Critical (5 files)
  • lnwallet/accountbackup/backup.go - new lnwallet/* package for wallet account backup logic
  • lnwallet/btcwallet/account_backup.go - lnwallet/btcwallet backend implementation of account backup/export
  • lnwallet/btcwallet/btcwallet.go - core btcwallet integration, wallet lifecycle changes
  • lnwallet/btcwallet/config.go - btcwallet configuration wiring for the new backup feature
  • lnwallet/btcwallet/psbt.go - PSBT signing path changes
🟡 Medium (4 files)
  • config.go - top-level daemon config wiring
  • config_builder.go - config builder wiring for account backup
  • go.mod - dependency bump
  • sample-lnd.conf - sample config documentation for new option
🟢 Low (5 files)
  • docs/release-notes/release-notes-0.22.0.md - release notes
  • docs/wallet-account-recovery.md - new documentation
  • lnwallet/accountbackup/backup_test.go - test-only
  • lnwallet/btcwallet/account_backup_test.go - test-only
  • scripts/test-account-backup.py - test/dev tooling script

Analysis

This PR adds a new wallet account backup/recovery feature that touches lnwallet/* and lnwallet/btcwallet/*, including changes to the PSBT signing path (psbt.go) and core wallet lifecycle code (btcwallet.go). Per policy, any change to lnwallet/* is CRITICAL since it involves wallet operations and signing. Excluding test files, the change spans 12 files and ~898 lines, which is substantial, though the severity is already at the maximum tier so no further bump applies. Recommend review by someone familiar with the wallet/signing internals, particularly around the PSBT and account export paths.


To override, add a severity-override-{critical,high,medium,low} label.

@lightninglabs-gateway

Copy link
Copy Markdown

👀 gateway review starting…

@bhandras
bhandras force-pushed the account-recovery-backup branch from 7fd3dad to 6ddf274 Compare September 10, 2026 17:33
@bhandras

Copy link
Copy Markdown
Collaborator Author

Current head is 6ddf274. The only change since the requested 7fd3dad review is line wrapping in one configuration error. Root-package custom lint, formatting and all-module tidiness now pass. Independent review confirms no behavior change; the original recovery test evidence remains valid.

@bhandras

Copy link
Copy Markdown
Collaborator Author

Review status: independent review is READY and the local recovery/race/lint/format/module checks pass. Gateway run 34508412499 was cancelled after reaching the review job's 15-minute limit without publishing a verdict. Gateway review remains blocked by that external run failure; cancellation is not approval. Hosted CI is still running/queued. This PR is not declared merge-ready.

The current signed head is 6ddf274. It differs from the initial requested-review head only by one error-message line wrap. A successful Gateway review of this head and completed green CI are required to remove the remaining review gates.

@bhandras
bhandras force-pushed the account-recovery-backup branch from 6ddf274 to 0244140 Compare September 10, 2026 17:52
@bhandras

Copy link
Copy Markdown
Collaborator Author

Fixed the hosted static-check failure in signed head 0244140. Direct bbolt import pulled an unsupported platform implementation into the WebAssembly RPC dependency graph. The recorder now uses LND's existing kvdb Bolt wrapper, preserving the same native lock and using the existing WebAssembly stub.

make rpc-js-compile passes locally. Native lock and wallet-issuance race regressions pass. Custom lint reports zero issues. Independent delta review is READY: native lock mode, one-second timeout, stable inode, and recovery-file fsync/rename are preserved. Hosted CI is restarting on this head.

Gateway's previous run exceeded its 15-minute execution limit without a verdict. That external review gate remains open; no merge or rollout is requested until review and hosted CI complete.

Persist account identity and both derivation branches before named
address and funding calls return success. Fail closed when durable
metadata cannot be written or startup would regress recorded bounds.

Document enrollment and restore limits, and prove recovery with wallet
loss, an internal-branch negative control and a confirmed spend.
@bhandras
bhandras force-pushed the account-recovery-backup branch from 0244140 to c83226f Compare September 11, 2026 10:06
@bhandras

Copy link
Copy Markdown
Collaborator Author

Added a separate signed integration-test commit, c83226f: itest: prove named account recovery after loss.

The normal LND integration harness now runs wallet-account_backup_recovery. It uses real daemon processes and RPCs to prove:

  • Account index 2 and receive bounds are durable immediately after successful issuance.
  • Inaccessible storage makes address and PSBT-funding RPCs return errors without usable results.
  • Both branch counts are durable immediately after successful FundPsbt, before signing/publication.
  • Restart accepts the surviving record without reenrollment.
  • After deleting the original wallet, seed restoration preserves the account path/xpub at its original index.
  • Omitting internal derivation fails protected startup. An unprotected rescan recovers zero of the remaining change.
  • Reconstructing both branches recovers exactly 799,225 sat of internal change and permits a confirmed spend, leaving 698,450 sat.

Passed:

make itest backend=bitcoind icase=wallet-account_backup_recovery
make itest backend=btcd icase=wallet-account_backup_recovery

Custom lint, Go documentation audit, formatting, module tidiness and commit checks pass. Independent review is READY after explicitly skipping native Windows: this mode requires directory fsync, which its current Windows handle cannot provide. The recovery guide now documents that platform limit and the harness commands.

The implementation commit is now 6ed7d9d. Its only changes since 0244140 are the missing lint blank line and the required release-note PR link. The integration test remains a separate commit. Prior CI failures in invoice PostgreSQL fixtures and peer startup happened with this opt-in feature disabled; they were not expanded into this test change. Hosted CI will rerun on the new head. Gateway's earlier timeout remains unresolved; no merge-ready claim is made.

Exercise real wallet RPCs and inspect the independent recovery file
before addresses and funded PSBTs can escape. Reject failed writes,
then restore the seed into a new wallet at the original account index.

Prove that omitting the internal branch fails protected startup and
recovers zero change without the guard. Reconstruct both branches and
confirm a spend of the recovered output. Run with bitcoind and btcd.

Document and skip native Windows because this durability mode requires
directory fsync. Keep the production fail-closed boundary unchanged.
@bhandras
bhandras force-pushed the account-recovery-backup branch from c83226f to 47834ee Compare September 11, 2026 14:28
@bhandras

Copy link
Copy Markdown
Collaborator Author

Addressed the concrete build blocker and recovery documentation gaps in signed head 47834ee. The integration test remains a separate commit.

  • Fixed references from ordinary itest source to helpers defined only in _test.go. go build ./itest/, go vet ./itest/, and go test -coverpkg=./... -run '^$' ./lnwallet/accountbackup pass.
  • Extended the real recovery test to recreate raw key family 300 with WalletKit.DeriveKey after seed restoration and verify the same public key. Families above 255 are recoverable through this maintenance step; the guide now explains it.
  • Documented that account creation/import can commit before backup persistence fails, so a retry can report an existing account. Operators must repair storage and verify identity.
  • The recovery integration case passes with bitcoind and btcd. Formatting, module tidiness and changed-code lint pass.

The storage latency under the coin-selection lock and repeated account scans remain availability/performance follow-ups. Checking only that the parent directory exists cannot prove that an independent volume is mounted; independent storage remains an explicit deployment requirement. Skipping writes merely because no new address was allocated also needs care: LastUnusedAddress can expose an allocation whose earlier backup failed.

CI is rerunning. The earlier Gateway run timed out without a verdict, so this update does not claim merge readiness.

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

Labels

severity-critical Requires expert review - security/consensus critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant