Merge bitcoin/bitcoin#32471: wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key

9c7e477 test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a6 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e982 descriptor: ToPrivateString() pass if  at least 1 priv key exists (Novo)
5c4db25 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3 wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb9 descriptors: add HavePrivateKeys() (Novo)

Pull request description:

  _TLDR:
  Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_

  In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](bitcoin/bitcoin#32078 (comment))). This change makes it possible to do so.

  This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys.

  ### Notes
  - The new behaviour is applied to all descriptors including miniscript descriptors
  - `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour bitcoin/bitcoin#24361 (comment)
  - Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet.

  ### Relevant PRs
  bitcoin/bitcoin#24361
  bitcoin/bitcoin#32186

  ### Testing
  Functional tests were added to test the new behaviour

  EDIT
  **`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error**

ACKs for top commit:
  Sjors:
    ACK 9c7e477
  achow101:
    ACK 9c7e477
  w0xlt:
    reACK bitcoin/bitcoin@9c7e477 with minor nits
  rkrux:
    re-ACK 9c7e477

Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60

Author: achow101

src/script/descriptor.cpp

@@ -204,7 +204,11 @@ struct PubkeyProvider
     /** Get the descriptor string form. */
     virtual std::string ToString(StringType type=StringType::PUBLIC) const = 0;
 
-    /** Get the descriptor string form including private data (if available in arg). */
+    /** Get the descriptor string form including private data (if available in arg).
+     *  If the private data is not available, the output string in the "out" parameter
+     *  will not contain any private key information,
+     *  and this function will return "false".
+     */
     virtual bool ToPrivateString(const SigningProvider& arg, std::string& out) const = 0;
 
     /** Get the descriptor string form with the xpub at the last hardened derivation,
@@ -260,9 +264,9 @@ class OriginPubkeyProvider final : public PubkeyProvider
     bool ToPrivateString(const SigningProvider& arg, std::string& ret) const override
     {
         std::string sub;
-        if (!m_provider->ToPrivateString(arg, sub)) return false;
+        bool has_priv_key{m_provider->ToPrivateString(arg, sub)};
         ret = "[" + OriginString(StringType::PUBLIC) + "]" + std::move(sub);
-        return true;
+        return has_priv_key;
     }
     bool ToNormalizedString(const SigningProvider& arg, std::string& ret, const DescriptorCache* cache) const override
     {
@@ -329,7 +333,10 @@ class ConstPubkeyProvider final : public PubkeyProvider
     bool ToPrivateString(const SigningProvider& arg, std::string& ret) const override
     {
         std::optional<CKey> key = GetPrivKey(arg);
-        if (!key) return false;
+        if (!key) {
+            ret = ToString(StringType::PUBLIC);
+            return false;
+        }
         ret = EncodeSecret(*key);
         return true;
     }
@@ -492,7 +499,10 @@ class BIP32PubkeyProvider final : public PubkeyProvider
     bool ToPrivateString(const SigningProvider& arg, std::string& out) const override
     {
         CExtKey key;
-        if (!GetExtKey(arg, key)) return false;
+        if (!GetExtKey(arg, key)) {
+            out = ToString(StringType::PUBLIC);
+            return false;
+        }
         out = EncodeExtKey(key) + FormatHDKeypath(m_path, /*apostrophe=*/m_apostrophe);
         if (IsRange()) {
             out += "/*";
@@ -710,17 +720,14 @@ class MuSigPubkeyProvider final : public PubkeyProvider
             std::string tmp;
             if (pubkey->ToPrivateString(arg, tmp)) {
                 any_privkeys = true;
-                out += tmp;
-            } else {
-                out += pubkey->ToString();
             }
+            out += tmp;
         }
         out += ")";
         out += FormatHDKeypath(m_path);
         if (IsRangedDerivation()) {
             out += "/*";
         }
-        if (!any_privkeys) out.clear();
         return any_privkeys;
     }
     bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache = nullptr) const override
@@ -837,6 +844,25 @@ class DescriptorImpl : public Descriptor
         return true;
     }
 
+    // NOLINTNEXTLINE(misc-no-recursion)
+    bool HavePrivateKeys(const SigningProvider& arg) const override
+    {
+        if (m_pubkey_args.empty() && m_subdescriptor_args.empty()) return false;
+
+        for (const auto& sub: m_subdescriptor_args) {
+            if (!sub->HavePrivateKeys(arg)) return false;
+        }
+
+        FlatSigningProvider tmp_provider;
+        for (const auto& pubkey : m_pubkey_args) {
+            tmp_provider.keys.clear();
+            pubkey->GetPrivKey(0, arg, tmp_provider);
+            if (tmp_provider.keys.empty()) return false;
+        }
+
+        return true;
+    }
+
     // NOLINTNEXTLINE(misc-no-recursion)
     bool IsRange() const final
     {
@@ -853,13 +879,19 @@ class DescriptorImpl : public Descriptor
     virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const
     {
         size_t pos = 0;
+        bool is_private{type == StringType::PRIVATE};
+        // For private string output, track if at least one key has a private key available.
+        // Initialize to true for non-private types.
+        bool any_success{!is_private};
         for (const auto& scriptarg : m_subdescriptor_args) {
             if (pos++) ret += ",";
             std::string tmp;
-            if (!scriptarg->ToStringHelper(arg, tmp, type, cache)) return false;
+            bool subscript_res{scriptarg->ToStringHelper(arg, tmp, type, cache)};
+            if (!is_private && !subscript_res) return false;
+            any_success = any_success || subscript_res;
             ret += tmp;
         }
-        return true;
+        return any_success;
     }
 
     // NOLINTNEXTLINE(misc-no-recursion)
@@ -868,6 +900,11 @@ class DescriptorImpl : public Descriptor
         std::string extra = ToStringExtra();
         size_t pos = extra.size() > 0 ? 1 : 0;
         std::string ret = m_name + "(" + extra;
+        bool is_private{type == StringType::PRIVATE};
+        // For private string output, track if at least one key has a private key available.
+        // Initialize to true for non-private types.
+        bool any_success{!is_private};
+
         for (const auto& pubkey : m_pubkey_args) {
             if (pos++) ret += ",";
             std::string tmp;
@@ -876,7 +913,7 @@ class DescriptorImpl : public Descriptor
                     if (!pubkey->ToNormalizedString(*arg, tmp, cache)) return false;
                     break;
                 case StringType::PRIVATE:
-                    if (!pubkey->ToPrivateString(*arg, tmp)) return false;
+                    any_success = pubkey->ToPrivateString(*arg, tmp) || any_success;
                     break;
                 case StringType::PUBLIC:
                     tmp = pubkey->ToString();
@@ -888,10 +925,12 @@ class DescriptorImpl : public Descriptor
             ret += tmp;
         }
         std::string subscript;
-        if (!ToStringSubScriptHelper(arg, subscript, type, cache)) return false;
+        bool subscript_res{ToStringSubScriptHelper(arg, subscript, type, cache)};
+        if (!is_private && !subscript_res) return false;
+        any_success = any_success || subscript_res;
         if (pos && subscript.size()) ret += ',';
         out = std::move(ret) + std::move(subscript) + ")";
-        return true;
+        return any_success;
     }
 
     std::string ToString(bool compat_format) const final
@@ -903,9 +942,9 @@ class DescriptorImpl : public Descriptor
 
     bool ToPrivateString(const SigningProvider& arg, std::string& out) const override
     {
-        bool ret = ToStringHelper(&arg, out, StringType::PRIVATE);
+        bool has_priv_key{ToStringHelper(&arg, out, StringType::PRIVATE)};
         out = AddChecksum(out);
-        return ret;
+        return has_priv_key;
     }
 
     bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache) const override final
@@ -1396,24 +1435,38 @@ class TRDescriptor final : public DescriptorImpl
     }
     bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const override
     {
-        if (m_depths.empty()) return true;
+        if (m_depths.empty()) {
+            // If there are no sub-descriptors and a PRIVATE string
+            // is requested, return `false` to indicate that the presence
+            // of a private key depends solely on the internal key (which is checked
+            // in the caller), not on any sub-descriptor. This ensures correct behavior for
+            // descriptors like tr(internal_key) when checking for private keys.
+            return type != StringType::PRIVATE;
+        }
         std::vector<bool> path;
+        bool is_private{type == StringType::PRIVATE};
+        // For private string output, track if at least one key has a private key available.
+        // Initialize to true for non-private types.
+        bool any_success{!is_private};
+
         for (size_t pos = 0; pos < m_depths.size(); ++pos) {
             if (pos) ret += ',';
             while ((int)path.size() <= m_depths[pos]) {
                 if (path.size()) ret += '{';
                 path.push_back(false);
             }
             std::string tmp;
-            if (!m_subdescriptor_args[pos]->ToStringHelper(arg, tmp, type, cache)) return false;
+            bool subscript_res{m_subdescriptor_args[pos]->ToStringHelper(arg, tmp, type, cache)};
+            if (!is_private && !subscript_res) return false;
+            any_success = any_success || subscript_res;
             ret += tmp;
             while (!path.empty() && path.back()) {
                 if (path.size() > 1) ret += '}';
                 path.pop_back();
             }
             if (!path.empty()) path.back() = true;
         }
-        return true;
+        return any_success;
     }
 public:
     TRDescriptor(std::unique_ptr<PubkeyProvider> internal_key, std::vector<std::unique_ptr<DescriptorImpl>> descs, std::vector<int> depths) :
@@ -1506,15 +1559,16 @@ class StringMaker {
                 const DescriptorCache* cache LIFETIMEBOUND)
         : m_arg(arg), m_pubkeys(pubkeys), m_type(type), m_cache(cache) {}
 
-    std::optional<std::string> ToString(uint32_t key) const
+    std::optional<std::string> ToString(uint32_t key, bool& has_priv_key) const
     {
         std::string ret;
+        has_priv_key = false;
         switch (m_type) {
         case DescriptorImpl::StringType::PUBLIC:
             ret = m_pubkeys[key]->ToString();
             break;
         case DescriptorImpl::StringType::PRIVATE:
-            if (!m_pubkeys[key]->ToPrivateString(*m_arg, ret)) return {};
+            has_priv_key = m_pubkeys[key]->ToPrivateString(*m_arg, ret);
             break;
         case DescriptorImpl::StringType::NORMALIZED:
             if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {};
@@ -1571,11 +1625,15 @@ class MiniscriptDescriptor final : public DescriptorImpl
     bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type,
                         const DescriptorCache* cache = nullptr) const override
     {
-        if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache))) {
-            out = *res;
-            return true;
+        bool has_priv_key{false};
+        auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache), has_priv_key);
+        if (res) out = *res;
+        if (type == StringType::PRIVATE) {
+            Assume(res.has_value());
+            return has_priv_key;
+        } else {
+            return res.has_value();
         }
-        return false;
     }
 
     bool IsSolvable() const override { return true; }
@@ -2113,7 +2171,7 @@ struct KeyParser {
         return key;
     }
 
-    std::optional<std::string> ToString(const Key& key) const
+    std::optional<std::string> ToString(const Key& key, bool&) const
     {
         return m_keys.at(key).at(0)->ToString();
     }
@@ -2510,7 +2568,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
                 // Try to find the first insane sub for better error reporting.
                 auto insane_node = node.get();
                 if (const auto sub = node->FindInsaneSub()) insane_node = sub;
