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

Add key origin support to descriptors #14150

Merged
merged 3 commits into from Oct 22, 2018
Merged

Conversation

@sipa
Copy link
Member

@sipa sipa commented Sep 4, 2018

This adds support for key origin information to the descriptor parser, and exposes the resulting key path information through FlatSigningProvider.

There is no observable functionality from this right now, except having the scantxoutset RPC accept descriptors that include key origin information.

Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

src/script/descriptor.cpp Outdated Show resolved Hide resolved
src/script/descriptor.cpp Show resolved Hide resolved
@sipa sipa force-pushed the 201807_minedesc_origin branch from e6727aa to e11d7ea Sep 6, 2018
@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Sep 6, 2018

utACK e11d7ea24b7c67793ab8e84b5ad05d233f43e7da

Copy link
Member

@Sjors Sjors left a comment

Concept ACK.

For me this would be easier to review when combined with a practical usage, like creating a PSBT with the origin info still in it. That could just be a WIP PR as it might involve quite a few changes, since descriptors currently are only used with scantxoutset.

src/test/descriptor_tests.cpp Show resolved Hide resolved
src/test/descriptor_tests.cpp Outdated Show resolved Hide resolved
src/test/descriptor_tests.cpp Outdated Show resolved Hide resolved
src/script/sign.h Outdated Show resolved Hide resolved
src/script/descriptor.cpp Outdated Show resolved Hide resolved
src/script/descriptor.cpp Outdated Show resolved Hide resolved
@sipa sipa force-pushed the 201807_minedesc_origin branch from e11d7ea to 78e5094 Sep 10, 2018
@Sjors
Copy link
Member

@Sjors Sjors commented Sep 13, 2018

Perhaps @achow101 can use this in #14021?

@achow101
Copy link
Member

@achow101 achow101 commented Sep 13, 2018

@Sjors I don't think this would be useful there. Instead we should have some new command that lets us import descriptors into the wallet.

CKeyID keyid = m_extkey.pubkey.GetID();
std::copy(keyid.begin(), keyid.begin() + 4, info.fingerprint);
info.path = m_path;
if (m_derive == DeriveType::UNHARDENED) info.path.push_back(pos);
Copy link
Contributor

@practicalswift practicalswift Sep 25, 2018

2018-09-25 20:23:55 clang(pr=14150): script/descriptor.cpp:179:69: warning: implicit conversion changes signedness: 'int' to 'std::vector<unsigned int, std::allocator<unsigned int> >::value_type' (aka 'unsigned int') [-Wsign-conversion]

Copy link
Member Author

@sipa sipa Oct 12, 2018

Fixed.

@sipa sipa force-pushed the 201807_minedesc_origin branch 2 times, most recently from 95897b6 to 2782910 Oct 12, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Oct 12, 2018

Rebased, and expanded documentation to include key origin information.

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 16, 2018

Was there a problem with the last rebase? It seems like some changes that were previously made, like #14150 (comment), have been reverted.

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Oct 16, 2018

Concept ACK, will wait for the rebase to be fixed before reviewing then

@sipa sipa force-pushed the 201807_minedesc_origin branch from 6053675 to 3fdc704 Oct 17, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Oct 17, 2018

@ryanofsky Wow, indeed, thanks for pointing that out. I don't know how that happened.

I went through all comments again, and reapplied the fixed I made earlier where necessary. I've also expanded the documentation change a bit.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK 3fdc704f06bb5618cb63c248b20447026509e00d. Left some comments below that aren't critical and you should feel free to ignore.

