Skip to content

Conversation

@mjdietzx
Copy link
Contributor

@mjdietzx mjdietzx commented Oct 19, 2021

This is on top of #22838, only my last commit should be considered here. This highlights how a basic multisig like our example can be cleaned up a bit with multipath descriptors. Also serves as some light additional testing for #22838

@laanwj laanwj added the Docs label Oct 19, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 20, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23512 (policy: Treat taproot as always active by MarcoFalke)
  • #23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
  • #23171 (qa: test descriptors with mixed xpubs and const pubkeys by darosior)
  • #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Rspigler
Copy link
Contributor

Concept ACK on doc changes. Thanks for working on this! Some questions though:

Should these doc changes also go in the descriptor BIP in addition to doc/descriptors.md? @achow101

@mjdietzx mjdietzx force-pushed the multisig_multipath_descriptor_wallet branch from ecbdd0b to 367d6a3 Compare October 23, 2021 21:56
@mjdietzx
Copy link
Contributor Author

Rebased

achow101 and others added 16 commits October 24, 2021 18:18
To prepare for returning multipath descriptors which will be a shorthand
for specifying 2 descriptors, change ParseScript's signature to return a
pair.
To prepare for multipath descriptors, have ParsePubkey return a pair of
PubkeyProviders and have ParseScript be able to handle them.
Multipath specifiers are derivation path indexes of the form `<r;c>`
used for specifying derivation paths for receiving and change
descriptors. Only one multipath specifier is allowed per PubkeyProvider,
and it must only contain two path indexes. This is syntactic sugar which
is parsed into two distinct descriptors. One descriptor will have all of
the `r` paths, the other all of the `c` paths.

ParseKeypath will always return a pair of keypaths. If no multipath
descriptor is provided, the second one will be a std::nullopt. The
callers of this function are updated to deal with this case and return
multiple PubkeyProviders. Their callers have also been updated to handle
pairs of PubkeyProviders.
When given a descriptor which contins a multipath derivation specifier,
a pair of descriptors will be returned.
When given a multipath descriptor, derive both the receiving and change
descriptors. The derived addresses will be returned in an object
consisting of 2 arrays. For compatibility, when given a single path
descriptor, the addresses are provided in a single array as before.
Instead of applying internal-ness to all keys being imported at the same
time, apply it on a per key basis. So each key that is imported will
carry with it whether it is for the change keypool.
Multipath descriptors will be imported as two separate descriptors with
the first one for receiving addreses, and the second for change
addresses. When importing a multipath descriptor, 'internal' cannot be
specified because of the implicit receive and change descriptors.
Multipath descriptors will be imported as two separate descriptors with
the first for receiving addresses and the second for change. This
mirrors importmulti.
Test that both importmulti and importdescriptors behave as expected when
importing a multipath descriptor.
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@Rspigler
Copy link
Contributor

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

1 similar comment
@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko
Copy link
Member

maflcko commented Jul 25, 2022

Closing for now. This can be reopened or a new pull be created.

@maflcko maflcko closed this Jul 25, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants