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

Basic output descriptor functions (generate only) #1270

Merged
merged 1 commit into from
May 25, 2022

Conversation

kristapsk
Copy link
Member

Adds two functions for generating output descriptors (not yet used anywhere in code):

  • get_address_descriptor(addr) to derive output descriptor from Bitcoin address;
  • get_xpub_descriptor(xpub_key, address_type) to derive output descriptor from an extended public key.

This is very basic, taken from some unfinished stuff I'm doing (slowly). More tests could be added, also there is no support for fidelity bond wsh() descriptors currently. But should be already useful in context of #1267.

Validness of checksums can be tested using bitcoin-cli getdescriptorinfo.

@kristapsk
Copy link
Member Author

Anyone plans to review / test this or I can just merge it as is (as it does not affect any existing JM functionality)?

@AdamISZ
Copy link
Member

AdamISZ commented May 16, 2022

I was planning to take a look at it, but I keep getting delayed by other things. Can you give a link to something I should read from the Core repo to compare?

@kristapsk
Copy link
Member Author

@AdamISZ
Copy link
Member

AdamISZ commented May 24, 2022

OK after further review I can't see a reason not to merge this. It's just a first step with no client side impact yet.

A few things struck me:

  • some more tests wouldn't hurt, maybe there are some more test vectors in Core we can use?
  • I presume (seems so from this code) you intend us to always be using checksums?

I guess there will be more to think about re: whether we use these as inputs to importmulti .. how and where we display these descriptors (if at all).
Also I don't yet understand how descriptors interact with BIP32, can one specify an extended public key/account as a descriptor?

I guess some of this stuff you've already thought about.

@AdamISZ
Copy link
Member

AdamISZ commented May 24, 2022

OK I was about to merge this but then I suddenly thought: have you checked which aspects of this are currently in python-bitcointx?

@kristapsk
Copy link
Member Author

some more tests wouldn't hurt, maybe there are some more test vectors in Core we can use?

Guess could add tests for p2pkh and p2sh-p2wpkh xpub descriptors. Was initially looking for existing tests in Core, didn't find ones.

I presume (seems so from this code) you intend us to always be using checksums?

Didn't get this one. Checksums are required for it to be a valid descriptor. Previously in #1064 I used getdescriptorinfo RPC for calculating them, but doing locally is better.

have you checked which aspects of this are currently in python-bitcointx?

Didn't check before, but searching for "descriptor" and "descriptors" gives no results in their github.

This code is actually taken mostly from the stuff I am slowly working on (not yet even in draft PR state) to write external signer HWI driver of JM for Bitcoin Core, which would allow to use stuff writen against Core, like my bitcoin-scripts, with JM wallet, so that JM wallet would be like a hardware wallet from Core's perspective. That required this basic descriptor functionality. Currently only useful stuff for this I see is adding "copy output descriptor to clipboard" in addition to "copy extended public key to clipboard" in Qt GUI.

@kristapsk
Copy link
Member Author

Added tests for p2pkh and p2sh-p2wpkh xpub descriptors.

@AdamISZ
Copy link
Member

AdamISZ commented May 25, 2022

some more tests wouldn't hurt, maybe there are some more test vectors in Core we can use?

Guess could add tests for p2pkh and p2sh-p2wpkh xpub descriptors. Was initially looking for existing tests in Core, didn't find ones.

Yeah, that makes sense. Doubtless we will add some later when we get to a more concrete implementation.

I presume (seems so from this code) you intend us to always be using checksums?

Didn't get this one. Checksums are required for it to be a valid descriptor. Previously in #1064 I used getdescriptorinfo RPC for calculating them, but doing locally is better.

OK I guess that's just my misunderstanding; I read "The top level expression is either a SCRIPT, or SCRIPT#CHECKSUM where CHECKSUM is an 8-character alphanumeric descriptor checksum." in the descriptors doc you linked, and took it to mean that checksums are optional. Anyway no issue here.

have you checked which aspects of this are currently in python-bitcointx?

Didn't check before, but searching for "descriptor" and "descriptors" gives no results in their github.

OK, thanks for the check. Confirmed same here.

This code is actually taken mostly from the stuff I am slowly working on (not yet even in draft PR state) to write external signer HWI driver of JM for Bitcoin Core, which would allow to use stuff writen against Core, like my bitcoin-scripts, with JM wallet, so that JM wallet would be like a hardware wallet from Core's perspective. That required this basic descriptor functionality. Currently only useful stuff for this I see is adding "copy output descriptor to clipboard" in addition to "copy extended public key to clipboard" in Qt GUI.

Good to hear you're moving forward with this very desirable feature, I know it's not going to be a simple task.

So given that, merging.

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