wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor

achow101 commented at 11:40 pm on December 22, 2023: member

It is sometimes useful for the wallet to have keys that it can sign with but are not (initially) involved in any scripts, e.g. for setting up a multisig. Ryanofsky suggested A unused(KEY) descriptor which allows for a key to be specified, but produces no scripts. These can be imported into the wallet, and subsequently retrieved with gethdkeys. Additionally, listdescriptors will output these descriptors so that they can be easily backed up.

In order to make it easier for people to add HD keys to their wallet, and to generate a new one if they want to rotate their descriptors, an addhdkey RPC is also added. Without arguments, it will generate a new HD key and add it to the wallet via a unused(KEY) descriptor. If provided a private key, it will construct the descriptor and add it to the wallet.

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
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:

#bitcoin-core/gui/872 (Menu action to export a watchonly wallet by achow101)
#32652 (wallet: add codex32 argument to addhdkey by roconnor-blockstream)
#32489 (wallet: Add exportwatchonlywallet RPC to export a watchonly version of a wallet by achow101)
#30243 (descriptors: taproot partial descriptors by Eunovo)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)

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.

DrahtBot added the label Wallet on Dec 22, 2023

DrahtBot added the label CI failed on Dec 23, 2023

ryanofsky commented at 4:40 am on December 23, 2023: contributor

This is great, amazed you could implement this so quickly!

I was thinking about the void(KEY) idea more today, and think it would be good to make 3 tweaks:

1 - rename “void(KEY)” to “unused(KEY)” 2 - add logic to importdescriptors and generatewalletdescriptor to delete any unused(KEY) descriptor as soon as the KEY which it references is used by an new descriptor (taking into account both public and private parts of the key) 3 - add logic to importdescriptors to disallow importing an unused(KEY) if KEY is used by another descriptor.

I think these changes would help make these descriptors easier to understand and also enhance backwards compatibility, because IIUC wallets containing unknown descriptor types can’t be opened by older software. Also keeping redundant descriptors in the wallet that were only temporarily needed is confusing.

If making these tweaks isn’t possible or is not a good idea. I still think probably we should rename void(KEY) to data(KEY) or something like that. I think while “void” makes a certain amount of sense as programming jargon, it’s not really a familiar term and doesn’t describe the purpose of these descriptors, which is just to hold an inert piece of data, and not be used for generating or matching scriptpubkeys.

