Skip to content
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

[New BIP 389] Multipath descriptors #1354

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

achow101
Copy link
Member

This was posted to the mailing list a couple of weeks ago.


This modification to Key Expressions uses two new characters: <tt><</tt> and <tt>;</tt>.
These are part of the descriptor character set and so are covered by the checksum algorithm.
As these are previously unused characters, old parsers will not accidentally mistake them for indicating something else.
Copy link
Member

Choose a reason for hiding this comment

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

What prevents wallets from requiring multipath syntax, and losing support for the current split model?

(Do the benefits outweigh the complexity of having two different ways to implement this?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it would make sense to only require multipath syntax. The single path syntax is still required in order to have multipath support as all of the other indices in the derivation path are standard single derivation items, and there's no guarantee of where the multipath specifier will even appear. The only way that multipath would be required is if the implementer went out of their way to specifically require it.

@luke-jr luke-jr changed the title [New BIP] Multipath descriptors [New BIP 389] Multipath descriptors Sep 29, 2022
@luke-jr
Copy link
Member

luke-jr commented Sep 29, 2022

Assigned BIP 389

** Optionally followed by a single <tt>/*</tt> or <tt>/*h</tt> final step to denote all direct unhardened or hardened children.

When a <tt>/<NUM;NUM;...;NUM></tt> is encountered, parsers should produce multiple descriptors where the first descriptor uses the first <tt>NUM</tt>, and a second descriptor uses the second <tt>NUM</tt>, and so on, until each <tt>NUM</tt> is accounted for.
Descriptors that contain multiple Key Expressions that each have a <tt>/<NUM;NUM;...;NUM></tt> must have tuples of exactly the same length.
Copy link
Member

@darosior darosior Oct 3, 2022

Choose a reason for hiding this comment

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

No strong opinion, but it's a bit awkward to have a parsing check for the key expression that isn't local to itself. By mandating a length of 2 there would be no need to go through all the keys of each descriptor you parsed to check if not 2 of them are "multipath keys" with different tuple lengths. And we can always change that in the future should we really need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, for this reason, I prefer having multipath(<NUM_PATHS>, ) syntax. But upon reading the ML post, seems like some people are already using the syntax presented in the BIP.

@dr-orlovsky
Copy link
Contributor

It seems to me that in "Backwards compatibility" section it would be nice to mention BIP88 - as well as in motivation to explain why this standard does not work and we need another one. I know about the discussion in bitcoin/bitcoin#17190, but the standard needs a summary of it.

@achow101
Copy link
Member Author

achow101 commented Oct 7, 2022

Updated with the number.

It seems to me that in "Backwards compatibility" section it would be nice to mention BIP88 - as well as in motivation to explain why this standard does not work and we need another one. I know about the discussion in bitcoin/bitcoin#17190, but the standard needs a summary of it.

I've added a brief paragraph on that.

@achow101 achow101 force-pushed the bip-multipath-descs branch 2 times, most recently from 117965c to 1109ef2 Compare October 7, 2022 00:49
Copy link
Contributor

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Two suggestions, otherwise looks good to me

bip-0389.mediawiki Outdated Show resolved Hide resolved
bip-0389.mediawiki Outdated Show resolved Hide resolved
darosior added a commit to wizardsardine/liana that referenced this pull request Oct 28, 2022
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
sanket1729 added a commit to rust-bitcoin/rust-miniscript that referenced this pull request Dec 14, 2022
ae0f3d9 desc: check for mismatch in the number of derivation paths between multipath keys (Antoine Poinsot)
131bb5b descriptors: BIP389 multipath descriptors support (Antoine Poinsot)
cf401b6 desc keys: add a method to get single-path keys from multipath keys (Antoine Poinsot)
b5eb317 desc keys: unit tests for multipath key expressions (Antoine Poinsot)
34472bb desc keys: implement BIP389 multipath key expressions (Antoine Poinsot)
e122255 desc keys: make 'at_derivation_index' return an error instead of panicing (Antoine Poinsot)
ccd7ef1 desc keys: make parse_xkey_deriv a standalone function (Antoine Poinsot)
fb3cb90 desc keys: rename parse_xkey_origin to parse_key_origin (Antoine Poinsot)
60732fb desc keys: make origin key parsing a standalone function (Antoine Poinsot)

Pull request description:

  This implements [BIP389 multipath descriptors](bitcoin/bips#1354). Fixes #469.

ACKs for top commit:
  sanket1729:
    ACK ae0f3d9.

Tree-SHA512: c7791b02b7bfef964db586eb962289c0058af9a80ad2243cdf4309d6ac1ce65db5cfbfd9b5aa86d733088aeca691449b9777f2d03b969e41ee988d11fe02dd1e
@craigraw
Copy link
Contributor

Would be great to see this get merged - implementations following this approach already exist.

bip-0389.mediawiki Outdated Show resolved Hide resolved
bip-0389.mediawiki Outdated Show resolved Hide resolved
bip-0389.mediawiki Outdated Show resolved Hide resolved
bip-0389.mediawiki Outdated Show resolved Hide resolved
Descriptors that contain multiple Key Expressions that each have a <tt>/<NUM;NUM;...;NUM></tt> must have tuples of exactly the same length.

The common use case for this is to represent descriptors for producing receiving and change addresses.
When interpreting for this use case, wallets should use the first descriptor for producing receiving addresses, and the second descriptor for producing change addresses.

Choose a reason for hiding this comment

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

Should comment here about what any 3rd and subsequent elements are used for, even if thats use-case dependent. This reads as though it was written before more than 2 elements were allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not aware of any such use cases currently, but this was a suggestion that makes sense and I don't see that it is harmful.

Choose a reason for hiding this comment

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

I'm thinking something like Key expressions with multi-path tuples larger than 2 elements should not be assumed to follow the above receive/change scheme.


==Test Vectors==

TBD

Choose a reason for hiding this comment

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

None of the descriptor BIPs have test vectors and this is a potential source of problems for people implementing these specs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks for reminding me to write those.


This modification to Key Expressions uses two new characters: <tt><</tt> and <tt>;</tt>.
These are part of the descriptor character set and so are covered by the checksum algorithm.
As these are previously unused characters, old parsers will not accidentally mistake them for indicating something else.
Copy link

@jgriffiths jgriffiths Feb 9, 2023

Choose a reason for hiding this comment

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

The biggest back compatibility change is that this allows all non-combo descriptors to produce multiple outputs instead of a simple bijection from descriptor to script/address/key, this should probably be noted. Additionally, for combo descriptors this now allows the wpkh and sh-wpkh variants to themselves produce multiple outputs.

Copy link
Member Author

@achow101 achow101 Feb 9, 2023

Choose a reason for hiding this comment

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

This was already the case for ranged descriptors (those with keys that have /*), so I don't think that's something that needs to be mentioned. For ranged descriptors, it's already impossible to get the real descriptor that generated a script since you don't know the parent xpub or the derivation.

Choose a reason for hiding this comment

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

This is a change because the mapping cardinality changes. Before multi-path:

  • A non-ranged descriptor resolves to a single script (0 inputs to 1 output).
  • A ranged descriptor plus bip32 child number resolves to a single script (1-1).
  • A non-ranged combo resolves to either 2 or 4 scripts (0-[2|4]).
  • A ranged combo plus bip32 child number resolves to 4 scripts (1-4).

Note that combo is its own BIP and may not be supported by some implementations.

After Multi-path, the above all hold if multi-path aren't used. Multipath adds the following cases:

  • A non-ranged descriptor with MP resolves to N scripts (0-N).
  • A ranged descriptor plus bip32 child number with MP resolves to N scripts (1-N).
  • A non-ranged combo with MP resolves to either 2N or 4N scripts (0-[2N|4N]).
  • A ranged combo plus bip32 child number with MP resolves to 4N scripts (1-4N).

i.e. the number of required inputs to resolve to a single address has changed when MP is present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants