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 ability to convert solvability info to descriptor #14477

Merged
merged 6 commits into from Nov 27, 2018

Conversation

Projects
None yet
8 participants
@sipa
Copy link
Member

commented Oct 13, 2018

This PR adds functionality to convert a script to a descriptor, given a SigningProvider with the relevant information about public keys and redeemscripts/witnessscripts.

The feature is exposed in listunspent, getaddressinfo, and scantxoutset whenever these calls are applied to solvable outputs/addresses.

This is not very useful on its own, though when we add RPCs to import descriptors, or sign PSBTs using descriptors, these strings become a compact and standalone way of conveying everything necessary to sign an output (excluding private keys).

Unit tests and rudimentary RPC tests are included (more relevant tests can be added once RPCs support descriptors).

Fixes #14503.

@meshcollider meshcollider added the Wallet label Oct 13, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK 002bdef765a7cdb4dcf66fa186a4c4a1a7fc7b71

Show resolved Hide resolved src/script/descriptor.cpp Outdated
Show resolved Hide resolved src/script/descriptor.h Outdated
Show resolved Hide resolved src/script/descriptor.h
Show resolved Hide resolved src/test/descriptor_tests.cpp Outdated

@sipa sipa force-pushed the sipa:201810_inferdescript branch Oct 17, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

Addressed all nits, and added some comments.

@sipa sipa force-pushed the sipa:201810_inferdescript branch Oct 17, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Concept ACK. Lightly tested a5e0b62; e.g. getaddressinfo shows a descriptor now.

The descriptors only show the public key, which is consistent with getaddressinfo not showing private keys and seeds. Maybe add a descriptor boolean argument to dumpprivkey as a way to obtain descriptors with a private key?

@promag
Copy link
Member

left a comment

For large wallets this will make listunspent perform a lot worse?

Show resolved Hide resolved test/functional/wallet_address_types.py Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp
@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

utACK a5e0b620062bf00a5f24d195bcaf52839681cb5d. Only changes since last review were making some suggested changes and expanding the InferDescriptor.

It would also be nice to add release notes for the RPC changes in this PR.

For large wallets this will make listunspent perform a lot worse?

It seems like the new work listunspent is doing is similar to the work it was doing before with calling ExtractDestination and pwallet->GetCScript.

@sipa sipa force-pushed the sipa:201810_inferdescript branch to d71424c Oct 19, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Sorry for the big change, but I realized that after #14150, we'll want to integrate origin information into the inferred descriptors this PR brings, so I went ahead and did that (it's based on 14150 now).

@sipa sipa force-pushed the sipa:201810_inferdescript branch from d71424c Oct 19, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK cf4b935a0301972d0e1fc6a5cb32c6ff564dbbbc. Changes: rebasing over #14150, adding InferPubkey() and a test for the inferred origin field, making the address_types test ignore origin information in inferred descriptors, and adding release notes.

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved test/functional/wallet_address_types.py Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14646 (Add expansion cache functions to descriptors (unused for now) by sipa)
  • #14601 ([rpc] Descriptions: Consistent arg labels for types 'object', 'array', 'boolean', and 'string' by ch4ot1c)

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 sipa:201810_inferdescript branch Oct 21, 2018

@sipa sipa force-pushed the sipa:201810_inferdescript branch to e6e4704 Oct 22, 2018

@DrahtBot DrahtBot removed the Needs rebase label Oct 22, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Lightly tested e6e4704; getaddressinfo descriptor now includes origin info.

@ryanofsky
Copy link
Contributor

left a comment

utACK e6e4704. Changes since last review are rebase, 'solvable' naming, and more comprehensive test code that compares getaddressinfo descriptors with listunspent descriptors and PBST bip32_derivs paths.

Show resolved Hide resolved test/functional/wallet_address_types.py Outdated
@meshcollider
Copy link
Member

left a comment

utACK e6e4704

Show resolved Hide resolved src/script/descriptor.cpp
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@sipa sipa force-pushed the sipa:201810_inferdescript branch from e6e4704 Oct 26, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

