Multipath descriptors #470

darosior · 2022-10-03T00:24:51Z

This implements BIP389 multipath descriptors. Fixes #469.

sanket1729 · 2022-10-07T07:38:56Z

I briefly looked at the code and I like the overall approach for the given BIP. Overall, a strong concept and approach ACK. Just two things:

I think Conversion to DefininateDescriptorKey should fail when there is MultiPath descriptor
We can add MiniscriptKey::num_derivation_paths/MiniscriptKey::num_paths method to MiniscriptKey trait. When converting Terminal to Miniscript, we check whether all keys have the same num_derivation_paths/num_pahts, otherwise an error. For all existing types that implement MiniscriptKey, this can return 1. I am undecided/unsure whether we want a default impl returning 1 or if we should force all implementers to write the function.
Lastly, for taproot descriptors, it would require checking the same thing in all miniscripts and internal keys in all the initialization constructors.

On another note, I don't like the BIP changes in KEY expressions, but I have not formed an opinion about it. Previously, we could reason about legality of KEY expressions only based on top level contexts, but with this a KEY expression can invalidate another KEY expression in some other part of descriptor

darosior · 2022-10-12T08:48:23Z

On another note, I don't like the BIP changes in KEY expressions, but I have not formed an opinion about it. Previously, we could reason about legality of KEY expressions only based on top level contexts, but with this a KEY expression can invalidate another KEY expression in some other part of descriptor

I very much agree. This is the feedback i gave to Achow when implementing this.

I was kinda waiting for an answer from him about this, but looks like it won't come. I'll address your feedback.

darosior · 2022-10-12T10:49:28Z

Implemented your suggestions and removed the WIP label from this PR. Having a num_der_paths method on MiniscriptKey allowed to check for inconsistent multipath keys directly in Descriptor<Pk>::from_str.

I am undecided/unsure whether we want a default impl returning 1 or if we should force all implementers to write the function

Rather returning 0 by default? In any case i don't think there is a sane default for this. It really depends on the implementer. So i went for no default implementation: should we want to, i think we can always introduce one later on without breaking backward compatibility.

darosior · 2022-10-12T10:49:40Z

src/descriptor/key.rs

+    /// Whether or not this key has multiple derivation paths.
+    pub fn is_multipath(&self) -> bool {
+        match *self {
+            DescriptorSecretKey::Single(..) | DescriptorSecretKey::XPrv(..) => false,
+            DescriptorSecretKey::MultiXPrv(_) => true,
+        }
+    }


This seems redundant with MiniscriptKey's num_der_paths introduced later on, but i kept it here in order to also have it for private keys.

It didn't need to be a method of an extended public key, especially not since non-extended keys can have an origin too.

It's not specific to extended keys.

We'll need it for extended keys with multipath.

…cing This is prep work for the multipath keys, for which another ConversionError variant can occur. It's also cleaner on its own as returning a ConversionError is what we do in other places where we might fail on a conversion.

darosior · 2022-10-20T16:21:33Z

Rebased on master following the recent merges for the 8.0.0 release.

darosior

I've integrated this in my downstream project. I found a bug for which i'm pushing the fix and a unit test triggering it. Aside from this bug i've successfully used it and by doing so cross-checked it against the Python implementation i'm using in the functional testing framework of my software (darosior/python-bip380#21).

darosior · 2022-10-21T14:23:32Z

src/descriptor/key.rs

+            }
+            write!(f, ">")?;
+        }
+        write!(f, "/{}", child)?;


It should be in the else clause. And that should be caught by a unit test!

I'm adding roundtrip unit tests that fail without the patch for this bug.

darosior · 2022-10-24T06:34:55Z

Got a spurious io issue on the CPP tests:

thread 'tests_from_cpp' panicked at 'called `Result::unwrap()` on an `Err` value: JsonRpc(Transport(SocketError(Os { code: 11, kind: WouldBlock, message: "Resource temporarily unavailable" })))', tests/test_cpp.rs:80:10

Force-pushing to trigger a re-run.

117171f commands: use a separate key chain for change addresses (Antoine Poinsot) d9f905a db: track the next unused derivation index for change, too (Antoine Poinsot) 58a0e57 db: record whether a coin was received on a change address (Antoine Poinsot) 9b04a55 db: store derivation index also for addresses from the change desc (Antoine Poinsot) 4f3daa7 descriptors: cache the receive and change descriptors (Antoine Poinsot) ca3d7c1 descriptors: introduce a newtype for the multipath descriptor (Antoine Poinsot) 1320ee3 daemon: use multipath descriptors (Antoine Poinsot) d4db804 qa: add a missing 'wait_for' in spend creation test (Antoine Poinsot) 7a18c58 bitcoind: filter received coins based on parent descriptors (Antoine Poinsot) ba4c1e0 bitcoind: include change outputs in listsinceblock (Antoine Poinsot) caaca1f descriptors: rename derive into derive_received (Antoine Poinsot) f985fd7 descriptors: remove as_inner method (Antoine Poinsot) 846d924 qa: upgrade python-bip380 to latest master (Antoine Poinsot) 3105b86 Use my own fork of rust-miniscript (Antoine Poinsot) Pull request description: This fixes #18 by implementing the de-facto standard of using a `/0/*` keychain for receiving addresses and a `/1/*` keychain for change addresses. Note that once we'll have multisig, reusing addresses will still be possible since wallet don't share the same "next derivation index". In order to avoid forcing the user to configure and backup two almost identical descriptors, we make use of the recently proposed BIP389 (bitcoin/bips#1354). In order to prevent as much as possible introducing a backward incompatibility in the configuration file after the first release, we restrict the usage of multipath descriptors to `<0;1>` here. We now derive public keys from `xpub/0/*` and `xpub/1/*` while we were previously deriving them from `xpub/*`. This triggered a pretty invasive refactoring, as most parts of the codebase had to be updated to support the new change/receive separation (even the functional test miniscript dependency had to be updated, see darosior/python-bip380#21). Broadly, this: 1. Update our Miniscript dependency to my upstream PR (rust-bitcoin/rust-miniscript#470) rebased on top of the 8.0.0 release. 2. Updates the descriptors module to handle somewhat safely the multipath descriptors (to avoid mixing up the single, multi, and derived descriptors). 3. Makes a multipath descriptor mandatory in the configuration file. 4. Updates the Bitcoin backend poller aware of descriptors for which to track coins. - Necessarily this updates the bitcoind implementation to import two descriptors 5. Record in database whether a coin was for the change or receive descriptor, in addition to its derivation index ACKs for top commit: edouardparis: ACK 117171f Tree-SHA512: efcb7267f1ba6a5a3072e96fd1c70272f81092e86ee1178833f83d0aa88f271f42c269b71ca9992e76bb3e103baf1a189a609cc20f14f29b7388ab133da99044

darosior · 2022-11-01T13:00:22Z

I hadn't noticed the key submodule was made private. I just pushed a patch to re-export DescriptorMultiXKey like DescriptorXKey. H/T @edouardparis for mentioning that to me.

darosior · 2022-11-17T15:48:29Z

Friendly ping to know when you'll be able to review this, or if you think it is blocked by something?

sanket1729 · 2022-11-17T16:15:18Z

@darosior, this is on my list for this week.

sanket1729

done reviewing a63d5a2. Just Left a couple of comments. The rest of it is good to go. I have some other changes in mind to come in the future that would clean up the translatePk trait so which would make it impossible to create Miniscripts with different multi-path lengths even after translating.

Right now, it would fall upon the user to explicitly call the mismatching API to make sure that the translation is done correctly.

src/descriptor/key.rs

sanket1729 · 2022-11-21T11:49:04Z

src/descriptor/mod.rs

