walletrpc: recover XCreateAccount funds on both address branches

[wieghx]

Change Description

The recovery procedure added with XCreateAccount treated "how many
addresses were issued" as a single count and told operators to replay
NextAddr. That RPC defaults to the external branch, so change
outputs created by FundPsbt stay unknown after a seed restore and
rescan.

This updates the proto, CLI help, and post-create warning so operators
record external_key_count and internal_key_count separately, replay
NextAddr on each branch, and only then rescan.

Related to #11087.

Steps to Test

make itest icase=xcreate_account_branch_recovery

The new itest:

1. Creates a preceding account so the target has a non-trivial index.
2. Funds an external address of the target account.
3. Spends with FundPsbt, leaving value on an internal change address.
4. Records the xpub and both branch counts.
5. Restores the seed into a fresh wallet and recreates the accounts in
   the original order.
6. Replays both NextAddr branches, then rescans with
   --reset-wallet-transactions.
7. Checks that funds on both branches are visible and spendable.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

AI assistance: I used an AI coding assistant while writing the docs and
itest. I reviewed the NextAddr / FundPsbt path and compiled
./itest and the CLI package locally. I could not run the integration
test here (no chain backend).

Signed-off-by: wieghx gudemail1991@gmail.com

[github-actions]

🟡 PR Severity: MEDIUM

gh pr diff stat | 9 files | 327 lines changed

🟡 Medium (2 files)