-                if (const auto str = insane_node->ToString(parser)) error = *str;
+                error = *insane_node->ToString(parser);
                 if (!insane_node->IsValid()) {
                     error += " is invalid";
                 } else if (!node->IsSane()) {

src/script/descriptor.h

@@ -111,7 +111,20 @@ struct Descriptor {
     /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
     virtual bool IsSingleType() const = 0;
 
-    /** Convert the descriptor to a private string. This fails if the provided provider does not have the relevant private keys. */
+    /** Whether the given provider has all private keys required by this descriptor.
+     * @return `false` if the descriptor doesn't have any keys or subdescriptors,
+     *         or if the provider does not have all private keys required by
+     *         the descriptor.
+     */
+    virtual bool HavePrivateKeys(const SigningProvider& provider) const = 0;
+
+    /** Convert the descriptor to a private string. This uses public keys if the relevant private keys are not in the SigningProvider.
+     *  If none of the relevant private keys are available, the output string in the "out" parameter will not contain any private key information,
+     *  and this function will return "false".
+     *  @param[in] provider The SigningProvider to query for private keys.
+     *  @param[out] out The resulting descriptor string, containing private keys if available.
+     *  @returns true if at least one private key available.
+     */
     virtual bool ToPrivateString(const SigningProvider& provider, std::string& out) const = 0;
 
     /** Convert the descriptor to a normalized string. Normalized descriptors have the xpub at the last hardened step. This fails if the provided provider does not have the private keys to derive that xpub. */

src/script/miniscript.h

@@ -843,6 +843,12 @@ struct Node {
 
     template<typename CTx>
     std::optional<std::string> ToString(const CTx& ctx) const {
+        bool dummy{false};
+        return ToString(ctx, dummy);
+    }
+
+    template<typename CTx>
+    std::optional<std::string> ToString(const CTx& ctx, bool& has_priv_key) const {
         // To construct the std::string representation for a Miniscript object, we use
         // the TreeEvalMaybe algorithm. The State is a boolean: whether the parent node is a
         // wrapper. If so, non-wrapper expressions must be prefixed with a ":".
@@ -855,10 +861,16 @@ struct Node {
                     (node.fragment == Fragment::OR_I && node.subs[0]->fragment == Fragment::JUST_0) ||
                     (node.fragment == Fragment::OR_I && node.subs[1]->fragment == Fragment::JUST_0));
         };
+        auto toString = [&ctx, &has_priv_key](Key key) -> std::optional<std::string> {
+            bool fragment_has_priv_key{false};
+            auto key_str{ctx.ToString(key, fragment_has_priv_key)};
+            if (key_str) has_priv_key = has_priv_key || fragment_has_priv_key;
+            return key_str;
+        };
         // The upward function computes for a node, given whether its parent is a wrapper,
         // and the string representations of its child nodes, the string representation of the node.
         const bool is_tapscript{IsTapscript(m_script_ctx)};
-        auto upfn = [&ctx, is_tapscript](bool wrapped, const Node& node, std::span<std::string> subs) -> std::optional<std::string> {
+        auto upfn = [is_tapscript, &toString](bool wrapped, const Node& node, std::span<std::string> subs) -> std::optional<std::string> {
             std::string ret = wrapped ? ":" : "";
 
             switch (node.fragment) {
@@ -867,13 +879,13 @@ struct Node {
                 case Fragment::WRAP_C:
                     if (node.subs[0]->fragment == Fragment::PK_K) {
                         // pk(K) is syntactic sugar for c:pk_k(K)
-                        auto key_str = ctx.ToString(node.subs[0]->keys[0]);
+                        auto key_str = toString(node.subs[0]->keys[0]);
                         if (!key_str) return {};
                         return std::move(ret) + "pk(" + std::move(*key_str) + ")";
                     }
                     if (node.subs[0]->fragment == Fragment::PK_H) {
                         // pkh(K) is syntactic sugar for c:pk_h(K)
-                        auto key_str = ctx.ToString(node.subs[0]->keys[0]);
+                        auto key_str = toString(node.subs[0]->keys[0]);
                         if (!key_str) return {};
                         return std::move(ret) + "pkh(" + std::move(*key_str) + ")";
                     }
@@ -894,12 +906,12 @@ struct Node {
             }
             switch (node.fragment) {
                 case Fragment::PK_K: {
-                    auto key_str = ctx.ToString(node.keys[0]);
+                    auto key_str = toString(node.keys[0]);
                     if (!key_str) return {};
                     return std::move(ret) + "pk_k(" + std::move(*key_str) + ")";
                 }
                 case Fragment::PK_H: {
-                    auto key_str = ctx.ToString(node.keys[0]);
+                    auto key_str = toString(node.keys[0]);
                     if (!key_str) return {};
                     return std::move(ret) + "pk_h(" + std::move(*key_str) + ")";
                 }
@@ -925,7 +937,7 @@ struct Node {
                     CHECK_NONFATAL(!is_tapscript);
                     auto str = std::move(ret) + "multi(" + util::ToString(node.k);
                     for (const auto& key : node.keys) {
-                        auto key_str = ctx.ToString(key);
+                        auto key_str = toString(key);
                         if (!key_str) return {};
                         str += "," + std::move(*key_str);
                     }
@@ -935,7 +947,7 @@ struct Node {
                     CHECK_NONFATAL(is_tapscript);
                     auto str = std::move(ret) + "multi_a(" + util::ToString(node.k);
                     for (const auto& key : node.keys) {
-                        auto key_str = ctx.ToString(key);
+                        auto key_str = toString(key);
                         if (!key_str) return {};
                         str += "," + std::move(*key_str);
                     }

src/test/descriptor_tests.cpp

@@ -49,7 +49,7 @@ constexpr int SIGNABLE = 1 << 3; // We can sign with this descriptor (this is no
 constexpr int DERIVE_HARDENED = 1 << 4; // The final derivation is hardened, i.e. ends with *' or *h
 constexpr int MIXED_PUBKEYS = 1 << 5;
 constexpr int XONLY_KEYS = 1 << 6; // X-only pubkeys are in use (and thus inferring/caching may swap parity of pubkeys/keyids)
-constexpr int MISSING_PRIVKEYS = 1 << 7; // Not all private keys are available, so ToPrivateString will fail.
+constexpr int MISSING_PRIVKEYS = 1 << 7; // Not all private keys are available. ToPrivateString() will return true if there is at least one private key and HavePrivateKeys() will return `false`.
 constexpr int SIGNABLE_FAILS = 1 << 8; // We can sign with this descriptor, but actually trying to sign will fail
 constexpr int MUSIG = 1 << 9; // This is a MuSig so key counts will have an extra key
 constexpr int MUSIG_DERIVATION = 1 << 10; // MuSig with BIP 328 derivation from the aggregate key
@@ -243,6 +243,9 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
         } else {
             BOOST_CHECK_MESSAGE(EqualDescriptor(prv, prv1), "Private ser: " + prv1 + " Private desc: " + prv);
         }
+        BOOST_CHECK(!parse_priv->HavePrivateKeys(keys_pub));
+        BOOST_CHECK(parse_pub->HavePrivateKeys(keys_priv));
+
         BOOST_CHECK(!parse_priv->ToPrivateString(keys_pub, prv1));
         BOOST_CHECK(parse_pub->ToPrivateString(keys_priv, prv1));
         if (expected_prv) {
@@ -261,6 +264,12 @@ void DoCheck(std::string prv, std::string pub, const std::string& norm_pub, int
         parse_pub->ExpandPrivate(0, keys_priv, pub_prov);
 
         BOOST_CHECK_MESSAGE(EqualSigningProviders(priv_prov, pub_prov), "Private desc: " + prv + " Pub desc: " + pub);
+    } else if (keys_priv.keys.size() > 0) {
+        // If there is at least one private key, ToPrivateString() should return true and include that key
+        std::string prv_str;
+        BOOST_CHECK(parse_priv->ToPrivateString(keys_priv, prv_str));
+        size_t checksum_len = 9; // Including the '#' character
+        BOOST_CHECK_MESSAGE(prv == prv_str.substr(0, prv_str.length() - checksum_len), prv);
     }
 
     // Check that private can produce the normalized descriptors