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

Support output descriptors in scantxoutset #13697

Merged
merged 9 commits into from Aug 1, 2018

Conversation

Projects
None yet
@sipa
Copy link
Member

commented Jul 17, 2018

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 +1,615 @@
#include <script/descriptor.h>

This comment has been minimized.

Copy link
@Empact

Empact Jul 18, 2018

Member

Missing copyright block

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Fixed.

@sipa sipa force-pushed the sipa:201806_minedesc branch Jul 18, 2018

@laanwj laanwj added the RPC/REST/ZMQ label Jul 18, 2018

@promag
Copy link
Member

left a comment

Few nits, it this for 0.17? Is scantxoutset considered API experimental?

src/script/sign.h Outdated
bool GetCScript(const CScriptID& scriptid, CScript& script) const override;
bool GetPubKey(const CKeyID& keyid, CPubKey& pubkey) const override;
bool GetKey(const CKeyID& keyid, CKey& key) const override;

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

nit, remove empty line.

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Done.

src/script/sign.h Outdated

};

FlatSigningProvider Merge(const FlatSigningProvider& a, const FlatSigningProvider& b);

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

At the moment this is only used in tests, move there?

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

I'd rather not. I think more things will be able to use this.

src/script/descriptor.cpp Outdated
return true;
}
};
#endif

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

nit, add // KEY_ORIGIN_SUPPORT.

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Done.

src/script/descriptor.cpp Outdated
typedef std::vector<uint32_t> KeyPath;

