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

Descriptors: allow the multipath step to be different from <0;1> #599

Merged

Conversation

darosior
Copy link
Member

@darosior darosior commented Aug 9, 2023

Ledger and wallet policies disallow having more than 2 depth after the
placeholder, therefore we can't do @1/0/<0;1>/*, @1/1/<0;1>/*, ..
Instead we have to do @1/<0;1>/*, @1/<2;3>/*, ..

Why not? Salvatore also says the cost of deriving another depth is
non-trivial on a signing device.

Don't pick a fight with Salvatore, instead just let the GUI (or whatever
creates the desc) use different multipath steps for keys derived from
the same xpubs.

Based on #584. We need to make sure we don't make the assumptions of the multipath step always being <0;1> anywhere else the codebase.

@darosior darosior changed the title Descriptors any multipath index Descriptors: allow the multipath step to be different from <0;1> Aug 9, 2023
It's supposed to represent the number of signature per "master" key, i
guess. At the moment it would always be 1 because the origin changes
when we queried more keys from the signing device (because we increased
the account).
@darosior darosior force-pushed the descriptors_any_multipath_index branch from 0ba1d8b to b3d30cf Compare August 9, 2023 16:50
@darosior
Copy link
Member Author

darosior commented Aug 9, 2023

Rebased.

Since wizardsardine#575 we started using
xpubs with an appended derivation path, and the analysis was assuming
there wasn't any.
Ledger and wallet policies disallow having more than 2 depth after the
placeholder, therefore we can't do `@1/0/<0;1>/*`, `@1/1/<0;1>/*`, ..
Instead we have to do `@1/<0;1>/*`, `@1/<2;3>/*`, ..

Why not? Salvatore also says the cost of deriving another depth is
non-trivial on a signing device.

Don't pick a fight with Salvatore, instead just let the GUI (or whatever
creates the desc) use different multipath steps for keys derived from
the same xpubs.
@darosior darosior force-pushed the descriptors_any_multipath_index branch from b3d30cf to 605c3f6 Compare August 10, 2023 06:51
@darosior
Copy link
Member Author

Rebased to get the MSRV fix from #584.

Copy link
Member

@edouardparis edouardparis left a comment

Choose a reason for hiding this comment

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

ACK 605c3f6
tested with GUI

darosior added a commit that referenced this pull request Aug 11, 2023
10acc48 descriptors: fix the signatures count analysis (Antoine Poinsot)
7a33040 descriptors: make signed_pubkeys a mapping from fingerprint (Antoine Poinsot)

Pull request description:

  Needs more testing but that should do the trick: we need to take into account the derivation path appended to the xpub in addition to the one from the origin!

ACKs for top commit:
  darosior:
    ACK 10acc48 -- Edouard ACK'd the child PR at #599 (review)

Tree-SHA512: 912f482c5e10ece4127ffc86620f8ec581d0a209f8e1d407fd9885a7b6d1dfb13e7d5898c75da782282b9d40f69a80ad835e5a7648ceddcfc809445cdf246efb
@darosior darosior merged commit d299867 into wizardsardine:master Aug 11, 2023
18 checks passed
@darosior darosior deleted the descriptors_any_multipath_index branch August 11, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants