Get the OutputType for a descriptor #18034

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:desc-getoutputtype changing 3 files +89 −39
  1. achow101 commented at 12:12 AM on January 31, 2020: member

    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.

  2. fanquake added the label Descriptors on Jan 31, 2020
  3. DrahtBot commented at 3:58 AM on January 31, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18163 (descriptors: Use xpub at last hardened step if possible by achow101)
    • #16710 (build: Enable -Wsuggest-override if available by hebasto)
    • #16546 ([WIP] External signer support - Wallet Box edition by Sjors)
    • #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.

  4. promag commented at 8:04 AM on February 3, 2020: member

    Do you have a use case for these?

  5. in src/script/descriptor.cpp:521 in db1e861fda outdated 
     514 | @@ -512,6 +515,19 @@ class AddressDescriptor final : public DescriptorImpl
 515 |  public:
 516 |      AddressDescriptor(CTxDestination destination) : DescriptorImpl({}, {}, "addr"), m_destination(std::move(destination)) {}
 517 |      bool IsSolvable() const final { return false; }
 518 | +
 519 | +    Optional<OutputType> GetOutputType() const override
 520 | +    {
 521 | +        switch(m_destination.which()) {
    promag commented at 2:58 PM on February 5, 2020:

    nit, add space after switch, same below.

    achow101 commented at 5:32 PM on February 10, 2020:

    Done

  6. in src/script/descriptor.cpp:547 in db1e861fda outdated 
     539 | @@ -524,6 +540,21 @@ class RawDescriptor final : public DescriptorImpl
 540 |  public:
 541 |      RawDescriptor(CScript script) : DescriptorImpl({}, {}, "raw"), m_script(std::move(script)) {}
 542 |      bool IsSolvable() const final { return false; }
 543 | +
 544 | +    Optional<OutputType> GetOutputType() const override
 545 | +    {
 546 | +        CTxDestination dest;
 547 | +        ExtractDestination(m_script, dest);
    promag commented at 3:00 PM on February 5, 2020:

    Assert result?

    achow101 commented at 5:32 PM on February 10, 2020:

    Done

    achow101 commented at 7:19 PM on February 10, 2020:

    Actually reverted that. It is possible, and perhaps expected, that we get a script which causes ExtractDestination to fail. Such a script would be a CNoDestination.

  7. in src/script/descriptor.h:8 in db1e861fda outdated 
       4 | @@ -5,6 +5,8 @@
   5 |  #ifndef BITCOIN_SCRIPT_DESCRIPTOR_H
   6 |  #define BITCOIN_SCRIPT_DESCRIPTOR_H
   7 |  
   8 | +#include <outputtype.h>
    promag commented at 3:00 PM on February 5, 2020:

    nit sort.

    achow101 commented at 5:32 PM on February 10, 2020:

    Done

  8. in src/script/descriptor.cpp:549 in db1e861fda outdated 
     544 | +    Optional<OutputType> GetOutputType() const override
 545 | +    {
 546 | +        CTxDestination dest;
 547 | +        ExtractDestination(m_script, dest);
 548 | +        switch(dest.which()) {
 549 | +            case 1:
    Sjors commented at 1:22 PM on February 10, 2020:

    Please use / comment PKHash (etc) here for readability.

    achow101 commented at 5:32 PM on February 10, 2020:

    Done

  9. Sjors cross-referenced this on Feb 10, 2020 from issue Descriptor: add GetAddressType() and IsSegWit() by Sjors
  10. Sjors commented at 1:32 PM on February 10, 2020: member

    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.

  11. achow101 force-pushed on Feb 10, 2020
  12. achow101 commented at 5:33 PM on February 10, 2020: member

    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.

  13. in src/script/descriptor.cpp:343 in b4fcf7ad56 outdated 
     338 | @@ -339,14 +339,15 @@ class DescriptorImpl : public Descriptor
 339 |  {
 340 |      //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size for Multisig).
 341 |      const std::vector<std::unique_ptr<PubkeyProvider>> m_pubkey_args;
 342 | +    //! The string name of the descriptor function.
 343 | +    const std::string m_name;
    Sjors commented at 6:13 PM on February 10, 2020:

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

    achow101 commented at 6:25 PM on February 10, 2020:

    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.

    Sjors commented at 6:41 PM on February 10, 2020:

    Oops, I misread that move. That makes sense.

    Sjors commented at 10:31 AM on February 11, 2020:

    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) {}
    achow101 commented at 6:28 PM on February 11, 2020:

    That should be fixed now.

  14. in src/script/descriptor.cpp:547 in b4fcf7ad56 outdated 
     539 | @@ -524,6 +540,21 @@ class RawDescriptor final : public DescriptorImpl
 540 |  public:
 541 |      RawDescriptor(CScript script) : DescriptorImpl({}, {}, "raw"), m_script(std::move(script)) {}
 542 |      bool IsSolvable() const final { return false; }
 543 | +
 544 | +    Optional<OutputType> GetOutputType() const override
 545 | +    {
 546 | +        CTxDestination dest;
 547 | +        assert(ExtractDestination(m_script, dest));
    Sjors commented at 6:17 PM on February 10, 2020:

    This seems a bit aggressive, maybe return nullopt is ExtractDestination fails?

    achow101 commented at 7:18 PM on February 10, 2020:

    Reverted the change. There's no need to check the result of ExtractDestination because it is directly implied by the resulting CTxDestination. If it fails, that will be CNoDestination.

  15. Sjors changes_requested
  16. Sjors commented at 6:20 PM on February 10, 2020: member

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

  17. achow101 force-pushed on Feb 10, 2020
  18. Get the OutputType for a descriptor 7e80f646b2
  19. achow101 force-pushed on Feb 11, 2020
  20. Sjors approved
  21. Sjors commented at 6:30 PM on February 11, 2020: member

    Code review ACK 7e80f646b24a2abf3c031a649bcc706a695f80da

  22. DrahtBot cross-referenced this on Feb 11, 2020 from issue build: Enable -Wsuggest-override if available by hebasto
  23. DrahtBot cross-referenced this on Feb 11, 2020 from issue Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101
  24. laanwj added this to the "Blockers" column in a project
  25. DrahtBot cross-referenced this on Feb 17, 2020 from issue descriptors: Use xpub at last hardened step if possible by achow101
  26. fjahr commented at 12:00 AM on February 19, 2020: contributor

    ACK 7e80f646b24a2abf3c031a649bcc706a695f80da

    Reviewed code, built and ran tests locally.

  27. jonatack commented at 7:13 PM on February 20, 2020: contributor

    ACK 7e80f64 code review/build/tests

  28. in src/script/descriptor.cpp:523 in 7e80f646b2 
     514 | @@ -512,6 +515,19 @@ class AddressDescriptor final : public DescriptorImpl
 515 |  public:
 516 |      AddressDescriptor(CTxDestination destination) : DescriptorImpl({}, {}, "addr"), m_destination(std::move(destination)) {}
 517 |      bool IsSolvable() const final { return false; }
 518 | +
 519 | +    Optional<OutputType> GetOutputType() const override
 520 | +    {
 521 | +        switch (m_destination.which()) {
 522 | +            case 1 /* PKHash */:
 523 | +            case 2 /* ScriptHash */: return OutputType::LEGACY;
    instagibbs commented at 7:34 PM on February 20, 2020:

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

    achow101 commented at 6:54 AM on February 21, 2020:

    What else would it be?

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

    instagibbs commented at 4:47 PM on February 21, 2020:

    ahhhh this is addr(), ignore this.

  29. instagibbs commented at 7:36 PM on February 20, 2020: member

    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?

  30. Sjors commented at 8:03 PM on February 20, 2020: member

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

  31. meshcollider commented at 10:15 AM on February 21, 2020: contributor

    utACK 7e80f646b24a2abf3c031a649bcc706a695f80da

    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

  32. Sjors commented at 10:30 AM on February 21, 2020: member

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

  33. achow101 commented at 4:15 PM on February 21, 2020: member

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

  35. meshcollider merged this on Feb 21, 2020
  36. meshcollider closed this on Feb 21, 2020
  37. DrahtBot cross-referenced this on Feb 21, 2020 from issue External signer support - Wallet Box edition by Sjors
  38. fanquake removed this from the "Blockers" column in a project
  39. sidhujag referenced this in commit e35618ea7c on Feb 22, 2020
  40. MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
  41. sidhujag referenced this in commit 6e62e0d0b0 on Nov 10, 2020
  42. jasonbcox referenced this in commit a9369bba40 on Nov 13, 2020
  43. bitcoin locked this on Feb 15, 2022
Contributors
achow101DrahtBotpromagSjorsfjahrjonatackinstagibbsmeshcollider
Labels
Descriptors
Linked (view graph)
#15590 Descriptor: add GetAddressType() and IsSegWit()#16546 External signer support - Wallet Box edition
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC