Skip to content

fix(wallet): fix into_descriptor_key for DescriptorPublicKey#1902

Closed
ValuedMammal wants to merge 1 commit intobitcoindevkit:masterfrom
ValuedMammal:fix/into-descriptor-key
Closed

fix(wallet): fix into_descriptor_key for DescriptorPublicKey#1902
ValuedMammal wants to merge 1 commit intobitcoindevkit:masterfrom
ValuedMammal:fix/into-descriptor-key

Conversation

@ValuedMammal
Copy link
Collaborator

Fix into_descriptor_key for DescriptorPublicKey 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.

fixes bitcoindevkit/bdk_wallet#10

Changelog notice

Fixed an issue preventing into_wallet_descriptor from correctly mapping a DescriptorPublicKey to the set of available networks.

Checklists

All Submissions:

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

Bugfixes:

  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

..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.
@ValuedMammal ValuedMammal added the bug Something isn't working label Mar 20, 2025
@ValuedMammal ValuedMammal added this to the 1.2.0 milestone Mar 20, 2025
@ValuedMammal ValuedMammal self-assigned this Mar 20, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Mar 20, 2025
Copy link
Contributor

@aagbotemi aagbotemi left a comment

Choose a reason for hiding this comment

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

Including the match arm for multipath fixes the issue. Welldone @ValuedMammal

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 8eaf35c

Copy link
Collaborator

@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.

cACK 8eaf35c

However, I think we should move with #1906 altogether, otherwise it would only partially solve the problem, at least while testing as this PR doesn't add support for multipath descriptor it'll still fail when trying to insert a descriptor, as it interprets as a single descriptor wallet and we're unable to calculate the descriptor_id as it's.

@ValuedMammal ValuedMammal modified the milestones: 1.2.0, 1.3.0 Apr 3, 2025
@ValuedMammal ValuedMammal deleted the fix/into-descriptor-key branch April 7, 2025 21:05
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Apr 7, 2025
@notmandatory notmandatory removed this from the Wallet 2.0.0 milestone Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multipath descriptor network association wrong

4 participants