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

Eunovo · 2025-05-12T13:26:20Z

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 Descriptor unit tests and simplifications #24361 (comment)
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 · 2025-05-12T13:26:23Z

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
ACK	Sjors
Concept NACK	vicjuma
Concept ACK	achow101
Stale ACK	w0xlt, rkrux

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

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)
#31713 (miniscript refactor: Remove unique_ptr-indirection by hodlinator)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

at least one private key available. -> at least one private key is available. [Missing verb "is" makes the sentence ungrammatical.]

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

assert_raises_rpc_error(-4, 'Can't get private descriptor string for watch-only wallets', watch_only_wallet.listdescriptors, True) in test/functional/wallet_listdescriptors.py

^2026-01-07

fanquake · 2025-05-12T13:35:50Z

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

DrahtBot · 2025-05-12T13:38:48Z

🚧 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.}

Hints

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 · 2025-05-12T13:51:15Z

Putting draft while I fix some issues with the fuzz tests

Eunovo · 2025-05-13T16:02:24Z

cc @rkrux @sipa

achow101 · 2025-05-13T17:32:49Z

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 · 2025-05-13T17:40:20Z

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 · 2025-05-13T18:10:25Z

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.

[
  {
    "success": true,
    "warnings": [
      "Not all private keys provided. Some wallet functionality may return unexpected errors"
    ]
  }
]

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 · 2025-05-13T18:29:06Z

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 · 2025-05-13T18:50:48Z

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 · 2025-05-14T06:19:20Z

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 · 2025-05-14T06:22:16Z

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 · 2025-05-14T06:29:49Z

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.

rkrux · 2025-05-21T13:02:19Z

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:

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

It appears to be coming from here when the xprv corresponding to the xpub can't be found:

bitcoin/src/wallet/rpc/wallet.cpp

Line 822 in 7763e86

xpub_info.pushKV("xprv", EncodeExtKey(wallet_xprvs.at(xpub)));

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

reACK 2b16014

git range-diff 8b4d4b3...2b16014

Sjors

Was in the middle of a review, but had to leave. Some comments.

src/script/descriptor.cpp

Eunovo · 2025-11-20T08:32:55Z

Rebased on master. I updated the Miniscript implementation slightly due to changes from #31734

Sjors

I still need to (re-)review the non-test changes of 4dc37d8, but everything else looks fine.

I wrote an expansion of the fuzzer so it actually covers has_priv_key, see Eunovo#3, but it can wait for a followup.

The commit message for 5837973 walletrpc: reject listdes with priv key on w-only wallets could mention that this improves the error message.

src/wallet/test/walletload_tests.cpp

Sjors · 2026-01-06T06:00:48Z

src/wallet/scriptpubkeyman.cpp

-        std::string desc_str;
-        bool watchonly = !desc->ToPrivateString(*this, desc_str);
-        if (watchonly && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+        if (!desc->HavePrivateKeys(*this) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {


In 7e2af06 wallet/migration: use HavePrivateKeys in place of ToPrivateString: we should have a comment here to explain what this if condition if trying to achieve. The original comment (now deleted) wasn't great either. Try:

// 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 (...

It's useful to know, as you write in the PR description:

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.

Not sure if we want to further expand the comment, but let's add that to the commit message. That way future devs can understand why migration is inconsistent with RPC behaviour.

I do agree it's good to limit the PR scope in this way.

I added your comment as is and updated the commit message.

src/test/fuzz/miniscript.cpp

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 <[email protected]>

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.

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 <[email protected]>

Sjors · 2026-01-07T09:20:46Z

src/script/descriptor.cpp

+        bool has_priv_key{false};
+        auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache), has_priv_key);
+        if (res) out = *res;
+        return type == StringType::PRIVATE ? has_priv_key : res.has_value();


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

The following makes it more clear why for private strings we only look at has_priv_key and don't care about whether res has a value:

if (type == StringType::PRIVATE) { Assume(res.has_value()); return has_priv_key; } else { return res.has_value(); }

Or a bit shorter:

return res.has_value() && (type != StringType::PRIVATE || has_priv_key);

Though in the edge case of StringType::PRIVATE with no res, this would return false which is a bit unexpected.

The Assume check here is useful because we expect res to always have a value, so I think the longer version is better. I have rebased the PR with the change.

Sounds good, I rebased Sjors#108 on top of your latest push to see if it survives the current fuzz corpus sanity check.

Sjors · 2026-01-07T09:42:21Z

ACK 165f728

Just left one more (vague) suggestion above.

- 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

Co-authored-by: rkrux <[email protected]>

Sjors · 2026-01-09T08:35:50Z

ACK 9c7e477

DrahtBot added the CI failed label May 12, 2025

Eunovo marked this pull request as draft May 12, 2025 13:50

Eunovo force-pushed the list-descriptors-with-partial-keys branch 2 times, most recently from 1ff2b00 to 4f4ca3a Compare May 12, 2025 19:14

This was referenced May 13, 2025

miniscript: account for all StringType variants in Miniscriptdescriptor::ToString() #31734

Merged

miniscript refactor: Remove unique_ptr-indirection #31713

Open

Eunovo force-pushed the list-descriptors-with-partial-keys branch from 4f4ca3a to bb14e8a Compare May 13, 2025 12:35

This was referenced May 13, 2025

descriptors: MuSig2 #31244

Merged

descriptors: taproot partial descriptors #30243

Closed

wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675

Merged

DrahtBot removed the CI failed label May 13, 2025

Eunovo marked this pull request as ready for review May 13, 2025 16:01

This was referenced May 14, 2025

Menu action to export a watchonly wallet bitcoin-core/gui#872

Draft

wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet #32489

Open

Eunovo changed the title ~~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 May 14, 2025

DrahtBot mentioned this pull request Jun 11, 2025

Musig2 tests #32724

Open

DrahtBot requested a review from rkrux October 22, 2025 07:53

rkrux approved these changes Nov 6, 2025

View reviewed changes

Sjors reviewed Nov 12, 2025

View reviewed changes

src/script/descriptor.cpp Show resolved Hide resolved

DrahtBot requested a review from Sjors November 12, 2025 13:26

kevkevinpal mentioned this pull request Nov 18, 2025

miniscript: account for all StringType variants in Miniscriptdescriptor::ToString() kevkevinpal/bitcoin#208

Open

DrahtBot added the Needs rebase label Nov 18, 2025

Eunovo force-pushed the list-descriptors-with-partial-keys branch 2 times, most recently from 56b14fb to 3151f8d Compare November 19, 2025 16:01

DrahtBot added CI failed and removed Needs rebase labels Nov 19, 2025

Eunovo force-pushed the list-descriptors-with-partial-keys branch from 3151f8d to 3297f24 Compare November 19, 2025 17:14

DrahtBot removed the CI failed label Nov 19, 2025

Sjors reviewed Jan 6, 2026

View reviewed changes

Sjors mentioned this pull request Jan 6, 2026

Fix has_priv_key in fuzzer and add coverage Eunovo/bitcoin#3

Open

Eunovo and others added 3 commits January 7, 2026 09:34

Sjors reviewed Jan 7, 2026

View reviewed changes

Eunovo force-pushed the list-descriptors-with-partial-keys branch from 3297f24 to 165f728 Compare January 7, 2026 09:34

DrahtBot requested review from rkrux and w0xlt January 7, 2026 09:42

Eunovo and others added 3 commits January 7, 2026 10:44

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

ed945a6

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

9c7e477

Co-authored-by: rkrux <[email protected]>

Eunovo force-pushed the list-descriptors-with-partial-keys branch from 165f728 to 9c7e477 Compare January 7, 2026 09:51

DrahtBot added CI failed and removed CI failed labels Jan 7, 2026

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

Are you sure you want to change the base?

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

Conversation

Eunovo commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Relevant PRs

Testing

Uh oh!

DrahtBot commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

fanquake commented May 12, 2025

Uh oh!

DrahtBot commented May 12, 2025

Uh oh!

Eunovo commented May 12, 2025

Uh oh!

Eunovo commented May 13, 2025

Uh oh!

achow101 commented May 13, 2025

Uh oh!

vicjuma commented May 13, 2025

Uh oh!

rkrux commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vicjuma commented May 13, 2025

Uh oh!

achow101 commented May 13, 2025

Uh oh!

Eunovo commented May 14, 2025

Uh oh!

Eunovo commented May 14, 2025

Uh oh!

Eunovo commented May 14, 2025

Uh oh!

rkrux commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Eunovo commented Nov 20, 2025

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Eunovo Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eunovo Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Eunovo commented May 12, 2025 •

edited

Loading

DrahtBot commented May 12, 2025 •

edited

Loading

rkrux commented May 13, 2025 •

edited

Loading

rkrux commented May 21, 2025 •

edited

Loading

Sjors left a comment •

edited

Loading

Sjors Jan 7, 2026 •

edited

Loading

Eunovo Jan 7, 2026 •

edited

Loading

Sjors Jan 7, 2026 •

edited

Loading

Sjors commented Jan 7, 2026 •

edited

Loading