Add ability to convert solvability info to descriptor

sipa commented at 8:37 pm on October 13, 2018: member

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 added the label Wallet on Oct 13, 2018

in src/script/descriptor.cpp:306 in 002bdef765 outdated

302@@ -298,6 +303,7 @@ class ConvertorDescriptor : public Descriptor
303     ConvertorDescriptor(std::unique_ptr<Descriptor> descriptor, const std::function<CScript(const CScript&)>& fn, const std::string& name) : m_convert_fn(fn), m_fn_name(name), m_descriptor(std::move(descriptor)) {}
304 
305     bool IsRange() const override { return m_descriptor->IsRange(); }
306+    bool IsSolvable() const override { return true; }

ryanofsky commented at 6:22 pm on October 16, 2018:

Should this be returning m_descriptor->IsSolvable() instead of true? Maybe add an explanatory comment if returning true here is actually correct.

sipa commented at 1:23 am on October 17, 2018:

It is actually correct, but so if your suggestion, which is much more obvious.

The reason is that the only non-solvable constructions (raw and addr) are only allowed at the top level anyway, so anything embedded in sh or wsh cannot be non-solvable.

in src/script/descriptor.h:35 in 002bdef765 outdated

31@@ -32,6 +32,9 @@ struct Descriptor {
32     /** Whether the expansion of this descriptor depends on the position. */
33     virtual bool IsRange() const = 0;
34 
35+    /** Whether this descriptor has all information about signing (ignoring private keys). */

ryanofsky commented at 6:36 pm on October 16, 2018:

Would it be useful to mention that raw and address descriptors are always considered unsolvable? It seems like this would make the description more concrete, and make it clear that a raw P2PK descriptor is not considered solvable (for example).

sipa commented at 1:26 am on October 17, 2018:

Good idea.

in src/script/descriptor.h:37 in 002bdef765 outdated

31@@ -32,6 +32,9 @@ struct Descriptor {
32     /** Whether the expansion of this descriptor depends on the position. */
33     virtual bool IsRange() const = 0;
34 
35+    /** Whether this descriptor has all information about signing (ignoring private keys). */
36+    virtual bool IsSolvable() const = 0;

ryanofsky commented at 6:36 pm on October 16, 2018:

Curious where this might be used in the future, since right now it is only called by test code.

sipa commented at 1:28 am on October 17, 2018:

For example an descriptorprocesspsbt RPC can complain if a non-solvable descriptor is passed (which is useless for its purpose).

in src/test/descriptor_tests.cpp:105 in 002bdef765 outdated

 98@@ -99,8 +99,17 @@ void Check(const std::string& prv, const std::string& pub, int flags, const std:
 99                     spend.vout.resize(1);
100                     BOOST_CHECK_MESSAGE(SignSignature(Merge(keys_priv, script_provider), spks[n], spend, 0, 1, SIGHASH_ALL), prv);
101                 }
102-            }
103 
104+                /* Infer a descriptor from the generated script, and verify its solvability and that it roundtrips. */
105+                auto inferred = InferDescriptor(spks[n], script_provider);
106+                BOOST_CHECK_EQUAL(!(flags & UNSOLVABLE), inferred->IsSolvable());

ryanofsky commented at 6:42 pm on October 16, 2018:

This is a very picky comment, but maybe reverse the arguments so the test more consistently uses the CHECK_EQUAL(actual, expected) form.

sipa commented at 1:29 am on October 17, 2018:

Done.

ryanofsky approved

ryanofsky commented at 6:59 pm on October 16, 2018: member

utACK 002bdef765a7cdb4dcf66fa186a4c4a1a7fc7b71

sipa force-pushed on Oct 17, 2018

sipa commented at 1:35 am on October 17, 2018: member

Addressed all nits, and added some comments.

sipa force-pushed on Oct 17, 2018

Sjors commented at 9:05 am on October 17, 2018: member

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?

in test/functional/wallet_address_types.py:271 in a5e0b62006 outdated

266+                for utxo in self.nodes[to_node].listunspent():
267+                    if utxo['address'] == descriptors[to_node][0]:
268+                        assert_equal(utxo['descriptor'], descriptors[to_node][1])
269+                        found = True
270+                        break
271+                assert(found)

promag commented at 2:31 pm on October 17, 2018:

assert found

sipa commented at 1:43 am on October 19, 2018:

Fixed.

in src/wallet/rpcwallet.cpp:23 in a5e0b62006 outdated

20@@ -21,6 +21,7 @@
21 #include <rpc/server.h>
22 #include <rpc/util.h>
23 #include <script/sign.h>
24+#include <script/descriptor.h>

promag commented at 2:34 pm on October 17, 2018:

nit sort.

sipa commented at 1:43 am on October 19, 2018:

Fixed.

promag commented at 2:38 pm on October 17, 2018: member

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

ryanofsky commented at 5:48 pm on October 17, 2018: member

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 force-pushed on Oct 19, 2018

sipa commented at 1:09 am on October 19, 2018: member

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 force-pushed on Oct 19, 2018

in src/wallet/rpcwallet.cpp:3548 in cf4b935a03 outdated

3544@@ -3539,6 +3545,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3545             "  \"scriptPubKey\" : \"hex\",       (string) The hex-encoded scriptPubKey generated by the address\n"
3546             "  \"ismine\" : true|false,        (boolean) If the address is yours or not\n"
3547             "  \"iswatchonly\" : true|false,   (boolean) If the address is watchonly\n"
3548+            "  \"issolvable\" : true|false,    (boolean) Whether we know how to spend coins sent to this address, ignoring the possible lack of private keys\n"

ryanofsky commented at 4:42 pm on October 19, 2018:

I think it would be nicer to call this “solvable” instead of “issolvable” for consistency with listunspent. This would be less consistent with other fields here like “iswatchonly”, but I think it’s worse to have different names for the same field than different styles of names for different fields.

sipa commented at 11:03 pm on October 19, 2018:

Done.

in test/functional/wallet_address_types.py:112 in cf4b935a03 outdated

107         if not multisig and typ == 'legacy':
108             # P2PKH
109             assert(not info['isscript'])
110             assert(not info['iswitness'])
111             assert('pubkey' in info)
112+            assert_equal(self.remove_origin(info['descriptor']), "pkh(%s)" % info['pubkey'])

ryanofsky commented at 4:44 pm on October 19, 2018:

It seems like it’d be good to check inferred origins instead of skipping them. If it isn’t easy to do this, maybe you could add a comment to the test saying why.

sipa commented at 11:05 pm on October 19, 2018:

Done, rewritten the test to compare with information put in PSBT (which also reports fingerprint and path in decodepsbt).

ryanofsky commented at 4:46 pm on October 19, 2018: member

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.

sipa force-pushed on Oct 19, 2018

DrahtBot commented at 10:00 am on October 20, 2018: member

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 force-pushed on Oct 21, 2018

DrahtBot added the label Needs rebase on Oct 21, 2018

in test/functional/wallet_address_types.py:102 in a5bcc624f5 outdated

 98@@ -99,6 +99,8 @@ def test_address(self, node, address, multisig, typ):
 99         """Run sanity checks on an address."""
100         info = self.nodes[node].getaddressinfo(address)
101         assert(self.nodes[node].validateaddress(address)['isvalid'])
102+        assert('solvable' in info and info['solvable'])

ryanofsky commented at 9:35 pm on October 22, 2018:

Minor python style suggestion: You can usually avoid the need to look up keys multiple times using .get(). For example, you could write this check as

0assert_equals(info.get('solvable'), True)

sipa commented at 7:00 pm on October 30, 2018:

Sorry I missed this, but I’d prefer not rewriting commits now.

sipa commented at 10:28 pm on November 14, 2018:

Fixed.

sipa force-pushed on Oct 22, 2018

DrahtBot removed the label Needs rebase on Oct 22, 2018

Sjors commented at 10:33 am on October 23, 2018: member

Lightly tested e6e4704; getaddressinfo descriptor now includes origin info.

ryanofsky approved

ryanofsky commented at 8:36 pm on October 23, 2018: member

utACK e6e47041a70d48f53dd2016cd5ae1cd822c5d3c6. 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.

in src/script/descriptor.cpp:635 in 49b93aa4a5 outdated

624@@ -625,6 +625,80 @@ std::unique_ptr<Descriptor> ParseScript(Span<const char>& sp, ParseScriptContext
625     return nullptr;
626 }
627 
628+std::unique_ptr<PubkeyProvider> InferPubkey(const CPubKey& pubkey, ParseScriptContext, const SigningProvider& provider)

meshcollider commented at 8:16 am on October 25, 2018:

What is the point of the ParseScriptContext if it is never used? Why not just remove it?

sipa commented at 7:02 pm on October 30, 2018:

It can be used for extra sanity checking (like rejecting malformed constructions with an uncompressed pubkey inside a witness) early, instead of having them fail when expanding the inferred descriptor.

promag commented at 6:50 pm on November 4, 2018:

Could add that note to the code otherwise it can be removed in the future?

in src/wallet/rpcwallet.cpp:2715 in ea3518d078 outdated

2832@@ -2831,6 +2833,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
2833         CTxDestination address;
2834         const CScript& scriptPubKey = out.tx->tx->vout[out.i].scriptPubKey;
2835         bool fValidAddress = ExtractDestination(scriptPubKey, address);
2836+        auto descriptor = InferDescriptor(scriptPubKey, *pwallet);

meshcollider commented at 8:18 am on October 25, 2018:

Why not just put the InferDescriptor() part in the if-statement below like you did for getaddressinfo? descriptor is not used otherwise

sipa commented at 10:28 pm on November 14, 2018:

Fixed.

meshcollider commented at 8:21 am on October 25, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14477/commits/e6e47041a70d48f53dd2016cd5ae1cd822c5d3c6

Add support for inferring descriptors from scripts 4d78bd93b5

Add Descriptor::IsSolvable() to distinguish addr/raw from others 225bf3e3b0

Add tests for InferDescriptor and Descriptor::IsSolvable 9b2a25b13f

sipa force-pushed on Oct 26, 2018

sipa commented at 5:41 pm on October 26, 2018: member

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 force-pushed on Oct 29, 2018

meshcollider commented at 6:25 am on October 29, 2018: contributor

re-utACK https://github.com/bitcoin/bitcoin/pull/14477/commits/fe9b68a81a6f3a9abac7b3b7740d5019129d4fff

Sjors commented at 12:27 pm on October 30, 2018: member

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.

in src/rpc/blockchain.cpp:2146 in fe9b68a81a outdated

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

ryanofsky commented at 6:00 pm on October 30, 2018:

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.

sipa commented at 7:07 pm on October 30, 2018:

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.

practicalswift commented at 4:41 pm on November 12, 2018:

Nit: Use another variable name to avoid showing desc declared on line 2133 (auto desc = Parse(desc_str, provider);)?

sipa commented at 10:28 pm on November 14, 2018:

Fixed.

ryanofsky approved

ryanofsky commented at 6:04 pm on October 30, 2018: member

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

sipa added this to the "Blockers" column in a project

in src/rpc/blockchain.cpp:2145 in 2286f206e4 outdated

2141@@ -2140,7 +2142,11 @@ UniValue scantxoutset(const JSONRPCRequest& request)
2142                 if (!desc->Expand(i, provider, scripts, provider)) {
2143                     throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys: '%s'", desc_str));
2144                 }
2145-                needles.insert(scripts.begin(), scripts.end());
2146+                for (const auto script : scripts) {

promag commented at 6:47 pm on November 4, 2018:

Could be const auto&?

sipa commented at 10:28 pm on November 14, 2018:

Fixed.

promag commented at 6:51 pm on November 4, 2018: member

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

laanwj removed this from the "Blockers" column in a project

Add descriptors to listunspent and getaddressinfo + tests 16203d5df7

Add matching descriptors to scantxoutset output + tests b65326b562

Add release notes 109699dd33

sipa force-pushed on Nov 14, 2018

laanwj commented at 12:07 pm on November 23, 2018: member

utACK 109699dd33c72539fc33619f7836b8088f63182c

ryanofsky approved

ryanofsky commented at 7:08 pm on November 27, 2018: member

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

sipa merged this on Nov 27, 2018

sipa closed this on Nov 27, 2018

sipa referenced this in commit fdf146f329 on Nov 27, 2018

deadalnix referenced this in commit 7f84dccb83 on May 18, 2020

deadalnix referenced this in commit f011dbcfa1 on May 18, 2020

deadalnix referenced this in commit 09a1fe4bbe on May 18, 2020

deadalnix referenced this in commit 3192309564 on May 19, 2020

deadalnix referenced this in commit 4b8b63a7c9 on May 19, 2020

deadalnix referenced this in commit 4de8d18b13 on May 19, 2020

ftrader referenced this in commit bc0d97cf6c on Aug 17, 2020

ftrader referenced this in commit 716225bf90 on Aug 17, 2020

Munkybooty referenced this in commit 169a7809cc on Jul 30, 2021

Munkybooty referenced this in commit b939c0968a on Jul 30, 2021

christiancfifi referenced this in commit a0852942d3 on Aug 25, 2021

christiancfifi referenced this in commit 980183457f on Aug 25, 2021

christiancfifi referenced this in commit f0725aebf1 on Aug 25, 2021

DrahtBot locked this on Sep 8, 2021

Add ability to convert solvability info to descriptor #14477

Conflicts