diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 89af1d9367f..89766b52cff 100644 --- a/src/script/descriptor.cpp +++ b/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 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 internal_key, std::vector> descs, std::vector depths) : @@ -1520,15 +1547,16 @@ public: const DescriptorCache* cache LIFETIMEBOUND) : m_arg(arg), m_pubkeys(pubkeys), m_type(type), m_cache(cache) {} - std::optional ToString(uint32_t key) const + std::optional 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))) { - 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; } @@ -2110,7 +2142,7 @@ struct KeyParser { return key; } - std::optional ToString(const Key& key) const + std::optional ToString(const Key& key, bool&) const { return m_keys.at(key).at(0)->ToString(); } @@ -2507,7 +2539,7 @@ std::vector> 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()) { diff --git a/src/script/descriptor.h b/src/script/descriptor.h index c7863933ef9..47c568857e5 100644 --- a/src/script/descriptor.h +++ b/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. */ diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 54ae777cf92..6e684e6d9f2 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -826,6 +826,12 @@ public: template std::optional ToString(const CTx& ctx) const { + bool dummy{false}; + return ToString(ctx, dummy); + } + + template + std::optional 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 { + 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 subs) -> std::optional { + auto upfn = [is_tapscript, &toString](bool wrapped, const Node& node, std::span subs) -> std::optional { 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); } diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 17c756b6033..d49a4885ba3 100644 --- a/src/test/descriptor_tests.cpp +++ b/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 diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index 42195313029..edc9772a9cc 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -124,10 +124,14 @@ struct ParserContext { return a < b; } - std::optional ToString(const Key& key) const + std::optional 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}); } diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 4d31968c787..9f7d4b7e46f 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -188,7 +188,7 @@ struct KeyConverter { return g_testdata->pkmap.at(keyid); } - std::optional ToString(const Key& key) const { + std::optional ToString(const Key& key, bool&) const { return HexStr(ToPKBytes(key)); }