As promised, here is an implementation of my output descriptor concept (https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82) and integration within the scantxoutset RPC that was just added through #12196.
It changes the RPC to use descriptors for everything; I hope the interface is simple enough to encompass all use cases. It includes support for P2PK, P2PKH, P2WPKH, P2SH, P2WSH, multisig, xpubs, xprvs, and chains of keys - combined in every possible way.
0 | @@ -0,0 +1,615 @@ 1 | +#include <script/descriptor.h>
~Missing copyright block~
Fixed.
35 | + std::map<CKeyID, CKey> keys; 36 | + 37 | + bool GetCScript(const CScriptID& scriptid, CScript& script) const override; 38 | + bool GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const override; 39 | + bool GetKey(const CKeyID& keyid, CKey& key) const override; 40 | +
nit, remove empty line.
Done.
38 | + bool GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const override; 39 | + bool GetKey(const CKeyID& keyid, CKey& key) const override; 40 | + 41 | +}; 42 | + 43 | +FlatSigningProvider Merge(const FlatSigningProvider& a, const FlatSigningProvider& b);
At the moment this is only used in tests, move there?
I'd rather not. I think more things will be able to use this.
77 | + if (!m_provider->ToPrivateString(arg, sub)) return false; 78 | + ret = OriginString() + ":" + std::move(sub); 79 | + return true; 80 | + } 81 | +}; 82 | +#endif
nit, add // KEY_ORIGIN_SUPPORT.
Done.
24 | +//////////////////////////////////////////////////////////////////////////// 25 | + 26 | +typedef std::vector<uint32_t> KeyPath; 27 | + 28 | +/** Interface for public key objects in descriptors. */ 29 | +struct PubkeyProvider {
nit, { in new line.
Done.
25 | + 26 | +typedef std::vector<uint32_t> KeyPath; 27 | + 28 | +/** Interface for public key objects in descriptors. */ 29 | +struct PubkeyProvider { 30 | + virtual ~PubkeyProvider(){};
nit, = default?
Done, here and elsewhere.
Few nits, it this for 0.17? Is scantxoutset considered API experimental?
452 | +std::vector<Span<const char>> Split(const Span<const char>& sp, char sep) 453 | +{ 454 | + std::vector<Span<const char>> ret; 455 | + auto it = sp.begin(); 456 | + auto start = it; 457 | + while (it != sp.end()) {
Add
if (it == sp.end()) return ret;
for when string is empty.
Why?
The statement ret.emplace_back(start, it); below should not be executed if sp is empty.
I mean, should Split("", sep) equal []?
No, it should be [""]. Callers can rely on there always being at least one argument in the result.
In that case 👍
612 | + 613 | +std::unique_ptr<Descriptor> Parse(const std::string& descriptor, FlatSigningProvider& out) 614 | +{ 615 | + Span<const char> sp(descriptor.data(), descriptor.size()); 616 | + auto ret = ParseScript(sp, ParseScriptContext::TOP, out); 617 | + if (sp.size() == 0 && ret) return std::move(ret);
Compiler warning:
script/descriptor.cpp:617:39: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
if (sp.size() == 0 && ret) return std::move(ret);
^
script/descriptor.cpp:617:39: note: remove std::move call here
if (sp.size() == 0 && ret) return std::move(ret);
^~~~~~~~~~
Fixed.
2055 | - " \"pubkey\" : \"<pubkey\">, (string, required) HEX encoded public key\n" 2056 | - " \"script_types\" : [ ... ], (array, optional) Array of script-types to derive from the pubkey (possible values: \"P2PK\", \"P2PKH\", \"P2SH-P2WPKH\", \"P2WPKH\")\n" 2057 | - " }\n" 2058 | + "2. \"scanobjects\" (array, required) Array of scan objects\n" 2059 | + " [ Every scan object is either a string descriptor or an object:\n" 2060 | + " \"descriptor\", (string, optional) An output descriptor\n"
Why give both options? [{ "desc": "...", ...}, ...] is enough?
It's pretty verbose to use the long form, and probably not all that often needed.
It's pretty verbose
On the client side I think it doesn't matter (unless it's a human making the call).
Yes, the reason to support the short notation is to simplify things for humans.
2115 | + int range = 1000; 2116 | + if (scanobject.isStr()) { 2117 | + desc_str = scanobject.get_str(); 2118 | + } else if (scanobject.isObject()) { 2119 | + UniValue desc_uni = find_value(scanobject, "desc"); 2120 | + if (desc_uni.isNull()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Descriptor needs to be provided in scan object");
Could have a test.
Yes, will add when there's sufficient agreement about RPC.
2120 | + if (desc_uni.isNull()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Descriptor needs to be provided in scan object"); 2121 | + desc_str = desc_uni.get_str(); 2122 | + UniValue range_uni = find_value(scanobject, "range"); 2123 | + if (!range_uni.isNull()) { 2124 | + range = range_uni.get_int(); 2125 | + if (range < 1 || range > 1000000) throw JSONRPCError(RPC_INVALID_PARAMETER, "range out of range");
Could have a test.
548 | +{ 549 | + auto expr = Expr(sp); 550 | + if (Func("pk", expr)) { 551 | + auto pubkey = ParsePubkey(expr, ctx != ParseScriptContext::P2WSH, out); 552 | + if (pubkey) return MakeUnique<SingleKeyDescriptor>(std::move(pubkey), P2PKGetScript, "pk"); 553 | + }
Why not return nullptr in these cases?
Done, here and elsewhere.
95 | +}; 96 | + 97 | +/** Parse a descriptor string. Included private keys are put in out. Returns nullptr if parsing fails. */ 98 | +std::unique_ptr<Descriptor> Parse(const std::string& descriptor, FlatSigningProvider& out); 99 | + 100 | +#endif
#endif // BITCOIN_SCRIPT_DESCRIPTOR_H
Done.
184 | + bool ToPrivateString(const SigningProvider& arg, std::string& out) const override 185 | + { 186 | + CExtKey key; 187 | + if (!GetExtKey(arg, key)) return false; 188 | + out = EncodeExtKey(key); 189 | + for (auto entry : m_path) {
Can the path writing be separated out into another function to avoid code duplication and so that we can use it elsewhere?
Fixed.
Rebased, removed future "key origin" support (not needed right now, ignore).
Also, I'd like people to first review the descriptor language itself (a very short summary is in the RPC help, more of a reference in script/descriptor.h, and a design document in https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82). In particular, I'm not convinced about the "old" function (which maps pubkeys to P2PK,P2PKH,P2WPKH, and P2SH-PWPKH) - it's very useful for dealing with existing Core wallets, but "old" doesn't really convey the right meaning. However, I don't see what else to use. "legacy" is confusing (as it would have a different meaning than legacy in -addresstype). "default" sounds like it could change over time (the intention is that it wouldn't). "sk" or "singlekey" sounds like it's encouraged and/or could change over time, ...
Concept ACK
Regarding the "old" function, I think it could receive the version, for instance default(args..., version) — it should validate if args are compatible with the version specified (at the moment args should be only a pubkey).
@promag There is no intention of ever adding another of those "defaults". Future versions should work with specific derivations (only P2WSH directly for example, rather than a collection). It's only for compatibility with the backward way we used to derive what scriptPubKeys are ours that such a default is useful.
2062 | + " \"desc\": \"descriptor\", (string, required) An output descriptor\n" 2063 | + " \"range\": n, (numeric, optional) Up to what child index HD chains should be explored (default: 1000)\n" 2064 | " },\n" 2065 | - " ]\n" 2066 | + " ...\n" 2067 | + " ]\n"
Don't forget to add a command-line example here, it look me a while to figure out the syntax from reading the above.
I suggest an example with a typical BIP44 (legacy) xpub + BIP49 xpub:
["pkh(legacy_xpub/0/*)", "pkh(legacy_xpub/1/*)", "sh(wpkh(segwit_xpub/0/*)", "sh(wpkh(segwit_xpub/1/*)"]
A second example could show the root master xpriv and then do hardened derivation of xpriv/44'/0'/0'/0/* or xpriv/49'/0'/0'/1/*
Done.
2033 | + "\nScans the unspent transaction output set for entries that match certain output descriptors.\n" 2034 | + "Examples of output descriptors are:\n" 2035 | + " addr(<address>) Outputs whose scriptPubKey corresponds to the specified address (does not include P2PK)\n" 2036 | + " raw(<hex script>) Outputs whose scriptPubKey equals the specified hex scripts\n" 2037 | + " old(<pubkey>) P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH outputs for the given pubkey\n" 2038 | + " pkh(<pubkey>) P2PKH outputs for the given pubkey\n"
sh(wpkh()) is probably quite common, so maybe add that here?
Done.
Concept ACK, and lightly tested on Ledger (P2SH-P2WPKH) and Blockchain (P2PKH) xpubs.
<!--e57a25ab6845829454e8d69fc972939a-->No more conflicts as of last run.
@sipa since you write that old is a "combination of P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH for public key P.", you could call it combo instead. Or ck if you want it to look more like the other things that use P. And potentially prepend bc to indicate it's for backwards compatibility (but in that case someone should check urban dictionary to make sure bcck isn't some newfangled swear word :-))
Travis failure:
test_framework.authproxy.JSONRPCException: Invalid descriptor 'old(03abcaca414970a8d36624011628045855cb01cded780445a33688a0b905a47224)' (-5)
First two findings:
I think supporting h as equivalent to ' in extended key derivation range would be great (JSON / shell / GUI). I suggest pulling in https://github.com/bitcoin/bitcoin/commit/203b4c94ce83981b0d76dd2adb778719ba868db1.
I think the IsHardened() function is not correct. Using 0'/0'/* will derive all hardened keys at the * level (same as 0'/0'/*'). Seems to be currently impossible to derive unhardened key with the asterisk.
2145 | + auto desc = Parse(desc_str, provider); 2146 | + if (!desc) { 2147 | + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor '%s'", desc_str)); 2148 | + } 2149 | + if (!desc->IsRange()) range = 1; 2150 | + for (int i = 0; i < range; ++i) {
Seems like that results in an off-by-one issue (misses last key): Suggest pulling fix: https://github.com/bitcoin/bitcoin/commit/875431149745cc517350c0620baa887c748d0b35
BIP32 indexes are 0-based, so I think the current code is correct. If you expand 1000 keys, those will be indices 0 through 999.
Additional xpub range tests can be pulled from here: https://github.com/bitcoin/bitcoin/commit/cc1ffb324b53855f7eae81a6b0df10bd677ca1a5
+1 on h alias
I only tested unhardened derivation (xpub/0/*) because most wallets only export account level xpubs. @jonasschnelli maybe add a test for the '/*) pattern to cc1ffb3? It currently only checks '/*')
@Sjors: A test for '/* would currently fail,... but it's simple to add (keys are already prepared in
https://github.com/bitcoin/bitcoin/commit/cc1ffb324b53855f7eae81a6b0df10bd677ca1a5), just waiting for a fix for the hardened/non-hardened mix.
Suggest pulling in the experimental warning for scantxoutset here: https://github.com/bitcoin/bitcoin/commit/3ec0958ea746d0db5edefff9f423cd2caab32857
@jonasschnelli Good catch; the derivation for "/*" with a parent hardened step is incorrect. Fixing.
This introduces a rudimentary begin(), end(), operator[], and subspan to Span.
@jonasschnelli Ok, fixed the derivation, changed the range to be max index rather than count, included your tests and experimental warning, and added a number of tests myself (generated with an independent BIP32 implementation).
@jonasschnelli Added support for h instead of ' in hardened descriptor paths as well (plus some tests for it). Serializing a descriptor will always use '.
Tested ACK f6b7fc349ccf9cfbeb7e91e19c20e2a2fcc9026f
Concept ACK f6b7fc3
utACK f6b7fc349ccf9cfbeb7e91e19c20e2a2fcc9026f
83 | + assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/1)"])['total_amount'], Decimal("8.192")) 84 | + assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/1500)"])['total_amount'], Decimal("16.384")) 85 | + assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/0)"])['total_amount'], Decimal("4.096")) 86 | + assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1)"])['total_amount'], Decimal("8.192")) 87 | + assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1500)"])['total_amount'], Decimal("16.384")) 88 | + assert_equal(self.nodes[0].scantxoutset("start", [ {"desc": "combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/*')", "range": 1499}])['total_amount'], Decimal("1.536"))
Maybe use range: 1 and range: 2 everywhere to speed up these tests?
Milestone
0.17.0