feat(wallet): `create_single` accepts a multipath descriptor by ValuedMammal · Pull Request #1906 · bitcoindevkit/bdk

ValuedMammal · 2025-03-21T15:06:20Z

Notes to the reviewers

We reuse Wallet::create_single to accommodate a wallet descriptor that parses as a MultiXPub. The documentation is updated to clarify the distinction between the single vs. dual-keychain use cases.

The logical change happens in create_with_params where we now look for whether the external descriptor is_multipath and if so, use the parsed single descriptors to update the descriptor and change_descriptor and everything proceeds as normal. This means that the wallet change set still consists of the individually split descriptors.

An alternative that was discussed was to make a new dedicated function such as create_from_multipath that enforces the provided input to be a multipath descriptor. However we arrived at the approach in this PR in order to promote create_single as a more central API long term.

This is based on #1902 as that seemed to be relatively non controversial.

fixes bitcoindevkit/bdk_wallet#11

Follow ups: Consider whether it might be needed to parse a multipath descriptor at load time bitcoindevkit/bdk_wallet#11

Changelog notice

Changed:

Wallet::create_single can be used to create a Wallet from a BIP389 multipath descriptor

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature

..by including the missing match arm for `DescriptorPublicKey::MultiXPub` when initializing `networks`. Test that `into_wallet_descriptor` correctly parses a multipath descriptor and fails if passed a network that is invalid for the descriptor. Note that rust-miniscript doesn't support parsing a multipath descriptor containing extended private keys.

Test that creating a Wallet from a multipath descriptor returns a Wallet with two keychains. The first part of the multipath goes to the first (External) keychain and the second part goes to the Internal keychain. Fixup error message for `DescriptorError::Multipath` which previously indicated they weren't supported. The error can still be triggered by the appearance of a MultiXPub where not explicitly allowed.

luisschwab · 2025-03-24T11:46:48Z

crates/wallet/src/wallet/mod.rs

+        // In case of multipath we parse the descriptor into single descriptors and
+        // assign each one back to the `descriptor` and `change_descriptor`.
+        if descriptor.is_multipath() {
+            let mut desc_iter = descriptor.into_single_descriptors()?.into_iter();
+            descriptor = desc_iter.next().expect("must have a descriptor");
+            descriptor_keymap = Descriptor::parse_descriptor(&secp, &descriptor.to_string())?.1;
+            params.change_descriptor = desc_iter.next().map(params::make_descriptor_to_extract);
+        }


I think it should throw an error if there are more than 2 descriptors.

Although you've explained that only the first 2 are used in the documentation, I also wonder if returning an error would be a better idea in order to prevent the user's unexpected behavior.

Can you make a code suggestion (preferably non breaking)?

I can't say I fully agree with throwing an error, and instead lean toward managing expectations through effective documentation. But I guess it can go either way.

aagbotemi · 2025-03-29T08:29:29Z

crates/wallet/src/wallet/mod.rs