Rebased, renamed the descriptor output field to desc (for consistency with the name of the input argument to scantxoutset), and added the output to scantxoutset as well (fixing #14503).

@sipa sipa force-pushed the sipa:201810_inferdescript branch Oct 29, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

re-utACK fe9b68a

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Also lightly tested 2286f20 using scantxoutset to check the balance of a watch-only "wallet". It's nice to see the origin information that I passed in appear in the desc field.

Even though it makes sense, it's less intuitive, that if you pass in origin like wpkh([fingerprint/44'/0'/0']xpub/0/* it comes out as wpkh([fingerprint/44'/0'/0'/0/0]pub_key). Might be worth pointing this on in the RPC help.

@ryanofsky
Copy link
Contributor

left a comment

utACK 2286f206e4ea139477b88867a13a491d8d14db3d. Changes since last review are renaming descriptor fields to desc, adding them to scantxoutset output, and tweaking release notes.

src/rpc/blockchain.cpp Outdated
@@ -2140,7 +2142,11 @@ UniValue scantxoutset(const JSONRPCRequest& request)
if (!desc->Expand(i, provider, scripts, provider)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys: '%s'", desc_str));
}
needles.insert(scripts.begin(), scripts.end());
for (const auto script : scripts) {
std::string desc = InferDescriptor(script, provider)->ToString();

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 30, 2018

Contributor

In commit "Add matching descriptors to scantxoutset output + tests" (fe9b68a81a6f3a9abac7b3b7740d5019129d4fff)

I think I agree with @Sjors in #14477 (comment) that behavior that comes from inferring a descriptor here, instead of deriving it directly from desc above, is a little unintuitive. IMO, it'd be nicer if Expand took an optional std::vector<std::unique_ptr<Descriptor>>* output_descriptors argument and filled it in parallel with the existing output_scripts argument.

This is just a suggestion, though, and maybe something to consider for a future PR.

This comment has been minimized.

Copy link
@sipa

sipa Oct 30, 2018

Author Member

That makes sense. The inferred descriptor is functionally equivalent with a more straightforward specialization of the descriptor you're suggesting, but descriptors are intended to at least be somewhat human readable, so this may matter.

However, it's quite a bit of code to implement that (it needs specialization code for a number of constructions, including combo() which would need to get split out into ``pkh()andpk()` etc), so I'd prefer to keep it for a later PR.

For that reason I also prefer not making any explicit promises about what is put in the RPC outputs; for now all it guarantees is something that encapsulates all the information. For scantxoutset we could indeed be better and maintain more of the input descriptor's structure.

@sipa sipa added this to Blockers in High-priority for review Nov 1, 2018

@promag
Copy link
Member

left a comment

Commit adding release note could be reworded to "doc: Add release notes for #14477".

Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
Show resolved Hide resolved src/script/descriptor.cpp

@laanwj laanwj removed this from Blockers in High-priority for review Nov 8, 2018

Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

utACK 109699d

@ryanofsky
Copy link
Contributor

left a comment

utACK 109699d. Only superficial changes since last review: replacing assert with assert_equal, moving a variable declaration, avoiding copies with & and std::move.

@sipa sipa merged commit 109699d into bitcoin:master Nov 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Nov 27, 2018

Merge #14477: Add ability to convert solvability info to descriptor
109699d Add release notes (Pieter Wuille)
b65326b Add matching descriptors to scantxoutset output + tests (Pieter Wuille)
16203d5 Add descriptors to listunspent and getaddressinfo + tests (Pieter Wuille)
9b2a25b Add tests for InferDescriptor and Descriptor::IsSolvable (Pieter Wuille)
225bf3e Add Descriptor::IsSolvable() to distinguish addr/raw from others (Pieter Wuille)
4d78bd9 Add support for inferring descriptors from scripts (Pieter Wuille)

Pull request description:

  This PR adds functionality to convert a script to a descriptor, given a `SigningProvider` with the relevant information about public keys and redeemscripts/witnessscripts.

  The feature is exposed in `listunspent`, `getaddressinfo`, and `scantxoutset` whenever these calls are applied to solvable outputs/addresses.

  This is not very useful on its own, though when we add RPCs to import descriptors, or sign PSBTs using descriptors, these strings become a compact and standalone way of conveying everything necessary to sign an output (excluding private keys).

  Unit tests and rudimentary RPC tests are included (more relevant tests can be added once RPCs support descriptors).

  Fixes #14503.

Tree-SHA512: cb36b84a3e0200375b7e06a98c7e750cfaf95cf5de132cad59f7ec3cbd201f739427de0dc108f515be7aca203652089fbf5f24ed283d4553bddf23a3224ab31f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.