Get the OutputType for a descriptor #18034

achow101 · 2020-01-31T00:12:10Z

Adds a GetOutputType() method to get the OutputType of a descriptor. Some descriptors don't have a determinate OutputType, so we actually use an Optional<OutputType>. For descriptors with indeterminate OutputType, we return nullopt.

addr() and raw() use OutputTypes as determined by the CTxDestination they have. For simplicity, ScriptHash destinations are LEGACY even though they could be P2SH_SEGWIT.
combo(), pk(), and multi() are nullopt as they either don't have an OutputType or they have multiple. DescriptorImpl defaults to nullopt.
pkh() is LEGACY as expected
wpkh() and wsh() are BECH32 as expected.
sh() checks whether the sub-descriptor is BECH32. If so, it is P2SH_SEGWIT. Otherwise it is LEGACY.

The descriptor tests are updated to check the OutputType too.

DrahtBot · 2020-01-31T03:58:53Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

descriptors: Use xpub at last hardened step if possible #18163 (descriptors: Use xpub at last hardened step if possible by achow101)
build: Enable -Wsuggest-override if available #16710 (build: Enable -Wsuggest-override if available by hebasto)
External signer support - Wallet Box edition #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)

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.

promag · 2020-02-03T08:04:52Z

Do you have a use case for these?

src/script/descriptor.cpp

src/script/descriptor.h

Sjors

Concept ACK. After some discussion in #15590 I think adding GetOutputType() to Descriptor is preferable over the approach there of adding GetAddressType() and IsSegWit().

I don't like the use of ExtractDestination though. Let's just determine the output type from the descriptor itself similar to #15590. That also avoids the next problem:

For simplicity, ScriptHash destinations are LEGACY even though they could be P2SH_SEGWIT

This assumption breaks BIP49 descriptor import with HWI. See e.g. https://github.com/bitcoin/bitcoin/pull/16546/files#diff-b2bb174788c7409b671c46ccc86034bdR4359-R4378. This current uses GetAddressType() and IsSegWit(), and if I switched it to use GetOutputType() it would import the wrapped segwit descriptor as legacy.

src/script/descriptor.cpp

achow101 · 2020-02-10T17:33:37Z

I don't like the use of ExtractDestination though. Let's just determine the output type from the descriptor itself similar to #15590. That also avoids the next problem:

For simplicity, ScriptHash destinations are LEGACY even though they could be P2SH_SEGWIT

These only apply to addr() and raw() descriptors. The rest where all solving information is available do the correct thing.

Sjors

Code review ACK b4fcf7ad56d0981797a4424f38f0524275d26e59 modulo (an explanation for) the assert.

Sjors · 2020-02-10T18:13:42Z

src/script/descriptor.cpp

I don't mind, but why are you making m_name public?

It doesn't. The default visibility is private.

You should really see this change as moving m_subdescriptor_arg into the protected area rather than moving m_mame.

Oops, I misread that move. That makes sense.

However there's a warning:

script/descriptor.cpp:368:176: warning: field 'm_subdescriptor_arg' will be initialized after field 'm_name' [-Wreorder] DescriptorImpl(std::vector<std::unique_ptr<PubkeyProvider>> pubkeys, std::unique_ptr<DescriptorImpl> script, const std::string& name) : m_pubkey_args(std::move(pubkeys)), m_subdescriptor_arg(std::move(script)), m_name(name) {}

That should be fixed now.

src/script/descriptor.cpp

Sjors

Code review ACK 7e80f64

fjahr · 2020-02-19T00:00:39Z

ACK 7e80f64

Reviewed code, built and ran tests locally.

jonatack

ACK 7e80f64 code review/build/tests

instagibbs

For simplicity, ScriptHash destinations are LEGACY even though they could be P2SH_SEGWIT.

This is not readily apparent to me as a reviewer or reader of the code why it's that way?

instagibbs · 2020-02-20T19:34:59Z

src/script/descriptor.cpp

