From e842eb90bb6db39076a43b010c0c7898d50b8d92 Mon Sep 17 00:00:00 2001 From: Novo Date: Tue, 22 Jul 2025 07:46:06 +0100 Subject: [PATCH 1/6] 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 a42943318d6..e2f7b8e873a 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -835,6 +835,25 @@ public: 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 9a018300ebe..c7863933ef9 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 098007c4cf9..17c756b6033 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 0c69849d0b6..adbd9aa1bae 100644 --- a/src/wallet/test/walletload_tests.cpp +++ b/src/wallet/test/walletload_tests.cpp @@ -26,6 +26,7 @@ public: 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/6] 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 276817146c6..708f59892eb 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/6] 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 e2f7b8e873a..89af1d9367f 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -204,7 +204,11 @@ public: /** 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 @@ public: 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 @@ public: 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 @@ public: 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 @@ public: 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/6] 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 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)); } From ed945a685473712c1a822379effa42fd49223515 Mon Sep 17 00:00:00 2001 From: Novo Date: Mon, 12 May 2025 11:55:19 +0100 Subject: [PATCH 5/6] 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 5cb40304a71..ebc678aac07 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 aaa5daac8bd..da73d5ee925 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -107,7 +107,7 @@ class ListDescriptorsTest(BitcoinTestFramework): '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 @@ class ListDescriptorsTest(BitcoinTestFramework): {'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/6] 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 da73d5ee925..12b0576c538 100755 --- a/test/functional/wallet_listdescriptors.py +++ b/test/functional/wallet_listdescriptors.py @@ -146,6 +146,36 @@ class ListDescriptorsTest(BitcoinTestFramework): } 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()