wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key

Eunovo commented at 1:26 pm on May 12, 2025: contributor

TLDR: Currently, listdescriptors [private=true] will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g tr(), multi(), etc.). This PR changes that while making sure listdescriptors [private=true] still fails if there no private keys. Closes #32078

In non-watch-only wallets, it’s possible to import descriptors as long as at least one private key is included. It’s important that users can still view these descriptors when they need to create a backup—even if some private keys are missing (#32078 (comment)). This change makes it possible to do so.

This change also helps prevent listdescriptors true from failing completely, because one descriptor is missing some private keys.

Notes

The new behaviour is applied to all descriptors including miniscript descriptors
listdescriptors true still fails for watch-only wallets to preserve existing behaviour #24361 (review)
Wallet migration logic previously used Descriptor::ToPrivateString() to determine which descriptor was watchonly. This means that modifying the ToPrivateString() behaviour caused descriptors that were previously recognized as “watchonly” to be “non-watchonly”. In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine watchonly descriptors for the purpose of wallet migration. A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the watchonly migration wallet.

Relevant PRs

#24361 #32186

Testing

Functional tests were added to test the new behaviour

EDIT listdescriptors [private=true] will still fail when there are no private keys because non-watchonly wallets must have private keys and calling listdescriptors [private=true] for watchonly wallet returns an error

DrahtBot commented at 1:26 pm on May 12, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32471.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	achow101, vicjuma
Concept ACK	rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#31734 (miniscript: account for all StringType variants in Miniscriptdescriptor::ToString() by pythcoiner)
#31713 (miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) by hodlinator)
#30243 (descriptors: taproot partial descriptors by Eunovo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake commented at 1:35 pm on May 12, 2025: member

 0/Users/runner/work/bitcoin/bitcoin/src/test/fuzz/miniscript.cpp:1040:42: error: no matching member function for call to 'ToString'
 1    std::optional<std::string> str{node->ToString(parser_ctx)};
 2                                   ~~~~~~^~~~~~~~
 3/Users/runner/work/bitcoin/bitcoin/src/script/miniscript.h:830:17: note: candidate function template not viable: requires 2 arguments, but 1 was provided
 4    std::string ToString(const CTx& ctx, bool& has_priv_key) const {
 5                ^
 6/Users/runner/work/bitcoin/bitcoin/src/test/fuzz/miniscript.cpp:1248:31: error: no matching member function for call to 'ToString'
 7    const auto str2 = parsed->ToString(parser_ctx);
 8                      ~~~~~~~~^~~~~~~~
 9/Users/runner/work/bitcoin/bitcoin/src/script/miniscript.h:830:17: note: candidate function template not viable: requires 2 arguments, but 1 was provided
10    std::string ToString(const CTx& ctx, bool& has_priv_key) const {
11                ^
122 errors generated.

DrahtBot added the label CI failed on May 12, 2025

DrahtBot commented at 1:38 pm on May 12, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/42059781761 LLM reason (✨ experimental): The CI failure is due to build errors in miniscript.cpp related to the ToString function call.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Eunovo marked this as a draft on May 12, 2025

Eunovo commented at 1:51 pm on May 12, 2025: contributor

Putting draft while I fix some issues with the fuzz tests

Eunovo force-pushed on May 12, 2025

Eunovo force-pushed on May 13, 2025

DrahtBot removed the label CI failed on May 13, 2025

Eunovo marked this as ready for review on May 13, 2025

Eunovo commented at 4:02 pm on May 13, 2025: contributor

cc @rkrux @sipa

achow101 commented at 5:32 pm on May 13, 2025: member

Concept NACK

The purpose of the private argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.

vicjuma commented at 5:40 pm on May 13, 2025: none

Concept NACK

It’s unclear why someone should receive public keys with private=true. Why not just use the default command, bitcoin-cli listdescriptors, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.

rkrux commented at 6:10 pm on May 13, 2025: contributor

If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors.

I have not checked this PR yet but the original intention came from this issue: #32078 (comment)

The idea was to show results for descriptors that don’t have all the private keys but do have at least one. This is not for the case when the descriptor doesn’t have any private key.

I do like how the importdescriptors RPC returns a warning when the descriptor contains partial private keys: #32078 (comment). Maybe a similar warning can be returned in this case for listdescriptors true as well - to avoid the footgun.

0[
1  {
2    "success": true,
3    "warnings": [
4      "Not all private keys provided. Some wallet functionality may return unexpected errors"
5    ]
6  }
7]

Also, if the importdescriptors RPC allows importing a descriptor with partial private keys, then it seems reasonable to me to list that descriptor in the response of listdescriptors true with those partial private keys. Is there any other way to see those partial private keys?

Regardless of the above point, I do feel that preventing listdescriptors true from failing completely in the case a descriptor is missing few private keys is helpful for the user as the other descriptors with all the private keys will be returned in the response.

This change also helps prevent listdescriptors true from failing completely, because one descriptor is missing some private keys.

vicjuma commented at 6:29 pm on May 13, 2025: none

Maybe -rpcwallet=<wallet> will come in handy in this situation where you would have to use a different wallet for these situations to prevent the conflicts.

achow101 commented at 6:50 pm on May 13, 2025: member

The idea was to show results for descriptors that don’t have all the private keys but do have at least one.

I think that’s fine to show a result, but I don’t think we should show any result if there are no private keys at all.

Eunovo commented at 6:19 am on May 14, 2025: contributor

Concept NACK

The purpose of the private argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.

I think there has been a misunderstanding. There are private keys to show. This PR doesn’t change the behaviour of listdescriptors true RPC to show any result when there are no private keys. Non-watchonly wallet descriptors must have at least one private key. The RPC still fails for watch only wallets

Eunovo commented at 6:22 am on May 14, 2025: contributor

I think that’s fine to show a result, but I don’t think we should show any result if there are no private keys at all.

No result is shown if there are no private keys

Eunovo commented at 6:29 am on May 14, 2025: contributor

It’s unclear why someone should receive public keys with private=true. Why not just use the default command, bitcoin-cli listdescriptors, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.

It is possible to have descriptors with missing private keys in a non-watchonly wallet. importdescriptors only rejects descriptors without any private keys. Trying listdescriptors private=true in such a wallet will throw an error, preventing the user from being able to backup their descriptors. The default command only returns public information, I believe that modifying the default command to return private information sometimes will be the wrong decision. It’s much better to allow private=true to work for these descriptors with some missing private keys.

Eunovo renamed this:
~~Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet~~
wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key
on May 14, 2025

rkrux commented at 1:02 pm on May 21, 2025: contributor

A similar problem happens in gethdkeys private=true as well and I think it’s due to partial private keys when the following error is thrown:

0➜  bitcoin git:(list-descriptors-with-partial-keys) ✗ bitcoinclireg -named gethdkeys private=true
1error code: -1
2error message:
3map::at:  key not found

It appears to be coming from here when the xprv corresponding to the xpub can’t be found: https://github.com/bitcoin/bitcoin/blob/7763e86afa045910a14ac9b2cd644927a447370b/src/wallet/rpc/wallet.cpp#L822

It’s due to mismatch in the count of wallet_xprvs and wallet_xpubs calculated in the previous loop over spkms. I can create a separate issue later.

rkrux commented at 1:30 pm on July 16, 2025: contributor

Concept ACK bb14e8ac7d38f3698d76ea3d6dce09e1387d59f3

Started reviewing the PR.

in src/script/descriptor.h:125 in a0a458965c outdated

115+    /** Convert the descriptor to a private string. This uses public keys if the relevant private keys are not in the SigningProvider.
116+     *  If none of the relevant private keys are available, the output string in the "out" parameter will not contain any private key information,
117+     *  and this function will return "false".
118+     *  @param[in] provider The SigningProvider to query for private keys.
119+     *  @param[out] out The resulting descriptor string, containing private keys if available.
120+     */

rkrux commented at 2:37 pm on July 16, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

0+ [@return](/bitcoin-bitcoin/contributor/return/) true if at least one private key available

Eunovo commented at 3:15 pm on July 18, 2025:

Done

in src/script/descriptor.cpp:709 in a0a458965c outdated

706 
707     bool ToPrivateString(const SigningProvider& arg, std::string& out) const override
708     {
709-        bool ret = ToStringHelper(&arg, out, StringType::PRIVATE);
710+        bool has_priv_key;
711+        assert(ToStringHelper(&arg, out, StringType::PRIVATE, has_priv_key));

rkrux commented at 2:43 pm on July 16, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

Fine to add assert here because for all intents and purposes I don’t suppose there would ever be a case where ToStringHelper returns false here. Perhaps we can add a helpful assert message here for debugging purposes?

0- assert(ToStringHelper(&arg, out, StringType::PRIVATE, has_priv_key));
1+ assert(ToStringHelper(&arg, out, StringType::PRIVATE, has_priv_key), "");

rkrux commented at 2:15 pm on July 17, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

This would also aid readability because under no condition has_priv_key can be false here.

0Assume(has_priv_key);

Eunovo commented at 3:16 pm on July 18, 2025:

I couldn’t add an assert message, but I added a comment.

Eunovo commented at 3:20 pm on July 18, 2025:

I’m confused here. Can you clarify what you’re suggesting?

rkrux commented at 4:01 pm on July 18, 2025:

I had something like this in mind because in all the descriptors that can be added in a non-watch-only wallet, there must be at least 1 private key that is available. So, it seemed reasonable to me to add an Assume here on has_priv_key being true. I don’t suppose ToPrivateString function would be called in any case for a watch-only wallet.

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index bd85ddd9e2..b22bf3fa4a 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -710,6 +710,7 @@ public:
 5         // ToStringHelper should never fail for StringType::PRIVATE,
 6         // because it falls back to StringType::PUBLIC when no private ke
 7y is available.
 8         assert(ToStringHelper(&arg, out, StringType::PRIVATE, &has_priv_k
 9ey));
10+        Assume(has_priv_key);
11         out = AddChecksum(out);
12         return has_priv_key;
13     }

The descriptor_tests unit test did fail for one case when I used an assert instead, the functional tests though pass on my system. Maybe I have missed some use case due to which it fails.

0test/descriptor_tests.cpp:212: info: check parse_priv->ToPrivateString(keys_priv, prv1) has passed
1test/descriptor_tests.cpp:216: info: check 'Private ser: combo(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)#p5326pcv Private desc: combo(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)' has passed
2Assertion failed: (has_priv_key), function ToPrivateString, file descriptor.cpp, line 713.
3unknown location:0: fatal error: in "descriptor_tests/descriptor_test": signal: SIGABRT (application abort requested)
4test/descriptor_tests.cpp:218: last checkpoint
5test/descriptor_tests.cpp:518: Leaving test case "descriptor_test"; testing time: 1795us

ISTM that the unit test case is more contrived than the functional tests as the below check fails.

0BOOST_CHECK(!parse_priv->ToPrivateString(keys_pub, prv1));

You can ignore if this doesn’t make sense.

Eunovo commented at 9:48 am on July 19, 2025:

ToPrivateString will be called on a non-watch-only wallet when the user tries RPC listdescriptors[private=true]. In this case, has_priv_key will be false and the RPC call should fail as expected.

rkrux commented at 11:51 am on July 21, 2025:

Sorry but I don’t understand how the second statement follows the first one above. For a non-watch-only wallet, isn’t it guaranteed that there must be at least one private key, which should turn has_priv_key to true?

Eunovo commented at 7:35 am on July 22, 2025:

I forgot about the guard I added in https://github.com/bitcoin/bitcoin/pull/32471/commits/4202dca524aeef13e09380804c483d7c5402092d

You are right in the sense that it’s not possible for a watch-only to even get to ToPrivateString() through the rpc; we could assert has_priv_key here. However, the descriptor unit tests often call ToPrivateString() on watch-only descriptors; asserting has_priv_key causes the tests to fail.

Eunovo commented at 7:38 am on July 22, 2025:

To add this assertion, we will have to remove those checks, and I haven’t seen the advantage in doing that.

rkrux commented at 9:47 am on July 22, 2025:

However, the descriptor unit tests often call ToPrivateString() on watch-only descriptors

Yeah, I don’t know why this was done.

To add this assertion, we will have to remove those checks, and I haven’t seen the advantage in doing that.

Agree, doesn’t need to be done right now.

in src/script/miniscript.h:847 in a0a458965c outdated

840@@ -840,10 +841,16 @@ struct Node {
841                     (node.fragment == Fragment::OR_I && node.subs[0]->fragment == Fragment::JUST_0) ||
842                     (node.fragment == Fragment::OR_I && node.subs[1]->fragment == Fragment::JUST_0));
843         };
844+        auto toString = [&ctx, &has_priv_key](Key key) -> std::string {
845+            bool tmp{false};
846+            auto key_str{ctx.ToString(key, tmp)};
847+            has_priv_key |= tmp;

rkrux commented at 2:44 pm on July 16, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

Not a bug but bitwise or for boolean operations is not ideal.

0- has_priv_key |= tmp;
1+ has_priv_key = has_priv_key || tmp;

Eunovo commented at 3:16 pm on July 18, 2025:

Done

in test/functional/wallet_listdescriptors.py:158 in bb14e8ac7d outdated

153+                {'active': False,
154+                 'desc': miniscript_desc,
155+                 'timestamp': TIME_GENESIS_BLOCK},
156+            ],
157+        }
158+        assert_equal(expected, wallet.listdescriptors(True))

rkrux commented at 2:47 pm on July 16, 2025:

In bb14e8ac7d38f3698d76ea3d6dce09e1387d59f3 “test: Test listdescs with priv works even with missing priv keys”

Nit: I prefer to only check for the data that we really want to assert on, helps in keeping the tests code cleaner as well besides being minutely faster. Checking desc for every imported descriptor in this case.

Eunovo commented at 3:16 pm on July 18, 2025:

Done

in src/script/descriptor.cpp:492 in a0a458965c outdated

487@@ -485,7 +488,10 @@ class BIP32PubkeyProvider final : public PubkeyProvider
488     bool ToPrivateString(const SigningProvider& arg, std::string& out) const override
489     {
490         CExtKey key;
491-        if (!GetExtKey(arg, key)) return false;
492+        if (!GetExtKey(arg, key)) {
493+            out = ToString();

rkrux commented at 2:54 pm on July 16, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

Nit: To keep it consistent with the other pubkey providers and to make it explicit here. Though a default is specified in ToString.

0- out = ToString();
1+ out = ToString(StringType::PUBLIC);

Eunovo commented at 3:16 pm on July 18, 2025:

Done

in src/script/descriptor.cpp:264 in a0a458965c outdated

254@@ -255,9 +255,9 @@ class OriginPubkeyProvider final : public PubkeyProvider
255     bool ToPrivateString(const SigningProvider& arg, std::string& ret) const override

rkrux commented at 12:52 pm on July 17, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

It would be helpful to update the function doc in PubkeyProvider struct to mention the new behaviour.

Eunovo commented at 3:16 pm on July 18, 2025:

Done

in src/script/descriptor.cpp:702 in a0a458965c outdated

697@@ -686,20 +698,23 @@ class DescriptorImpl : public Descriptor
698     std::string ToString(bool compat_format) const final
699     {
700         std::string ret;
701-        ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC);
702+        bool has_priv_key;
703+        ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC, has_priv_key);

rkrux commented at 1:04 pm on July 17, 2025:

In https://github.com/bitcoin/bitcoin/commit/a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

0- bool has_priv_key;
1- ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC, has_priv_key);
2+ bool dummy;
3+ ToStringHelper(nullptr, ret, compat_format ? StringType::COMPAT : StringType::PUBLIC, dummy);

Because there is no use of this variable in ToString. Same for ToNormalizedString function down below.

An alternative could be to use a pointer for has_priv_key instead of reference because, as noted here, there are use cases for nullptr. The ToStringHelper would need to accordingly check for the presence of pointer value though.

Eunovo commented at 3:18 pm on July 18, 2025:

ToStringHelper now accepts a pointer to has_priv_key in https://github.com/bitcoin/bitcoin/pull/32471/commits/2a1f1f0e32829af43436eb4220164c1be86229bb It does look cleaner, but there’s a need to check for nullptr now.

in src/script/miniscript.h:846 in a0a458965c outdated

840@@ -840,10 +841,16 @@ struct Node {
841                     (node.fragment == Fragment::OR_I && node.subs[0]->fragment == Fragment::JUST_0) ||
842                     (node.fragment == Fragment::OR_I && node.subs[1]->fragment == Fragment::JUST_0));
843         };
844+        auto toString = [&ctx, &has_priv_key](Key key) -> std::string {
845+            bool tmp{false};
846+            auto key_str{ctx.ToString(key, tmp)};

rkrux commented at 1:58 pm on July 17, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

0- bool tmp{false};
1- auto key_str{ctx.ToString(key, tmp)};
2+ bool fragment_has_private_key{false};
3+ auto key_str{ctx.ToString(key, fragment_has_private_key)};

Eunovo commented at 3:18 pm on July 18, 2025:

Done

in src/script/descriptor.cpp:1720 in a0a458965c outdated

1714@@ -1700,8 +1715,9 @@ struct KeyParser {
1715         return key;
1716     }
1717 
1718-    std::optional<std::string> ToString(const Key& key) const
1719+    std::string ToString(const Key& key, bool& has_priv_key) const
1720     {
1721+        has_priv_key = false;

rkrux commented at 2:02 pm on July 17, 2025:

In a0a458965c944434b1f3e4d83277257793435edb “descriptor: ToPrivateString() pass if at least 1 priv key exists”

Maybe the pointer approach would avoid forcibly setting variables to false?

Eunovo commented at 3:19 pm on July 18, 2025:

Done

rkrux commented at 3:19 pm on July 17, 2025: contributor

Partial review bb14e8ac7d38f3698d76ea3d6dce09e1387d59f3

Eunovo force-pushed on Jul 18, 2025

in src/wallet/scriptpubkeyman.cpp:744 in 2551fdfdd6 outdated

742+            if (GetKey(extpubkey.pubkey.GetID(), key)) {
743+                privkey_count += 1;
744+            }
745+        }
746+        size_t pubkey_count{pubkeys.size() + ext_pubkeys.size()};
747+        bool watchonly{privkey_count == 0 || pubkey_count > privkey_count};

rkrux commented at 11:45 am on July 21, 2025:

In 2551fdfdd662dfb4077c3059b437acf56bdbaf5b “wallet/migration: use count private key count to identify watchonly descs”

This evaluation of watchonly at a barebones level looks great to me! Though maybe we can put this inside a IsWatchOnly function in the Descriptor{Impl} class? Reading all these details in the migration flow might be distracting, and moreover this generic function might have its own use cases separate from this later. Following patch builds and tests fine on my system. I have added few minor renaming changes as well.

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index bd85ddd9e2..bf67b8c43e 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -646,6 +646,31 @@ public:
 5         return false;
 6     }
 7 
 8+    bool IsWatchOnly(const SigningProvider& arg) const override
 9+    {
10+        size_t privkey_count{0};
11+        std::set<CPubKey> pubkeys;
12+        std::set<CExtPubKey> ext_pubkeys;
13+        this->GetPubKeys(pubkeys, ext_pubkeys);
14+        auto output_type{this->GetOutputType()};
15+        for (auto pubkey : pubkeys) {
16+            CKey key;
17+            if (arg.GetKey(pubkey.GetID(), key)) {
18+                privkey_count += 1;
19+            } else if (output_type.has_value() && *output_type == OutputType::BECH32M && arg.GetKeyByXOnly(XOnlyPubKey(pubkey), key)) {
20+                privkey_count += 1;
21+            }
22+        }
23+        for (auto extpubkey : ext_pubkeys) {
24+            CKey dummy;
25+            if (arg.GetKey(extpubkey.pubkey.GetID(), dummy)) {
26+                privkey_count += 1;
27+            }
28+        }
29+        size_t pubkey_count{pubkeys.size() + ext_pubkeys.size()};
30+        return (privkey_count == 0 || pubkey_count > privkey_count);
31+    }
32+
33     // NOLINTNEXTLINE(misc-no-recursion)
34     virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, bool* has_priv_key, const DescriptorCache* cache = nullptr) const
35     {
36diff --git a/src/script/descriptor.h b/src/script/descriptor.h
37index 09e9840645..f2e40b7f7a 100644
38--- a/src/script/descriptor.h
39+++ b/src/script/descriptor.h
40@@ -111,6 +111,9 @@ struct Descriptor {
41     /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
42     virtual bool IsSingleType() const = 0;
43 
44+    /** Whether this descriptor doesn't contain any private key material */
45+    virtual bool IsWatchOnly(const SigningProvider& provider) const = 0;
46+
47     /** Convert the descriptor to a private string. This uses public keys if the relevant private keys are not in the SigningProvider.
48      *  If none of the relevant private keys are available, the output string in the "out" parameter will not contain any private key information,
49      *  and this function will return "false".
50diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
51index 117fdd3a5e..e403bf3bbe 100644
52--- a/src/wallet/scriptpubkeyman.cpp
53+++ b/src/wallet/scriptpubkeyman.cpp
54@@ -721,27 +721,7 @@ std::optional<MigrationData> LegacyDataSPKM::MigrateToDescriptor()
55 
56         std::vector<CScript> desc_spks;
57 
58-        size_t privkey_count{0};
59-        std::set<CPubKey> pubkeys;
60-        std::set<CExtPubKey> ext_pubkeys;
61-        desc->GetPubKeys(pubkeys, ext_pubkeys);
62-        auto output_type{desc->GetOutputType()};
63-        for (auto pubkey : pubkeys) {
64-            CKey key;
65-            if (GetKey(pubkey.GetID(), key)) {
66-                privkey_count += 1;
67-            } else if (output_type.has_value() && *output_type == OutputType::BECH32M && GetKeyByXOnly(XOnlyPubKey(pubkey), key)) {
68-                privkey_count += 1;
69-            }
70-        }
71-        for (auto extpubkey : ext_pubkeys) {
72-            CKey key;
73-            if (GetKey(extpubkey.pubkey.GetID(), key)) {
74-                privkey_count += 1;
75-            }
76-        }
77-        size_t pubkey_count{pubkeys.size() + ext_pubkeys.size()};
78-        bool watchonly{privkey_count == 0 || pubkey_count > privkey_count};
79+        bool watchonly = desc->IsWatchOnly(*this);
80         if (watchonly && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
81             out.watch_descs.emplace_back(desc->ToString(), creation_time);
82 
83diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
84index 0c69849d0b..29f174be81 100644
85--- a/src/wallet/test/walletload_tests.cpp
86+++ b/src/wallet/test/walletload_tests.cpp
87@@ -26,6 +26,7 @@ public:
88     bool IsRange() const override { return false; }
89     bool IsSolvable() const override { return false; }
90     bool IsSingleType() const override { return true; }
91+    bool IsWatchOnly(const SigningProvider&) const override { return false; }
92     bool ToPrivateString(const SigningProvider& provider, std::string& out) const override { return false; }
93     bool ToNormalizedString(const SigningProvider& provider, std::string& out, const DescriptorCache* cache = nullptr) const override { return false; }
94     bool Expand(int pos, const SigningProvider& provider, std::vector<CScript>& output_scripts, FlatSigningProvider& out, DescriptorCache* write_cache = nullptr) const override { return false; };

Eunovo commented at 7:14 am on July 22, 2025:

Nice idea. Added you as Co-author in the relevant commit.

Eunovo force-pushed on Jul 22, 2025

rkrux commented at 6:44 am on July 30, 2025: contributor

Thanks for addressing the comments, I will take one final look and then a-c-k.

Tagging @furszy for review as he noticed this issue as well few months back.

in src/script/descriptor.h:114 in 2c0a84f1e2 outdated

110@@ -111,6 +111,9 @@ struct Descriptor {
111     /** Whether this descriptor will return one scriptPubKey or multiple (aka is or is not combo) */
112     virtual bool IsSingleType() const = 0;
113 
114+    /** Whether this descriptor contains any private key material */

Sjors commented at 8:01 am on July 30, 2025:

In 2c0a84f1e2ebe98bcf5066acb0de0ba5138c2101 descriptors: add IsWatchOnly(): this doesn’t describe the actual behavior. IIUC:

0/** Whether this descriptor is missing any private key material */

Eunovo commented at 7:52 pm on August 1, 2025:

Thanks for this. I’ve updated the comment.

in src/script/descriptor.cpp:640 in 2c0a84f1e2 outdated

635+        auto output_type{this->GetOutputType()};
636+        for (auto pubkey : pubkeys) {
637+            CKey key;
638+            if (arg.GetKey(pubkey.GetID(), key)) {
639+                privkey_count += 1;
640+            } else if (output_type.has_value() && *output_type == OutputType::BECH32M && arg.GetKeyByXOnly(XOnlyPubKey(pubkey), key)) {

Sjors commented at 8:19 am on July 30, 2025:

In https://github.com/bitcoin/bitcoin/commit/2c0a84f1e2ebe98bcf5066acb0de0ba5138c2101 descriptors: add IsWatchOnly(): since c++17 you can safely compare std::optional with T, see item 21 here: https://en.cppreference.com/w/cpp/utility/optional/operator_cmp.html

I also find it easier to read if privkey_count and pubkey_count are tracked in the same manner, but feel free to ignore:

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index 98ae29ceb3..a86b4743b3 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -628,26 +628,27 @@ public:
 5
 6     bool IsWatchOnly(const SigningProvider& arg) const override
 7     {
 8+        size_t pubkey_count{0};
 9         size_t privkey_count{0};
10         std::set<CPubKey> pubkeys;
11         std::set<CExtPubKey> ext_pubkeys;
12         this->GetPubKeys(pubkeys, ext_pubkeys);
13-        auto output_type{this->GetOutputType()};
14-        for (auto pubkey : pubkeys) {
15-            CKey key;
16-            if (arg.GetKey(pubkey.GetID(), key)) {
17+        for (const CPubKey pubkey : pubkeys) {
18+            pubkey_count++;
19+            CKey dummy;
20+            if (arg.GetKey(pubkey.GetID(), dummy)) {
21                 privkey_count += 1;
22-            } else if (output_type.has_value() && *output_type == OutputType::BECH32M && arg.GetKeyByXOnly(XOnlyPubKey(pubkey), key)) {
23+            } else if (this->GetOutputType() == OutputType::BECH32M && arg.GetKeyByXOnly(XOnlyPubKey(pubkey), dummy)) {
24                 privkey_count += 1;
25             }
26         }
27-        for (auto extpubkey : ext_pubkeys) {
28+        for (const CExtPubKey extpubkey : ext_pubkeys) {
29+            pubkey_count++;
30             CKey dummy;
31             if (arg.GetKey(extpubkey.pubkey.GetID(), dummy)) {
32                 privkey_count += 1;
33             }
34         }
35-        size_t pubkey_count{pubkeys.size() + ext_pubkeys.size()};
36         return (privkey_count == 0 || pubkey_count > privkey_count);
37     }

Eunovo commented at 7:52 pm on August 1, 2025:

I didn’t use this patch, but I put some effort into making the code more readable.

in src/test/miniscript_tests.cpp:191 in b32c2b290f outdated

187@@ -188,7 +188,7 @@ struct KeyConverter {
188         return g_testdata->pkmap.at(keyid);
189     }
190 
191-    std::optional<std::string> ToString(const Key& key) const {
192+    std::string ToString(const Key& key, bool* has_priv_key = nullptr) const {

Sjors commented at 8:44 am on July 30, 2025:

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

To preserve existing behaviour:

0if (has_priv_key) *has_priv_key = true;

It’s a bit worrying that the test passes whether I set this to false or true. But that’s a pre-existing issue, as the following change to the original code doesn’t break the test either:

 0diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
 1index 0a32727f37..0989f0c50a 100644
 2--- a/src/test/miniscript_tests.cpp
 3+++ b/src/test/miniscript_tests.cpp
 4@@ -189,7 +189,7 @@ struct KeyConverter {
 5     }
 6
 7     std::optional<std::string> ToString(const Key& key) const {
 8-        return HexStr(ToPKBytes(key));
 9+        return {};
10     }
11
12     miniscript::MiniscriptContext MsContext() const {

cc @darosior

Eunovo commented at 8:06 pm on August 1, 2025:

has_priv_key is only relevant when ToPrivateString is called on an actual Descriptor instance. IIUC, this test doesn’t do that, so the value of has_priv_key does not matter here. I’m not sure we should set it here, apart from the fact that it doesn’t affect the test, the string returned doesn’t have a private key.

Sjors commented at 8:52 am on July 30, 2025: member

Mostly happy with d82dcf2eaa7efb3809ae559c8f97f74403a2ccd6, but in b32c2b290f8e4dcb7fe54ee4c81714a3785920a7 descriptor: ToPrivateString() pass if at least 1 priv key exists I find the miniscript change hard to follow (mostly because I find the miniscript code itself hard to follow). Can you split that out in a seperate commit that doesn’t change behaviour?

Tested with https://github.com/Sjors/bitcoin/pull/91.

Also left some code style suggestions.

DrahtBot added the label Needs rebase on Jul 31, 2025

Eunovo force-pushed on Aug 1, 2025

DrahtBot added the label CI failed on Aug 1, 2025

DrahtBot commented at 2:41 pm on August 1, 2025: contributor

🚧 At least one of the CI tasks failed. Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/47207844179 LLM reason (✨ experimental): The CI failure is caused by an assertion failure and subprocess abortion during a benchmark test, indicating a test-related error rather than a build or compilation issue.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

descriptors: add IsWatchOnly()

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.

IsWatchOnly() is added here to replace the use of ToPrivateString() for determining
if a descriptor is 'watchonly'.

Co-authored-by: rkrux <rkrux.connect@gmail.com>

64213213b3

wallet/migration: use IsWatchOnly in place of ToPrivateString

ToPrivateString() behaviour will be modified in the following commits.
This change ensures that wallet migration does not break.

37c934e559

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.

23431cde20

descriptor: refactor miniscript Node::ToString

This commit prepares the miniscript Node::ToString for more complicated
miniscript key toString logic comming up in future commits.

d2b84af580

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

49f41141c6

walletrpc: reject listdes with priv key on w-only wallets 8e6a50cec0

test: Test listdescs with priv works even with missing priv keys 45e06e05e7

Eunovo force-pushed on Aug 1, 2025

DrahtBot removed the label Needs rebase on Aug 1, 2025

DrahtBot removed the label CI failed on Aug 1, 2025

Eunovo commented at 7:55 pm on August 1, 2025: contributor

Mostly happy with d82dcf2, but in b32c2b2 descriptor: ToPrivateString() pass if at least 1 priv key exists I find the miniscript change hard to follow (mostly because I find the miniscript code itself hard to follow). Can you split that out in a seperate commit that doesn’t change behaviour?

I split the original commit into the following commits:

I’m not sure https://github.com/bitcoin/bitcoin/pull/32471/commits/d2b84af58073630f87df32a7b785971fe32cb9be makes the miniscript change in https://github.com/bitcoin/bitcoin/pull/32471/commits/49f41141c674f83d0b66262a5b21aad623704264 easier to follow. I still think they should be one commit, but let me know if it helps. @Sjors

wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key #32471

Notes

Relevant PRs

Testing

Code Coverage & Benchmarks

Reviews

Conflicts