src/script/descriptor.cpp Show resolved Hide resolved
src/script/descriptor.cpp Outdated Show resolved Hide resolved
src/script/descriptor.cpp Show resolved Hide resolved
src/script/descriptor.cpp Outdated Show resolved Hide resolved
@@ -34,6 +34,7 @@ Output descriptors currently support:
- `sh(wsh(multi(1,03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8,03499fdf9e895e719cfd64e67f07d38e3226aa7b63678949e6e49b241a60e823e4,02d7924d4f7d43ea965a465ae3095ff41131e5946f3c85f79e44adbcf8e27e080e)))` represents a P2SH-P2WSH *1-of-3* multisig.
- `pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8)` refers to a single P2PK output, using the public key part from the specified xpub.
- `pkh(xpub68Gmy5EdvgibQVfPdqkBBCHxA5htiqg55crXYuXoQRKfDBFA1WEjWgP6LHhwBZeNK1VTsfTFUHCdrfp1bgwQ9xv5ski8PX9rL2dZXvgGDnw/1'/2)` refers to a single P2PKH output, using child key *1'/2* of the specified xpub.
- `pkh(d34db33f/44'/0'/0':xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)` refers to a chain of P2PKH outputs, but additionally specifies that the specified xpub is a child of a master with fingerprint `d34db33f`, and derived using path `44'/0'/0'`.
Copy link
Contributor

@ryanofsky ryanofsky Oct 17, 2018

This syntax is slightly weird to me because if you're looking at an expression like:

fingerprint/path1:xpub/path2

The fingerprint:path1 part only describes xpub, not xpub/path2, but there's no logical precedence of : and / operators that would lead me to see:

(fingerprint/path1:xpub)/path2

instead of:

(fingerprint/path1):(xpub/path2)

Maybe this is an unimportant aesthetic objection, but I'd almost rather there be an explicit origin function like:

origin(fingerprint/path1, xpub)/path2

or

origin(xpub, fingerprint/path1)/path2

that at least would group things together correctly.

src/script/descriptor.cpp Show resolved Hide resolved
@sipa sipa force-pushed the 201807_minedesc_origin branch from 3fdc704 to 71cbcd9 Oct 17, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Oct 17, 2018

@ryanofsky Addressed all your comments except the syntax issue, which we should probably discuss at the PR level rather than buried inside a code comment.

I agree that fpr/path:key/path doesn't perfectly map with intuition about precedence of operation. On the other hand, I'd prefer to keep "key operations" as pure syntax, separated from "script operations" which are functions.

After thinking a bit more about it, how do you feel about fpr/path[key]/path? That way the entire expression maintains the order of path elements, and it just looks like you're "clarifying" the actual key somewhere inside the path itself, rather than as a separate thing prefixed onto it.

