-
Notifications
You must be signed in to change notification settings - Fork 41
Multipath descriptor support (BIP 389) #275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multipath descriptor support (BIP 389) #275
Conversation
Pull Request Test Coverage Report for Build 16654062685Details
💛 - Coveralls |
Thanks for working on this @schjonhaug. |
Glad to contribute, @ValuedMammal. Let me know if there’s anything I can improve! |
5893601
to
2911658
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this one on, overall your PR looks great. I especially like the detailed docs and new validation to ensure only two indexes are used.
I have two small suggestions and you should do a rebase and then I think it's ready to merge.
2911658
to
7bbada5
Compare
I like the direction here and think this would be a great edition. In working with common hardware signer export files, these are very common descriptor representations. My one nit is to be careful naming the APIs with the more general |
@rustaceanrob what do you think about a little shorter name like |
@schjonhaug do you think you'll have time this week to make the small changes ValuedMammal and Rob suggested above? You also need to do a Or if you are OK with the changes but won't have time to work on it let me know and I can push a commit to your PR to wrap it up. Thanks! |
I created a commit you can copy or I can push here if it looks OK. Then all the commits should be squashed to one with a conventional commits style message. I think the name @rustaceanrob suggested is better even though it's long. Then later when we're able to support any |
- Add create_from_two_path_descriptor() method to create wallets from BIP 389 multipath descriptors - Support descriptors with exactly 2 paths (receive/change) using <0;1> syntax - Update MultiPath error to clarify requirement for exactly 2 paths - Add helper function make_two_path_descriptor_to_extract() for parsing two-path descriptors - Add comprehensive tests for two-path wallet creation and validation
7bbada5
to
9e3adf2
Compare
Thanks for the commit @notmandatory. I included it and renamed some functions and parameters to drive home the point that we are talking about two-path descriptors. |
Approach ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9e3adf2
Co-authored-by: ValuedMammal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK da0b577
Thanks for getting this one wrapped up for the next release. I know there are downstream projects looking forward to having it.
After playing with this today to expose it in the bindings I realized that this constructor cannot be used with private keys. This is a fairly important aspect/limitation of using the method, and I think it should be made more explicit in the API docs. I will open a PR for this tomorrow. FYI if my investigation is correct, the issue is that miniscript cannot parse an xprv (I saw a comment from Sanket 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 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. This limitation should also be reflected in the tests; I'll add that to my PR as well. |
…structor e647ed2 docs: add wording on required use of xpub for multipath constructor (thunderbiscuit) Pull request description: 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](#275 (comment)) 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: ```rust 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: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `just p` before pushing ACKs for top commit: ValuedMammal: ACK e647ed2 oleonardolima: utACK e647ed2 Tree-SHA512: 15085dedc07c69f9e04876c174c4a8d54d186f0b04c6d1bf8ea76829cccdbf3f3644b7169392be5d03f0b983e4db6fd01afda6fb97ae348438f585cdde3fc08b
Description
Key Features:
Wallet::create_multipath(descriptor)
following the same pattern ascreate()
andcreate_single()
Usage Example:
Notes to the reviewers
Design Decisions:
make_multipath_descriptor_to_extract()
helper following the same pattern as existingmake_descriptor_to_extract()
functioncreate_multipath()
method returnsCreateParams
just likecreate()
andcreate_single()
, maintaining the fluent builder patternImplementation Notes:
make_multipath_descriptor_to_extract()
twice (once for receive, once for change) which is intentional and follows the established pattern of lazy evaluationChangelog notice
Added:
Wallet::create_multipath()
method for creating wallets from BIP 389 multipath descriptorsCreateParams::new_multipath()
for multipath descriptor parameter creationChecklists
All Submissions:
just p
before pushingNew Features:
Bugfixes:
Closes #11