Skip to content

Commit 4dc37d8

Browse files
EunovoEunovo
committed
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
1 parent 0566466 commit 4dc37d8

File tree

6 files changed

+87
-33
lines changed

6 files changed

+87
-33
lines changed

‎src/script/descriptor.cpp‎

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -877,13 +877,19 @@ class DescriptorImpl : public Descriptor
877877
virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const
878878
{
879879
size_t pos = 0;
880+
bool is_private{type == StringType::PRIVATE};
881+
// For private string output, track if at least one key has a private key available.
882+
// Initialize to true for non-private types.
883+
bool any_success{!is_private};
880884
for (const auto& scriptarg : m_subdescriptor_args) {
881885
if (pos++) ret += ",";
882886
std::string tmp;
883-
if (!scriptarg->ToStringHelper(arg, tmp, type, cache)) return false;
887+
bool subscript_res{scriptarg->ToStringHelper(arg, tmp, type, cache)};
888+
if (!is_private && !subscript_res) return false;
889+
any_success = any_success || subscript_res;
884890
ret += tmp;
885891
}
886-
return true;
892+
return any_success;
887893
}
888894

889895
// NOLINTNEXTLINE(misc-no-recursion)
@@ -892,6 +898,11 @@ class DescriptorImpl : public Descriptor
892898
std::string extra = ToStringExtra();
893899
size_t pos = extra.size() > 0 ? 1 : 0;
894900
std::string ret = m_name + "(" + extra;
901+
bool is_private{type == StringType::PRIVATE};
902+
// For private string output, track if at least one key has a private key available.
903+
// Initialize to true for non-private types.
904+
bool any_success{!is_private};
905+
895906
for (const auto& pubkey : m_pubkey_args) {
896907
if (pos++) ret += ",";
897908
std::string tmp;
@@ -900,7 +911,7 @@ class DescriptorImpl : public Descriptor
900911
if (!pubkey->ToNormalizedString(*arg, tmp, cache)) return false;
901912
break;
902913
case StringType::PRIVATE:
903-
if (!pubkey->ToPrivateString(*arg, tmp)) return false;
914+
any_success = pubkey->ToPrivateString(*arg, tmp) || any_success;
904915
break;
905916
case StringType::PUBLIC:
906917
tmp = pubkey->ToString();
@@ -912,10 +923,12 @@ class DescriptorImpl : public Descriptor
912923
ret += tmp;
913924
}
914925
std::string subscript;
915-
if (!ToStringSubScriptHelper(arg, subscript, type, cache)) return false;
926+
bool subscript_res{ToStringSubScriptHelper(arg, subscript, type, cache)};
927+
if (!is_private && !subscript_res) return false;
928+
any_success = any_success || subscript_res;
916929
if (pos && subscript.size()) ret += ',';
917930
out = std::move(ret) + std::move(subscript) + ")";
918-
return true;
931+
return any_success;
919932
}
920933

921934
std::string ToString(bool compat_format) const final
@@ -927,9 +940,9 @@ class DescriptorImpl : public Descriptor
927940

928941
bool ToPrivateString(const SigningProvider& arg, std::string& out) const override
929942
{
930-
bool ret = ToStringHelper(&arg, out, StringType::PRIVATE);
943+
bool has_priv_key{ToStringHelper(&arg, out, StringType::PRIVATE)};
931944
out = AddChecksum(out);
932-
return ret;
945+
return has_priv_key;
933946
}
934947