An alternative which is somewhat easier to parse I think is [fpr/path]key/path, which can still be read as "([fpr/path]key)/path".

Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK 71cbcd9da8b31cb7cbe19001df32c8e1c46a603b. Only changes since last review were suggested changes (replacing +4's, adding a few test cases and a comment).


I think I like both of the [fpr/path]key/path and fpr/path[key]/path syntaxes more than the current fpr/path:key/path syntax and more than my origin() suggestion, and I agree that the [fpr/path]key/path version is probably more understandable.

I might also propose a more generalized syntax for adding attributes to keys like:

key[origin=fpr/path, device=myhardwarewallet]/path

Which might have other uses like:

key[birthday=timestamp]

But this might be too heavyweight. Anyway, you've thought about this a lot more than I have and I think whatever syntax you like, including the current syntax, should be fine.

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 17, 2018

Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Could you explain how these use-cases benefit from having origin information embedded inside descriptor expressions? It seems like it might be simpler if the descriptor language only described how things should be signed, and if key information was stored (or passed) in a separate a key id -> key metadata mapping.

@sipa
Copy link
Member Author

@sipa sipa commented Oct 17, 2018

@ryanofsky A PSBT updater needs to add key origin information to the PSBT file, or future signers may not be able to find the key to sign with.

I really think this information needs to be part of descriptors for them to be generally useful. Just listing the public keys is useless if those using the information don't know how to find the private key corresponding to said public key. In a way, that derivation information should from our point of view be treated as part of the public information about that key.

Another argument is so that you can 'specialize' a ranged descriptor to an individual one (like what may be desirable for #14503), without losing information. With origin support you can go from xpub/a/b/c/* to [fpr:a/b/c/i]pubkey when specializing for position i.

@sipa sipa force-pushed the 201807_minedesc_origin branch from 71cbcd9 to cfc8129 Oct 17, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Oct 17, 2018

Switched to the [fpr/path]key/path notation, updating the parser, serializer, tests, and documentation.

As far as other attributes go, I'm not sure descriptors are the right place (this may be getting philosophical, though):

  • a timestamp is more a descriptor-level property than a key specific thing (you care about when any output generated by the descriptor can be used at the earliest, not differences in timestamps between the individual keys inside one).
  • the name of a hardware device is not portable (if included in descriptors, you wouldn't be able to copy the descriptor to another software/system that knows the hw device by a different name)

My preference is keeping those two as metadata on the descriptor (together with things like gap limit/range, whether the outputs are treated as change, ...). The same could be done with key origin information, but it feels more fundamental to not lose that information.

If there are generally useful properties of keys or path elements later we can still adopt such a syntax, though; it looks pretty readable.

@sipa
Copy link
Member Author

@sipa sipa commented Oct 18, 2018

Addressed further comments in separate commits, will squash when ready.

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Oct 18, 2018

re-ACK

Copy link
Member

@meshcollider meshcollider left a comment

utACK f4a8ccb

src/script/descriptor.cpp Outdated Show resolved Hide resolved
doc/descriptors.md Outdated Show resolved Hide resolved
@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Oct 19, 2018

utACK 91cff98264ba5c6a86ca5d18b2591f759cffe793, just documentation fixes and two variable cleanups

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Oct 20, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14505 (Make all single parameter constructors explicit (C++11) by practicalswift)

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.

@sipa sipa force-pushed the 201807_minedesc_origin branch from 91cff98 to afa9224 Oct 21, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Oct 21, 2018

Squashed fixups, no tree changes.

@sipa sipa force-pushed the 201807_minedesc_origin branch from afa9224 to 8afb166 Oct 21, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Oct 21, 2018

Rebased afa9224b14 -> 8afb166

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Oct 22, 2018

utACK 8afb166

Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK 8afb166. Only change since last review is rebase/squash, resolving a merge conflict with #14161 in the documentation

yashbhutwala pushed a commit to yashbhutwala/bitcoin that referenced this issue Oct 22, 2018
8afb166 Update documentation to incude origin information (Pieter Wuille)
ff37459 Add tests for key origin support (Pieter Wuille)
2c6281f Add key origin support to descriptors (Pieter Wuille)

Pull request description:

  This adds support for [key origin](https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82#key-origin-identification) information to the descriptor parser, and exposes the resulting key path information through `FlatSigningProvider`.

  There is no observable functionality from this right now, except having the `scantxoutset` RPC accept descriptors that include key origin information.

  Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Tree-SHA512: 399828127b2e90a2f32d81ecc30a8a9261d08f4182d5d1744f05e46b25fde1bd383c54835b0820ca668e7d17353fa92c0fb2987e211ce269e0824c9395d210c2
@sipa sipa merged commit 8afb166 into bitcoin:master Oct 22, 2018
2 checks passed
@fanquake fanquake removed this from Blockers in High-priority for review Oct 22, 2018
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue May 12, 2020
Summary:
8afb166875 Update documentation to incude origin information (Pieter Wuille)
ff37459abc Add tests for key origin support (Pieter Wuille)
2c6281f180 Add key origin support to descriptors (Pieter Wuille)

Pull request description:

  This adds support for [key origin](https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82#key-origin-identification) information to the descriptor parser, and exposes the resulting key path information through `FlatSigningProvider`.

  There is no observable functionality from this right now, except having the `scantxoutset` RPC accept descriptors that include key origin information.

  Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Tree-SHA512: 399828127b2e90a2f32d81ecc30a8a9261d08f4182d5d1744f05e46b25fde1bd383c54835b0820ca668e7d17353fa92c0fb2987e211ce269e0824c9395d210c2

Backport of Core [[bitcoin/bitcoin#14150 | PR14150]]

Test Plan:
  ninja
  ninja check
  ninja check-functional

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6006
ftrader added a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this issue Aug 17, 2020
Summary:
8afb166875 Update documentation to incude origin information (Pieter Wuille)
ff37459abc Add tests for key origin support (Pieter Wuille)
2c6281f180 Add key origin support to descriptors (Pieter Wuille)

Pull request description:

  This adds support for [key origin](https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82#key-origin-identification) information to the descriptor parser, and exposes the resulting key path information through `FlatSigningProvider`.

  There is no observable functionality from this right now, except having the `scantxoutset` RPC accept descriptors that include key origin information.

  Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Tree-SHA512: 399828127b2e90a2f32d81ecc30a8a9261d08f4182d5d1744f05e46b25fde1bd383c54835b0820ca668e7d17353fa92c0fb2987e211ce269e0824c9395d210c2

Backport of Core [[bitcoin/bitcoin#14150 | PR14150]]

Test Plan:
  ninja
  ninja check
  ninja check-functional

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6006
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 25, 2021
8afb166 Update documentation to incude origin information (Pieter Wuille)
ff37459 Add tests for key origin support (Pieter Wuille)
2c6281f Add key origin support to descriptors (Pieter Wuille)

Pull request description:

  This adds support for [key origin](https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82#key-origin-identification) information to the descriptor parser, and exposes the resulting key path information through `FlatSigningProvider`.

  There is no observable functionality from this right now, except having the `scantxoutset` RPC accept descriptors that include key origin information.

  Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Tree-SHA512: 399828127b2e90a2f32d81ecc30a8a9261d08f4182d5d1744f05e46b25fde1bd383c54835b0820ca668e7d17353fa92c0fb2987e211ce269e0824c9395d210c2
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 25, 2021
Does not include 8afb166

8afb166 Update documentation to incude origin information (Pieter Wuille)
ff37459 Add tests for key origin support (Pieter Wuille)
2c6281f Add key origin support to descriptors (Pieter Wuille)

Pull request description:

  This adds support for [key origin](https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82#key-origin-identification) information to the descriptor parser, and exposes the resulting key path information through `FlatSigningProvider`.

  There is no observable functionality from this right now, except having the `scantxoutset` RPC accept descriptors that include key origin information.

  Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Tree-SHA512: 399828127b2e90a2f32d81ecc30a8a9261d08f4182d5d1744f05e46b25fde1bd383c54835b0820ca668e7d17353fa92c0fb2987e211ce269e0824c9395d210c2
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 25, 2021
Does not include 8afb166

8afb166 Update documentation to incude origin information (Pieter Wuille)
ff37459 Add tests for key origin support (Pieter Wuille)
2c6281f Add key origin support to descriptors (Pieter Wuille)

Pull request description:

  This adds support for [key origin](https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82#key-origin-identification) information to the descriptor parser, and exposes the resulting key path information through `FlatSigningProvider`.

  There is no observable functionality from this right now, except having the `scantxoutset` RPC accept descriptors that include key origin information.

  Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Tree-SHA512: 399828127b2e90a2f32d81ecc30a8a9261d08f4182d5d1744f05e46b25fde1bd383c54835b0820ca668e7d17353fa92c0fb2987e211ce269e0824c9395d210c2
christiancfifi added a commit to christiancfifi/dash that referenced this issue Aug 25, 2021
Does not include 8afb166

8afb166 Update documentation to incude origin information (Pieter Wuille)
ff37459 Add tests for key origin support (Pieter Wuille)
2c6281f Add key origin support to descriptors (Pieter Wuille)

Pull request description:

  This adds support for [key origin](https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82#key-origin-identification) information to the descriptor parser, and exposes the resulting key path information through `FlatSigningProvider`.

  There is no observable functionality from this right now, except having the `scantxoutset` RPC accept descriptors that include key origin information.

  Longer term this feature helps with a potential descriptors-based walletless PSBT updater, or for importing hardware wallet xpubs (once the wallet can import descriptors).

Tree-SHA512: 399828127b2e90a2f32d81ecc30a8a9261d08f4182d5d1744f05e46b25fde1bd383c54835b0820ca668e7d17353fa92c0fb2987e211ce269e0824c9395d210c2
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet