wallet: add parent_desc to getaddressinfo #19136

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:export-descriptor changing 7 files +219 −40
  1. achow101 commented at 8:18 pm on June 1, 2020: member

    Adds parent_desc field to the getaddressinfo RPC to export a public descriptor. Using the given address, getaddressinfo will look up which DescriptorScriptPubKeyMan can be used to produce that address. It will then return the descriptor for that DescriptorScriptPubKeyMan in the parent_desc field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet’s addresses and that these descriptors can be imported and used in other wallets.

    As part of this PR, a ToNormalizedString function is added to the descriptor classes. This really only has an effect on BIP32PubkeyProviders that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.

  2. DrahtBot added the label RPC/REST/ZMQ on Jun 1, 2020
  3. DrahtBot added the label Tests on Jun 1, 2020
  4. DrahtBot added the label Wallet on Jun 1, 2020
  5. achow101 force-pushed on Jun 1, 2020
  6. MarcoFalke removed the label Tests on Jun 1, 2020
  7. DrahtBot commented at 1:19 am on June 2, 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:

    • #19651 (wallet: importdescriptors update existing by S3RK)
    • #19602 (wallet: Migrate legacy wallets to descriptor wallets 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.

  8. Sjors commented at 11:19 am on June 8, 2020: member
    Do we need a whole new RPC for this? Why not expand getaddressinfo?
  9. meshcollider commented at 11:39 am on July 11, 2020: contributor
    Agree with Sjors, unless you have plans to somehow build upon this later, I think it might as well just be added to getaddressinfo
  10. achow101 force-pushed on Jul 21, 2020
  11. achow101 commented at 10:36 pm on July 21, 2020: member
    Changed this to be a new field parent_desc in getaddressinfo. Also rebased.
  12. achow101 renamed this:
    wallet: add dumpwalletdescriptor RPC
    wallet: add parent_desc to getaddressinfo
    on Jul 21, 2020
  13. in src/script/descriptor.cpp:223 in fc2f2a008f outdated 
    214@@ -212,6 +215,18 @@ class OriginPubkeyProvider final : public PubkeyProvider
