Skip to content

Conversation

@thunderbiscuit
Copy link
Member

This PR makes explicit a limitation of the Wallet::create_from_two_path_descriptor method, in that you must use public descriptors to build the wallet using this constructor, meaning you can only build watch-only wallets.

I also added a test to showcase this and the error returned.

See my comment here for more:

From what I can tell the issue is that miniscript cannot parse an xprv (I saw a comment from Sanket about this somewhere but now I can't find it anymore) if the multipath contains hardened derivation paths, and therefore miniscipt cannot ensure the xprv will be parsable into public keys in all cases? Something like that. But our arguments for bdk wallet constructors require the IntoWalletDescriptor trait:

pub trait IntoWalletDescriptor {
    /// Convert to wallet descriptor
    fn into_wallet_descriptor(
        self,
        secp: &SecpCtx,
        network: Network,
    ) -> Result<(ExtendedDescriptor, KeyMap), DescriptorError>;
}

pub type ExtendedDescriptor = Descriptor<DescriptorPublicKey>;

In other words, upon creation, the wallet will always try to parse the descriptor string into its public form, and here since it cannot be done by miniscript we fail to build.

Changelog notice

No changelog notice.

Checklists

All Submissions:

@thunderbiscuit thunderbiscuit changed the title docs: add wording on required use of xpub for multipath constructor Add docs on required use of xpub for multipath constructor Sep 4, 2025
@thunderbiscuit thunderbiscuit changed the title Add docs on required use of xpub for multipath constructor Add docs + test on required use of xpub for multipath constructor Sep 4, 2025
@coveralls
Copy link

Pull Request Test Coverage Report for Build 17476589408

Details

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 84.922%

Totals Coverage Status
Change from base Build 17330390829: 0.01%
Covered Lines: 6674
Relevant Lines: 7859

💛 - Coveralls

@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Sep 4, 2025
@ValuedMammal ValuedMammal added tests New or improved tests documentation Improvements or additions to documentation labels Sep 4, 2025
@ValuedMammal ValuedMammal added this to the Wallet 2.2.0 milestone Sep 4, 2025
@ValuedMammal
Copy link
Collaborator

ACK e647ed2

Further, the presence of hardened steps doesn't seem to make a difference, as this test shows. It's important to document the limitation as this PR does but to be honest I haven't found a good explanation for why it exists.

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK e647ed2

@thunderbiscuit
Copy link
Member Author

to be honest I haven't found a good explanation for why it exists

Yeah it's fuzzy for me too but I think the answer probably is in this video if I remember correctly. He goes over the ideas and tricks that make extended keys able to create children keys, and how this ability is broken by the "hardening" process.

It's maybe that because you can't ensure that some of the multipath paths are not hardened, miniscript just errors out on all multipath xprv. But also, could you not just error out if one or more of those paths are hardened instead of in all cases? I might be thinking about this wrong.

@ValuedMammal ValuedMammal merged commit 740c8c9 into bitcoindevkit:master Sep 5, 2025
59 of 71 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Sep 5, 2025
@ValuedMammal ValuedMammal mentioned this pull request Sep 5, 2025
2 tasks
ValuedMammal added a commit that referenced this pull request Sep 8, 2025
f73596e docs: add wording on required use of xpub for multipath constructor (thunderbiscuit)

Pull request description:

  This is just a cherry pick of my #306 PR onto the 2.2 release branch.

ACKs for top commit:
  ValuedMammal:
    ACK f73596e
  oleonardolima:
    ACK f73596e

Tree-SHA512: f1e0f2acf9ab832e33fcf071b880de8df09b40bc755236c80f0045c48f667c10e4b38133f50f210acafe6d335e6a62b77037b6a5068d16d44a25dd46e138307c
@thunderbiscuit thunderbiscuit deleted the docs/multipath branch September 24, 2025 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation tests New or improved tests

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants