wallet: store multipath descriptor #36133

Sjors commented at 6:12 PM on August 31, 2026: member

Status: this PR needs further conceptual review / discussion, before going too deep into code review.

HWI recently added a registerdescriptor command (https://github.com/bitcoin-core/HWI/pull/842) which some hardware wallets require in order to (more safely) support multisig. This registration can then be used with displayaddress and signtx (using the --registration argument, see https://github.com/bitcoin-core/HWI/pull/841 and https://github.com/bitcoin-core/HWI/pull/841). Internally HWI converts the descriptor to a BIP388 policy. The only catch is that we need to provide a multipath descriptor, rather than separate receive and change descriptors. This PR makes that possible.

Our wallet already supported importing multipath descriptors, but upon import they are irreversibly turned into change and receive descriptors. We don't overhaul this fundamental descriptor wallet design. Rather (as per approach 5 from #36075) we store an additional record to hold on to the multipath descriptor strings and point to its matching receive and change descriptor.

Existing wallets are not impacted (and not "upgraded").

The main commits are:

refactor: introduce descriptor ParseState: the parser first collects keys, generates replacements (not yet normalized, just converting to public keys) and then applies those replacements (in reverse order).
descriptor: normalize keys in the multipath descriptor string: no record is generated if hardened derivation blocks normalization (those would be useless).
descriptor: let Parse return the multipath descriptor string
wallet: store multipath descriptor record on import

These commits make wallet handling consistent and expose the record through RPC:

wallet: store multipath descriptor record on wallet creation: covers default wallets and createwalletdescriptor.
wallet: expand multipath descriptors with 'h' hardened marker: we don't preserve the exact imported string anyway, so might as well make it consistent
wallet: copy multipath record in exportwatchonlywallet: remaps descriptor IDs to the destination wallet.
rpc: return multipath descriptor from getaddressinfo
rpc: return multipath descriptor from listdescriptors

The remaining commits provide parser plumbing, tests, and refactors that make the main changes easier to follow and keep the descriptor code maintainable:

miniscript: let FromString take a string_view
test: add MULTIPATH flag to descriptor test vectors: tags existing vectors
refactor: use util::Expected for descriptor parsers: replaces &error and introduces ParsePubkeyResult and other aliases that are expanded later (with less churn).
Four small helper extractions, kept as separate commits to make each change easier to review:
- refactor: extract LastHardenedIndex helper
- refactor: extract OriginKeyString helper
- refactor: extract MergeNormalizedOrigin helper
- refactor: extract HardenedPrefix helper
refactor: setup wallet descriptors per output type pair (dropping the loop over internal)

Based on:

https://github.com/bitcoin/bitcoin/pull/36230

DrahtBot added the label Wallet on Aug 31, 2026

DrahtBot commented at 6:12 PM on August 31, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36133.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36143 (descriptor: add CreateMultisigDescriptor() by rxbryan)
#36013 (test: Descriptor roundtrip and raw()/ addr() coverage by pablomartin4btc)
#35998 (wallet: Handle or explicitly ignore WalletBatch write failures by achow101)
#35377 (wallet: Allow importing of descriptors without private keys when the wallet has the private keys by achow101)
#35041 (descriptor: speed-up Parse (xpub/xpriv) in ~30% by brunoerg)
#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#34861 (wallet: Add importdescriptors interface by polespinasa)
#32861 (Have createwalletdescriptor auto-detect an unused(KEY) by Sjors)

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 places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

std::make_unique<ConstPubkeyProvider>(state.key_exp_index, pubkey, false) in src/script/descriptor.cpp
std::make_unique<ConstPubkeyProvider>(state.key_exp_index, pubkey, true) in src/script/descriptor.cpp

2026-09-15 09:58:50

jeanpablojp commented at 2:53 PM on September 1, 2026: contributor

Concept ACK

The matching approaches in #36075 work off descriptors a wallet already has, and storing a record instead means wallets created before this get none on their own.

Those get no record, and createwalletdescriptor on a pair that exists just answers Descriptor already exists. You can still rebuild one by hand, dumping the private descriptor, turning /0/* into /<0;1>/* and re-importing, which stores exactly what the new code would have. That needs the xprv though, since the public form is rejected on a wallet with private keys. Is that the intended path for existing wallets?

in src/wallet/walletdb.cpp:955 in 0948b92435 outdated

 951 | +            if (!pwallet->GetScriptPubKeyMan(desc_id)) {
 952 | +                strErr = strprintf("Error: Multipath descriptor record references unknown descriptor id '%s' in wallet %s.", desc_id.ToString(), pwallet->GetName());
 953 | +                return DBErrors::CORRUPT;
 954 | +            }
 955 | +        }
 956 | +        pwallet->LoadMultipathDescriptor(std::move(multipath_desc));

jeanpablojp commented at 2:53 PM on September 1, 2026:

The two checks above reject a record whose id doesn't match its own string, and one that names a descriptor the wallet doesn't have. The string itself is never checked against those descriptors, so a record saying wpkh(record/<0;1>/*) next to an unrelated descriptor loads fine and both RPCs serve it. Is this deliberate?

Sjors commented at 4:13 PM on September 1, 2026:

IIUC you're asking if we should Parse() the multipath string at load time and verify that it expands as expected. That's more heavy-lifting than we typically do at wallet load.

It could make sense to introduce a bitcoin-wallet command to perform a more thorough integrity check..

in src/wallet/wallet.cpp:3897 in 0948b92435 outdated

3892 | +std::optional<std::string> CWallet::GetMultipathDescriptor(const uint256& desc_id) const
3893 | +{
3894 | +    AssertLockHeld(cs_wallet);
3895 | +    for (const auto& [id, multipath_desc] : m_multipath_descriptors) {
3896 | +        if (std::find(multipath_desc.desc_ids.begin(), multipath_desc.desc_ids.end(), desc_id) != multipath_desc.desc_ids.end()) {
3897 | +            return multipath_desc.descriptor;

jeanpablojp commented at 2:53 PM on September 1, 2026:

<0;1> and <0;2> are different policies that share their /0/* expansion, so one descriptor can sit in two records and this hands back whichever the map reaches first. Would it be worth returning all of them, or refusing the ambiguity?

Sjors commented at 4:05 PM on September 1, 2026:

It doesn't seem realistic for someone to import a <0;1> and <0;2> descriptor, so I'm not too worried about supporting that. Is there a more realistic scenario?

jeanpablojp commented at 6:27 PM on September 1, 2026:

Fair enough, I don't have a more realistic one. The idea was to avoid an arbitrary pick, but I take the point that it's over-protective for a case nobody hits.

Sjors commented at 6:31 PM on September 1, 2026:

This is now prevented. But what you can do is import a multipath descriptor into a pre-existing wallet that lacked it; this then adds the record. Not a documented feature, but it's tested.

in src/wallet/wallet.cpp:3655 in 0948b92435

3657 | +
3658 | +    // Store a record with the multipath descriptor, once the whole pair
3659 | +    // exists and if this call created at least one of its members
3660 | +    const bool pair_complete{GetScriptPubKeyMan(receive_id) && GetScriptPubKeyMan(change_id)};
3661 | +    if (!new_descs.empty() && pair_complete) {
3662 | +        AddMultipathDescriptor(batch, MultipathDescriptorRecord(std::move(*multipath_normalized), {receive_id, change_id}));

jeanpablojp commented at 2:53 PM on September 1, 2026:

The other two callers check this return, one warns and the other errors out. Dropped here, a failed write leaves the wallet created and reporting success with no record, and the transaction wrapping wallet setup never learns of it. Is this intended?

Sjors commented at 6:31 PM on September 1, 2026:

Added the missing handling and a [[nodiscard]].

in src/script/descriptor.cpp:3068 in 0948b92435 outdated

3065 | +                Assume(pos <= next_pos);
3066 | +                Assume(kr.text.size() <= next_pos - pos);
3067 | +                multipath_str.replace(pos, kr.text.size(), kr.replacement);
3068 | +                next_pos = pos;
3069 | +            }
3070 | +            *multipath = AddChecksum(multipath_str);

jeanpablojp commented at 2:53 PM on September 1, 2026:

The origin comes through as typed here when the key is an xpub, and normalized when it's the xprv, so desc and multipath can end up disagreeing on 84h versus 84' for the same key. Is this intended?

Sjors commented at 6:32 PM on September 1, 2026:

Good catch, fixed multipath normalization to always uses h.

Additionally, 620bd8ffa57dbda36d0e3a21d0eba952e2406e09 now uses h when expanding multipath, so the expanded descriptors are consistent with the parent.

Sjors commented at 4:00 PM on September 1, 2026: member

wallets created before this get none on their own.

Correct, I'll clarify in the description that this doesn't "upgrade" existing wallets. I think that's fine, because any multisig wallet using BIP388 registration is going to be new - since we don't support it yet.

since the public form is rejected on a wallet with private keys

See #35377.

Sjors force-pushed on Sep 1, 2026

DrahtBot added the label CI failed on Sep 1, 2026

DrahtBot commented at 7:37 PM on September 1, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS native, fuzz: https://github.com/bitcoin/bitcoin/actions/runs/33544094392/job/99977225999 LLM reason (✨ experimental): Fuzz test failure: descriptor_parse hit an assertion in script/descriptor.cpp (canonical_result && sp_canonical.empty()) and exited with code 1.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

achow101 commented at 2:36 AM on September 2, 2026: member

Concept NACKish

My suggestion was not to store an extra record, but rather an extra field in WalletDescriptor which contains the ids of the other descriptors in the multipath. Reconstructing the multipath can be done by doing a pairwise traversal in Descriptor::ToString(), i.e. traverse all of the descriptors at the same time, entering the sub descriptors and PubkeyProviders located at the same indexes. The only difference should be a single derivation path element, so it should not be that hard to figure out where the multipath is. I'm pretty sure it would be less code too to do that. Currently on vacation, but I can look at implementing this method once I'm back.

Additionally, multipath descriptors do not inherently have a concept of receive and change descriptors. They are not limited to 2 descriptors once expanded. There should be no mention of "receive" or "change" when discussing multipath descriptors in the code.

Sjors force-pushed on Sep 2, 2026

Sjors commented at 11:23 AM on September 2, 2026: member

IIUC you're objecting to two separate things:

Storing the multipath record. You prefer to reconstruct it on the fly.
The method of deriving the (normalized) multipath descriptor. You prefer to reconstruct it from individual descriptors, rather than have Parse yield it based on the original.

Given that we know the actual intended multipath descriptor at import (and wallet creation) time, it seems better to store it then. But doing so is indeed more involved, and storing the multipath becomes pointless if we do (1).

I'm pretty sure it would be less code too to do that.

The meat of the Parse change is not big, i.e. descriptor: let Parse return the multipath descriptor string and descriptor: normalize keys in the multipath descriptor string. Most of the churn in this PR comes from prep refactors.

The easiest implementation is just a regex. But having a mix of parser strategies in the already hard to understand Descriptor class, does not sound appealing.

My approach tries to stick to the existing recursive parser design.

Reconstructing the multipath can be done by doing a pairwise traversal [...] so it should not be that hard to figure out where the multipath is.

So we give a multipath descriptor to Parse, it gets split apart and then we reconstruct it? It feels wrong to throw away information this way, and essentially treat Parse as a black box. It's a sign we need to reconsider the entire Descriptor class.

I tried the approach in 2026/09/multipath-reconstruct, commit descriptor: reconstruct a multipath descriptor from its expansion 5694a99992fbe314779b1ce2c127dbcb50644025 (unpolished), with one additional prep refactor descriptor: let ToNormalizedString reject forms that are not publicly derivable fcd48f8a31e34bf6cbb653aeae98ce19d9573f7f. For that branch, I dropped the refactor commits it doesn't need, but I did keep the approach of storing a wallet record.

multipath descriptors do not inherently have a concept of receive and change descriptors.

Indeed, the Descriptor class should be agnostic, so I adjusted MultipathExtKeyNormalizationPrefix to handle more than two paths.

Also fixed the CI fuzzer failure.

Sjors force-pushed on Sep 2, 2026

DrahtBot removed the label CI failed on Sep 2, 2026

Sjors referenced this in commit 770f091ee4 on Sep 3, 2026

achow101 commented at 10:07 PM on September 7, 2026: member

The easiest implementation is just a regex. But having a mix of parser strategies in the already hard to understand Descriptor class, does not sound appealing.

No one is suggesting a separate implementation, especially not one using regex.

So we give a multipath descriptor to Parse, it gets split apart and then we reconstruct it?

Yes

It feels wrong to throw away information this way

What information is being thrown away? Parse returns multiple descriptors when it's a multipath descriptor. We don't lose any information.

It's a sign we need to reconsider the entire Descriptor class.

How so?

DrahtBot added the label Needs rebase on Sep 9, 2026

Sjors commented at 7:35 AM on September 15, 2026: member

So we give a multipath descriptor to Parse, it gets split apart and then we reconstruct it?

Yes

That throws the Parse design out the window and sticks a second parser into the Descriptor class (even if we hide this second pass inside Parse).

No one is suggesting a separate implementation, especially not one using regex.

Then I don't understand what you have in mind.

It feels wrong to throw away information this way

What information is being thrown away? Parse returns multiple descriptors when it's a multipath descriptor. We don't lose any information.

Inside Parse we already have the <0;1> notation. That's lost in the multiple descriptor strings, and then reconstructed. It's true that the original can still be reconstructed, at least I can't think of an obvious edge case where it can't, but that wasn't my point.

Imo Parse should be able to take a multipath descriptor with private keys and produce a (normalized) public key descriptor, without the intermediate state of two individual descriptors. It has all the information it needs.

It's a sign we need to reconsider the entire Descriptor class.

How so?

Because we keep making it more complicated, and then duct-taping fixes. E.g. we grew to four different ToString() formats, in part to deal with bugs, and then #35445 (review) mixes their implementations to fix another downgrade bug.

Also, while working on this PR, I found it quite challenging to wrap my head around all the moving parts. Especially the miniscript stuff, which was also added in a later stage, is really hard to understand.

That said, I don't have a concrete redesign in mind that's easier to understand and maintain.

Sjors force-pushed on Sep 15, 2026

Sjors commented at 9:21 AM on September 15, 2026: member

Rebased after #35445, cherry-picked wallet: Compare descriptors by hash of canonical string from #36230 and adjusted things accordingly.

Added emphasis to the PR description that this needs conceptual review / discussion before diving too deep into code review, given @achow101's objection so far.

Also marked draft since we use part of #36230.

Sjors marked this as a draft on Sep 15, 2026

wallet: Compare descriptors by hash of canonical string

The canonical string comparison was slow because it would compute the
canonical string for each comparison. This can be sped up by holding the
canonical string in memory, computed upon construction of
WalletDescriptor. To reduce memory usage, this string is further hashed
so that the comparison operates over the hash of the canonical string.

435f4ea598

miniscript: let FromString take a string_view

This lets the next commit record key positions in the descriptor string
while its miniscript expression is parsed in place.

8d4407ba3f

test: add MULTIPATH flag to descriptor test vectors

Use it to check whether parsing expands a descriptor into multiple descriptor.

67669c5487

wallet: expand multipath descriptors with 'h' hardened marker

The descriptors that a multipath descriptor expands to are new, derived
descriptors rather than user input, so give them canonical form.

When a multipath descriptor contains an apostrophe, replace them all
with 'h' and parse again.

417d6f350c

refactor: use util::Expected for descriptor parsers

Use util::Expected for ParsePubkeyInner, ParsePubkey, and ParseScript, so errors are returned directly instead of passed through an output argument.

The only behavior change is that a preexisting error string is cleared when nested descriptor parsing fails without producing an error message. Previously, the stale error was left intact.

6b684d593b

refactor: introduce descriptor ParseState

Group the key expression index and signing provider shared by ParsePubkeyInner, ParsePubkey, and ParseScript in ParseState.

A later commit extends this state with key replacement collection.

8712918dbc

descriptor: let Parse return the multipath descriptor string

Add an optional Parse() output for the unexpanded multipath descriptor. This is always a public descriptor, so ParseState optionally collects KeyReplacement entries tracking key locations in the input for private-to-public conversion.

The replacement sink is null unless the caller requests this output.

d2bba09b3f

refactor: extract LastHardenedIndex helper

Split finding the last hardened derivation step out of
BIP32PubkeyProvider::ToNormalizedString. No behavior change. A later
commit reuses it when normalizing the keys of a multipath descriptor
string.

120db58fcf

refactor: extract OriginKeyString helper

Split formatting the normalized public form of a derived key, i.e. its
origin followed by the extended public key at the last hardened step,
out of BIP32PubkeyProvider::ToNormalizedString. No behavior change. A
later commit reuses it when normalizing the keys of a multipath
descriptor string.

8d5686535c

refactor: extract MergeNormalizedOrigin helper

Split merging an outer key origin into an already normalized key
expression out of OriginPubkeyProvider::ToNormalizedString. No behavior
change. A later commit reuses it when normalizing the keys of a
multipath descriptor string.

c98c74df18

refactor: extract HardenedPrefix helper 163fa349e8

descriptor: normalize keys in the multipath descriptor string

Private extended keys followed by a fixed hardened prefix are now replaced by their key origin and the extended public key at the last hardened step, merging any explicit origin. This matches Descriptor::ToNormalizedString and produces BIP 388 wallet policy key expressions suitable for registering the multipath descriptor on a signing device.

A hardened multipath element or wildcard cannot be represented by a single publicly derivable multipath descriptor. Leave the Parse multipath output unset in those cases.

ParsePubkeyInner now returns an optional replacement alongside parsed providers so ParsePubkey can merge explicit key origin information before collecting it.

68cf669f90

wallet: store multipath descriptor record on import

When importdescriptors expands a BIP 389 multipath descriptor into
separate receive and change descriptors, additionally store a record
containing the multipath descriptor string, as reported by the parser
in public form, along with the IDs of the wallet descriptors it
expanded to.

Reject overlap with a different multipath record before importing any of
the expanded descriptors.

Older wallet software ignores the new record.

1423d9e126

rpc: return multipath descriptor from getaddressinfo

Add an optional multipath field to getaddressinfo, containing the
multipath descriptor that this address' parent descriptor was expanded
from, in public form.

df3420fc67

rpc: return multipath descriptor from listdescriptors

Add an optional multipath field to each listdescriptors entry that was
expanded from a multipath descriptor, containing the stored multipath
descriptor record. Every descriptor from the same multipath descriptor
returns the same value, always in public form.

55d1533881

wallet: copy multipath record in exportwatchonlywallet

The descriptor IDs in the record are translated to those of the
watchonly wallet.

e2306d6c38

refactor: setup wallet descriptors per output type pair

Add SetupDescriptorScriptPubKeyManPair, used by default wallet creation
and createwalletdescriptor, which derives the receive and change
descriptors of an output type from a single multipath descriptor
string, built by a new GenerateMultipathDescriptorString.

This commit does not change behavior.

3ef8688f2a

wallet: store multipath descriptor record on wallet creation

When SetupDescriptorScriptPubKeyManPair creates a descriptor pair, store
a record with the normalized multipath descriptor they derive from.

When createwalletdescriptor is used with the internal option, the record
is stored by whichever call completes the pair.

f26ecc2e31

Sjors force-pushed on Sep 15, 2026

DrahtBot added the label CI failed on Sep 15, 2026

DrahtBot commented at 9:59 AM on September 15, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/34951927012/job/104324684735 LLM reason (✨ experimental): CI failed because the IWYU (include-what-you-use) lint check reported required header/include changes (failure generated from IWYU).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Sep 15, 2026

DrahtBot removed the label Needs rebase on Sep 15, 2026

achow101 commented at 6:21 PM on September 15, 2026: member

Imo Parse should be able to take a multipath descriptor with private keys and produce a (normalized) public key descriptor, without the intermediate state of two individual descriptors. It has all the information it needs.

Parse should never return strings. it's job is to parse strings, not produce them. I think that having it produce a multipath string so that it can be stored is fundamentally the wrong way to go about this.

Even if parse could produce multipath descriptor strings, that doesn't help. We need to go from individual separated descriptor objects (not strings) to a multipath string. The wallet must not use multipath strings internally, it must not make any assumptions about the purposes of derivation paths.

Then I don't understand what you have in mind.

What I have in mind is that given multiple descriptor objects (not strings), you can walk them in simultaneously (a la ToStringHelper) to reconstruct a multipath descriptor.