config_builder: preserve watch-only account numbers#10644
config_builder: preserve watch-only account numbers#10644starius wants to merge 4 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the lnd wallet's watch-only functionality, specifically for remote signer configurations. It modifies the account import process to ensure that explicit, non-zero account numbers are retained rather than being remapped contiguously. This change is critical for remote signer setups that utilize sparse custom key families, as it guarantees that key derivation requests for these accounts will correctly resolve to their intended indices, thereby improving the robustness and flexibility of remote signing operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement for watch-only wallets, particularly in remote signer setups, by preserving explicit account numbers during import. The changes are well-implemented, with clear refactoring in config_builder.go and comprehensive new unit and integration tests. The documentation has also been updated accordingly. I have one minor suggestion regarding code style to improve consistency with the repository's guidelines.
config_builder.go
Outdated
| // validateWatchOnlyAccountPubKey ensures that a watch-only account xpub has | ||
| // the same structure and network restrictions as btcwallet's | ||
| // ImportAccountWithScope path. | ||
| // | ||
| // NOTE: This intentionally mirrors btcwallet's internal validation in | ||
| // wallet/import.go: (*Wallet).validateExtendedPubKey(..., isAccountKey=true). | ||
| // Keep both implementations aligned. | ||
| func validateWatchOnlyAccountPubKey(wallet *wallet.Wallet, |
There was a problem hiding this comment.
I guess having to duplicate this & isPubKeyForNet from btcwallet is the only thing I don't really like with this approach as it'll be hard to ensure that the implementations remain aligned between the different repos. It would be much better if you could use some public API that ensures both repos use the same validation logic.
As a potentially suggestion that already exists today, you could look into if you could use wallet.ImportAccountDryRun for the non if scope.Index == 0 clause, and check if that errors or not.
There was a problem hiding this comment.
Thanks for the suggestion! I re-used wallet.ImportAccountDryRun for validation and got rid of validateWatchOnlyAccountPubKey and isPubKeyForNet.
| scopedMgr, err := wallet.Manager.FetchScopedKeyManager( | ||
| scope.Scope, | ||
| ) | ||
| if err != nil { | ||
| var manErr waddrmgr.ManagerError | ||
| if !errors.As(err, &manErr) || | ||
| manErr.ErrorCode != waddrmgr.ErrScopeNotFound { | ||
|
|
||
| return err | ||
| } | ||
|
|
||
| scopedMgr, err = wallet.Manager.NewScopedKeyManager( | ||
| addrmgrNs, scope.Scope, addrSchema, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
I think there's value in adding some docs for explaining the logic here.
There was a problem hiding this comment.
I added a comment:
// Mirror what ImportAccountWithScope in btcwallet itself does:
// try to find an already-registered manager for this key scope.
// If the scope was already set up, we reuse it. Otherwise we
// create the scope in the DB and register the in-memory
// manager.
docs/remote-signing.md
Outdated
| - Any additional custom `m/1017'/X'/N'` accounts that your signer actively | ||
| uses (for example `N=43210`). Newer `lnd` versions preserve these explicit | ||
| account numbers in watch-only wallets, so `DeriveKey(KeyFamily=N)` works | ||
| without requiring contiguous account creation. |
There was a problem hiding this comment.
Do you believe there'd be value in mentioning the implications of downgrading a watch-only node to an lnd version which hasn't implemented this new behaviour, if this new behaviour has been utilised? Potentially that'd belong to the release notes though.
There was a problem hiding this comment.
I added it both here and in the release notes. Downgrading should be safe in terms that the keys already imported in 0.21 should remain properly mapped after downgrading to 0.20, since they are already stored in proper places in the wallet DB. However, if after downgrading another key is imported, it will be added at next available slot, breaking the mapping, but this is separate from downgrading (this is just what the current behavior is).
Import non-zero watch-only accounts at explicit account numbers instead of allocating them contiguously. Keep account 0 on the existing ImportAccountWithScope path to preserve the "default" account semantics and naming that the rest of lnd expects. Non-zero accounts use NewRawAccountWatchingOnly so the btcwallet account number matches the key family index, which is critical for remote-signer setups where DeriveKey(KeyFamily=N) must resolve to account N. Validate non-zero account xpubs via wallet.ImportAccountDryRun, reusing btcwallet's own validation (validateExtendedPubKey / isPubKeyForNet) rather than duplicating its internals. The dry-run rolls back its database transaction, so no persistent state is created. The previous code already used one walletdb transaction per account (each ImportAccountWithScope call opens its own). This commit preserves that existing behaviour rather than introducing a single wrapping transaction. The inline error comment previously claimed all accounts ran in the "same transaction"; that was already inaccurate before this change and has been corrected. Add a regression test that verifies sparse key families (e.g. 43210) can be derived after watch-only import.
Add a dedicated remote signer integration test for sparse custom key families. The test initializes the watch-only node with account 43210 in the 1017 scope and asserts DeriveKey returns the same key on signer and watch-only nodes. Also extend deriveCustomScopeAccounts to accept extra account indices so the sparse case can be constructed explicitly.
Clarify that watch-only remote-signer wallets should include custom 1017/X/N accounts used by the signer. Document that lnd v0.21.0+ preserves explicit account numbers so DeriveKey(KeyFamily=N) works for sparse families. Note that older versions import accounts contiguously, but downgrading after wallet creation is safe because accounts are already persisted at the correct indices and are not re-imported on unlock.
Add a bug-fix entry for preserving explicit watch-only account numbers in remote-signer mode. Mention the sparse key-family derivation case where accounts were previously remapped contiguously. Note that account numbers are written at wallet creation time and not re-imported on unlock, so downgrading does not remap them.
starius
force-pushed
the
preserve-accounts
branch
from
a4d62d9 to
e8f2cdc
Compare
starius
changed the title
2 participants
Change Description
Import non-zero watch-only accounts at explicit account numbers instead of allocating them contiguously. Allow using an arbitrary account number and to have space account number space on watch-only.
Steps to Test
Unit test:
itest:
Manual test:
lncli wallet accounts liston watch-only shows account pathm/1017'/1'/43210'(not remapped to account 256).Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.