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 0: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: 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:

    • #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 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.

  10. achow101 force-pushed on Feb 10, 2020
  11. 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.

  12. 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:

    0script/descriptor.cpp:368:176: warning: field 'm_subdescriptor_arg' will be initialized after field 'm_name' [-Wreorder]
1    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.
  13. 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.
  14. Sjors changes_requested
  15. Sjors commented at 6:20 pm on February 10, 2020: member
    Code review ACK b4fcf7ad56d0981797a4424f38f0524275d26e59 modulo (an explanation for) the assert.
  16. achow101 force-pushed on Feb 10, 2020
  17. Get the OutputType for a descriptor 7e80f646b2
  18. achow101 force-pushed on Feb 11, 2020
  19. Sjors approved
  20. Sjors commented at 6:30 pm on February 11, 2020: member
    Code review ACK 7e80f646b24a2abf3c031a649bcc706a695f80da
  21. laanwj added this to the "Blockers" column in a project
  22. fjahr commented at 0:00 am on February 19, 2020: member

    ACK 7e80f646b24a2abf3c031a649bcc706a695f80da

    Reviewed code, built and ran tests locally.

  23. jonatack commented at 7:13 pm on February 20, 2020: member
    ACK 7e80f64 code review/build/tests
  24. 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.
  25. 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?

  26. 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.
  27. 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

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

  31. meshcollider merged this on Feb 21, 2020
  32. meshcollider closed this on Feb 21, 2020
  33. fanquake removed this from the "Blockers" column in a project
  34. sidhujag referenced this in commit e35618ea7c on Feb 22, 2020
  35. sidhujag referenced this in commit 6e62e0d0b0 on Nov 10, 2020
  36. jasonbcox referenced this in commit a9369bba40 on Nov 13, 2020
  37. DrahtBot locked this on Feb 15, 2022


achow101 DrahtBot promag Sjors fjahr jonatack instagibbs meshcollider

Labels
Descriptors

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: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me