wallet: derivehdkey RPC to get xpub at arbitrary path#32784
Conversation
DrahtBot
changed the title
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32784. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-07-03 11:48:38 |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Sjors
force-pushed
the
2025/06/gethdkey
branch
2 times, most recently
from
de7f5c6 to
3f35b02
Compare
There was a problem hiding this comment.
In util.h: “@params[in] path” → “@param[in] path” [Doxygen tag typo]
There was a problem hiding this comment.
addressesd -> addresses [extra “d” makes “addresses” misspelled]
|
Very nice, Concept ACK. |
Sjors
force-pushed
the
2025/06/gethdkey
branch
2 times, most recently
from
380a57f to
017fb68
Compare
|
Ok, this should address the last review round: #32784 (review) |
| } | ||
| } | ||
|
|
||
| std::optional<CKey> key{wallet->GetKey(xpub.pubkey.GetID())}; |
There was a problem hiding this comment.
In 63450ca rpc: add derivehdkey
IIUC this is checking if the wallet has the private key for xpub.pubkey.
Should we verify that the supplied xpub exactly matches one of the wallet’s known descriptor xpubs before using it.
There was a problem hiding this comment.
CExtPubKey xpub comes from the hdkey argument of the RPC call. It's the xpub for the master xprv. It's not expected to appear in regular wallet descriptors, because those use xpubs at a deeper derivation path (usually account level). The only exception is the new unused() descriptor.
However it does make sense to require that the xpub matches either an unused() descriptor or an xpub corresponding to an hd key. I added that check plus a test (63450ca -> ...). I first extracted two news helpers: GetExtKey in b407258 and GetHDPubKeys in f488a77.
|
Addressed @pseudoramdom's feedback #32784 (comment). This adds a safety check and gets rid of code duplication in the process. I also expanded the PR description. Some commits could be a standalone refactor pull request if the review burden is too much, but they all towards the goal of adding |
| std::vector<uint32_t> out; | ||
| if (!ParseHDKeypath(path, out)) { | ||
| throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid BIP32 keypath"); | ||
| } |
There was a problem hiding this comment.
ParsePathBIP32() currently accepts m/2147483648, which is 0x80000000. Since BIP32 uses that high bit to mark hardened derivation, the RPC ends up treating this as m/0h instead of rejecting it.
Suggestion:
diff
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index ab27a84a67..d1770f0bdf 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -29,6 +29,7 @@
#include <algorithm>
#include <iterator>
+#include <sstream>
#include <string_view>
#include <tuple>
#include <utility>
@@ -1381,6 +1382,25 @@ std::vector<uint32_t> ParsePathBIP32(const std::string& path)
if (!ParseHDKeypath(path, out)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid BIP32 keypath");
}
+
+ // BIP32 child indices must fit in 31 bits; the high bit is reserved for
+ // the hardened derivation marker.
+ std::stringstream ss(path);
+ std::string item;
+ bool first{true};
+ while (std::getline(ss, item, '/')) {
+ if (first && item == "m") {
+ first = false;
+ continue;
+ }
+ if (item.ends_with('\'') || item.ends_with('h')) item.pop_back();
+
+ const auto number{ToIntegral<uint32_t>(item)};
+ if (!number || *number > 0x7fffffffU) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid BIP32 keypath");
+ }
+ first = false;
+ }
return out;
}
diff --git a/test/functional/wallet_derivehdkey.py b/test/functional/wallet_derivehdkey.py
index 5ed8cc0eb0..32287c9464 100755
--- a/test/functional/wallet_derivehdkey.py
+++ b/test/functional/wallet_derivehdkey.py
@@ -44,6 +44,13 @@ class WalletDeriveHDKeyTest(BitcoinTestFramework):
wallet.derivehdkey,
"m/87/0/0",
)
+ for path in ["m/2147483648", "m/2147483648h", "m/2147483648'"]:
+ assert_raises_rpc_error(
+ -8,
+ "Invalid BIP32 keypath",
+ wallet.derivehdkey,
+ path,
+ )
assert_raises_rpc_error(
-5,
"No active or unused(KEY) descriptor found",
|
Added two commits:
|
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
ParseHDKeypath() lives in util/bip32, so its unit test belongs in bip32_tests rather than psbt_wallet_tests. Pure move, no changes to the test itself; subsequent commits extend it in its new home.
ParseHDKeypath() parsed each path element with ToIntegral<uint32_t>, so a bare decimal >= 2^31 (e.g. "m/2147483648" == 0x80000000) was silently treated as "m/0h". This commit rejects such overflow instead.
GetHDPubKeys() centralizes the descriptor xpub lookup used by gethdkeys and createwalletdescriptor, and by the derivehdkey RPC added in a later commit. The HDKeyFilter argument serves gethdkeys' active_only mode (Active vs All) and createwalletdescriptor's active descriptor selection. No behavior change, except the dynamic_cast now uses Assert() instead of gethdkeys' CHECK_NONFATAL, since it is not a recoverable input.
Reconstruct a descriptor's extended private key from its xpub by looking up the corresponding private key. This is the extended-key analog of GetKey(). It is used here to simplify gethdkeys, and again by the derivehdkey RPC introduced in a later commit.
Add an UnusedKey filter to GetHDPubKeys() so the new RPC can prefer unused(KEY) descriptors before falling back to active descriptors. Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Use derivehdkey instead of extracting each participant xpub (and derivation info) from the listdescriptors output. Additionally use the new <0;1> descriptor syntax. Finally this commits adds a few debug log lines, and expand the explanation for why we use m/44h/1h/0h.
Use derivehdkey instead of extracting each participant xpub from the listdescriptors output. Additionally use the new <0;1> descriptor syntax.
|
Rebased after #32489. I moved the new |
…tor' into HEAD Resolve the createwalletdescriptor conflict between bitcoin#32784 and bitcoin#32861 by keeping bitcoin#32861's fallback to unused KEY descriptors, but adapting the active descriptor lookup to bitcoin#32784's GetHDPubKeys(HDKeyFilter::Active) API and extracting the xpub from the map key.
5 participants
Adds a
derivehdkeyRPC that returns an xpub, or optionally the xprv, at an arbitrary BIP32 path (with at least one hardened step), derived from a wallet HD key.The main use case is coordinating a multisig setup, where each participant shares an xpub derived at a hardened path (e.g.
m/87h/0h/0h) distinct from their default single-signature descriptors. See the (updated)doc/multisig-tutorial.mdand (updated) functional test to see how that workflow improves.The first commits are some helpful helpers:
psbt_wallet_testsParseHDKeypathwould previously map overflowing values withouthto hardened.HasHardenedDerivation(), to enforce the "at least one hardened step" rulegethdkeyswhichderivehdkeyneedsGetKey()); behavior-preserving prep, also simplifiesgethdkeys.Meat and potatoes:
UnusedKeyfilter onGetHDPubKeysthat drives key selection.<0;1>syntax.