+    /// Whether this descriptor is a multipath descriptor that contains any 2 multipath keys
+    /// with a different number of derivation paths.
+    /// Such a descriptor is invalid according to BIP389.
+    pub fn multipath_length_mismatch(&self) -> bool {


Just a review comment. Nothing to be addressed here. Ideally, we want to make sure that such invalid descriptors cannot be created in the first place. Adding this pub might confuse users to call it to double-check everything.

I think this is true for all APIs that deal with Miniscript, but not for the internal Terminal. I have some changes in mind in future PRs to remove all the confusing sanity_check/check APIs.

This makes it possible to get multiple descriptors out of a multipath descriptor, and test the parsing and detection of multipath descriptors.

…ltipath keys This is invalid by BIP389 (as combinations would blowup when getting single descriptors otherwise). We add a num_der_paths method to MiniscriptKey in order to be able to check for this at parsing time without specializing from_str for Descriptor<DescriptorPublicKey>.

sanket1729

ACK ae0f3d9.

This also updates the multipaths descriptor patch to the review PR rust-bitcoin/rust-miniscript#470 (rebased on 0.9)

3224f08 daemon: update rust-miniscript to 0.9 (Antoine Poinsot) Pull request description: This also updates the multipaths descriptor patch to the review PR rust-bitcoin/rust-miniscript#470 (rebased on 0.9) Fixes #123. ACKs for top commit: darosior: self-ACK 3224f08 -- trivial. Tree-SHA512: 03c8b181e96df8d73953cefadfbb090049859f01e6c8d03cf8417ad4d46795dc295b097f8df6c0f683fb9da03b78f39e824cb7768376d1d5718bdce177edcb6d

sanket1729 · 2022-12-13T14:55:08Z

@apoelstra, any thoughts on merging this? This is not a merged BIP yet and could be changed. But so are many things in this repo that are not even BIP yet.

apoelstra · 2022-12-13T20:46:31Z

Yeah, let's go for it. I took a quick look at the code and it looks good to me.

darosior mentioned this pull request Oct 6, 2022

Multipath descriptors support #469

Closed

darosior force-pushed the multipath_descriptors branch 2 times, most recently from a45f320 to f18d792 Compare October 12, 2022 10:37

darosior changed the title ~~WIP: Multipath descriptors~~ Multipath descriptors Oct 12, 2022

darosior commented Oct 12, 2022

View reviewed changes

darosior mentioned this pull request Oct 12, 2022

Release 8.0.0 #462

Merged

darosior force-pushed the multipath_descriptors branch from f18d792 to 99bc84a Compare October 12, 2022 11:04

darosior added 4 commits October 20, 2022 18:01

desc keys: make origin key parsing a standalone function

60732fb

It didn't need to be a method of an extended public key, especially not since non-extended keys can have an origin too.

desc keys: rename parse_xkey_origin to parse_key_origin

fb3cb90

It's not specific to extended keys.

desc keys: make parse_xkey_deriv a standalone function

ccd7ef1

We'll need it for extended keys with multipath.

darosior force-pushed the multipath_descriptors branch from 99bc84a to 9b0d761 Compare October 20, 2022 16:21

darosior commented Oct 23, 2022

View reviewed changes

darosior force-pushed the multipath_descriptors branch 2 times, most recently from 7d756f2 to d64e83f Compare October 23, 2022 18:26

darosior force-pushed the multipath_descriptors branch from d64e83f to 6d7864e Compare October 24, 2022 06:35

This was referenced Oct 24, 2022

Multipath descriptors support darosior/python-bip380#21

Merged

Decent change management (using multipath descriptors) wizardsardine/liana#74

Merged

darosior force-pushed the multipath_descriptors branch from 6d7864e to a63d5a2 Compare November 1, 2022 12:58

sanket1729 reviewed Nov 21, 2022

View reviewed changes

desc keys: implement BIP389 multipath key expressions

34472bb

darosior added 4 commits November 25, 2022 16:00

desc keys: unit tests for multipath key expressions

b5eb317

desc keys: add a method to get single-path keys from multipath keys

cf401b6

descriptors: BIP389 multipath descriptors support

131bb5b

This makes it possible to get multiple descriptors out of a multipath descriptor, and test the parsing and detection of multipath descriptors.

darosior force-pushed the multipath_descriptors branch from a63d5a2 to ae0f3d9 Compare November 25, 2022 15:07

sanket1729 approved these changes Dec 3, 2022

View reviewed changes

darosior added a commit to darosior/liana that referenced this pull request Dec 7, 2022

daemon: update rust-miniscript to 0.9

3224f08

This also updates the multipaths descriptor patch to the review PR rust-bitcoin/rust-miniscript#470 (rebased on 0.9)

darosior mentioned this pull request Dec 7, 2022

daemon: update rust-miniscript to 0.9 wizardsardine/liana#161

Merged

sanket1729 merged commit 0daded4 into rust-bitcoin:master Dec 14, 2022

darosior deleted the multipath_descriptors branch June 2, 2023 09:43

darosior mentioned this pull request Jun 13, 2023

descriptors: Be able to specify change and receiving in a single descriptor string bitcoin/bitcoin#22838

Merged

notmandatory mentioned this pull request Apr 4, 2025

Support multipath descriptors bitcoindevkit/bdk_wallet#11

Open

andreasgriffin mentioned this pull request Jul 30, 2024

Make wallet constructor compatible with multipath descriptors bitcoindevkit/bdk-ffi#578

Open

Multipath descriptors #470

Multipath descriptors #470

Uh oh!

Conversation

darosior commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior commented Oct 12, 2022

Uh oh!

darosior commented Oct 12, 2022

Uh oh!

darosior Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

darosior commented Oct 20, 2022

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

darosior Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

darosior commented Oct 24, 2022

Uh oh!

darosior commented Nov 1, 2022

Uh oh!

darosior commented Nov 17, 2022

Uh oh!

sanket1729 commented Nov 17, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanket1729 Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Dec 13, 2022

Uh oh!

apoelstra commented Dec 13, 2022

Uh oh!

Uh oh!

darosior commented Oct 3, 2022 •

edited

Loading

sanket1729 commented Oct 7, 2022 •

edited

Loading