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

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 (descriptor: Add proper Clone function to miniscript::Node #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 · 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.

DrahtBot · 2025-08-01T14:41:51Z

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

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-08-01T19:55:36Z

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 d2b84af makes the miniscript change in 49f4114 easier to follow. I still think they should be one commit, but let me know if it helps.

@Sjors

Sjors

ACK 45e06e0

I find that the separation of 23431cd makes it easier to follow. The separation of d2b84af from 49f4114 is less useful, but fine by me.

@achow101 can you update your concept opinion based on the updated behaviour? I think it makes sense to return a result if any private key is present in descriptor, rather than all.

src/script/descriptor.cpp

Sjors · 2025-08-04T12:16:46Z

src/test/fuzz/miniscript.cpp

    {
        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()) {


In 49f4114 descriptor: ToPrivateString() pass if at least 1 priv key exists: maybe for good measure:

*has_priv_key = false;

In both places it seems better to check that priv_key exists, even if that's always the case in this fuzzer. IIUC in that case you can leave out tmp{false} and just pass nullptr below.

achow101

Concept ACK

achow101 · 2025-08-06T22:24:26Z

src/script/descriptor.cpp

@@ -841,6 +841,38 @@ class DescriptorImpl : public Descriptor
        return true;
    }

+    bool IsWatchOnly(const SigningProvider& arg) const override


In 6421321 "descriptors: add IsWatchOnly()"

I think this function can be vastly simplified by calling GetPrivKey on all the PubkeyProvider, and IsWatchOnly on all subdescriptors.

nit: I don't like this function name since whether the descriptor is watchonly depends on the contents of the provided parameter. It's not a property like a Is* name implies. Watchonly also is not a property applied to descriptors as private keys are not actually part of the descriptor. Perhaps something like HavePrivateKeys instead.

I was able to simplify the code as suggested, and I renamed it to HavePrivateKeys()

achow101 · 2025-08-06T22:32:58Z

src/script/descriptor.cpp

@@ -896,24 +896,27 @@ class DescriptorImpl : public Descriptor
    }

    // NOLINTNEXTLINE(misc-no-recursion)
-    virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, const DescriptorCache* cache = nullptr) const
+    virtual bool ToStringSubScriptHelper(const SigningProvider* arg, std::string& ret, const StringType type, bool* has_priv_key, const DescriptorCache* cache = nullptr) const


In 49f4114 "descriptor: ToPrivateString() pass if at least 1 priv key exists"

has_priv_key should be a reference not a pointer. The pointer-ness is never used in this function.

The has_priv_key parameter has been removed.

achow101 · 2025-08-06T22:33:42Z

src/script/descriptor.cpp

@@ -922,7 +925,8 @@ class DescriptorImpl : public Descriptor
                    if (!pubkey->ToNormalizedString(*arg, tmp, cache)) return false;
                    break;
                case StringType::PRIVATE:
-                    if (!pubkey->ToPrivateString(*arg, tmp)) return false;
+                    assert(has_priv_key != nullptr);


In 49f4114 "descriptor: ToPrivateString() pass if at least 1 priv key exists"

If has_priv_key is never supposed to be null, then it should be a reference. Otherwise, this should be an if, we should not rely on caller behavior.

achow101 · 2025-08-06T22:36:03Z

src/script/descriptor.cpp

+        bool has_priv_key{false};
+        // ToStringHelper should never fail for StringType::PRIVATE,
+        // because it falls back to StringType::PUBLIC when no private key is available.
+        assert(ToStringHelper(&arg, out, StringType::PRIVATE, &has_priv_key));


In 49f4114 "descriptor: ToPrivateString() pass if at least 1 priv key exists"

asserts should not have side effects. If we are calling a function to actually do something, it should not be in an assert.

However, if the return value of ToStringHelper is always supposed to be true, then it should not be returning anything anyways. Furthermore, I don't see why ToStringHelper doesn't return whether the string has a private key, as we are doing in this function. That would allow us to drop has_priv_key from all of the function.