+    {
+        switch (m_destination.which()) {
+            case 1 /* PKHash */:
+            case 2 /* ScriptHash */: return OutputType::LEGACY;


motivation for having ScriptHash be "LEGACY" which I think means p2pkh in most contexts?

What else would it be?

I would consider just a P2SH scriptPubKey to be legacy if I had no information about the redeemScript.

ahhhh this is addr(), ignore this.

Sjors · 2020-02-20T20:03:06Z

@instagibbs I suppose the alternative is to treat addr() and raw() descriptors as null. It's probably a bad idea to import these things.

meshcollider · 2020-02-21T10:15:57Z

utACK 7e80f64

I guess the same kind of complaint could apply here too, that the descriptor implementation shouldn't really be the thing worrying about how the destinations are encoded. But that's okay. I assume there is a use-case, even though promag's question was never responded to? I still don't really see the point of this, could you just quickly explain where this will be used? I had a quick look at #16528 and it isn't immediately clear to me that this PR made things easier there vs doing things a different way

Sjors · 2020-02-21T10:30:44Z

@meshcollider I also use this to determine how to import descriptors obtained with HWI: https://github.com/Sjors/bitcoin/blob/2020/02/external_signer_scriptpubkeyman/src/wallet/wallet.cpp#L4460-L4474 (rebase WIP of #16546 which previously used #15590)

achow101 · 2020-02-21T16:15:51Z

@meshcollider The use case is that in #16528, we need to be able to determine the OutputType for a descriptor so that we can assign it the correct slot in m_external_spk_managers/m_internal_spk_managers. The point is that we can tell what kind of address it will produce and automatically make it so that doing getnewaddress will give the correct address type from the correct descriptor.

And I guess @Sjors is using it for something.

instagibbs · 2020-02-21T16:48:31Z

cursory ACK 7e80f64

7e80f64 Get the OutputType for a descriptor (Andrew Chow) Pull request description: Adds a `GetOutputType()` method to get the OutputType of a descriptor. Some descriptors don't have a determinate OutputType, so we actually use an `Optional<OutputType>`. For descriptors with indeterminate OutputType, we return `nullopt`. `addr()` and `raw()` use OutputTypes as determined by the CTxDestination they have. For simplicity, `ScriptHash` destinations are `LEGACY` even though they could be `P2SH_SEGWIT`. `combo()`, `pk()`, and `multi()` are `nullopt` as they either don't have an OutputType or they have multiple. `DescriptorImpl` defaults to `nullopt`. `pkh()` is `LEGACY` as expected `wpkh()` and `wsh()` are `BECH32` as expected. `sh()` checks whether the sub-descriptor is `BECH32`. If so, it is `P2SH_SEGWIT`. Otherwise it is `LEGACY`. The descriptor tests are updated to check the OutputType too. ACKs for top commit: fjahr: ACK 7e80f64 meshcollider: utACK 7e80f64 instagibbs: cursory ACK bitcoin@7e80f64 Sjors: Code review ACK 7e80f64 jonatack: ACK 7e80f64 code review/build/tests Tree-SHA512: c5a813447b62e982435e1c948066f8d6c148c9ebffb0a5eb5a9028b173b01d5ead2f076a5ca3f7f37698538baa346f82a977ee48f583d89cb4e5ebd9111b2341

Summary: Backport of Core [[bitcoin/bitcoin#18034 | PR18034]] Test Plan: ninja all check Reviewers: #bitcoin_abc, Fabien, majcosta Reviewed By: #bitcoin_abc, Fabien, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8395

fanquake added the Descriptors label Jan 31, 2020

promag reviewed Feb 5, 2020

View reviewed changes

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

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

src/script/descriptor.h Outdated Show resolved Hide resolved

Sjors mentioned this pull request Feb 10, 2020

Descriptor: add GetAddressType() and IsSegWit() #15590

Closed

Sjors reviewed Feb 10, 2020

View reviewed changes

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

achow101 force-pushed the desc-getoutputtype branch from db1e861 to b4fcf7a Compare February 10, 2020 17:32

Sjors suggested changes Feb 10, 2020

View reviewed changes

achow101 force-pushed the desc-getoutputtype branch from b4fcf7a to 079862d Compare February 10, 2020 19:17

Get the OutputType for a descriptor

7e80f64

achow101 force-pushed the desc-getoutputtype branch from 079862d to 7e80f64 Compare February 11, 2020 18:24

Sjors approved these changes Feb 11, 2020

View reviewed changes

This was referenced Feb 11, 2020

build: Enable -Wsuggest-override if available #16710

Merged

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

descriptors: Use xpub at last hardened step if possible #18163

Closed

jonatack reviewed Feb 20, 2020

View reviewed changes

instagibbs reviewed Feb 20, 2020

View reviewed changes

meshcollider merged commit 9dd7bd4 into bitcoin:master Feb 21, 2020

DrahtBot mentioned this pull request Feb 21, 2020

External signer support - Wallet Box edition #16546

Merged

2 tasks

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Get the OutputType for a descriptor #18034

Get the OutputType for a descriptor #18034

Uh oh!

Conversation

achow101 commented Jan 31, 2020

Uh oh!

DrahtBot commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Feb 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Feb 10, 2020

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Feb 19, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 20, 2020

Uh oh!

meshcollider commented Feb 21, 2020

Uh oh!

Sjors commented Feb 21, 2020

Uh oh!

achow101 commented Feb 21, 2020

Uh oh!

instagibbs commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Jan 31, 2020 •

edited

Loading

achow101 Feb 10, 2020 •

edited

Loading