+        // assign each one back to the `descriptor` and `change_descriptor`.
+        if descriptor.is_multipath() {
+            let mut desc_iter = descriptor.into_single_descriptors()?.into_iter();
+            descriptor = desc_iter.next().expect("must have a descriptor");


Hi @ValuedMammal, your implementation is solid.
You can return this error gracefully, rather than panic here.

What error to return though?

Probably another variant like MissingDescriptor can be added to the error enum

I would say the presence of a descriptor is invariant in the structure of CreateParams, meaning we've ruled out the possibility of a missing descriptor.

Okay. I understand. That's a good development. I was initially concerned about the panic.

oleonardolima

cACK 4cf5537

Great work! I left a suggestion for a fix documentation typo, and another minor question. I agree with Schwab's, and wonder if we shouldn't return an error instead when multipath's with 3+ descriptors are used.

crates/wallet/src/wallet/mod.rs

oleonardolima · 2025-04-02T18:03:16Z

crates/wallet/src/wallet/mod.rs

+        // In case of multipath we parse the descriptor into single descriptors and
+        // assign each one back to the `descriptor` and `change_descriptor`.
+        if descriptor.is_multipath() {
+            let mut desc_iter = descriptor.into_single_descriptors()?.into_iter();
+            descriptor = desc_iter.next().expect("must have a descriptor");
+            descriptor_keymap = Descriptor::parse_descriptor(&secp, &descriptor.to_string())?.1;
+            params.change_descriptor = desc_iter.next().map(params::make_descriptor_to_extract);
+        }


Although you've explained that only the first 2 are used in the documentation, I also wonder if returning an error would be a better idea in order to prevent the user's unexpected behavior.

crates/wallet/src/wallet/mod.rs

Co-authored-by: Leonardo L. <[email protected]>

thunderbiscuit · 2025-04-07T14:47:31Z

Pardon me jumping here a bit late (and maybe with some missing information).

One comment I have here is that once this feature is in, devs cannot expect that a wallet created with Wallet::create_single() will not have an internal keychain. Note also that as mentioned in the docs:

for a wallet with no internal (change) keychain, any methods requiring a [KeychainKind] as input, e.g. [reveal_next_address] should only be called with the [External] variant. In some cases passing [Internal] is treated the same as [External] but this behavior must not be relied on.

I'm wondering where that certainty (around the number of keychains and hence reliance on certain APIs) should come from (and maybe relying on Wallet::create_single was not the right place to do that anyway?). A wallet could parse the descriptor before using it in its construction phase (Descriptor::is_multipath or similar), or call the Wallet::keychains() method and then pull the length of that iterator (seems hacky, or at least not what the method was intended for) to know if it has an internal keychain.

I initially thought of those as constructors that offer certainties about what types of wallets (i.e. single vs double keychains wallets) will come out of them (Wallet::create needs 2 descriptors and if you don't it errors out, Wallet::create_single takes only one descriptor and errors out if you give it more, and Wallet::create_multi takes one descriptors but must expand into 2 or more). Was this maybe the wrong mental model for it? Should this logic/checks live somewhere else maybe and I was sort of relying on the API in the wrong way?

Happy to hear everyone's thoughts. Again I'm coming in last minute a little bit on this, and I feel like I don't have complete knowledge on it so feel free to tell me I was wrong all along haha.

ValuedMammal added 2 commits March 20, 2025 11:20

ValuedMammal added this to the 1.2.0 milestone Mar 21, 2025

ValuedMammal self-assigned this Mar 21, 2025

ValuedMammal added this to BDK Wallet Mar 21, 2025

ValuedMammal moved this to Needs Review in BDK Wallet Mar 21, 2025

oleonardolima added the module-wallet label Mar 23, 2025

luisschwab reviewed Mar 24, 2025

View reviewed changes

aagbotemi reviewed Mar 29, 2025

View reviewed changes

oleonardolima mentioned this pull request Apr 2, 2025

fix(wallet): fix into_descriptor_key for DescriptorPublicKey #1902

Closed

5 tasks

oleonardolima reviewed Apr 2, 2025

View reviewed changes

docs(wallet): fix spelling mistake

d27d117

Co-authored-by: Leonardo L. <[email protected]>

ValuedMammal modified the milestones: 1.2.0, 1.3.0 Apr 3, 2025

ValuedMammal closed this Apr 7, 2025

ValuedMammal deleted the feat/create-multipath branch April 7, 2025 21:09

github-project-automation bot moved this from Needs Review to Done in BDK Wallet Apr 7, 2025

thunderbiscuit mentioned this pull request Apr 8, 2025

Support multipath descriptors bitcoindevkit/bdk_wallet#11

Closed

notmandatory removed this from BDK Wallet Apr 24, 2025

notmandatory removed this from the Wallet 2.0.0 milestone Apr 24, 2025

Conversation

ValuedMammal commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

luisschwab Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thunderbiscuit commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ValuedMammal commented Mar 21, 2025 •

edited

Loading

luisschwab Mar 24, 2025 •

edited

Loading

thunderbiscuit commented Apr 7, 2025 •

edited

Loading