From e842eb90bb6db39076a43b010c0c7898d50b8d92 Mon Sep 17 00:00:00 2001 From: Novo Date: Tue, 22 Jul 2025 07:46:06 +0100 Subject: [PATCH 1/8] descriptors: add HavePrivateKeys() Previously, to determine if a desc is watchonly, `ToPrivateString()`, was used. It returns `false` if there is at least one pubkey in the descriptor for which the provider does not have a private key. ToPrivateString() behaviour will change in the following commits to only return `false` if no priv keys could be found for the pub keys in the descriptor. HavePrivateKeys() is added here to replace the use of ToPrivateString() for determining if a descriptor is 'watchonly'. Co-authored-by: rkrux --- src/script/descriptor.cpp | 19 +++++++++++++++++++ src/script/descriptor.h | 7 +++++++ src/test/descriptor_tests.cpp | 5 ++++- src/wallet/test/walletload_tests.cpp | 1 + 4 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index a42943318d65..e2f7b8e873a6 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -835,6 +835,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 { diff --git a/src/script/descriptor.h b/src/script/descriptor.h index 9a018300ebe5..c7863933ef96 100644 --- a/src/script/descriptor.h +++ b/src/script/descriptor.h @@ -111,6 +111,13 @@ struct Descriptor { /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */ virtual bool IsSingleType() const = 0; + /** 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 fails if the provided provider does not have the relevant private keys. */ virtual bool ToPrivateString(const SigningProvider& provider, std::string& out) const = 0; diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 098007c4cf90..17c756b60338 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, so ToPrivateString will fail. +constexpr int MISSING_PRIVKEYS = 1 << 7; // Not all private keys are available. ToPrivateString() will fail 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) { diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp index 0c69849d0b64..adbd9aa1baec 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -26,6 +26,7 @@ class DummyDescriptor final : public Descriptor { bool IsRange() const override { return false; } bool IsSolvable() const override { return false; } bool IsSingleType() const override { return true; } + bool HavePrivateKeys(const SigningProvider&) const override { return false; } bool ToPrivateString(const SigningProvider& provider, std::string& out) const override { return false; } bool ToNormalizedString(const SigningProvider& provider, std::string& out, const DescriptorCache* cache = nullptr) const override { return false; } bool Expand(int pos, const SigningProvider& provider, std::vector& output_scripts, FlatSigningProvider& out, DescriptorCache* write_cache = nullptr) const override { return false; }; From 2dc74e3f4e5e6f01c8810359b91041bc6865f1c7 Mon Sep 17 00:00:00 2001 From: Novo Date: Tue, 13 May 2025 13:32:06 +0100 Subject: [PATCH 2/8] wallet/migration: use HavePrivateKeys in place of ToPrivateString ToPrivateString() behaviour will be modified in the following commits. In order to keep the scope of this PR limited to the RPC behaviour, this commit updates wallet migration to use 'Descriptor::HavePrivateKeys()' in place of 'Descriptor::ToPrivateString()' to determine watchonly descriptors. A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the watchonly migration wallet. --- src/wallet/scriptpubkeyman.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 276817146c6e..708f59892eba 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -717,10 +717,9 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() std::vector desc_spks; - // Make the descriptor string with private keys - std::string desc_str; - bool watchonly = !desc->ToPrivateString(*this, desc_str); - if (watchonly && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + // If we can't provide all private keys for this inferred descriptor, + // but this wallet is not watch-only, migrate it to the watch-only wallet. + if (!desc->HavePrivateKeys(*this) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { out.watch_descs.emplace_back(desc->ToString(), creation_time); // Get the scriptPubKeys without writing this to the wallet From 5c4db25b61d417a567f152169f4ab21a491afb95 Mon Sep 17 00:00:00 2001 From: Novo Date: Fri, 1 Aug 2025 14:39:11 +0100 Subject: [PATCH 3/8] descriptor: refactor ToPrivateString for providers This commit modifies the Pubkey providers to return the public string if private data is not available. This is setup for a future commit to make Descriptor::ToPrivateString return strings with missing private key information. Co-authored-by: rkrux --- src/script/descriptor.cpp | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index e2f7b8e873a6..89af1d9367fc 100644 --- a/src/script/descriptor.cpp +++ b/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 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 From 9e5e9824f11b1b0f9e2a4e28124edbb1616af519 Mon Sep 17 00:00:00 2001 From: Novo Date: Fri, 1 Aug 2025 14:31:25 +0100 Subject: [PATCH 4/8] 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 | 68 +++++++++++++++++++++++++---------- src/script/descriptor.h | 8 ++++- src/script/miniscript.h | 26 ++++++++++---- src/test/descriptor_tests.cpp | 8 ++++- src/test/fuzz/miniscript.cpp | 8 +++-- src/test/miniscript_tests.cpp | 2 +- 6 files changed, 90 insertions(+), 30 deletions(-) diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 89af1d9367fc..89766b52cffb 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -877,13 +877,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) @@ -892,6 +898,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; @@ -900,7 +911,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(); @@ -912,10 +923,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 @@ -927,9 +940,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 @@ -1410,8 +1423,20 @@ 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 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 @@ class TRDescriptor final : public DescriptorImpl 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 @@ class TRDescriptor final : public DescriptorImpl } 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 @@ class StringMaker { 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 @@ 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; } @@ -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 c7863933ef96..47c568857e51 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 54ae777cf926..6e684e6d9f26 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -826,6 +826,12 @@ struct Node { 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 @@ 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 { + 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 @@ 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) + ")"; } @@ -877,12 +889,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) + ")"; } @@ -908,7 +920,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); } @@ -918,7 +930,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); } diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 17c756b60338..d49a4885ba3d 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 421953130294..edc9772a9ccd 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 4d31968c787f..9f7d4b7e46fb 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)); } From ed945a685473712c1a822379effa42fd49223515 Mon Sep 17 00:00:00 2001 From: Novo Date: Mon, 12 May 2025 11:55:19 +0100 Subject: [PATCH 5/8] walletrpc: reject listdes with priv key on w-only wallets --- src/wallet/rpc/backup.cpp | 8 +++++--- test/functional/wallet_listdescriptors.py | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 5cb40304a714..ebc678aac070 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -493,6 +494,9 @@ RPCHelpMan listdescriptors() if (!wallet) return UniValue::VNULL; const bool priv = !request.params[0].isNull() && request.params[0].get_bool(); + if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && priv) { + throw JSONRPCError(RPC_WALLET_ERROR, "Can't get private descriptor string for watch-only wallets"); + } if (priv) { EnsureWalletIsUnlocked(*wallet); } @@ -519,9 +523,7 @@ RPCHelpMan listdescriptors() LOCK(desc_spk_man->cs_desc_man); const auto& wallet_descriptor = desc_spk_man->GetWalletDescriptor(); std::string descriptor; - if (!desc_spk_man->GetDescriptorString(descriptor, priv)) { - throw JSONRPCError(RPC_WALLET_ERROR, "Can't get descriptor string."); - } + CHECK_NONFATAL(desc_spk_man->GetDescriptorString(descriptor, priv)); const bool is_range = wallet_descriptor.descriptor->IsRange(); wallet_descriptors.push_back({ descriptor, diff --git a/test/functional/wallet_listdescriptors.py b/test/functional/wallet_listdescriptors.py index aaa5daac8bda..da73d5ee925c 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -107,7 +107,7 @@ def run_test(self): 'desc': descsum_create('wpkh(' + xpub_acc + ')'), 'timestamp': TIME_GENESIS_BLOCK, }]) - assert_raises_rpc_error(-4, 'Can\'t get descriptor string', watch_only_wallet.listdescriptors, True) + assert_raises_rpc_error(-4, 'Can\'t get private descriptor string for watch-only wallets', watch_only_wallet.listdescriptors, True) self.log.info('Test non-active non-range combo descriptor') node.createwallet(wallet_name='w4', blank=True) @@ -122,7 +122,7 @@ def run_test(self): {'active': False, 'desc': 'combo(0227d85ba011276cf25b51df6a188b75e604b38770a462b2d0e9fb2fc839ef5d3f)#np574htj', 'timestamp': TIME_GENESIS_BLOCK}, - ] + ], } assert_equal(expected, wallet.listdescriptors()) From 9c7e4771b13d4729fd20ea08b7e2e3209b134fff Mon Sep 17 00:00:00 2001 From: Novo Date: Mon, 12 May 2025 13:39:03 +0100 Subject: [PATCH 6/8] test: Test listdescs with priv works even with missing priv keys Co-authored-by: rkrux --- test/functional/wallet_listdescriptors.py | 30 +++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/functional/wallet_listdescriptors.py b/test/functional/wallet_listdescriptors.py index da73d5ee925c..12b0576c538d 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -146,6 +146,36 @@ def run_test(self): } assert_equal(expected, wallet.listdescriptors()) + self.log.info('Test descriptor with missing private keys') + node.createwallet(wallet_name='w6', blank=True) + wallet = node.get_wallet_rpc('w6') + + expected_descs = { + descsum_create('tr(' + node.get_deterministic_priv_key().key + + ',{pk(03cdabb7f2dce7bfbd8a0b9570c6fd1e712e5d64045e9d6b517b3d5072251dc204)' + + ',pk([d34db33f/44h/0h/0h]tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/0)})'), + descsum_create('wsh(and_v(v:ripemd160(095ff41131e5946f3c85f79e44adbcf8e27e080e),multi(1,' + node.get_deterministic_priv_key().key + + ',tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/0)))'), + descsum_create('tr(03dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659,pk(musig(tprv8ZgxMBicQKsPeNLUGrbv3b7qhUk1LQJZAGMuk9gVuKh9sd4BWGp1eMsehUni6qGb8bjkdwBxCbgNGdh2bYGACK5C5dRTaif9KBKGVnSezxV,tpubD6NzVbkrYhZ4XcACN3PEwNjRpR1g4tZjBVk5pdMR2B6dbd3HYhdGVZNKofAiFZd9okBserZvv58A6tBX4pE64UpXGNTSesfUW7PpW36HuKz)/7/8/*))'), + descsum_create('tr(03dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659,pk(musig(tprv8ZgxMBicQKsPeNLUGrbv3b7qhUk1LQJZAGMuk9gVuKh9sd4BWGp1eMsehUni6qGb8bjkdwBxCbgNGdh2bYGACK5C5dRTaif9KBKGVnSezxV/10,tpubD6NzVbkrYhZ4XcACN3PEwNjRpR1g4tZjBVk5pdMR2B6dbd3HYhdGVZNKofAiFZd9okBserZvv58A6tBX4pE64UpXGNTSesfUW7PpW36HuKz/11)/*))'), + descsum_create('tr(03dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659,{pk(musig(tpubD6NzVbkrYhZ4Wo2WcFSgSqRD9QWkGxddo6WSqsVBx7uQ8QEtM7WncKDRjhFEexK119NigyCsFygA4b7sAPQxqebyFGAZ9XVV1BtcgNzbCRR,tprv8ZgxMBicQKsPen4PGtDwURYnCtVMDejyE8vVwMGhQWfVqB2FBPdekhTacDW4vmsKTsgC1wsncVqXiZdX2YFGAnKoLXYf42M78fQJFzuDYFN)/12/*),pk(musig(tprv8ZgxMBicQKsPeNLUGrbv3b7qhUk1LQJZAGMuk9gVuKh9sd4BWGp1eMsehUni6qGb8bjkdwBxCbgNGdh2bYGACK5C5dRTaif9KBKGVnSezxV,tpubD6NzVbkrYhZ4XWb6fGPjyhgLxapUhXszv7ehQYrQWDgDX4nYWcNcbgWcM2RhYo9s2mbZcfZJ8t5LzYcr24FK79zVybsw5Qj3Rtqug8jpJMy)/13/*)})'), + descsum_create('tr(03dff1d77f2a671c5f36183726db2341be58feae1da2deced843240f7b502ba659,{pk(musig(tpubD6NzVbkrYhZ4Wo2WcFSgSqRD9QWkGxddo6WSqsVBx7uQ8QEtM7WncKDRjhFEexK119NigyCsFygA4b7sAPQxqebyFGAZ9XVV1BtcgNzbCRR,tpubD6NzVbkrYhZ4Wc3i6L6N1Pp7cyVeyMcdLrFGXGDGzCfdCa5F4Zs3EY46N72Ws8QDEUYBVwXfDfda2UKSseSdU1fsBegJBhGCZyxkf28bkQ6)/12/*),pk(musig(tprv8ZgxMBicQKsPeNLUGrbv3b7qhUk1LQJZAGMuk9gVuKh9sd4BWGp1eMsehUni6qGb8bjkdwBxCbgNGdh2bYGACK5C5dRTaif9KBKGVnSezxV,tpubD6NzVbkrYhZ4XWb6fGPjyhgLxapUhXszv7ehQYrQWDgDX4nYWcNcbgWcM2RhYo9s2mbZcfZJ8t5LzYcr24FK79zVybsw5Qj3Rtqug8jpJMy)/13/*)})') + } + + descs_to_import = [] + for desc in expected_descs: + descs_to_import.append({'desc': desc, 'timestamp': TIME_GENESIS_BLOCK}) + + wallet.importdescriptors(descs_to_import) + result = wallet.listdescriptors(True) + actual_descs = [d['desc'] for d in result['descriptors']] + + assert_equal(len(actual_descs), len(expected_descs)) + for desc in actual_descs: + if desc not in expected_descs: + raise AssertionError(f"{desc} missing") + + if __name__ == '__main__': ListDescriptorsTest(__file__).main() From b3026b10dc9e075d116528435008d8aad25a04d5 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 6 Jan 2026 18:41:26 +0700 Subject: [PATCH 7/8] fuzz: add HasPrivKey miniscript helper --- src/test/fuzz/miniscript.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index edc9772a9ccd..0b7fa8e14423 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -106,6 +106,12 @@ struct TestData { return &it->second; } } + + //! Whether signing material is available for this pubkey in this script context. + bool HasPrivKey(const MsCtx script_ctx, const Key& key) const { + const auto sig_ptr = GetSig(script_ctx, key); + return sig_ptr && sig_ptr->second; + } } TEST_DATA; /** @@ -1162,8 +1168,7 @@ void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& p // are identical, this implies that for such nodes, the non-malleable // satisfaction will also match the expected policy. const auto is_key_satisfiable = [script_ctx](const CPubKey& pubkey) -> bool { - auto sig_ptr{TEST_DATA.GetSig(script_ctx, pubkey)}; - return sig_ptr != nullptr && sig_ptr->second; + return TEST_DATA.HasPrivKey(script_ctx, pubkey); }; bool satisfiable = node->IsSatisfiable([&](const Node& node) -> bool { switch (node.fragment) { From fa022433cc4b4b6601a5965e88c8fe34b428f9a1 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 7 Jan 2026 14:32:31 +0700 Subject: [PATCH 8/8] fuzz: verify has_priv_key Use HasPrivKey to ensure the correct return value for has_priv_key. Add coverage for it in the harness. --- src/test/fuzz/miniscript.cpp | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/test/fuzz/miniscript.cpp b/src/test/fuzz/miniscript.cpp index 0b7fa8e14423..57aae0ef3eb9 100644 --- a/src/test/fuzz/miniscript.cpp +++ b/src/test/fuzz/miniscript.cpp @@ -132,13 +132,10 @@ struct ParserContext { 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 HexStr(key); - } - has_priv_key = true; - uint8_t idx = it->second; + const auto it = TEST_DATA.dummy_key_idx_map.find(key); + assert(it != TEST_DATA.dummy_key_idx_map.end()); + has_priv_key = TEST_DATA.HasPrivKey(script_ctx, key); + const uint8_t idx = it->second; return HexStr(std::span{&idx, 1}); } @@ -1043,8 +1040,23 @@ void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& p // Check that it roundtrips to text representation const ParserContext parser_ctx{script_ctx}; - std::optional str{node->ToString(parser_ctx)}; + bool has_priv_key{false}; + std::optional str{node->ToString(parser_ctx, has_priv_key)}; assert(str); + + // Check has_priv_key against expectation + bool expected_has_priv_key{false}; + std::vector nodes{node.get()}; + while (!nodes.empty() && !expected_has_priv_key) { + const Node* n{nodes.back()}; + nodes.pop_back(); + expected_has_priv_key = std::any_of(n->keys.begin(), n->keys.end(), [&](const auto& key) { + return TEST_DATA.HasPrivKey(script_ctx, key); + }); + for (const auto& sub : n->subs) nodes.push_back(sub.get()); + } + assert(has_priv_key == expected_has_priv_key); + auto parsed = miniscript::FromString(*str, parser_ctx); assert(parsed); assert(*parsed == *node);