• cmd/commands/walletrpc_active.go - CLI command changes for the new XCreateAccount recovery flag (cmd/* is always MEDIUM regardless of related package)
• lnrpc/walletrpc/walletkit.proto - API/wire definition change adding a new field to WalletKit RPC

🟢 Low (7 files)

• docs/release-notes/release-notes-0.22.0.md - release notes update
• itest/lnd_wallet.go - integration test
• itest/lnd_wallet_xcreate_account.go - new integration test (204 lines)
• lntest/rpc/wallet_kit.go - test harness RPC helper
• lnrpc/walletrpc/walletkit.pb.go - auto-generated protobuf stub
• lnrpc/walletrpc/walletkit_grpc.pb.go - auto-generated gRPC stub
• lnrpc/walletrpc/walletkit.swagger.json - auto-generated swagger spec

Analysis

The functional change is confined to a proto field addition for XCreateAccount and its corresponding CLI wiring, with the remainder of the diff being generated stubs, test harness updates, a new itest, and release notes. No critical or high-severity packages (lnwallet, keychain, channeldb, etc.) are touched, and the non-test/non-generated footprint (4 files, ~58 lines) is well below the thresholds for a severity bump. Classified as MEDIUM due to the API surface change in walletkit.proto.

---

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

[Lrifton92]

Nice change — the two-counter distinction is a real trap and the proto text now explains it clearly. I read
the branch and have one concrete inconsistency plus one question about the test.

The CLI note points at counters that are zero at that moment

The proto doc is explicit that the counters are useless at creation time:

Recording only the derivation path when the account is created is not enough, because both counters start
at zero and increase as the account is used.

But the CLI note printed by createAccount says the opposite in the same PR:

Record its derivation path and both external_key_count and internal_key_count (above) with your seed.

At that point printRespJSON(resp) has just printed an account whose external_key_count and
internal_key_count are both 0, and "(above)" refers to exactly that output. An operator who follows the
instruction literally records two zeros and is no better off than before this PR — which is the failure mode
the change is trying to prevent.

The same applies to the command's Description, which says to record "both external_key_count and
internal_key_count ... printed below".

I think the note wants to say something closer to: record the derivation path now, and read both counters
from ListAccounts before you need to restore, since they only become meaningful once the account has
issued addresses.

The test proves the remedy, never the failure

testXCreateAccountBranchRecovery establishes that replaying both branches recovers both. It never
establishes that replaying only the external branch does not — which is the claim the documentation now
rests on:

Replaying a single aggregate address count, or calling NextAddr without change=true, leaves internal/change
scripts unknown to the restored wallet even after a transaction rescan.

As written, the test would still pass if the internal-branch funds were recovered by some other mechanism and
the change=true replay were redundant. A short negative assertion — restore, replay only change=false,
rescan, and assert the internal output is absent — would make the documented claim load-bearing rather than
asserted. That is the part a future reader will want evidence for.

Question

RestoreNodeWithSeed(..., 0, nil) pins the recovery window to 0. Is that load-bearing for this scenario, or
incidental? My understanding from the existing doc is that the window would not help here anyway, since the
recovery scan only rederives the default account and the custom account does not exist until it is
re-created — but a one-line comment saying why 0 is the right value would save the next reader the
detour I just took.

[wieghx]

Addressed the three items:

CLI note pointing at zeros. The create-time response has external_key_count and internal_key_count both still 0, so telling the operator to record those values "(above)" would store two zeros. The command Description and the post-create stderr note now say: record the derivation path at create time, and read both counters from ListAccounts before restore.

Test only proved the remedy. testXCreateAccountBranchRecovery now restores, replays only change=false, rescans, and asserts the internal change output is absent (account balance is just the external dest amount). It then replays change=true, rescans again, and asserts both branches recover.

RestoreNodeWithSeed(..., 0, nil). RecoveryWindow is 0 because a non-zero window would not help here: the recovery scan only rederives the default account, and the custom account does not exist until it is re-created. Commented at the call site.

[Lrifton92]

Re-read at 4536b8b0. The CLI note and the RecoveryWindow comment both land — the note now records the path at create time and sends the operator to ListAccounts for the counters, which is the sequence that actually works.

The negative assertion is better than what I asked for: pinning the external-only balance to destAmt instead of "less than wantBal" makes the check fee-independent, so it will not drift when the FundPsbt fee changes. Hoisting destAmt into the const block rather than re-spelling fundAmt / 2 is the right call too.

One gap left, and it is in the new assertion rather than the old one.

The absence check is not synchronised with the rescan

AssertWalletAccountBalance is a wait.NoError poll (lntest/harness_assertion.go:2647) — it returns at the first tick where the confirmed balance equals the expected value. Nothing between it and

require.False(ht, sawInt, "internal change should still be unknown")

waits for the rescan triggered by --reset-wallet-transactions to finish. So the snapshot establishes that the change is not visible yet, not that it cannot be found.

The exact match does most of the work: if a regression ever made the internal branch derivable during recovery, the settled balance would be wantBal, not destAmt, and the poll would time out instead of passing. But there is still a window — external credited, internal not yet — in which the balance is exactly destAmt and sawInt is legitimately false, and both checks pass on a rescan that is only half-done. That is the exact regression this test exists to catch, so it seems worth closing.

ht.WaitForBlockchainSync(restored) between the restart and the two assertions should be enough, assuming SyncedToChain only flips once the reset rescan has caught up. I am reasonably confident that holds but have not run it, so worth confirming rather than taking my word for it.

Nothing else from my side — the rest reads correct.

[wieghx]

The absence check was racing a still-running rescan, and it is a real gap — just not for the reason of a missing WaitForBlockchainSync on a normal restart.

RestartNodeWithExtraArgs already calls RestartNode, which waits for SyncedToChain unless SkipUnlock is set. RestoreNodeWithSeed creates the node with noAuth=true, which leaves SkipUnlock true, so that wait is skipped. SyncedToChain does include wallet.IsSynced() (rpcserver.go getChainSyncInfo): after --reset-wallet-transactions the wallet height is the birthday until the rescan reaches tip, so once that flag flips the rescan is done.

Added ht.WaitForBlockchainSync(restored) after both restarts, before the destAmt / sawInt snapshot.

(Dest and change live in the same mined tx, so a mid-rescan destAmt without change would still require the spend block to have been processed with only the external scripts in the address manager. Waiting for tip still closes the window the snapshot was taking.)