935948
bool ToNormalizedString(const SigningProvider& arg, std::string& out, const DescriptorCache* cache) const override final
@@ -1410,24 +1423,38 @@ class TRDescriptor final : public DescriptorImpl
14101423
}
14111424
bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const override
14121425
{
1413-
if (m_depths.empty()) return true;
1426+
if (m_depths.empty()) {
1427+
// If there are no sub-descriptors and a PRIVATE string
1428+
// is requested, return `false` to indicate that the presence
1429+
// of a private key depends solely on the internal key (which is checked
1430+
// in the caller), not on any sub-descriptor. This ensures correct behavior for
1431+
// descriptors like tr(internal_key) when checking for private keys.
1432+
return type != StringType::PRIVATE;
1433+
}
14141434
std::vector<bool> path;
1435+
bool is_private{type == StringType::PRIVATE};
1436+
// For private string output, track if at least one key has a private key available.
1437+
// Initialize to true for non-private types.
1438+
bool any_success{!is_private};
1439+
14151440
for (size_t pos = 0; pos < m_depths.size(); ++pos) {
14161441
if (pos) ret += ',';
14171442
while ((int)path.size() <= m_depths[pos]) {
14181443
if (path.size()) ret += '{';
14191444
path.push_back(false);
14201445
}
14211446
std::string tmp;
1422-
if (!m_subdescriptor_args[pos]->ToStringHelper(arg, tmp, type, cache)) return false;
1447+
bool subscript_res{m_subdescriptor_args[pos]->ToStringHelper(arg, tmp, type, cache)};
1448+
if (!is_private && !subscript_res) return false;
1449+
any_success = any_success || subscript_res;
14231450
ret += tmp;
14241451
while (!path.empty() && path.back()) {
14251452
if (path.size() > 1) ret += '}';
14261453
path.pop_back();
14271454
}
14281455
if (!path.empty()) path.back() = true;
14291456
}
1430-
return true;
1457+
return any_success;
14311458
}
14321459
public:
14331460
TRDescriptor(std::unique_ptr<PubkeyProvider> internal_key, std::vector<std::unique_ptr<DescriptorImpl>> descs, std::vector<int> depths) :
@@ -1520,15 +1547,16 @@ class StringMaker {
15201547
const DescriptorCache* cache LIFETIMEBOUND)
15211548
: m_arg(arg), m_pubkeys(pubkeys), m_type(type), m_cache(cache) {}
15221549

1523-
std::optional<std::string> ToString(uint32_t key) const
1550+
std::optional<std::string> ToString(uint32_t key, bool& has_priv_key) const
15241551
{
15251552
std::string ret;
1553+
has_priv_key = false;
15261554
switch (m_type) {
15271555
case DescriptorImpl::StringType::PUBLIC:
15281556
ret = m_pubkeys[key]->ToString();
15291557
break;
15301558
case DescriptorImpl::StringType::PRIVATE:
1531-
if (!m_pubkeys[key]->ToPrivateString(*m_arg, ret)) return {};
1559+
has_priv_key = m_pubkeys[key]->ToPrivateString(*m_arg, ret);
15321560
break;
15331561
case DescriptorImpl::StringType::NORMALIZED:
15341562
if (!m_pubkeys[key]->ToNormalizedString(*m_arg, ret, m_cache)) return {};
@@ -1568,11 +1596,10 @@ class MiniscriptDescriptor final : public DescriptorImpl
15681596
bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type,
15691597
const DescriptorCache* cache = nullptr) const override
15701598
{
1571-
if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache))) {
1572-
out = *res;
1573-
return true;
1574-
}
1575-
return false;
1599+
bool has_priv_key{false};
1600+
auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache), has_priv_key);
1601+
if (res) out = *res;
1602+
return type == StringType::PRIVATE ? has_priv_key : res.has_value();
15761603
}
15771604

15781605
bool IsSolvable() const override { return true; }
@@ -2110,7 +2137,7 @@ struct KeyParser {
21102137
return key;
21112138
}
21122139