I refactored the code as suggested and was able to drop has_priv_key from the parameter list. Now, has_priv_key is returned if type == StringType::PRIVATE.

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 <rkrux.connect@gmail.com>

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

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.

DrahtBot · 2025-08-08T15:54:21Z

🚧 At least one of the CI tasks failed.
_{Task tidy: https://github.com/bitcoin/bitcoin/runs/47688035952}
_{LLM reason (✨ experimental): The CI failure is caused by a clang-tidy error detecting recursive call chains in src/script/descriptor.cpp.}

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-08-09T04:03:47Z

Thanks for the reviews! I have:

renamed IsWatchOnly() to HavePrivateKeys()
Added a test for HavePrivateKeys() to descriptor_tests
Removed the unnecessary asserts from ToStringHelper() and ToPrivateString()
Removed has_priv_key from ToStringHelper() and ToSubStringHelper() parameter lists

rkrux

The miniscript code took some time for me to understand.

lgtm ACK 4241edf

src/script/descriptor.cpp

rkrux · 2025-08-11T13:01:35Z

src/script/descriptor.h

+     *         or if the provider does not have all private keys required by
+     *         the descriptor.
+     */
+    virtual bool HavePrivateKeys(const SigningProvider& provider) const = 0;


In 726181b "descriptors: add HavePrivateKeys()"

Naming nit because the function is a part of a singular Descriptor class, but I see there is a combination of Have* (not my preference) and Has* functions in the singular object classes in the codebase, so feel free to ignore.

- virtual bool HavePrivateKeys(const SigningProvider& provider) const = 0; + virtual bool HasPrivateKeys(const SigningProvider& provider) const = 0;

- 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

rkrux

ACK 46860de

git range-diff 4241edf...46860de

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

Open

miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) #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

Open

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

Open

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

Eunovo force-pushed the list-descriptors-with-partial-keys branch 3 times, most recently from 37b1fdf to a26825c Compare August 1, 2025 14:41

DrahtBot added the CI failed label Aug 1, 2025

Eunovo force-pushed the list-descriptors-with-partial-keys branch from a26825c to 45e06e0 Compare August 1, 2025 15:17

DrahtBot removed Needs rebase CI failed labels Aug 1, 2025

Sjors approved these changes Aug 4, 2025

View reviewed changes

DrahtBot requested a review from rkrux August 4, 2025 12:27

achow101 reviewed Aug 6, 2025

View reviewed changes

Eunovo force-pushed the list-descriptors-with-partial-keys branch from 45e06e0 to 8244486 Compare August 8, 2025 15:08

Eunovo and others added 3 commits August 8, 2025 16:53

wallet/migration: use HavePrivateKeys in place of ToPrivateString

2b11087

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

descriptor: refactor ToPrivateString for providers

de6acc2

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.

Eunovo force-pushed the list-descriptors-with-partial-keys branch from 8244486 to 4241edf Compare August 8, 2025 15:53

DrahtBot added the CI failed label Aug 8, 2025

DrahtBot removed the CI failed label Aug 8, 2025

rkrux reviewed Aug 11, 2025

View reviewed changes

src/script/descriptor.cpp Outdated Show resolved Hide resolved

src/script/descriptor.cpp Outdated Show resolved Hide resolved

src/script/descriptor.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from Sjors and achow101 August 11, 2025 12:45

rkrux reviewed Aug 11, 2025

View reviewed changes

Eunovo added 3 commits August 11, 2025 16:55

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

d9cf49c

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

46860de

Eunovo force-pushed the list-descriptors-with-partial-keys branch from 4241edf to 46860de Compare August 11, 2025 14:57

rkrux approved these changes Aug 15, 2025

View reviewed changes

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

Uh oh!

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

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!

DrahtBot commented Aug 1, 2025

Uh oh!

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eunovo Aug 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Eunovo commented Aug 1, 2025 •

edited

Loading

Sjors left a comment •

edited

Loading

Eunovo Aug 8, 2025 •

edited

Loading

rkrux left a comment •

edited

Loading

rkrux Aug 11, 2025 •

edited

Loading