215         ret = "[" + OriginString() + "]" + std::move(sub);
216         return true;
217     }
218+    bool ToNormalizedString(const SigningProvider& arg, std::string& ret, bool priv) const override
219+    {
220+        std::string sub;
221+        if (!m_provider->ToNormalizedString(arg, sub, priv)) return false;
222+        if (sub[0] == '[') {
223+            sub = sub.substr(10);
    S3RK commented at 11:41 am on August 31, 2020:

    I’m not sure I fully understand this part. From what I figured here you’re handling a case when we wrap another OriginPubkeyProvider. If so, I have a few questions:

    1. What do we even expect to happen in such case? I can’t think of realistic scenario
    2. Why handle it here if we don’t handle it in other methods, like ToPrivateString or ToString()
    3. Why do you think the origin from wrapped provider is exactly 10 chars long? Even if so, than there is a closing bracket missing

    Hope my questions make at least some sense

    achow101 commented at 4:07 pm on August 31, 2020:

    The goal of ToNormalizedString is to create a descriptor that can be imported into another wallet. A descriptor might not be able to be imported because hardened derivation is in use. So for BIP32PubkeyProvider::ToNormalizedString, we are deriving the key at the last hardened step and returning that key. As we did additional derivation not present in the original descriptor, we are generating new origin info that needs to be combined with any existing origin info.

    1. We don’t actually wrap another OriginPubkeyProvider. Rather we might be wrapping a BIP32PubkeyProvider where it’s ToNormalizedString might return us a string containing origin info as it did additional derivation.
    2. ToPrivateString and ToString return the descriptor as-is, no additional derivation is done. Thus there is no need to handle this “origin within the origin provider.”
    3. This is to cut off the first 10 characters because we don’t need them. Specifically, that’s the [ followed by 8 characters for the fingerprint, followed by the first /. Actually, there might be an off-by-one here as I think we need that /.

    Edit: Fixed the off by one.

    S3RK commented at 5:02 am on September 3, 2020:

    I created and verified a test case to cover this part of code and which would have exposed the bug as well. Feel free to add it.

    0Check(
1"pkh([01234567/10/20]xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/2147483647'/0)",
2"pkh([01234567/10/20]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/2147483647'/0)",
3"pkh([01234567/10/20/2147483647']xprv9vHkqa6XAPwKqSKSEJMcAB3yoCZhaSVsGZbSkFY5L3Lfjjk8sjZucbsbvEw5o3QrSA69nPfZDCgFnNnLhQ2ohpZuwummndnPasDw2Qr6dC2/0)",
4"pkh([01234567/10/20/2147483647']xpub69H7F5dQzmVd3vPuLKtcXJziMEQByuDidnX3YdwgtNsecY5HRGtAAQC5mXTt4dsv9RzyjgDjAQs9VGVV6ydYCHnprc9vvaA5YtqWyL6hyds/0)",
5HARDENED,
6{{"76a914ebdc90806a9c4356c1c88e42216611e1cb4c1c1788ac"}},
7OutputType::LEGACY,
8{{10,20,0xFFFFFFFFUL,0}}
9);
    achow101 commented at 5:21 am on September 3, 2020:
    Added this test.
  14. Sjors commented at 12:41 pm on August 31, 2020: member
    Concept ACK. This has a silent merge conflict with either #19373 or #19660: script/descriptor.cpp:455:34: error: no matching function for call to 'HexStr'
  15. in src/script/descriptor.cpp:263 in fc2f2a008f outdated 
    257@@ -243,6 +258,15 @@ class ConstPubkeyProvider final : public PubkeyProvider
258         ret = EncodeSecret(key);
259         return true;
260     }
261+    bool ToNormalizedString(const SigningProvider& arg, std::string& ret, bool priv) const override
262+    {
263+        if (priv) {
    promag commented at 1:36 pm on August 31, 2020: 
    0if (priv) return ToPrivateString(arg, ret);
1ret = ToString();
2return true;
    achow101 commented at 5:05 pm on August 31, 2020:
    Done
  16. promag commented at 1:38 pm on August 31, 2020: member
    Concept ACK.
  17. achow101 force-pushed on Aug 31, 2020
  18. achow101 force-pushed on Aug 31, 2020
  19. achow101 commented at 5:12 pm on August 31, 2020: member

    Concept ACK. This has a silent merge conflict with either #19373 or #19660: script/descriptor.cpp:455:34: error: no matching function for call to 'HexStr'

    Rebased and fixed.

  20. achow101 force-pushed on Sep 3, 2020
  21. S3RK commented at 10:18 am on September 13, 2020: member

    I believe it’s cumbersome to work with normalised descriptor represented as a string. It has been proved to be prone to errors and also less expressive. Luckily we already have a struct for this!

    What if we use object representation for such a descriptor and convert it to string only at the last step. I have something like this in my mind:

    1. Add PubkeyProvider::GetLastUnhardenedKey(...). The function will be shorter and easier to understand than ToNormalizedString which does two things right now: a) derives the key and b) converts it to string.
    2. Use it in Descriptor::GetNormalizedDescriptor(...). By using GetLastUnhardenedKey it’s obvious now what do we mean by “normalized” without going to deep into the code.
    3. In DescriptorScriptPubKeyMan::GetDescriptorString we call GetNormalizedDescriptor() first and than use usual ToPrivateSring() or ToString() functions on the returned descriptor.

    Another benefit of this approach is that we don’t need more arguments and conditions in ToStringHelepr.

    Let me know what you think, I can do a PoC if you’d endorse this idea.

  22. achow101 commented at 4:46 pm on September 14, 2020: member
    @S3RK I originally attempted an approach like that but was not able to make it work well. The problem is largely in the same place that the problem with this PR is: nested origin pubkey providers. In both this PR and your proposal, you will still have some weirdness to handle when the bip32 pubkey provider returns an origin pubkey provider, but this itself is already within an origin pubkey provider.
  23. S3RK commented at 8:18 am on September 15, 2020: member

    @achow101 I understand that it’s an intrinsic part of the logic here. However my point is that we need to strive to make that weird part as easy to understand as possible. And this is why I believe working with objects/structs is better. When you work with KeyOriginInfo you don’t need to rely on a magic number to strip the info from a string. Even bigger issue is not even the constant itself, but tight coupling of unrelated parts of the codebase — BIP32PubkeyProvider and OriginPubkeyProvider are now invisibly tied together by the convention of formatting origin info as a string.

    The other benefits hold up as well: single responsibility and less cyclomatic complexity for functions, self-documented code.

  24. achow101 commented at 3:55 pm on September 15, 2020: member
    @S3RK If you think you have a better solution, feel free to open a PR with it. I can close this one in favor of that if it does look better.
  25. promag commented at 9:02 pm on September 27, 2020: member
    Add release note?
  26. S3RK commented at 10:21 am on October 1, 2020: member

    @S3RK If you think you have a better solution, feel free to open a PR with it. I can close this one in favor of that if it does look better.

    For history. I tried to implement alternative version, and indeed it also has its own problems. Both with nested origin/bip32 providers and also with private key management for intermediate xpub. And the diff is bigger 😞

  27. in src/script/descriptor.h:96 in 6f5d66d2ab outdated 
    92@@ -93,6 +93,9 @@ struct Descriptor {
93     /** Convert the descriptor to a private string. This fails if the provided provider does not have the relevant private keys. */
94     virtual bool ToPrivateString(const SigningProvider& provider, std::string& out) const = 0;
95 
96+    /** Convert the descriptor to a normalized string. Normalized descriptors have the xpub at the last hardened step. This fail if the provided provider does not have the private keys to derive that xpub. */
    Sjors commented at 8:54 am on October 9, 2020:

    Nit: fail -> fails

    Could document the arguments.

    achow101 commented at 1:04 pm on October 9, 2020:
    Fixed.
  28. in src/script/descriptor.h:97 in 6f5d66d2ab outdated 
    92@@ -93,6 +93,9 @@ struct Descriptor {
93     /** Convert the descriptor to a private string. This fails if the provided provider does not have the relevant private keys. */
94     virtual bool ToPrivateString(const SigningProvider& provider, std::string& out) const = 0;
95 
96+    /** Convert the descriptor to a normalized string. Normalized descriptors have the xpub at the last hardened step. This fail if the provided provider does not have the private keys to derive that xpub. */
97+    virtual bool ToNormalizedString(const SigningProvider& provier, std::string& out, bool priv) const = 0;
    Sjors commented at 8:55 am on October 9, 2020:
    provier -> provider
    achow101 commented at 1:04 pm on October 9, 2020:
    Fixed.
  29. in src/script/descriptor.cpp:225 in 6f5d66d2ab outdated 
    214@@ -212,6 +215,18 @@ class OriginPubkeyProvider final : public PubkeyProvider
215         ret = "[" + OriginString() + "]" + std::move(sub);
216         return true;
217     }
218+    bool ToNormalizedString(const SigningProvider& arg, std::string& ret, bool priv) const override
219+    {
220+        std::string sub;
221+        if (!m_provider->ToNormalizedString(arg, sub, priv)) return false;
222+        if (sub[0] == '[') {
    Sjors commented at 9:30 am on October 9, 2020:

    6f5d66d2abfe241df9f27bf2b1e62e6604918034 : I’m confused about what’s going on here. I assume related to this comment:

    you will still have some weirdness to handle when the bip32 pubkey provider returns an origin pubkey provider, but this itself is already within an origin pubkey provider.

    Maybe explain the “weirdness” in a comment, and add a comment to the relevant test vector.

    achow101 commented at 1:05 pm on October 9, 2020:
    Added a comment.
  30. Sjors commented at 9:35 am on October 9, 2020: member
    e92546a looks mostly good, if only because the tests pass…
  31. descriptors: Add ToNormalizedString and tests 9be1437c49
  32. wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan 
    GetDescriptorString returns a normalized descriptor for a
DescriptorScriptPubKeyMan.
    bbe4a36152
  33. rpc: Add parent descriptor to getaddressinfo output 
    Adds parent_desc field to getaddressinfo output which contains the
descriptor that was used to derive the address if it is available.
    e4ac869a0a
  34. tests: Test getaddressinfo parent_desc de6b389d5d
  35. achow101 force-pushed on Oct 9, 2020
  36. Sjors commented at 3:11 pm on October 9, 2020: member
    utACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
  37. in src/script/descriptor.cpp:422 in de6b389d5d 
    417+            if (priv) return ToPrivateString(arg, out);
418+            out = ToString();
419+            return true;
420+        }
421+        // Step backwards to find the last hardened step in the path
422+        int i = (int)m_path.size() - 1;
    jonatack commented at 6:08 pm on December 31, 2020:

    9be1437c named cast per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Res-casts-named

    0        int i = static_cast<int>(m_path.size()) - 1;
  38. in src/script/descriptor.cpp:455 in de6b389d5d 
    450+        KeyPath end_path;
451+        for (; k < (int)m_path.size(); ++k) {
452+            end_path.push_back(m_path.at(k));
453+        }
454+        // Build the string
455+        std::string origin_str = HexStr(origin.fingerprint) + FormatHDKeypath(origin.path);
    jonatack commented at 6:10 pm on December 31, 2020:

    9be1437c49f986e8ed964d5f863b4bbcec340751

    0        const std::string origin_str = HexStr(origin.fingerprint) + FormatHDKeypath(origin.path);
  39. in src/script/descriptor.cpp:264 in de6b389d5d 
    260@@ -243,6 +261,12 @@ class ConstPubkeyProvider final : public PubkeyProvider
261         ret = EncodeSecret(key);
262         return true;
263     }
264+    bool ToNormalizedString(const SigningProvider& arg, std::string& ret, bool priv) const override
    jonatack commented at 6:13 pm on December 31, 2020:

    9be1437c49f986e8ed964d5f863b4bbcec340751 here and L218, maybe name the arg out rather than ret like the header file declaration and the other definitions

    0    bool ToNormalizedString(const SigningProvider& arg, std::string& out, bool priv) const override
  40. in src/wallet/rpcwallet.cpp:3666 in de6b389d5d 
    3662@@ -3663,6 +3663,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3663                         {RPCResult::Type::BOOL, "iswatchonly", "If the address is watchonly."},
3664                         {RPCResult::Type::BOOL, "solvable", "If we know how to spend coins sent to this address, ignoring the possible lack of private keys."},
3665                         {RPCResult::Type::STR, "desc", /* optional */ true, "A descriptor for spending coins sent to this address (only when solvable)."},
3666+                        {RPCResult::Type::STR, "parent_desc", /* optional */ true, "The descriptor used to derive this address if this is a descriptor wallet"},
    jonatack commented at 6:14 pm on December 31, 2020:

    e4ac869

    • missing period like the neighboring sentences and maybe add a comma
    0                        {RPCResult::Type::STR, "parent_desc", /* optional */ true, "The descriptor used to derive this address, if this is a descriptor wallet."},

    • not sure, but maybe change from:

      “The descriptor used to derive this address, if this is a descriptor wallet.”

      to

      “The descriptor used to derive this address, if generated by a descriptor wallet.”

  41. in test/functional/wallet_descriptor.py:191 in de6b389d5d 
    186+                'next_index': 11,
187+                'timestamp': 'now',
188+                'internal': internal
189+            }])
190+
191+            for i in range(0, 10):
    jonatack commented at 6:27 pm on December 31, 2020:

    de6b389 nit, if iterator is not used in the for-loop block

    0            for _ in range(0, 10):
  42. in test/functional/wallet_descriptor.py:180 in de6b389d5d 
    175+                if internal:
176+                    addr = exp_rpc.getrawchangeaddress(address_type=addr_type)
177+                else:
178+                    addr = exp_rpc.getnewaddress(address_type=addr_type)
179+                test_desc = exp_rpc.getaddressinfo(addr)['parent_desc']
180+                assert_equal(desc, test_desc)
    jonatack commented at 6:30 pm on December 31, 2020:
    de6b389 nit, the if internal...else...assert_equal logic is repeated 4 times, maybe extract to a helper
  43. jonatack commented at 6:49 pm on December 31, 2020: member
    Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8 modulo a few minor comments
  44. meshcollider referenced this in commit 9deba2de76 on Jan 28, 2021
  45. sidhujag referenced this in commit a57031f639 on Jan 28, 2021
  46. in src/wallet/rpcwallet.cpp:3736 in e4ac869a0a outdated 
    3732@@ -3732,6 +3733,14 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3733        ret.pushKV("desc", InferDescriptor(scriptPubKey, *provider)->ToString());
3734     }
3735 
3736+    DescriptorScriptPubKeyMan* desc_spk_man = dynamic_cast<DescriptorScriptPubKeyMan*>(pwallet->GetScriptPubKeyMan(scriptPubKey));
    fjahr commented at 11:34 pm on February 1, 2021:

    in e4ac869a0a0083e2e3af3b56301bd5c8e0cf650b:

    Below, on line 3742, there is another call to GetScriptPubKeyMan(). Maybe move that one up here to save one.

  47. fjahr commented at 5:03 pm on February 7, 2021: member

    Code review ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8

    Also did some light manual testing.

  48. S3RK commented at 8:36 am on February 9, 2021: member
    Tested ACK de6b389
  49. fanquake requested review from meshcollider on Feb 9, 2021
  50. meshcollider commented at 7:21 am on February 18, 2021: contributor
    Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
  51. meshcollider merged this on Feb 18, 2021
  52. meshcollider closed this on Feb 18, 2021
  53. sidhujag referenced this in commit 8369e4f3a4 on Feb 18, 2021
  54. laanwj referenced this in commit 8d37841cdf on Feb 26, 2021
  55. sidhujag referenced this in commit e9cbf91787 on Feb 26, 2021
  56. fanquake deleted a comment on Mar 15, 2021
  57. fanquake locked this on Mar 15, 2021


achow101 DrahtBot Sjors meshcollider S3RK promag jonatack fjahr

Labels
Wallet RPC/REST/ZMQ

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