I also have a number of ideas to improve the RPC interface for generating keys and descriptors, and will try to post them soon. (EDIT: now posted #29130 (review)). But this seems like a very good start and very functional.

achow101 force-pushed on Jan 3, 2024

DrahtBot removed the label CI failed on Jan 3, 2024

achow101 force-pushed on Jan 4, 2024

achow101 commented at 8:03 pm on January 4, 2024: member

1 - rename “void(KEY)” to “unused(KEY)” 3 - add logic to importdescriptors to disallow importing an unused(KEY) if KEY is used by another descriptor.

Done these. I’ve also added a further restriction that unused() cannot be import to a wallet without private keys. It isn’t useful in such wallets so I think it makes sense to disallow their import.

2 - add logic to importdescriptors and generatewalletdescriptor to delete any unused(KEY) descriptor as soon as the KEY which it references is used by an new descriptor (taking into account both public and private parts of the key)

Still working and thinking on this. However we’ve generally held to the policy of not deleting any records from the wallet since that could result in private keys being accidentally deleted. The only exception to that was adding encryption.

achow101 force-pushed on Jan 4, 2024

DrahtBot added the label CI failed on Jan 4, 2024

DrahtBot removed the label CI failed on Jan 4, 2024

ryanofsky commented at 5:41 pm on January 8, 2024: contributor

However we’ve generally held to the policy of not deleting any records from the wallet since that could result in private keys being accidentally deleted. The only exception to that was adding encryption.

Oh, I didn’t know that but it makes sense.

One approach you could take would just be to delete the descriptor from memory without actually deleting it from the database, and ignore it the next time the wallet is loaded. But a drawback of this would be that once addhdkey was used the wallet would be forever incompatible with older versions of the software, when one of the benefits of deleting the descriptor was to make wallets backward compatible.

Another approach that might be acceptable could be to add a DatabaseBatch::MarkErased method to call instead of DatabaseBatch::Erase. This could just prepend a serialized string like const std::string TOMBSTONE{"tombstone"} to the key and otherwise leave the record unchanged.

Or maybe just decide in this case that it is ok to delete a record after verifying all the information it contains is present in other records.

Would also want to think about it more, though.

DrahtBot added the label CI failed on Jan 15, 2024

DrahtBot added the label Needs rebase on Feb 20, 2024

achow101 force-pushed on Feb 20, 2024

DrahtBot removed the label Needs rebase on Feb 20, 2024

DrahtBot removed the label CI failed on Feb 20, 2024

DrahtBot added the label Needs rebase on Mar 29, 2024

achow101 force-pushed on Mar 29, 2024

achow101 marked this as ready for review on Mar 29, 2024

DrahtBot removed the label Needs rebase on Mar 29, 2024

DrahtBot added the label Needs rebase on Aug 28, 2024

achow101 force-pushed on Aug 29, 2024

DrahtBot removed the label Needs rebase on Aug 29, 2024

DrahtBot added the label CI failed on Aug 30, 2024

achow101 force-pushed on Sep 3, 2024

achow101 force-pushed on Sep 10, 2024

achow101 force-pushed on Sep 18, 2024

DrahtBot removed the label CI failed on Sep 18, 2024

DrahtBot added the label CI failed on Oct 20, 2024

DrahtBot removed the label CI failed on Oct 24, 2024

Sjors commented at 3:18 pm on January 16, 2025: member

Concept ACK. This seems like a step in the right direction.

I tried updating the multisig tutorial to take advantage of this in https://github.com/Sjors/bitcoin/commit/7bb9a172c6539de5c02ac70f9e38724a0e3e82dc. It also uses gethdkeys and the new <0;1> notation.

But it’s still a massive pain for two reasons:

There’s no way to get the xpub at m/87'/1'/0'; I created a workaround which inserts a dummy pk() with that derivation, but this is ugly and requires passing the master xprv around. So we still need something similiar to gethdkey PATH like I attempted in #22341.
When importing a descriptor with only xpubs the wallet isn’t smart enough to know it owns one of them. So each participant either: a) creates a watch-only wallet, and signs with the “empty” private key wallet; or b) inserts their xpriv into the right position of the multisig descriptor

2a is what the tutorial does. I played around with 2b but completely messed up the test wallet.

That doesn’t have to be solved in this PR though.

DrahtBot added the label CI failed on Mar 13, 2025

achow101 force-pushed on Apr 10, 2025

DrahtBot removed the label CI failed on Apr 10, 2025

DrahtBot added the label CI failed on May 1, 2025

DrahtBot commented at 3:07 pm on May 1, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/40291492090 LLM reason (✨ experimental): The CI failure is due to a compilation error caused by an invalid use of dynamic_cast in wallet/rpc/wallet.cpp.

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.

achow101 force-pushed on May 1, 2025

DrahtBot removed the label CI failed on May 1, 2025

rkrux commented at 12:42 pm on May 2, 2025: contributor

I have not gone into the details yet.

Quick comment - the PR title and description would now need to be updated to use unused instead of void. Also, some documentation in descriptors.md file would be nice.

DrahtBot added the label Needs rebase on May 7, 2025

achow101 force-pushed on May 7, 2025

DrahtBot removed the label Needs rebase on May 7, 2025

achow101 renamed this:
~~wallet: `addhdkey` RPC to add just keys to wallets via new `void(KEY)` descriptor~~
wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor
on May 7, 2025