2113-
std::optional<std::string> ToString(const Key& key) const
2140+
std::optional<std::string> ToString(const Key& key, bool&) const
21142141
{
21152142
return m_keys.at(key).at(0)->ToString();
21162143
}
@@ -2507,7 +2534,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
25072534
// Try to find the first insane sub for better error reporting.
25082535
auto insane_node = node.get();
25092536
if (const auto sub = node->FindInsaneSub()) insane_node = sub;
2510-
if (const auto str = insane_node->ToString(parser)) error = *str;
2537+
error = *insane_node->ToString(parser);
25112538
if (!insane_node->IsValid()) {
25122539
error += " is invalid";
25132540
} else if (!node->IsSane()) {

‎src/script/descriptor.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,13 @@ struct Descriptor {
118118
*/
119119
virtual bool HavePrivateKeys(const SigningProvider& provider) const = 0;
120120

121-
/** Convert the descriptor to a private string. This fails if the provided provider does not have the relevant private keys. */
121+
/** Convert the descriptor to a private string. This uses public keys if the relevant private keys are not in the SigningProvider.
122+
* If none of the relevant private keys are available, the output string in the "out" parameter will not contain any private key information,
123+
* and this function will return "false".
124+
* @param[in] provider The SigningProvider to query for private keys.
125+
* @param[out] out The resulting descriptor string, containing private keys if available.
126+
* @returns true if at least one private key available.
127+
*/
122128
virtual bool ToPrivateString(const SigningProvider& provider, std::string& out) const = 0;
123129

124130
/** 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‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -826,6 +826,12 @@ struct Node {
826826

827827
template<typename CTx>
828828
std::optional<std::string> ToString(const CTx& ctx) const {
829+
bool dummy{false};
830+
return ToString(ctx, dummy);
831+
}
832+
833+
template<typename CTx>
834+
std::optional<std::string> ToString(const CTx& ctx, bool& has_priv_key) const {
829835
// To construct the std::string representation for a Miniscript object, we use
830836
// the TreeEvalMaybe algorithm. The State is a boolean: whether the parent node is a
831837
// wrapper. If so, non-wrapper expressions must be prefixed with a ":".
@@ -838,10 +844,16 @@ struct Node {
838844
(node.fragment == Fragment::OR_I && node.subs[0]->fragment == Fragment::JUST_0) ||
839845
(node.fragment == Fragment::OR_I && node.subs[1]->fragment == Fragment::JUST_0));
840846
};
847+
auto toString = [&ctx, &has_priv_key](Key key) -> std::optional<std::string> {
848+
bool fragment_has_priv_key{false};
849+
auto key_str{ctx.ToString(key, fragment_has_priv_key)};
850+
if (key_str) has_priv_key = has_priv_key || fragment_has_priv_key;
851+
return key_str;
852+
};
841853
// The upward function computes for a node, given whether its parent is a wrapper,
842854
// and the string representations of its child nodes, the string representation of the node.
843855
const bool is_tapscript{IsTapscript(m_script_ctx)};
844-
auto upfn = [&ctx, is_tapscript](bool wrapped, const Node& node, std::span<std::string> subs) -> std::optional<std::string> {
856+
auto upfn = [is_tapscript, &toString](bool wrapped, const Node& node, std::span<std::string> subs) -> std::optional<std::string> {
845857
std::string ret = wrapped ? ":" : "";
846858

847859
switch (node.fragment) {
@@ -850,13 +862,13 @@ struct Node {
850862
case Fragment::WRAP_C:
851863
if (node.subs[0]->fragment == Fragment::PK_K) {
852864
// pk(K) is syntactic sugar for c:pk_k(K)
853-
auto key_str = ctx.ToString(node.subs[0]->keys[0]);
865+
auto key_str = toString(node.subs[0]->keys[0]);
854866
if (!key_str) return {};
855867
return std::move(ret) + "pk(" + std::move(*key_str) + ")";
856868
}
857869
if (node.subs[0]->fragment == Fragment::PK_H) {
858870
// pkh(K) is syntactic sugar for c:pk_h(K)
859-
auto key_str = ctx.ToString(node.subs[0]->keys[0]);
871+
auto key_str = toString(node.subs[0]->keys[0]);
860872
if (!key_str) return {};
861873
return std::move(ret) + "pkh(" + std::move(*key_str) + ")";
862874
}
@@ -877,12 +889,12 @@ struct Node {
877889
}
878890
switch (node.fragment) {
879891
case Fragment::PK_K: {
880-
auto key_str = ctx.ToString(node.keys[0]);
892+
auto key_str = toString(node.keys[0]);
881893
if (!key_str) return {};
882894
return std::move(ret) + "pk_k(" + std::move(*key_str) + ")";
883895
}
884896
case Fragment::PK_H: {
885-
auto key_str = ctx.ToString(node.keys[0]);
897+
auto key_str = toString(node.keys[0]);
886898
if (!key_str) return {};
887899
return std::move(ret) + "pk_h(" + std::move(*key_str) + ")";
888900
}
@@ -908,7 +920,7 @@ struct Node {
908920
CHECK_NONFATAL(!is_tapscript);
909921
auto str = std::move(ret) + "multi(" + util::ToString(node.k);
910922
for (const auto& key : node.keys) {
911-
auto key_str = ctx.ToString(key);
923+
auto key_str = toString(key);
912924
if (!key_str) return {};
913925
str += "," + std::move(*key_str);
914926
}
@@ -918,7 +930,7 @@ struct Node {
918930
CHECK_NONFATAL(is_tapscript);
919931
auto str = std::move(ret) + "multi_a(" + util::ToString(node.k);
920932
for (const auto& key : node.keys) {
921-
auto key_str = ctx.ToString(key);
933+
auto key_str = toString(key);
922934
if (!key_str) return {};
923935
str += "," + std::move(*key_str);
924936
}

‎src/test/descriptor_tests.cpp‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ constexpr int SIGNABLE = 1 << 3; // We can sign with this descriptor (this is no
4949
constexpr int DERIVE_HARDENED = 1 << 4; // The final derivation is hardened, i.e. ends with *' or *h
5050
constexpr int MIXED_PUBKEYS = 1 << 5;
5151
constexpr int XONLY_KEYS = 1 << 6; // X-only pubkeys are in use (and thus inferring/caching may swap parity of pubkeys/keyids)
52-
constexpr int MISSING_PRIVKEYS = 1 << 7; // Not all private keys are available. ToPrivateString() will fail and HavePrivateKeys() will return `false`.
52+
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`.
5353
constexpr int SIGNABLE_FAILS = 1 << 8; // We can sign with this descriptor, but actually trying to sign will fail
5454
constexpr int MUSIG = 1 << 9; // This is a MuSig so key counts will have an extra key
5555
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
264264
parse_pub->ExpandPrivate(0, keys_priv, pub_prov);
265265

266266
BOOST_CHECK_MESSAGE(EqualSigningProviders(priv_prov, pub_prov), "Private desc: " + prv + " Pub desc: " + pub);
267+
} else if (keys_priv.keys.size() > 0) {
268+
// If there is at least one private key, ToPrivateString() should return true and include that key
269+
std::string prv_str;
270+
BOOST_CHECK(parse_priv->ToPrivateString(keys_priv, prv_str));
271+
size_t checksum_len = 9; // Including the '#' character
272+
BOOST_CHECK_MESSAGE(prv == prv_str.substr(0, prv_str.length() - checksum_len), prv);
267273
}
268274

269275
// Check that private can produce the normalized descriptors

‎src/test/fuzz/miniscript.cpp‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,14 @@ struct ParserContext {
124124
return a < b;
125125
}
126126

127-
std::optional<std::string> ToString(const Key& key) const
127+
std::optional<std::string> ToString(const Key& key, bool& has_priv_key) const
128128
{
129+
has_priv_key = false;
129130
auto it = TEST_DATA.dummy_key_idx_map.find(key);
130-
if (it == TEST_DATA.dummy_key_idx_map.end()) return {};
131+
if (it == TEST_DATA.dummy_key_idx_map.end()) {
132+
return HexStr(key);
133+
}
134+
has_priv_key = true;
131135
uint8_t idx = it->second;
132136
return HexStr(std::span{&idx, 1});
133137
}
@@ -1033,7 +1037,7 @@ void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& p
10331037

10341038
// Check that it roundtrips to text representation
10351039
const ParserContext parser_ctx{script_ctx};
1036-
std::optional<std::string> str{node->ToString(parser_ctx)};
1040+
auto str{node->ToString(parser_ctx)};
10371041
assert(str);
10381042
auto parsed = miniscript::FromString(*str, parser_ctx);
10391043
assert(parsed);
@@ -1242,7 +1246,6 @@ FUZZ_TARGET(miniscript_string, .init = FuzzInit)
12421246
if (!parsed) return;
12431247

12441248
const auto str2 = parsed->ToString(parser_ctx);
1245-
assert(str2);
12461249
auto parsed2 = miniscript::FromString(*str2, parser_ctx);
12471250
assert(parsed2);
12481251
assert(*parsed == *parsed2);

‎src/test/miniscript_tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ struct KeyConverter {
188188
return g_testdata->pkmap.at(keyid);
189189
}
190190

191-
std::optional<std::string> ToString(const Key& key) const {
191+
std::optional<std::string> ToString(const Key& key, bool&) const {
192192
return HexStr(ToPKBytes(key));
193193
}
194194

0 commit comments

Comments
 (0)