Repository navigation
Conversation
|
/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. |
🔴 PR Severity: CRITICAL
🔴 Critical (5 files)
🟡 Medium (4 files)
🟢 Low (5 files)
AnalysisThis PR adds a new wallet account backup/recovery feature that touches To override, add a |
|
👀 gateway review starting… |
7fd3dad to
6ddf274
Compare
|
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. |
6ddf274 to
0244140
Compare
|
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.
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.
0244140 to
c83226f
Compare
|
Added a separate signed integration-test commit, c83226f: The normal LND integration harness now runs
Passed: make itest backend=bitcoind icase=wallet-account_backup_recovery
make itest backend=btcd icase=wallet-account_backup_recoveryCustom 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.
c83226f to
47834ee
Compare
|
Addressed the concrete build blocker and recovery documentation gaps in signed head 47834ee. The integration test remains a separate commit.
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. |
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-backupfile. 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:
wallet-account_backup_recoveryintegration 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._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../lnwallet/accountbackup ./lnwallet/btcwalletpassed.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.