#36070 wallet: Add deriveHDKey interface
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36070 wallet: Add deriveHDKey interface | Ready | 0 | Positive | 97 + 45 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Extracts the HD key derivation and parameter validation logic from the `derivehdkey` RPC into `CWallet::DeriveHDKey` and exposes it via `interfaces::Wallet::deriveHDKey`. The wallet interface returns a neutered `CExtPubKey` and `KeyOriginInfo`, keeping private key material unexposed across the interface boundary.
Problem: The GUI interacts with the wallet via `interfaces::Wallet` rather than RPC. Because key derivation logic lived solely within the `derivehdkey` RPC implementation, the GUI could not derive shareable xpubs from HD keys during multisig setup.
Evidence of importance:
- Author notes the interface is needed because "the GUI does not go through RPC, so the logic is currently out of its reach" and is required "alongside the addhdkey interface... to add an HD key and derive a shareable xpub from it" for multisig setup.
- Explicitly tracked as the deriveHDKey item in the Multisig Wizard tracking issue #35645.
Scope: Refactoring derivehdkey to return WalletError changes several error responses from RPC_INVALID_ADDRESS_OR_KEY (-5) to RPC_INVALID_PARAMETER (-8) and aligns the locked-wallet error string with other wallet RPCs.
Risk of a mistake: medium (wallet, rpc_api). Altering error codes and error messages returned by the derivehdkey RPC could break external client applications that match on specific error codes or strings.
Changed since the previous assessment: No substantial structural changes; the implementation remains a refactor of derivehdkey logic into CWallet and interfaces::Wallet.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: reviewers to check the reworked commit structure and error handling 2026-10-29
Agreement
Positive
support without stated reasons, no open objection (polespinasa, jeanpablojp, pseudoramdom, w0xlt)
Thread notes: Author reworked the commit history to separate the check moves, error changes, and CWallet move as suggested, resolving achow101's objection on collapsing parameter errors.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | objection, correctness | Interface test passes even when derivation derives from an unrelated key because it only checks trivial properties | fixed · fix pushed · fixed in follow-up push | changed | yes | 2026-08-27: 'This case passes even when the derivation comes from a different key: `origin.path` just echoes the argument, and `IsValid()` holds for any derived key.' Status from: 2026-08-29: PraneethGunas: 'Done. Split into "Add CWallet::DeriveHDKey" and "Add deriveHDKey interface".' |
| jeanpablojp (contributor) | question, correctness | Is the branch checking wallet lock status reachable given EnsureWalletIsUnlocked runs shortly before with cs_wallet held? | fixed · fix pushed · author confirmed branch was dead code and removed it | answered | yes | 2026-08-27: 'Is this branch reachable? `EnsureWalletIsUnlocked` runs a few lines above with `cs_wallet` already held, and `vMasterKey` is `GUARDED_BY(cs_wallet)`, so nothing can lock the wallet in between.' Status from: 2026-08-29: PraneethGunas: 'Right, removed this branch' |
| polespinasa (member) | objection, correctness | Unreachable dead code checking wallet lock status which is already guarded by EnsureWalletIsUnlocked | fixed · fix pushed · author removed the dead code branch | changed | yes | 2026-08-28: 'This is dead code, it can never be executed because if the wallet is locked, `EnsureWalletIsUnlocked(*wallet);` (L1006) will throw before.' Status from: 2026-08-29: PraneethGunas: 'Right, removed this branch' |
| polespinasa (member) | objection, approach | Duplicating logic between the interface and RPC causes them to return differing error codes for the same failures | fixed · fix pushed · author consolidated interface and RPC to both rely on CWallet::DeriveHDKey | changed | yes | 2026-08-28: 'All this is duplicated from RPC code. Why not just make both the interface and the RPC rely on the same code? Because of both using different code functions, they both return different error codes for' Status from: 2026-09-10: PraneethGunas: 'Now both the interface and the RPC rely on the same CWallet::DeriveHDKey' |
| jeanpablojp (contributor) | objection, correctness | Unit test in wallet_tests.cpp does not verify derivation against the wallet's active key and passes with incorrect keys or blank fingerprints | fixed · fix pushed · fixed in follow-up push | changed | yes | 2026-08-29: 'Same problem the interface test had: `origin.path` just echoes the argument, and `IsValid()` holds for any key.' Status from: 2026-09-01: PraneethGunas: 'Fixed' |
| achow101 (member) | objection, approach | Splitting SelectHDKey into an RPC helper duplicates logic instead of calling CWallet::DeriveHDKey directly | fixed · fix pushed · rebased to make both RPC and interface call CWallet::DeriveHDKey directly | changed | yes | 2026-09-09: 'Why is `SelectHDKey` being split into a separate function? This PR should only create one function: `DeriveHDKey`, and that should be called by RPC.' Status from: 2026-09-10: PraneethGunas: 'Now both the interface and the RPC rely on the same CWallet::DeriveHDKey' |
| pseudoramdom (member) | objection, interface | Collapsing RPC error codes from -8 and -5 to -4 modifies the error contract for derivehdkey shipping in v32 | answered · author explained error handling changes; achow101 and polespinasa followed up on acceptable error codes | answered | yes | 2026-09-14: 'I'm not sure if we're okay with changing the error contract since `derivehdkey` will ship in v32.' Status from: 2026-09-15: PraneethGunas: 'Restored the watch-only and unlock checks in the RPC, so -4 and -13 are unchanged from v32.' |
| achow101 (member) | objection, interface | Collapsing caller parameter errors into WALLET_ERROR makes error reporting too vague by conflating caller issues with internal wallet errors | fixed · fix pushed · author reworked commits to separate check moves and error changes as proposed | changed | yes | 2026-09-29: 'collapsing those into `WALLET_ERROR` makes the error code to the user too vague. `WALLET_ERROR` is really for "something bad happened in the wallet internally", as opposed to "the user gave us a bad a"' Status from: 2026-10-29: PraneethGunas: 'I’ve reworked the history along those lines, separating the check moves, error changes, and move into CWallet' |
Support:
- polespinasa (member) (Concept ACK, 2026-08-25): will review soon :) [not substantive]
- jeanpablojp (contributor) (Concept ACK, 2026-08-27): Left some comments. [not substantive]
- pseudoramdom (member) (Concept ACK, 2026-09-14): Left some comments [not substantive]
- w0xlt (contributor) (Approach ACK, 2026-10-01): Will review soon. [not substantive]
Participants:
- polespinasa (member, support, 9 statements 2026-08-25 to 2026-10-01): Concept ACK; raised concerns on dead code and duplication that were fixed, and suggested rework structure
- jeanpablojp (contributor, support, 5 statements 2026-08-27 to 2026-08-29): Concept ACK; asked about unreachable unlock check and identified weak assertions in tests, all resolved
- hebasto (member, neutral, 1 statements 2026-09-02 to 2026-09-02): Posted unused header and forward-declaration cleanup diff from CI
- achow101 (member, objection, 2 statements 2026-09-09 to 2026-09-29): Objected to helper splitting (resolved); objected to collapsing caller parameter errors into WALLET_ERROR (addressed in rework)
- pseudoramdom (member, objection, 5 statements 2026-09-14 to 2026-09-14): Concept ACK, but raised concern regarding altering the RPC error contract for derivehdkey shipping in v32
- w0xlt (contributor, support, 1 statements 2026-10-01 to 2026-10-01): Approach ACK; will review soon
Dependencies
Enables:
Files
52 lines under test/bench/ci.
- src/wallet/wallet.cpp +75/-0
- src/wallet/rpc/wallet.cpp +5/-59
- src/wallet/test/wallet_interfaces_tests.cpp +38/-0
- test/functional/wallet_derivehdkey.py +7/-7
- src/wallet/interfaces.cpp +7/-0
- src/wallet/wallet.h +6/-0
- src/interfaces/wallet.h +4/-0
Card
The short text the ranking pass compares across PRs.
This PR extracts HD key derivation from the derivehdkey RPC into CWallet::DeriveHDKey and exposes it through interfaces::Wallet::deriveHDKey. This allows the GUI to derive shareable xpubs for multisig setup without relying on the RPC layer, directly unblocking the deriveHDKey step in the Multisig Wizard tracking project (#35645). As a side effect, several derivehdkey RPC error return codes are converted from RPC_INVALID_ADDRESS_OR_KEY to RPC_INVALID_PARAMETER.
Data
thread read through 2026-10-29 (35 statements), head ea18029ba8
labels: Wallet
Categories
Wallet
P3 · unblocks #35645 (ranking pass: Moved up to sit with the other #35645 prerequisites it shares a project with.)
Extracts HD key derivation from RPC into CWallet and exposes it over interfaces::Wallet so the GUI can derive xpubs without RPC access. This directly unblocks the deriveHDKey component of the Multisig Wizard tracking issue (#35645).
Membership: Labeled 'Wallet' and modifies wallet internals in src/wallet/wallet.cpp, RPC handling, and src/interfaces/wallet.h.
Factors: security/stability 0, bug 0, performance 0, user value 1, leverage 2