descriptor: ToPrivateString() pass if at least 1 priv key exists

- Refactor Descriptor::ToPrivateString() to allow descriptors with
  missing private keys to be printed. Useful in descriptors with
  multiple keys e.g tr() etc.
- The existing behaviour of listdescriptors is preserved as much as
  possible, if no private keys are availablle ToPrivateString will
  return false

src/script/descriptor.cpp

@@ -877,13 +877,19 @@ public:
     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)
@@ -892,6 +898,11 @@ public:
         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;
@@ -900,7 +911,7 @@ public:
                     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();
@@ -912,10 +923,12 @@ public:
             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
@@ -927,9 +940,9 @@ public:
 
     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
@@ -1410,8 +1423,20 @@ protected:
     }
     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]) {
@@ -1419,7 +1444,9 @@ protected:
                 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 += '}';
@@ -1427,7 +1454,7 @@ protected:
             }
             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) :
@@ -1520,15 +1547,16 @@ public:
                 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 {};
@@ -1568,11 +1596,15 @@ public:
     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))) {
+        bool has_priv_key{false};
-            out = *res;
+        auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache), has_priv_key);
-            return true;
+        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; }
@@ -2110,7 +2142,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();
     }
@@ -2507,7 +2539,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

@@ -118,7 +118,13 @@ struct Descriptor {
      */
     virtual bool HavePrivateKeys(const SigningProvider& provider) const = 0;
 
-    /** Convert the descriptor to a private string. This fails if the provided provider does not have the relevant private keys. */
+    /** 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

@@ -826,6 +826,12 @@ public:
 
     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 ":".
@@ -838,10 +844,16 @@ public:
                     (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) {
@@ -850,13 +862,13 @@ public:
                 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) + ")";
                     }
@@ -877,12 +889,12 @@ public:
             }
             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) + ")";
                 }
@@ -908,7 +920,7 @@ public:
                     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);
                     }
@@ -918,7 +930,7 @@ public:
                     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. ToPrivateString() will fail and HavePrivateKeys() will return `false`.
+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
@@ -264,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

src/test/fuzz/miniscript.cpp

@@ -124,10 +124,14 @@ struct ParserContext {
         return a < b;
     }
 
-    std::optional<std::string> ToString(const Key& key) const
+    std::optional<std::string> ToString(const Key& key, bool& has_priv_key) const
     {
+        has_priv_key = false;
         auto it = TEST_DATA.dummy_key_idx_map.find(key);
-        if (it == TEST_DATA.dummy_key_idx_map.end()) return {};
+        if (it == TEST_DATA.dummy_key_idx_map.end()) {
+            return HexStr(key);
+        }
+        has_priv_key = true;
         uint8_t idx = it->second;
         return HexStr(std::span{&idx, 1});
     }

src/test/miniscript_tests.cpp

@@ -188,7 +188,7 @@ struct KeyConverter {
         return g_testdata->pkmap.at(keyid);
     }
 
-    std::optional<std::string> ToString(const Key& key) const {
+    std::optional<std::string> ToString(const Key& key, bool&) const {
         return HexStr(ToPKBytes(key));
     }