/** Interface for public key objects in descriptors. */
struct PubkeyProvider {

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

nit, { in new line.

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Done.

src/script/descriptor.cpp Outdated

/** Interface for public key objects in descriptors. */
struct PubkeyProvider {
virtual ~PubkeyProvider(){};

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

nit, = default?

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Done, here and elsewhere.

std::vector<Span<const char>> ret;
auto it = sp.begin();
auto start = it;
while (it != sp.end()) {

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

Add

if (it == sp.end()) return ret;

for when string is empty.

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Why?

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

The statement ret.emplace_back(start, it); below should not be executed if sp is empty.

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

I mean, should Split("", sep) equal []?

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

No, it should be [""]. Callers can rely on there always being at least one argument in the result.

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

In that case 👍

src/script/descriptor.cpp Outdated
{
Span<const char> sp(descriptor.data(), descriptor.size());
auto ret = ParseScript(sp, ParseScriptContext::TOP, out);
if (sp.size() == 0 && ret) return std::move(ret);

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

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);
                                      ^~~~~~~~~~  

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Fixed.

" }\n"
"2. \"scanobjects\" (array, required) Array of scan objects\n"
" [ Every scan object is either a string descriptor or an object:\n"
" \"descriptor\", (string, optional) An output descriptor\n"

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

Why give both options? [{ "desc": "...", ...}, ...] is enough?

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

It's pretty verbose to use the long form, and probably not all that often needed.

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

It's pretty verbose

On the client side I think it doesn't matter (unless it's a human making the call).

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Yes, the reason to support the short notation is to simplify things for humans.

desc_str = scanobject.get_str();
} else if (scanobject.isObject()) {
UniValue desc_uni = find_value(scanobject, "desc");
if (desc_uni.isNull()) throw JSONRPCError(RPC_INVALID_PARAMETER, "Descriptor needs to be provided in scan object");

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

Could have a test.

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Yes, will add when there's sufficient agreement about RPC.

src/rpc/blockchain.cpp Outdated
UniValue range_uni = find_value(scanobject, "range");
if (!range_uni.isNull()) {
range = range_uni.get_int();
if (range < 1 || range > 1000000) throw JSONRPCError(RPC_INVALID_PARAMETER, "range out of range");

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

Could have a test.

src/script/descriptor.cpp Outdated
if (Func("pk", expr)) {
auto pubkey = ParsePubkey(expr, ctx != ParseScriptContext::P2WSH, out);
if (pubkey) return MakeUnique<SingleKeyDescriptor>(std::move(pubkey), P2PKGetScript, "pk");
}

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member

Why not return nullptr in these cases?

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Done, here and elsewhere.

src/script/descriptor.h Outdated
/** Parse a descriptor string. Included private keys are put in out. Returns nullptr if parsing fails. */
std::unique_ptr<Descriptor> Parse(const std::string& descriptor, FlatSigningProvider& out);

#endif

This comment has been minimized.

Copy link
@promag

promag Jul 18, 2018

Member
#endif // BITCOIN_SCRIPT_DESCRIPTOR_H

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Done.

@sipa sipa force-pushed the sipa:201806_minedesc branch to 4a5cd30 Jul 18, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

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, ...

@achow101
Copy link
Member

left a comment

Concept ACK

src/script/descriptor.cpp Outdated
CExtKey key;
if (!GetExtKey(arg, key)) return false;
out = EncodeExtKey(key);
for (auto entry : m_path) {

This comment has been minimized.

Copy link
@achow101

achow101 Jul 18, 2018

Member

Can the path writing be separated out into another function to avoid code duplication and so that we can use it elsewhere?

This comment has been minimized.

Copy link
@sipa

sipa Jul 18, 2018

Author Member

Fixed.

@promag

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

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).

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

@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.

@sipa sipa force-pushed the sipa:201806_minedesc branch 2 times, most recently Jul 18, 2018

@laanwj laanwj added this to the 0.17.0 milestone Jul 19, 2018

@jonasschnelli jonasschnelli self-requested a review Jul 19, 2018

@sipa sipa force-pushed the sipa:201806_minedesc branch to 79b8939 Jul 20, 2018

@Sjors

Sjors approved these changes Jul 21, 2018

Copy link
Member

left a comment

Concept ACK, and lightly tested on Ledger (P2SH-P2WPKH) and Blockchain (P2PKH) xpubs.

" },\n"
" ]\n"
" ...\n"
" ]\n"

This comment has been minimized.

Copy link
@Sjors

Sjors Jul 21, 2018

Member

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/*

This comment has been minimized.

Copy link
@sipa

sipa Jul 21, 2018

Author Member

Done.

" addr(<address>) Outputs whose scriptPubKey corresponds to the specified address (does not include P2PK)\n"
" raw(<hex script>) Outputs whose scriptPubKey equals the specified hex scripts\n"
" old(<pubkey>) P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH outputs for the given pubkey\n"
" pkh(<pubkey>) P2PKH outputs for the given pubkey\n"

This comment has been minimized.

Copy link
@Sjors

Sjors Jul 21, 2018

Member

sh(wpkh()) is probably quite common, so maybe add that here?

This comment has been minimized.

Copy link
@sipa

sipa Jul 21, 2018

Author Member

Done.

@sipa sipa force-pushed the sipa:201806_minedesc branch 2 times, most recently Jul 21, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

No more conflicts as of last run.
@flack

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2018

@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 :-))

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2018

@flack Oh, I like that. Changed to "combo".

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

Travis failure:

test_framework.authproxy.JSONRPCException: Invalid descriptor 'old(03abcaca414970a8d36624011628045855cb01cded780445a33688a0b905a47224)' (-5)
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

Additional xpub range tests can be pulled from here: cc1ffb3

@Sjors

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

+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 '/*')

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

@Sjors: A test for '/* would currently fail,... but it's simple to add (keys are already prepared in
cc1ffb3), just waiting for a fix for the hardened/non-hardened mix.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

Suggest pulling in the experimental warning for scantxoutset here: 3ec0958

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2018

@jonasschnelli Good catch; the derivation for "/*" with a parent hardened step is incorrect. Fixing.

@sipa sipa force-pushed the sipa:201806_minedesc branch from 852cf1f to fddea67 Jul 27, 2018

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2018

@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).

@sipa

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2018

@jonasschnelli Added support for h instead of ' in hardened descriptor paths as well (plus some tests for it). Serializing a descriptor will always use '.

@jonasschnelli
Copy link
Member

left a comment

Tested ACK f6b7fc3

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

Concept ACK f6b7fc3

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

utACK f6b7fc3

@laanwj laanwj merged commit f6b7fc3 into bitcoin:master Aug 1, 2018

1 check passed

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

laanwj added a commit that referenced this pull request Aug 1, 2018

Merge #13697: Support output descriptors in scantxoutset
f6b7fc3 Support h instead of ' in hardened descriptor paths (Pieter Wuille)
fddea67 Add experimental warning to scantxoutset (Jonas Schnelli)
6495849 [QA] Extend tests to more combinations (Pieter Wuille)
1af237f [QA] Add xpub range tests in scantxoutset tests (Jonas Schnelli)
151600b Swap in descriptors support into scantxoutset (Pieter Wuille)
0652c32 Descriptor tests (Pieter Wuille)
fe8a7dc Output descriptors module (Pieter Wuille)
e54d760 Add simple FlatSigningProvider (Pieter Wuille)
29943a9 Add more methods to Span class (Pieter Wuille)

Pull request description:

  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.

Tree-SHA512: 63b54a96e7a72f5b04a8d645b8517d43ecd6a65a41f9f4e593931ce725a8845ab0baa1e9db6a7243190d8ac841f6e7e2f520d98c539312d78f7fd687d2c7b88f
assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/0)"])['total_amount'], Decimal("4.096"))
assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1)"])['total_amount'], Decimal("8.192"))
assert_equal(self.nodes[0].scantxoutset("start", [ "combo(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1500)"])['total_amount'], Decimal("16.384"))
assert_equal(self.nodes[0].scantxoutset("start", [ {"desc": "combo(tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/1/*')", "range": 1499}])['total_amount'], Decimal("1.536"))

This comment has been minimized.

Copy link
@Sjors

Sjors Aug 2, 2018

Member

Maybe use range: 1 and range: 2 everywhere to speed up these tests?

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 27, 2018

Merge bitcoin#14820: test: Fix descriptor_tests not checking ToString…
… output of public descriptors

c77f092 Fix descriptor_tests not checking ToString output of public descriptors (Russell Yanofsky)

Pull request description:

  This fixes a minor test bug introduced in bitcoin#13697 that I noticed while reviewing bitcoin#14646

Tree-SHA512: efed91200cdff5f86ba5de3461ac00759d285e2905f6cb24cea15d3e23e0581ce5fc14b24a40db093f7ebd662ee1ee2cf67f8798bac1903a78298eda08909cfb
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.