achow101 commented at 6:59 pm on May 7, 2025: member

Quick comment - the PR title and description would now need to be updated to use unused instead of void

Done

Also, some documentation in descriptors.md file would be nice.

I don’t think that unused() is really a descriptor that we people should be importing by themselves. I’m not planning on standardizing it with a BIP as I think it’s more of an internal implementation detail rather than something that actually fits with the rest of the descriptor standards. So I don’t think it should be documented as if it were.

DrahtBot added the label CI failed on May 11, 2025

DrahtBot commented at 10:40 am on May 11, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/41818821482 LLM reason (✨ experimental): The build failed due to a compiler error in script/descriptor.cpp, specifically an override error.

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.

achow101 force-pushed on May 14, 2025

DrahtBot removed the label CI failed on May 14, 2025

DrahtBot added the label Needs rebase on May 17, 2025

achow101 force-pushed on May 19, 2025

Sjors commented at 5:34 pm on May 19, 2025: member

So I don’t think it should be documented as if it were.

Indeed, this is a bit of a hack that happens to work nicely with our wallet design. If other wallets end up using it too, we could reconsider that.

DrahtBot removed the label Needs rebase on May 19, 2025

in src/wallet/wallet.cpp:3756 in 556fe4bc71 outdated

3752@@ -3753,7 +3753,7 @@ util::Result<ScriptPubKeyMan*> CWallet::AddWalletDescriptor(WalletDescriptor& de
3753     // Note: we disable labels for ranged descriptors
3754     if (!desc.descriptor->IsRange()) {
3755         auto script_pub_keys = spk_man->GetScriptPubKeys();
3756-        if (script_pub_keys.empty()) {
3757+        if (script_pub_keys.empty() && desc.descriptor->HasScripts()) {

Sjors commented at 8:55 am on May 20, 2025:

In 556fe4bc71e29ca695122adbfd8dd6c9aa0f9a9e “descriptor: Add unused(KEY) descriptor”: do unused(KEY) key descriptors have labels? If not, then it would be more clear to check this condition next to !desc.descriptor->IsRange(), and update the comment.

achow101 commented at 6:48 pm on May 20, 2025:

Done

in test/functional/wallet_hd.py:53 in c6be6ba764 outdated

48+
49+        imp_xpub_info = def_wallet.gethdkeys(private=True)[0]
50+        imp_xpub = imp_xpub_info["xpub"]
51+        imp_xprv = imp_xpub_info["xprv"]
52+
53+        assert_raises_rpc_error(-5, "Could not parse HD key", wallet.addhdkey, imp_xpub)

Sjors commented at 9:07 am on May 20, 2025:

In c6be6ba76493892d42bb3ddc671cac9582f7d4b3 “test: Test for addhdkey”: it would be nice to distinguish between garbage and an xpub, and handle the latter more clearly, e.g. “Extended public key (xpub) provided, but private key (xpriv) is required.”

Also nit: could squash this test into the previous commit, since there’s no useful way test before and after when introducing a new RPC method.

achow101 commented at 6:48 pm on May 20, 2025:

Done

in src/script/descriptor.cpp:2091 in 556fe4bc71 outdated

2084@@ -2066,6 +2085,27 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
2085         error = "Can only have rawtr at top level";
2086         return {};
2087     }
2088+    if (ctx == ParseScriptContext::TOP && Func("unused", expr)) {
2089+        auto arg = Expr(expr);
2090+        if (expr.size()) {

Sjors commented at 9:14 am on May 20, 2025:

In 556fe4bc71e29ca695122adbfd8dd6c9aa0f9a9e “descriptor: Add unused(KEY) descriptor”: can you add a comment to elucidate the magic here?

achow101 commented at 6:48 pm on May 20, 2025:

Done

in src/test/descriptor_tests.cpp:1129 in 556fe4bc71 outdated

1124+    parse_pub->GetPubKeys(pub_pubkeys, pub_extpubs);
1125+    BOOST_CHECK_EQUAL(pub_pubkeys.size() + pub_extpubs.size(), 1);
1126+}
1127+
1128+// unused() descriptors don't produce scripts, so these need to be tested separately
1129+BOOST_AUTO_TEST_CASE(unused_descriptor_test)

Sjors commented at 9:16 am on May 20, 2025:

In 556fe4bc71e29ca695122adbfd8dd6c9aa0f9a9e “descriptor: Add unused(KEY) descriptor”: could add a check for empty unused():

0CheckUnparsable("unused()", "unused()", "No key provided");

achow101 commented at 6:49 pm on May 20, 2025:

Done

in src/script/descriptor.cpp:2096 in 556fe4bc71 outdated

2091+            error = strprintf("unused(): only one key expected");
2092+            return {};
2093+        }
2094+        auto keys = ParsePubkey(key_exp_index, arg, ctx, out, error);
2095+        if (keys.empty()) return {};
2096+        ++key_exp_index;

Sjors commented at 9:30 am on May 20, 2025:

In https://github.com/bitcoin/bitcoin/commit/556fe4bc71e29ca695122adbfd8dd6c9aa0f9a9e “descriptor: Add unused(KEY) descriptor”: this might be a good time to document what key_exp_index is actually for.

No test breaks if I do this:

 0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
 1index ff0782d769..ff694155e9 100644
 2--- a/src/script/descriptor.cpp
 3+++ b/src/script/descriptor.cpp
 4@@ -2093,14 +2093,12 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
 5         }
 6         auto keys = ParsePubkey(key_exp_index, arg, ctx, out, error);
 7         if (keys.empty()) return {};
 8-        ++key_exp_index;
 9-        for (auto& pubkey : keys) {
10-            if (pubkey->IsRange()) {
11-                error = "unused(): key cannot be ranged";
12-                return {};
13-            }
14-            ret.emplace_back(std::make_unique<UnusedDescriptor>(std::move(pubkey)));
15+        auto& pubkey{keys.at(0)};
16+        if (pubkey->IsRange()) {
17+            error = "unused(): key cannot be ranged";
18+            return {};
19         }
20+        ret.emplace_back(std::make_unique<UnusedDescriptor>(std::move(pubkey)));
21         return ret;

achow101 commented at 6:49 pm on May 20, 2025:

key_exp_index doesn’t really do anything for single key descriptors. I’ve dropped it here.

in test/functional/wallet_importdescriptors.py:81 in 7e9557572d outdated

70+
71+        assert_equal(len(wallet.gethdkeys()), 0)
72+
73+        xprv = "tprv8ZgxMBicQKsPeuVhWwi6wuMQGfPKi9Li5GtX35jVNknACgqe3CY4g5xgkfDDJcmtF7o1QnxWDRYw4H5P26PXq7sbcUkEqeR4fg3Kxp2tigg"
74+        xpub = "tpubD6NzVbkrYhZ4YNXVQbNhMK1WqguFsUXceaVJKbmno2aZ3B6QfbMeraaYvnBSGpV3vxLyTTK9DYT1yoEck4XUScMzXoQ2U2oSmE2JyMedq3H"
75+        self.test_importdesc({"timestamp": "now", "desc": descsum_create(f"unused({xprv})")},

Sjors commented at 9:39 am on May 20, 2025:

In 7e9557572d2f2a958d740b6dd93e8f4b92141e67 “test: Simple test for importing unused(KEY)”: can you add a test to verify that you can’t import the tpub?

This passes (when put above the xprv import):

0self.test_importdesc({"timestamp": "now", "desc": descsum_create(f"unused({xpub})")},
1                             success=False,
2                             wallet=wallet)

achow101 commented at 6:49 pm on May 20, 2025:

Done

in src/wallet/rpc/wallet.cpp:960 in 213103fdc1 outdated

955+        {
956+            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
957+            if (!wallet) return UniValue::VNULL;
958+
959+            if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
960+                throw JSONRPCError(RPC_WALLET_ERROR, "addhdkey is not available for non-descriptor wallets");

Sjors commented at 9:40 am on May 20, 2025:

In 213103fdc1c9ebcabbe01e1fc87345409a90dee2 “wallet: Add addhdkey RPC”: I guess we don’t need this anymore

achow101 commented at 6:49 pm on May 20, 2025:

Done

Sjors commented at 9:42 am on May 20, 2025: member

Mostly happy with d457faf91e31c33063e45ee050f7480436506635

achow101 force-pushed on May 20, 2025

Sjors commented at 7:52 am on May 21, 2025: member

utACK b23cefb4f3abe33c7bc933b60f1c0d15137c627c

DrahtBot added the label CI failed on May 28, 2025

DrahtBot commented at 6:01 pm on May 28, 2025: contributor

🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/42580005912 LLM reason (✨ experimental): The CI failure is caused by a compilation error in wallet/rpc/wallet.cpp due to an invalid dynamic_cast attempt on a non-pointer type.

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.

descriptor: Add unused(KEY) descriptor

unused() descriptors do not have scriptPubKeys. Instead, the wallet uses
them to store keys without having any scripts to watch for.

10b677530e

test: Simple test for importing unused(KEY) 92849bdd9b

achow101 force-pushed on May 28, 2025

achow101 commented at 7:44 pm on May 28, 2025: member

Rebased for silent merge conflict

DrahtBot removed the label CI failed on May 28, 2025

Sjors commented at 7:16 am on May 29, 2025: member

re-utACK b4ef67a3971f19c939135aeec59f11c6250f5368

Changes seem to be due to #32475.

Sjors commented at 10:06 am on June 2, 2025: member

There might be a silent conflict with #32514.

in src/wallet/rpc/wallet.cpp:936 in 07128034f0 outdated

928@@ -929,6 +929,83 @@ static RPCHelpMan createwalletdescriptor()
929     };
930 }
931 
932+RPCHelpMan addhdkey()
933+{
934+    return RPCHelpMan{
935+        "addhdkey",
936+        "\nAdd a BIP 32 HD key to the wallet that can be used with 'createwalletdescriptor'\n",

Sjors commented at 10:09 am on June 2, 2025:

First \n needs to be dropped or help addhdkey throws.

cc @maflcko is there no test that calls help on every method?

maflcko commented at 10:20 am on June 2, 2025:

cc @maflcko is there no test that calls help on every method?

There should be one. However, I don’t think it can catch this right now, because the help command will internally iterate over the individual help calls and ignore the type of the exception (the help is returned via an exception).

See:

0        catch (const std::exception& e)
1        {
2            // Help text is returned in an exception
3            std::string strHelp = std::string(e.what());

It would be good to write more type-safe code in the RPC server, or file an issue to track it.

maflcko commented at 10:20 am on June 2, 2025:

#32588 may also help to catch it, but the code should probably still be made more type-safe.

maflcko commented at 12:13 pm on June 2, 2025:

type-safe

fixed in https://github.com/bitcoin/bitcoin/pull/32660

achow101 commented at 7:36 pm on June 2, 2025:

Fixed

Sjors referenced this in commit fe52a37b1c on Jun 2, 2025

wallet: Add addhdkey RPC 0d24f0af7a

wallet, rpc: Disallow import of unused() if key already exists cba3bcb6a7

wallet, rpc: Disallow importing unused() to wallets without privkeys 6100108594

achow101 force-pushed on Jun 2, 2025

Sjors commented at 5:49 am on June 3, 2025: member

re-ACK 61001085941fd6d78f4cd46c9b3d65914b6e961e

wallet: addhdkey RPC to add just keys to wallets via new unused(KEY) descriptor #29136

Code Coverage & Benchmarks

Reviews

Conflicts

wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor #29136