wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors #29130

pull achow101 wants to merge 13 commits into bitcoin:master from achow101:createwalletdescriptor-without-new-records changing 16 files +775 −63
  1. achow101 commented at 0:05 am on December 22, 2023: member

    This PR adds a createwalletdescriptor RPC which allows users to add new automatically generated descriptors to their wallet, e.g. to upgrade a 0.21.x wallet to contain a taproot descriptor. This RPC takes 3 arguments: the output type to create a descriptor for, whether the descriptor will be internal or external, and the HD key to use if the user wishes to use a specific key. The HD key is an optional parameter. If it is not specified, the wallet will use the key shared by the active descriptors, if they are all single key. For most users in the expected upgrade scenario, this should be sufficient. In more advanced cases, the user must specify the HD key to use.

    Currently, specified HD keys must already exist in the wallet. To make it easier for the user to know, gethdkeys is also added to list out the HD keys in use by all of the descriptors in the wallet. This will include all HD keys, whether we have the private key, for it, which descriptors use it and their activeness, and optionally the extended private key. In this way, users with more complex wallets will be still be able to get HD keys from their wallet for use in other scenarios, and if they want to use createwalletdescriptor, they can easily get the keys that they can specify to it.

    See also #26728 (comment)

  2. DrahtBot commented at 0:05 am on December 22, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, Sjors, ryanofsky

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
    • #29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
    • #28724 (wallet: Cleanup accidental encryption keys in watchonly wallets by achow101)
    • #28574 (wallet: optimize migration process, batch db transactions by furszy)
    • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
    • #25979 ([WIP] wallet: standardize change output detection process by furszy)
    • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

  3. DrahtBot added the label Wallet on Dec 22, 2023
  4. DrahtBot added the label CI failed on Dec 22, 2023
  5. achow101 force-pushed on Dec 22, 2023
  6. achow101 force-pushed on Dec 22, 2023
  7. DrahtBot removed the label CI failed on Dec 22, 2023
  8. in src/wallet/scriptpubkeyman.cpp:2141 in 484711baa9 outdated 
    2137@@ -2137,6 +2138,35 @@ std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const
2138     return m_map_keys;
2139 }
2140 
2141+bool DescriptorScriptPubKeyMan::HasPrivKey(const CPubKey& pubkey) const
    ryanofsky commented at 9:34 pm on December 22, 2023:

    In commit “desc spkm: Add functions to retrieve specific private keys” (484711baa994d482110edaf510377181b8b2b465)

    It looks like this function would be a little more flexible and efficient if took a CKeyID parameter instead of a CPubKey

    achow101 commented at 2:10 am on December 23, 2023:
    Done
  9. achow101 force-pushed on Dec 22, 2023
  10. achow101 force-pushed on Dec 22, 2023
  11. DrahtBot added the label CI failed on Dec 22, 2023
  12. in src/wallet/rpc/wallet.cpp:823 in 5deafad2f6 outdated 
    818+    return RPCHelpMan{
819+        "gethdkeys",
820+        "\nList all BIP 32 HD keys in the wallet and which descriptors use them.\n",
821+        {
822+            {"active_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show the keys for only active descriptors"},
823+            {"private", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show private keys."}
    ryanofsky commented at 10:13 pm on December 22, 2023:

    In commit “wallet, rpc: Add gethdkeys RPC” (5deafad2f61f626b50b6c173558ac7236cceff13)

    Would suggest making these name-only parameters to avoid different bool options being confused with each other, and to leave room to add new positional parameters in the future. You could do this with:

     0--- a/src/rpc/client.cpp
 1+++ b/src/rpc/client.cpp
 2@@ -274,8 +274,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
 3     { "logging", 1, "exclude" },
 4     { "disconnectnode", 1, "nodeid" },
 5     { "upgradewallet", 0, "version" },
 6+    { "gethdkeys", 0, "options" },
 7     { "gethdkeys", 0, "active_only" },
 8-    { "gethdkeys", 1, "private" },
 9+    { "gethdkeys", 0, "private" },
10     { "createwalletdescriptor", 1, "internal" },
11     // Echo with conversion (For testing only)
12     { "echojson", 0, "arg0" },
13--- a/src/wallet/rpc/wallet.cpp
14+++ b/src/wallet/rpc/wallet.cpp
15@@ -819,8 +819,9 @@ RPCHelpMan gethdkeys()
16         "gethdkeys",
17         "\nList all BIP 32 HD keys in the wallet and which descriptors use them.\n",
18         {
19-            {"active_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show the keys for only active descriptors"},
20-            {"private", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show private keys."}
21+            {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", {
22+                {"active_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show the keys for only active descriptors"},
23+                {"private", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show private keys."}}},
24         },
25         RPCResult{RPCResult::Type::ARR, "", "", {
26             {
27@@ -851,8 +852,9 @@ RPCHelpMan gethdkeys()
28                 throw JSONRPCError(RPC_WALLET_ERROR, "gethdkeys is not available for non-descriptor wallets");
29             }
30 
31-            const bool active_only = !request.params[0].isNull() && request.params[0].get_bool();
32-            const bool priv = !request.params[1].isNull() && request.params[1].get_bool();
33+            UniValue options{request.params[0].isNull() ? UniValue::VOBJ : request.params[0]};
34+            const bool active_only{options.exists("active_only") ? options["active_only"].get_bool() : false};
35+            const bool priv{options.exists("private") ? options["private"].get_bool() : false};
36             if (priv) {
37                 EnsureWalletIsUnlocked(*wallet);
38             }
    achow101 commented at 2:10 am on December 23, 2023:
    Done as suggested
  13. in src/wallet/rpc/wallet.cpp:822 in 5deafad2f6 outdated 
    817+{
818+    return RPCHelpMan{
819+        "gethdkeys",
820+        "\nList all BIP 32 HD keys in the wallet and which descriptors use them.\n",
821+        {
822+            {"active_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show the keys for only active descriptors"},
    ryanofsky commented at 10:23 pm on December 22, 2023:

    In commit “wallet, rpc: Add gethdkeys RPC” (5deafad2f61f626b50b6c173558ac7236cceff13)

    Note: This is better than what I suggested. I was originally thinking this default should be true, to make it more convenient to get active hd key and ignore other keys. But defaulting to false is actually better, because it’s still easy to get the active hd key with an option, and it’s probably more confusing to see missing keys than extra keys. Also this default makes gethdkeys output more consistent with listdescriptors output.

  14. in src/wallet/rpc/wallet.cpp:882 in 5deafad2f6 outdated 
    868+
869+            std::map<CExtPubKey, std::set<std::tuple<std::string, bool, bool>>> wallet_xpubs;
870+            std::map<CExtPubKey, CExtKey> wallet_xprvs;
871+            for (auto spkm : spkms) {
872+                auto desc_spkm{dynamic_cast<DescriptorScriptPubKeyMan*>(spkm)};
873+                LOCK(desc_spkm->cs_desc_man);
    ryanofsky commented at 10:27 pm on December 22, 2023:

    In commit “wallet, rpc: Add gethdkeys RPC” (5deafad2f61f626b50b6c173558ac7236cceff13)

    Could use Assert(desc_spkm) to avoid undefined behavior if it is null.

    achow101 commented at 2:11 am on December 23, 2023:
    Added a CHECK_NONFATAL (one of the linters doesn’t like Assert)
  15. in src/wallet/rpc/wallet.cpp:872 in 5deafad2f6 outdated 
    867+            }
868+
869+            std::map<CExtPubKey, std::set<std::tuple<std::string, bool, bool>>> wallet_xpubs;
870+            std::map<CExtPubKey, CExtKey> wallet_xprvs;
871+            for (auto spkm : spkms) {
872+                auto desc_spkm{dynamic_cast<DescriptorScriptPubKeyMan*>(spkm)};
    ryanofsky commented at 10:38 pm on December 22, 2023:

    In commit “wallet, rpc: Add gethdkeys RPC” (5deafad2f61f626b50b6c173558ac7236cceff13)

    This is just general style feedback, but I think where possible it’s better to use auto& or auto* instead of bare auto because with bare auto in c++, you don’t know if a potentially expensive object copy will happen without manually checking the type. With auto& or auto* you know there won’t be an big copy without having to think about it. Up to you though if you prefer less verbosity instead

    achow101 commented at 2:11 am on December 23, 2023:
    Done
  16. in src/wallet/rpc/wallet.cpp:896 in 5deafad2f6 outdated 
    879+                w_desc.descriptor->GetPubKeys(desc_pubkeys, desc_xpubs);
880+                for (const CExtPubKey& xpub : desc_xpubs) {
881+                    wallet_xpubs[xpub].emplace(w_desc.descriptor->ToString(), wallet->IsActiveScriptPubKeyMan(spkm), desc_spkm->HasPrivKey(xpub.pubkey));
882+                    if (std::optional<CKey> key = desc_spkm->GetKey(xpub.pubkey)) {
883+                        wallet_xprvs[xpub] = CExtKey(xpub, *key);
884+                    }
    ryanofsky commented at 10:49 pm on December 22, 2023:

    In commit “wallet, rpc: Add gethdkeys RPC” (5deafad2f61f626b50b6c173558ac7236cceff13)

    It seems like theoretically there could be a race condition if the wallet became locked during this loop. Also the code ignores other potential errors. Would maybe suggest:

    0if (priv && desc_spkm->HasPrivKey(xpub.pubkey)) {
1  wallet_xprvs[xpub] = CExtKey(xpub, *CHECK_NONFATAL(desc_spkm->GetKey(xpub.pubkey)));
2}
    achow101 commented at 1:22 am on December 23, 2023:
    cs_wallet is being held earlier, and that will prevent the wallet from locked.
  17. in src/wallet/rpc/wallet.cpp:906 in 5deafad2f6 outdated 
    889+            for (const auto& [xpub, descs] : wallet_xpubs) {
890+                bool has_xprv = false;
891+                UniValue descriptors(UniValue::VARR);
892+                for (const auto& [desc, active, has_priv] : descs) {
893+                    UniValue d(UniValue::VOBJ);
894+                    d.pushKV("desc", desc);
    ryanofsky commented at 10:52 pm on December 22, 2023:

    In commit “wallet, rpc: Add gethdkeys RPC” (5deafad2f61f626b50b6c173558ac7236cceff13)

    Not important but I think I would suggest switching to auto& in the loops above and using std::move(desc) here to avoid copying strings when not necessary.

    achow101 commented at 2:11 am on December 23, 2023:
    Done
  18. DrahtBot removed the label CI failed on Dec 22, 2023
  19. in src/wallet/rpc/wallet.cpp:898 in 5deafad2f6 outdated 
    893+                    UniValue d(UniValue::VOBJ);
894+                    d.pushKV("desc", desc);
895+                    d.pushKV("active", active);
896+                    has_xprv |= has_priv;
897+
898+                    descriptors.push_back(d);
    ryanofsky commented at 10:56 pm on December 22, 2023:

    In commit “wallet, rpc: Add gethdkeys RPC” (5deafad2f61f626b50b6c173558ac7236cceff13)

    Could use std::move here and on line 906 as well to avoid copying univalue objects. I will stop commenting about copies, though. I don’t think they are important, I just figure it’s very easy to avoid copying and we don’t need 3 copies of each descriptor string so why not avoid it.

    achow101 commented at 2:11 am on December 23, 2023:
    Done
  20. in src/wallet/wallet.cpp:3564 in 280d26b19e outdated 
    3559+    auto spk_manager = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, m_keypool_size));
3560+    if (IsCrypted()) {
3561+        if (IsLocked()) {
3562+            throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors");
3563+        }
3564+        if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, nullptr)) {
    ryanofsky commented at 11:05 pm on December 22, 2023:

    In commit “wallet: Refactor function for single DescSPKM setup” (280d26b19e9222afa902ad09cd7b42c2eb3e0044)

    Seems like the &batch argument option is replaced with nullptr here. This seems like a bug, but if it is intended behavior should definitely have a comment explaining the nullptr.

    achow101 commented at 2:11 am on December 23, 2023:
    Indeed, fixed.
  21. in src/wallet/wallet.cpp:3556 in 280d26b19e outdated 
    3552@@ -3553,6 +3553,25 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc)
3553     }
3554 }
3555 
3556+uint256 CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal)
    ryanofsky commented at 11:11 pm on December 22, 2023:

    In commit “wallet: Refactor function for single DescSPKM setup” (280d26b19e9222afa902ad09cd7b42c2eb3e0044)

    Not important, but I think it would be a little better to return DescriptorScriptPubKeyMan& to make the return type self-documenting. Also to avoid the need for callers to have to look up the keyman object after they just created it.

    achow101 commented at 2:22 am on December 23, 2023:
    Done
  22. in src/wallet/scriptpubkeyman.h:627 in fc3ccf068f outdated 
    623@@ -624,6 +624,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
624 
625     bool IsHDEnabled() const override;
626 
627+    static WalletDescriptor GenerateWalletDescriptor(const CExtKey& master_key, const OutputType& output_type, bool internal);
    ryanofsky commented at 11:21 pm on December 22, 2023:

    In commit “wallet, descspkm: Refactor wallet descriptor generation to static func” (fc3ccf068fbbf429eff9dce072bc33d78ad27509)

    Not important, but it doesn’t seem like it makes sense for this function to be attached to the keyman class. If you want it to be a static method, it would probably make more sense being a static method of WalletDescriptor, since it is basically just a constructor for it. But I would probably make it a standalone function not attached to any class, just to reduce interdependencies in this code.

    Also since the private key is not used here, I think it would be clearer for this to take a CExtPubKey parameter instead of a CPubKey.

    achow101 commented at 2:12 am on December 23, 2023:
    Done
  23. in src/wallet/wallet.h:1052 in 647118e365 outdated 
    1043@@ -1044,6 +1044,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
1044 
1045     //! Whether the (external) signer performs R-value signature grinding
1046     bool CanGrindR() const;
1047+
1048+    //! Find the single xpub used by all active descriptors, or return nullopt
1049+    std::optional<CExtPubKey> GetActiveHDPubKey() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
1050+
1051+    //! Find the private key for the given public key from the wallet's descriptors
1052+    std::optional<CKey> CWallet::GetKey(const CPubKey& pubkey) const;
    ryanofsky commented at 11:24 pm on December 22, 2023:

    In commit “wallet: Add GetActiveHDPubKey to retrieve hd key from active descriptors” (647118e36563ef421b6c4f6edeac33ce6aff5588)

    This is probably in the wrong commit, I don’t see this method being added here.

    achow101 commented at 2:12 am on December 23, 2023:
    Moved to the right commit.
  24. in src/wallet/wallet.cpp:4391 in 647118e365 outdated 
    4386+        if (desc_xpubs.size() != 1 || desc_pubkeys.size() != 0) {
4387+            return std::nullopt;
4388+        }
4389+        const CExtPubKey& xpub = *desc_xpubs.begin();
4390+
4391+        // All active descriptors must have the same xpub
    ryanofsky commented at 11:33 pm on December 22, 2023:

    In commit “wallet: Add GetActiveHDPubKey to retrieve hd key from active descriptors” (647118e36563ef421b6c4f6edeac33ce6aff5588)

    I’m confused. Is this actually true? I thought importdescriptors let you freely import descriptors with different keys and control which ones are active.

    If this is always true, it would be helpful if comment mentioned what was enforcing it (since it’s not obvious where to look). If it’s not true, I think would be better if this function returned std::set<CExtPubKey> instead of std::optional<CExtPubKey> and callers decided what to do if the set size is more than 1.

    achow101 commented at 2:14 am on December 23, 2023:

    This function was intended to only return a CExtPubKey if all active descriptors shared the same xpub, so if they differed, it would return std::nullopt. The comment was stating what it was attempting to do, not what it was expecting.

    In any case, having the caller determine what to do if there is more than one xpub in the active descriptors is probably better, so I’ve changed it to do that and this comment is no longer relevant.

  25. in src/wallet/rpc/wallet.cpp:926 in 01b2aab1f8 outdated 
    921+        "The address type must be one that the wallet does not already have a descriptor for."
922+        + HELP_REQUIRING_PASSPHRASE,
923+        {
924+            {"address_type", RPCArg::Type::STR, RPCArg::Optional::NO, "The address type the descriptor will produce. Options are \"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"."},
925+            {"internal", RPCArg::Type::BOOL, RPCArg::DefaultHint{"Both external and internal will be generated unless this parameter is specified"}, "Whether to only make one descriptor that is internal (if parameter is true) or external (if parameter is false)"},
926+            {"hdkey", RPCArg::Type::STR, RPCArg::DefaultHint{"The HD key used by all other active descriptors"}, "The HD key that the wallet knows the private key of, listed using 'gethdkeys', to use for this descriptor's key"},
    ryanofsky commented at 11:56 pm on December 22, 2023:

    In commit “wallet, rpc: Add createwalletdescriptor RPC” (01b2aab1f8f61bc85345e106893e0ff2e590a56a)

    Would suggest making the internal and hdkey paramers name-only parameters using OBJ_NAMED_PARAMS (see earlier comment #29130 (review)) to avoid usage errors and make it easier to add new options and arguments later.

    Would suggest keeping address_type as a positional parameter, though, since it’s required and the values are unambiguous. Also could consider renaming “address_type” to just “type” to make it easier to pass by name and because maybe we will want to support creating other types of descriptors that don’t correspond exactly to address types.

    achow101 commented at 2:14 am on December 23, 2023:
    Done as suggested.
  26. ryanofsky commented at 0:03 am on December 23, 2023: contributor
    Concept ACK 08b9b414d9f4b4b65bf5ec0ff875889997631a79. I think I’m maybe halfway through reviewing, but here are my comments so far.
  27. achow101 force-pushed on Dec 23, 2023
  28. achow101 force-pushed on Dec 23, 2023
  29. achow101 force-pushed on Dec 23, 2023
  30. achow101 force-pushed on Dec 23, 2023
  31. DrahtBot added the label CI failed on Dec 23, 2023
  32. in src/wallet/rpc/wallet.cpp:928 in e0ad5bce21 outdated 
    916+            return response;
917+        },
918+    };
919+}
920+
921+static RPCHelpMan createwalletdescriptor()
    ryanofsky commented at 1:45 pm on December 23, 2023:

    Since this PR is adding the createwalletdescriptor method, maybe this is a good place to list some ways it could be extended in the future:

    • Probably it would be good not to require wallet to be unlocked when dealing with public keys. Currently the specified hdkey is unencrypted and reencrypted, but this shouldn’t be necessary because the key is already in the wallet. (The only reason this seems to happen now is to copy the key, because internally we store keys in a slightly denormalized format, once per descriptor.)

    • It would be nice if hdkey parameter accepted not just public hd keys, but also private hd keys, and a “generate” value in a blank wallet so users don’t need to chain together multiple commands to accomplish simple tasks, and so we only need two RPC methods which add descriptors and keys to the wallet: importdescriptors and createwalletdescriptor, without a third addhdkey method. Examples:

      0# In a blank wallet, generate an hd key and use it in a new descriptor
1createwalletdescriptor bech32 generate
2
3 # In a blank wallet, import an hd key and use it in a new descriptor
4createwalletdescriptor bech32 xprv...

    • It sounds like we want to discourage having multiple hd keys per wallet, and encourage having separate wallets instead. But if we did want to allow these, we could add a force option to allow creating descriptors with new hd keys even when existing hd keys are present. We could also allow a rotate option to allow this and additionally set descriptors using old keys to be inactive.

    • The hdkey parameter could accept hd master keys in different formats, for example as seed keys like the current sethdseed method, or seed hex strings like #29054, or seed shares like #27351.

    • It would be nice if type parameter accepted a “defaults” value to set up descriptors for all default output types. Examples:

      0# In a blank wallet, generate an hd key and generate default set of descriptors
1createwalletdescriptor defaults generate
2
3# Add missing descriptors to an existing wallet. For example, upgrade
4# an older wallet not supporting bech32m to support it.
5createwalletdescriptor defaults

    • To support multisig, type parameter could accept an “hdkey” value to generate an unused hd key in a blank wallet:

      0# In a blank wallet, generate an hd key in an unused descriptor and output unused(xpub...)
1createwalletdescriptor hdkey generate
2
3# Alternately, import an unused key
4createwalletdescriptor hdkey xprv...
5
6# Import multisig descriptor with wallet public keys
7importdescriptors [...]

    • Maybe in the future to make multisig setup easier, type parameter could accept a “multisig” value and additional options to make it easier to create the descriptor using the right keys without doing extra work or using an outside tool.

    • Not recommending it, but I could imagine type and hdkey parameters being extended to accept other values in the future. For example maybe with #27351, a “codex32” type could be useful. And if a wallet didn’t have the same hdkey for every output type, it might be useful to be able specify “default’ for the hdkey parameter to use the hd key from the default output type, like getnewaddress.

    • It could make sense for createwalletdescriptor and importdescriptors to have other options in common. For example, it might make sense for createwalletdescriptor to accept a timestamp option when adding a descriptor with an existing hdkey, to rescan for transactions with the new descriptor. In general createwalletdescriptor could be a higher level alternative to importdescriptors that is less flexible but easier to use for common tasks.

    • It could make sense to add an “external” type and hd_account option (see #29129 (comment)) for external signer wallets :

      0# Add descriptors from the external signer with the specified BIP44 account
1createwalletdescriptor type=external hd_account=123
  33. achow101 force-pushed on Jan 3, 2024
  34. DrahtBot removed the label CI failed on Jan 3, 2024
  35. in src/wallet/scriptpubkeyman.h:638 in ebd02d7324 outdated 
    627@@ -628,6 +628,8 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
628     bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal);
629 
630     bool HavePrivateKeys() const override;
631+    bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
632+    std::optional<CKey> GetKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
    ryanofsky commented at 7:55 pm on January 8, 2024:

    In commit “desc spkm: Add functions to retrieve specific private keys” (ebd02d73241c0922f486cdf14719281c079b45c7)

    Maybe add a comment saying it returns nullopt if the key doesn’t exist, or can’t be decrypted because the wallet is locked, or because there’s decryption error. Otherwise it’s not clear what the function is assuming or when it returns nullopt.

    achow101 commented at 0:27 am on January 10, 2024:
    Done
  36. in src/wallet/rpc/wallet.cpp:845 in cbe990b1a9 outdated 
    840+                }},
841+            }
842+        }},
843+        RPCExamples{
844+            HelpExampleCli("gethdkeys", "") + HelpExampleRpc("gethdkeys", "")
845+            + HelpExampleCli("gethdkeys", "true") + HelpExampleRpc("gethdkeys", "true")
    ryanofsky commented at 7:57 pm on January 8, 2024:

    In commit “wallet, rpc: Add gethdkeys RPC” (cbe990b1a95e6331106c3ce6d1f2abd860f88ea8)

    I think this example is not right anymore since the parameter needs to be named. There seems to be a HelpExampleCliNamed function for this.

    achow101 commented at 0:27 am on January 10, 2024:
    Fixed
  37. in src/wallet/wallet.cpp:4383 in 2621ca0e64 outdated 
    4378+        const DescriptorScriptPubKeyMan* desc_spkm = dynamic_cast<DescriptorScriptPubKeyMan*>(spkm);
4379+        assert(desc_spkm);
4380+        LOCK(desc_spkm->cs_desc_man);
4381+        WalletDescriptor w_desc = desc_spkm->GetWalletDescriptor();
4382+
4383+        // We can only determine the active hd key if all descriptors have exactly 1 xpub
    ryanofsky commented at 9:49 pm on January 8, 2024:

    In commit “wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors” (2621ca0e6418b406161b21b6afac159b0be775b7)

    Should delete this comment if it just applies to calling code (it doesn’t seem to be enforced here).

    achow101 commented at 0:27 am on January 10, 2024:
    Done
  38. in src/wallet/wallet.h:939 in 2c98b2f395 outdated 
    935@@ -936,6 +936,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
936 
937     //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
938     std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
939+    bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan* spkm) const;
    ryanofsky commented at 6:10 pm on January 9, 2024:

    In commit “wallet: Add IsActiveScriptPubKeyMan” (2c98b2f395eafa125054e2f6e7c570e18795e318)

    Would be a little better to take a reference than a pointer, since it’s not useful to be able to pass null.

    achow101 commented at 0:27 am on January 10, 2024:
    Done
  39. in src/wallet/wallet.h:1015 in 8d107d5f48 outdated 
    1010@@ -1011,6 +1011,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
1011     //! @param[in] internal Whether this ScriptPubKeyMan provides change addresses
1012     void DeactivateScriptPubKeyMan(uint256 id, OutputType type, bool internal);
1013 
1014+    //! Create new DescriptorScriptPubKeyMan and add it to the wallet
1015+    DescriptorScriptPubKeyMan* SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
    ryanofsky commented at 6:28 pm on January 9, 2024:

    In commit “wallet: Refactor function for single DescSPKM setup” (8d107d5f48c2fae0d09bf9be79f9cf47daad6241)

    Would be good for this to return a reference instead of a pointer. It seems to never return null, always return non-null or throw an exception.

    achow101 commented at 0:27 am on January 10, 2024:
    Done
  40. in src/wallet/wallet.cpp:4374 in 2621ca0e64 outdated 
    4365@@ -4366,4 +4366,26 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4366     }
4367     return res;
4368 }
4369+
4370+std::set<CExtPubKey> CWallet::GetActiveHDPubKeys() const
4371+{
4372+    AssertLockHeld(cs_wallet);
4373+
4374+    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) return {};
    ryanofsky commented at 6:34 pm on January 9, 2024:

    In commit “wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors” (2621ca0e6418b406161b21b6afac159b0be775b7)

    Would probably be good to change this to an assert. Otherwise it could be called in legacy code and seem to succeed but never return data.

    achow101 commented at 0:27 am on January 10, 2024:
    Done
  41. in src/wallet/wallet.cpp:4387 in 2621ca0e64 outdated 
    4382+
4383+        // We can only determine the active hd key if all descriptors have exactly 1 xpub
4384+        std::set<CPubKey> desc_pubkeys;
4385+        std::set<CExtPubKey> desc_xpubs;
4386+        w_desc.descriptor->GetPubKeys(desc_pubkeys, desc_xpubs);
4387+        active_xpubs.merge(desc_xpubs);
    ryanofsky commented at 6:37 pm on January 9, 2024:

    In commit “wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors” (2621ca0e6418b406161b21b6afac159b0be775b7)

    std::move would be appropriate here

    achow101 commented at 0:28 am on January 10, 2024:
    Done
  42. in src/wallet/wallet.cpp:4394 in 8bcb89d373 outdated 
    4387@@ -4388,4 +4388,19 @@ std::set<CExtPubKey> CWallet::GetActiveHDPubKeys() const
4388     }
4389     return active_xpubs;
4390 }
4391+
4392+std::optional<CKey> CWallet::GetKey(const CKeyID& keyid) const
4393+{
4394+    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) return std::nullopt;
    ryanofsky commented at 6:39 pm on January 9, 2024:

    In commit “wallet: Be able to retrieve single key from descriptors” (8bcb89d3730d30a3202fcee172fd709a9f588a4e)

    Again I think this would be a little better as an assert so if this is called in legacy wallet code it doesn’t appear to succeed but never return anything.

    achow101 commented at 0:28 am on January 10, 2024:
    Done
  43. in src/wallet/wallet.h:1065 in 8bcb89d373 outdated 
    1046@@ -1047,6 +1047,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
1047 
1048     //! Retrieve the xpubs in use by the active descriptors
1049     std::set<CExtPubKey> GetActiveHDPubKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
1050+
1051+    //! Find the private key for the given public key from the wallet's descriptors
1052+    std::optional<CKey> GetKey(const CKeyID& keyid) const;
    ryanofsky commented at 6:41 pm on January 9, 2024:

    In commit “wallet: Be able to retrieve single key from descriptors” (8bcb89d3730d30a3202fcee172fd709a9f588a4e)

    Again would be good to say this returns null when the key is missing or the wallet is locked.

    achow101 commented at 0:28 am on January 10, 2024:
    Done
  44. in src/wallet/rpc/wallet.cpp:963 in ed177c90c8 outdated 
    958+            if (!output_type) {
959+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str()));
960+            }
961+
962+            UniValue options{request.params[1].isNull() ? UniValue::VOBJ : request.params[1]};
963+            UniValue internal{options["internal"]};
    ryanofsky commented at 6:45 pm on January 9, 2024:

    In commit “wallet, rpc: Add createwalletdescriptor RPC” (ed177c90c8e5d3b5f22b6744b725ff760b5897c1)

    Maybe consider calling the local variable something like like opt_internal or internal_only since there is another variable below called internal which shadows this.

    achow101 commented at 0:28 am on January 10, 2024:
    Done
  45. in src/wallet/rpc/wallet.cpp:981 in ed177c90c8 outdated 
    970+            } else {
971+                internals.push_back(internal.get_bool());
972+            }
973+
974+            LOCK(pwallet->cs_wallet);
975+            EnsureWalletIsUnlocked(*pwallet);
    ryanofsky commented at 6:46 pm on January 9, 2024:

    In commit “wallet, rpc: Add createwalletdescriptor RPC” (ed177c90c8e5d3b5f22b6744b725ff760b5897c1)

    It would be nice to drop the requirement that the wallet is unlocked. If this is just copying the key from one descriptor to another, there should not be a need to decrypt it, I would think.

    achow101 commented at 0:29 am on January 10, 2024:
    I think that would require a much more invasive refactor as it requires accessing much lower level data. Perhaps for a followup.
  46. in src/wallet/rpc/wallet.cpp:1020 in ed177c90c8 outdated 
    1015+                bool ok = spkm->GetDescriptorString(desc_str, false);
1016+                CHECK_NONFATAL(ok);
1017+                descs.push_back(desc_str);
1018+            }
1019+            UniValue out{UniValue::VOBJ};
1020+            out.pushKV("descs", descs);
    ryanofsky commented at 6:49 pm on January 9, 2024:

    In commit “wallet, rpc: Add createwalletdescriptor RPC” (ed177c90c8e5d3b5f22b6744b725ff760b5897c1)

    Could std::move

    achow101 commented at 0:28 am on January 10, 2024:
    Done
  47. in test/functional/wallet_createwalletdescriptor.py:52 in c10150b615 outdated 
    47+        curr_descs = set([(d["desc"], d["active"], d["internal"]) for d in wallet.listdescriptors(private=True)["descriptors"]])
48+        assert_equal(len(curr_descs), 1)
49+        assert_equal(len(wallet.gethdkeys()), 1)
50+
51+        old_descs = curr_descs
52+        wallet.createwalletdescriptor("bech32")
    ryanofsky commented at 6:53 pm on January 9, 2024:

    In commit “test: Add test for createwalletdescriptor” (c10150b6150083440af4f0aa1110c8aa99ba2dc8)

    Seems no test is not checking the createwalletdescriptor return value. Might be useful to add.

    achow101 commented at 0:28 am on January 10, 2024:
    Updated the test to check this.
  48. in src/wallet/rpc/wallet.cpp:1003 in ed177c90c8 outdated 
     998+            WalletBatch batch{pwallet->GetDatabase()};
 999+            for (bool internal : internals) {
1000+                WalletDescriptor w_desc = GenerateWalletDescriptor(xpub, *output_type, internal);
1001+                uint256 w_id = DescriptorID(*w_desc.descriptor);
1002+                if (pwallet->GetScriptPubKeyMan(w_id)) {
1003+                    throw JSONRPCError(RPC_WALLET_ERROR, "Descriptor already exists");
    ryanofsky commented at 7:06 pm on January 9, 2024:

    In commit “wallet, rpc: Add createwalletdescriptor RPC” (ed177c90c8e5d3b5f22b6744b725ff760b5897c1)

    It doesn’t seem great to throw in the middle of this loop, since it could result in a situation where one descriptor is partially created, but the RPC returns an error because the second descriptor already exists.

    Probably the nicer to thing would be to only throw if no descriptor was created:

    0for (bool internal : internals) {
1    WalletDescriptor w_desc = GenerateWalletDescriptor(xpub, *output_type, internal);
2    uint256 w_id = DescriptorID(*w_desc.descriptor);
3    // Don't create descriptors that already exist.
4    if (!pwallet->GetScriptPubKeyMan(w_id)) {
5        spkms.emplace_back(pwallet->SetupDescriptorScriptPubKeyMan(batch, active_hdkey, *output_type, internal));
6    }
7}
8if (spkms.empty()) throw JSONRPCError(RPC_WALLET_ERROR, "Descriptor already exists");
    achow101 commented at 0:28 am on January 10, 2024:
    Done
  49. ryanofsky approved
  50. ryanofsky commented at 7:11 pm on January 9, 2024: contributor
    Code review ACK c10150b6150083440af4f0aa1110c8aa99ba2dc8. Left a lot of suggestions, but none are important. Overall this looks great and is very cleanly implemented.
  51. achow101 force-pushed on Jan 10, 2024
  52. ryanofsky approved
  53. ryanofsky commented at 5:28 pm on January 10, 2024: contributor
    Code review ACK b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2. Looks great! Just suggested changes since last review
  54. DrahtBot added the label CI failed on Jan 15, 2024
  55. maflcko commented at 12:21 pm on February 12, 2024: member 
    0wallet/scriptpubkeyman.cpp:2161:42: error: no member named 'GetEncryptionKey' in 'wallet::WalletStorage'
1        if (!Assume(DecryptKey(m_storage.GetEncryptionKey(), crypted_secret, it->second.first, key))) {
2                               ~~~~~~~~~ ^
  56. in test/functional/wallet_backwards_compatibility.py:372 in b09e9ae6a6 outdated 
    367+                wallet.createwalletdescriptor("bech32m")
368+                xpubs = wallet.gethdkeys(active_only=True)
369+                assert_equal(len(xpubs), 1)
370+                assert_equal(len(xpubs[0]["descriptors"]), 8)
371+                tr_descs = [desc["desc"] for desc in xpubs[0]["descriptors"] if desc["desc"].startswith("tr(")]
372+                print(tr_descs)
    Sjors commented at 9:59 am on February 13, 2024:
    b09e9ae6a61a96d3e04619f7514a57470e220abe: nit drop print
    achow101 commented at 6:53 pm on February 13, 2024:
    Done
  57. in test/functional/wallet_createwalletdescriptor.py:61 in b55b46f07a outdated 
    56+        assert_equal(len(wallet.gethdkeys()), 1)
57+        assert_equal(new_descs, expected_descs)
58+
59+        self.log.info("Test descriptor creation options")
60+        old_descs = set([(d["desc"], d["active"], d["internal"]) for d in wallet.listdescriptors(private=True)["descriptors"]])
61+        wallet.createwalletdescriptor(type="bech32m", internal=False)["descs"]
    Sjors commented at 10:19 am on February 13, 2024:
    b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2: nit: ["descs"] is not used
    achow101 commented at 6:54 pm on February 13, 2024:
    Done
  58. in test/functional/wallet_gethdkeys.py:119 in 8221484b95 outdated 
    114+
115+        assert_equal(wallet.gethdkeys(), [])
116+        wallet.importdescriptors([{"desc": descsum_create("wpkh(cTe1f5rdT8A8DFgVWTjyPwACsDPJM9ff4QngFxUixCSvvbg1x6sh)"), "timestamp": "now"}])
117+        assert_equal(wallet.gethdkeys(), [])
118+
119+        self.log.info("Non-ranged HD keys should appear in gethdkeys")
    Sjors commented at 12:08 pm on February 13, 2024:
    8221484b95c9e376ea6cb437eb972457485b65a7: “HD keys of non-ranged descriptors” ?
    achow101 commented at 6:54 pm on February 13, 2024:
    Done
  59. Sjors approved
  60. Sjors commented at 12:21 pm on February 13, 2024: member

    Concept ACK. This removes a lot of the complexity compared to #26728.

    Did a light code review of b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2 which looks good, though clearly CI is unhappy.

  61. achow101 force-pushed on Feb 13, 2024
  62. achow101 commented at 6:54 pm on February 13, 2024: member
    Rebased for silent merge conflict.
  63. DrahtBot removed the label CI failed on Feb 13, 2024
  64. ryanofsky approved
  65. ryanofsky commented at 1:28 pm on February 15, 2024: contributor
    Code review ACK 0a3b5739ce8b9a2d219bcf3208069436c66fd5bb. Just rebase and some python test cleanups since last review
  66. DrahtBot requested review from Sjors on Feb 15, 2024
  67. Sjors commented at 2:02 pm on February 15, 2024: member
    Light utACK 0a3b5739ce8b9a2d219bcf3208069436c66fd5bb
  68. DrahtBot removed review request from Sjors on Feb 15, 2024
  69. DrahtBot added the label Needs rebase on Feb 20, 2024
  70. key: Add constructor for CExtKey that takes CExtPubKey and CKey 
    We often need to construct a CExtKey given an CExtPubKey and CKey, so
implement a constructor that does that for us.
    ef6745879d
  71. descriptor: Be able to get the pubkeys involved in a descriptor fe67841464
  72. achow101 force-pushed on Feb 20, 2024
  73. DrahtBot removed the label Needs rebase on Feb 20, 2024
  74. Sjors commented at 10:19 am on February 22, 2024: member
    re-utACK 36c82cd0e23b4149fb145d2dd271ab1de385c53a
  75. DrahtBot requested review from ryanofsky on Feb 22, 2024
  76. ryanofsky approved
  77. ryanofsky commented at 5:26 pm on February 23, 2024: contributor
    Code review ACK 36c82cd0e23b4149fb145d2dd271ab1de385c53a. No changes since last review other than rebase
  78. Pelambrds approved
  79. in src/wallet/scriptpubkeyman.cpp:2151 in 76a9e3e6e1 outdated 
    2143@@ -2143,6 +2144,37 @@ std::map<CKeyID, CKey> DescriptorScriptPubKeyMan::GetKeys() const
2144     return m_map_keys;
2145 }
2146 
2147+bool DescriptorScriptPubKeyMan::HasPrivKey(const CKeyID& keyid) const
2148+{
2149+    AssertLockHeld(cs_desc_man);
2150+    return m_map_keys.find(keyid) != m_map_keys.end()
2151+        || m_map_crypted_keys.find(keyid) != m_map_crypted_keys.end();
    furszy commented at 2:04 pm on March 13, 2024:

    In 76a9e3e6e1:

    tiny tiny nit: Since c++20, we got map.contains().

    0return m_map_keys.contains(keyid) || m_map_crypted_keys.contains(keyid);
    achow101 commented at 4:21 pm on March 18, 2024:
    Done
  80. in src/wallet/rpc/wallet.cpp:896 in 449636474a outdated 
    888+                w_desc.descriptor->GetPubKeys(desc_pubkeys, desc_xpubs);
889+                for (const CExtPubKey& xpub : desc_xpubs) {
890+                    wallet_xpubs[xpub].emplace(w_desc.descriptor->ToString(), wallet->IsActiveScriptPubKeyMan(*spkm), desc_spkm->HasPrivKey(xpub.pubkey.GetID()));
891+                    if (std::optional<CKey> key = desc_spkm->GetKey(xpub.pubkey.GetID())) {
892+                        wallet_xprvs[xpub] = CExtKey(xpub, *key);
893+                    }
    furszy commented at 10:00 pm on March 15, 2024:

    In 449636474a:

    Should only get the key when the “private” arg is true.

    achow101 commented at 4:21 pm on March 18, 2024:
    Done
  81. in src/wallet/rpc/wallet.cpp:929 in 9e3e731641 outdated 
    921@@ -922,6 +922,113 @@ RPCHelpMan gethdkeys()
922     };
923 }
924 
925+static RPCHelpMan createwalletdescriptor()
926+{
927+    return RPCHelpMan{"createwalletdescriptor",
928+        "Creates the wallet's descriptor for the given address type. "
929+        "Only works on wallets that contain automatically generated descriptors. "
    furszy commented at 1:41 pm on March 16, 2024:
    In 9e3e73164: The command also work on manually generated wallets; for instance, the user could have imported one (or many) ranged descriptor on an empty wallet.
    achow101 commented at 4:21 pm on March 18, 2024:
    Removed
  82. in src/wallet/rpc/wallet.cpp:1015 in 9e3e731641 outdated 
    1010+            if (spkms.empty()) {
1011+                throw JSONRPCError(RPC_WALLET_ERROR, "Descriptor already exists");
1012+            }
1013+
1014+            // Generate spks, fill caches, etc.
1015+            pwallet->TopUpKeyPool();
    furszy commented at 2:42 pm on March 16, 2024:
    Shouldn’t be needed, SetupDescriptorGeneration() calls TopUpWithDB() internally.
    achow101 commented at 4:21 pm on March 18, 2024:
    Removed
  83. in src/wallet/rpc/wallet.cpp:836 in 449636474a outdated 
    831+        RPCResult{RPCResult::Type::ARR, "", "", {
832+            {
833+                {RPCResult::Type::OBJ, "", "", {
834+                    {RPCResult::Type::STR, "xpub", "The extended public key"},
835+                    {RPCResult::Type::BOOL, "has_private", "Whether the wallet has the private key for this xpub"},
836+                    {RPCResult::Type::STR, "xprv", /*optional=*/true, "The extended private key if private is true"},
    furszy commented at 8:27 pm on March 16, 2024: 
    0                    {RPCResult::Type::STR, "xprv", /*optional=*/true, "The extended private key if \"private\" is true"},
    achow101 commented at 4:21 pm on March 18, 2024:
    Done
  84. in src/wallet/rpc/wallet.cpp:828 in 449636474a outdated 
    823+        "gethdkeys",
824+        "\nList all BIP 32 HD keys in the wallet and which descriptors use them.\n",
825+        {
826+            {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", {
827+                {"active_only", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show the keys for only active descriptors"},
828+                {"private", RPCArg::Type::BOOL, RPCArg::Default{false}, "Show private keys."}
    furszy commented at 8:29 pm on March 16, 2024:
    s/“Show private keys.”/“Show private keys” (without the “.” at the end). Or add the dot to the first argument.
    achow101 commented at 4:21 pm on March 18, 2024:
    Done
  85. in test/functional/wallet_gethdkeys.py:53 in 79206eaaf8 outdated 
    48+        assert_equal(xpub_info[0]["has_private"], True)
49+
50+        descs = wallet.listdescriptors(True)
51+        for desc in descs["descriptors"]:
52+            if "range" in desc:
53+                assert xprv in desc["desc"]
    furszy commented at 8:36 pm on March 16, 2024:

    In 79206eaaf8c4: nit: As the wallet generates ranged descriptors by default, can remove the if "range" in desc: line.

    Also, why you did not perform the same check with the xpub?

    achow101 commented at 4:21 pm on March 18, 2024:

    nit: As the wallet generates ranged descriptors by default, can remove the if "range" in desc: line.

    Done

    Also, why you did not perform the same check with the xpub?

    Since gethdkeys returns the master xpub but listdescriptors false returns normalized descriptors, we can’t easily do this check.

  86. in test/functional/wallet_gethdkeys.py:157 in 79206eaaf8 outdated 
    152+                for d in x["descriptors"]:
153+                    if d["desc"] == pub_multi_desc:
154+                        assert_equal(d["active"], False)
155+                        break
156+                else:
157+                    assert False
    furszy commented at 8:49 pm on March 16, 2024:
    In 79206eaa: What is the purpose behind the last ’else’ path? Existence check?
    achow101 commented at 3:44 pm on March 18, 2024:
    Yes. It is to check that the target descriptor was actually found.
    achow101 commented at 4:21 pm on March 18, 2024:
    Changed to use the suggestion below
  87. in test/functional/wallet_gethdkeys.py:188 in 79206eaaf8 outdated 
    183+        for d in xpub_info[0]["descriptors"]:
184+            if d["desc"] == pub_multi_desc:
185+                assert_equal(d["active"], False)
186+                break
187+        else:
188+            assert False
    furszy commented at 8:53 pm on March 16, 2024:

    In 79206eaa: Same as above; not sure what is the purpose behind the last ’else’ path. Could replace it for:

    0found_desc = next((d for d in xpub_info[0]["descriptors"] if d["desc"] == pub_multi_desc), None)
1assert found_desc is not None
2assert_equal(found_desc["active"], False)
    achow101 commented at 4:21 pm on March 18, 2024:
    Changed as suggested
  88. in src/wallet/rpc/wallet.cpp:1010 in 9e3e731641 outdated 
    1003+            for (bool internal : internals) {
1004+                WalletDescriptor w_desc = GenerateWalletDescriptor(xpub, *output_type, internal);
1005+                uint256 w_id = DescriptorID(*w_desc.descriptor);
1006+                if (!pwallet->GetScriptPubKeyMan(w_id)) {
1007+                    spkms.emplace_back(pwallet->SetupDescriptorScriptPubKeyMan(batch, active_hdkey, *output_type, internal));
1008+                }
    furszy commented at 9:31 pm on March 16, 2024:
    Future note: If we ever decide to batch this processes’ db write, will need to change the flow a bit (the db write failure at the end would leave the wallet in an inconsistent state with the spkm loaded in memory).
  89. furszy commented at 9:37 pm on March 16, 2024: member
    Concept ACK, haven’t finished yet. Only very small things so far. Will continue.
  90. furszy commented at 2:22 pm on March 18, 2024: member

    The introduced gethdkeys returns the descriptors in “public” format (the xpub at depth 0) while the existing listdescriptors returns them normalized (the xpub at the last hardened child).

    Users will not be able to derive the last hardened child only with the root xpub info on the descriptor. Is there any rationale behind this or was just an overlook?

  91. achow101 commented at 3:52 pm on March 18, 2024: member

    Users will not be able to derive the last hardened child only with the root xpub info on the descriptor. Is there any rationale behind this or was just an overlook?

    This was intentional since the last hardened child is different for each script type. I think future work, such as a revival of #22341, can make it possible to get keys at other derivation paths.

  92. desc spkm: Add functions to retrieve specific private keys fa6a259985
  93. wallet: Add IsActiveScriptPubKeyMan 
    Given a ScriptPubKeyMan, it's useful to ask the wallet whether it is
currently active.
    66632e5c24
  94. achow101 force-pushed on Mar 18, 2024
  95. furszy commented at 2:22 pm on March 19, 2024: member

    Users will not be able to derive the last hardened child only with the root xpub info on the descriptor. Is there any rationale behind this or was just an overlook?

    This was intentional since the last hardened child is different for each script type. I think future work, such as a revival of #22341, can make it possible to get keys at other derivation paths.

    But how the current output is useful? No outputted descriptor can be imported in another wallet. The wallet will throw a “Cannot expand descriptor. Probably because of hardened derivations without private keys provided”. What do you think about also retrieving the normalized descriptor in another field desc_normalized?

  96. achow101 force-pushed on Mar 19, 2024
  97. achow101 commented at 4:26 pm on March 19, 2024: member

    But how the current output is useful? No outputted descriptor can be imported in another wallet. The wallet will throw a “Cannot expand descriptor. Probably because of hardened derivations without private keys provided”. What do you think about also retrieving the normalized descriptor in another field desc_normalized?

    Sorry, misread your comment. The key is supposed to be the root, but the descriptors should be normalized. I’ve changed that.

  98. in src/wallet/rpc/wallet.cpp:891 in 21d0bc38ab outdated 
    886+                std::set<CPubKey> desc_pubkeys;
887+                std::set<CExtPubKey> desc_xpubs;
888+                w_desc.descriptor->GetPubKeys(desc_pubkeys, desc_xpubs);
889+                for (const CExtPubKey& xpub : desc_xpubs) {
890+                    std::string desc_str;
891+                    desc_spkm->GetDescriptorString(desc_str, false);
    furszy commented at 1:59 pm on March 20, 2024:

    In 21d0bc38ab:

    Note about this: This works on encrypted wallets because the last hardened extended pubkey is cached after creation. If it wouldn’t be cached for some reason, this command would fail (gethdkeys does not require the wallet to be unlocked).

    Could, as a simple error catching method, check the GetDescriptorString return value.

    achow101 commented at 8:16 pm on March 20, 2024:
    It shouldn’t fail so I’ve made added a CHECK_NONFATAL. Also added [[nodiscard]] to GetDescriptorString.
  99. in src/wallet/rpc/wallet.cpp:906 in 21d0bc38ab outdated 
    900+            for (const auto& [xpub, descs] : wallet_xpubs) {
901+                bool has_xprv = false;
902+                UniValue descriptors(UniValue::VARR);
903+                for (const auto& [desc, active, has_priv] : descs) {
904+                    UniValue d(UniValue::VOBJ);
905+                    d.pushKV("desc", desc);
    furszy commented at 2:14 pm on March 20, 2024:

    In 21d0bc38ab:

    Better to return the descriptors sorted:

     0diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
 1--- a/src/wallet/rpc/wallet.cpp	(revision d7fc8a8bec5edb657f65ef35042d801e2913ab81)
 2+++ b/src/wallet/rpc/wallet.cpp	(date 1710943877002)
 3@@ -874,7 +874,7 @@
 4                 spkms = wallet->GetAllScriptPubKeyMans();
 5             }
 6 
 7-            std::map<CExtPubKey, std::set<std::tuple<std::string, bool, bool>>> wallet_xpubs;
 8+            std::map<CExtPubKey, std::vector<std::tuple<std::string, bool, bool>>> wallet_xpubs;
 9             std::map<CExtPubKey, CExtKey> wallet_xprvs;
10             for (auto* spkm : spkms) {
11                 auto* desc_spkm{dynamic_cast<DescriptorScriptPubKeyMan*>(spkm)};
12@@ -889,7 +889,7 @@
13                 for (const CExtPubKey& xpub : desc_xpubs) {
14                     std::string desc_str;
15                     desc_spkm->GetDescriptorString(desc_str, false);
16-                    wallet_xpubs[xpub].emplace(desc_str, wallet->IsActiveScriptPubKeyMan(*spkm), desc_spkm->HasPrivKey(xpub.pubkey.GetID()));
17+                    wallet_xpubs[xpub].emplace_back(desc_str, wallet->IsActiveScriptPubKeyMan(*spkm), desc_spkm->HasPrivKey(xpub.pubkey.GetID()));
18                     if (std::optional<CKey> key = priv ? desc_spkm->GetKey(xpub.pubkey.GetID()) : std::nullopt) {
19                         wallet_xprvs[xpub] = CExtKey(xpub, *key);
20                     }
21@@ -897,9 +897,15 @@
22             }
23 
24             UniValue response(UniValue::VARR);
25-            for (const auto& [xpub, descs] : wallet_xpubs) {
26-                bool has_xprv = false;
27+            for (auto& [xpub, descs] : wallet_xpubs) {
28                 UniValue descriptors(UniValue::VARR);
29+
30+                // Sort descriptors
31+                std::sort(descs.begin(), descs.end(), [](const auto& a, const auto& b) {
32+                    return std::get<0>(a) < std::get<0>(b);
33+                });
34+
35+                bool has_xprv = false;
36                 for (const auto& [desc, active, has_priv] : descs) {
37                     UniValue d(UniValue::VOBJ);
38                     d.pushKV("desc", desc);
    achow101 commented at 8:05 pm on March 20, 2024:
    I don’t think it’s meaningfully useful to explicitly sort them, and we don’t do that anywhere else. Also, std::set does sorting itself, so I don’t think this would even change the output.
    furszy commented at 8:17 pm on March 20, 2024:

    I don’t think it’s meaningfully useful to explicitly sort them, and we don’t do that anywhere else.

    We sort them in listdescriptors: src/wallet/rpc/backup.cpp#L1827.

    Also, std::set does sorting itself, so I don’t think this would even change the output.

    std::set sorts based on the top element, the std::tuple in this case. Not based on the descriptor string.

    Yet, this was just an UX nit. Either way is fine for me.

    achow101 commented at 8:23 pm on March 20, 2024:
    Sorting on std::tuple will sort lexicographically on the items in the tuple. Since the descriptor string is the first item in the tuple, and are unique, it should be sorted by the descriptor strings.
    furszy commented at 9:17 pm on March 20, 2024:

    Sorting on std::tuple will sort lexicographically on the items in the tuple. Since the descriptor string is the first item in the tuple, and are unique, it should be sorted by the descriptor strings.

    Hmm, I was sure I saw a different ordering this morning. But 🤷🏼 ..

    Sjors commented at 10:33 am on March 25, 2024:
    Just tried and they’re neatly grouped by type. tr() is listed before wpkh which takes getting used to since we introduced tr() later, but such is the alphabet.
  100. in src/wallet/wallet.h:945 in 66632e5c24 outdated 
    941@@ -942,6 +942,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
942 
943     //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
944     std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
945+    bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const;
    furszy commented at 2:29 pm on March 20, 2024:

    tiny nit: To follow the same pattern used across this class, should pass the spkm pointer instead of the const ref (and return early if it is nullptr).

    Side note: It is not really something that we should tackle nor discuss here but I think it would be better to return the is_active, and other spkm information, on the initial lookup function instead of introducing multiple functions that retrieves the information after wise. E.g. GetAllScriptPubKeyMansInfo() could return a struct instead of the bare spkm pointers.

    achow101 commented at 8:01 pm on March 20, 2024:
    It’s probably not a great idea to be passing around raw pointers and we should be moving away from doing that.
  101. in src/wallet/wallet.cpp:4490 in 5f3e3cd131 outdated 
    4485@@ -4486,4 +4486,25 @@ void CWallet::TopUpCallback(const std::set<CScript>& spks, ScriptPubKeyMan* spkm
4486     // Update scriptPubKey cache
4487     CacheNewScriptPubKeys(spks, spkm);
4488 }
4489+
4490+std::set<CExtPubKey> CWallet::GetActiveHDPubKeys() const
    furszy commented at 2:36 pm on March 20, 2024:

    I do agree on encapsulating this function but have to say that it’s inconsistent to how previous commits on this PR were implemented (gethdkeys accesses the wallet internals to obtain the same information).

    Yet, this is not blocking. Just a comment. We lack of an structure here.

    achow101 commented at 8:06 pm on March 20, 2024:
    gethdkeys can get pubkeys for inactive descriptors, so it can’t use this function.
  102. furszy commented at 2:44 pm on March 20, 2024: member
    Code review ACK d7fc8a8bec5ed
  103. DrahtBot requested review from Sjors on Mar 20, 2024
  104. DrahtBot requested review from ryanofsky on Mar 20, 2024
  105. wallet, rpc: Add gethdkeys RPC 
    gethdkeys retrieves all HD keys stored in the wallet's descriptors.
    5febe28c9e
  106. tests: Test for gethdkeys 3b09d0eb7f
  107. wallet: Refactor function for single DescSPKM setup 
    We will need access to a function that sets up a singular
DescriptorSPKM, so refactor this out of the multiple DescriptorSPKM
setup function.
    54e74f46ea
  108. wallet, descspkm: Refactor wallet descriptor generation to standalone func 73926f2d31
  109. wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors 85b1fb19dd
  110. wallet: Be able to retrieve single key from descriptors 
    Adds CWallet::GetKey which retrieves a single key from the descriptors
stored in the wallet.
    8e1a475062
  111. wallet, rpc: Add createwalletdescriptor RPC 460ae1bf67
  112. wallet: Test upgrade of pre-taproot wallet to have tr() descriptors 2402b63062
  113. test: Add test for createwalletdescriptor 746b6d8839
  114. achow101 force-pushed on Mar 20, 2024
  115. furszy commented at 9:20 pm on March 20, 2024: member
    ACK 746b6d8
  116. bitcoin deleted a comment on Mar 24, 2024
  117. Sjors commented at 10:41 am on March 25, 2024: member

    re-utACK 746b6d88395607abbd3c13bbdcdd4ca83e9bc9e4

    The key is supposed to be the root, but the descriptors should be normalized. I’ve changed that.

    That’s much better.

    It would be nice if you can call createwalletdescriptor on a blank wallet and it just creates a seed and adds the descriptor. @ryanofsky above suggested an addition generate keyword for that: #29130 (review)

    But we might as well do that in a followup along with some of the other suggestions in the that comment. For setting a up a multisig it’s useful to have a seed without descriptor and a way to get an xpub at an arbitrary path from it. Right now, with or without this PR, you can only achieve that by creating a dummy wallet and taking the root xpriv. You then manually create the multisig descriptor, with the xpriv as your key and the xpub from your counterpary and import that that in the eventual multisig wallet. Then you use listdescriptors to get the xpub to hand to your counterparty.

    But if they also use Bitcoin Core you have a chicken-egg problem (which involves more workarounds).

  118. furszy commented at 1:15 pm on March 25, 2024: member

    It would be nice if you can call createwalletdescriptor on a blank wallet and it just creates a seed and adds the descriptor. @ryanofsky above suggested an addition generate keyword for that: #29130 (review)

    Agree. Also @Sjors, another possibility could be adding a ‘generate-seed’ command to the bitcoin-wallet binary.

  119. ryanofsky approved
  120. ryanofsky commented at 4:11 pm on March 28, 2024: contributor

    Code review ACK 746b6d88395607abbd3c13bbdcdd4ca83e9bc9e4, and this looks ready to merge. There were various suggested changes since last review where main change seems to be switching gethdkeys output to use normalized descriptors (removing hardened path components).

    It does seem good to normalize descriptors so they can be imported without private keys, and to make gethdkeys consistent with listdescriptors. But I’m a little unclear about whether we always normalize descriptors in RPC output, or if there are cases where they are not normalized? It does seem like it could be potentially confusing to normalize descriptors and make them look different, but I’m not sure if we always do this.

  121. ryanofsky merged this on Mar 29, 2024
  122. ryanofsky closed this on Mar 29, 2024
  123. ryanofsky commented at 10:46 am on March 29, 2024: contributor

    But I’m a little unclear about whether we always normalize descriptors in RPC output, or if there are cases where they are not normalized?

    Merged but still curious about this. I wonder if we should rename descriptor ToString() method to something like ToInternalString() or OriginalString() to prevent this mistake from happening again, if preference is to output normalized descriptors.

  124. Sjors commented at 9:53 am on April 2, 2024: member

    🎉

    Normalized strings (and with public keys only) should be the default behavior imo. There’s (at least) one reason to deviate:

    • when you import a descriptor from an external source, and then that external source asks for it again, it may end up confused (and the checksum won’t match). An example of this might be (though haven’t tested recently) Specter Desktop, which composes a multisig descriptors and gives it to Bitcoin Core. It also stores them in its own storage. If it then later needs to use the wallet, it needs some way to sanity check that it loaded the right one, and calling listdescriptors is one way. Similarly the displayaddress call in HWI expects a descriptor and (last time I checked) it’s a bit picky about the format.
  125. Hivoltazh approved


achow101 DrahtBot ryanofsky maflcko Sjors Pelambrds furszy Hivoltazh


Sjors

Labels
Wallet

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