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

achow101 · 2023-12-22T23:40:15Z

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, 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:

#33135 (wallet: prevent accidental unsafe older() import by Sjors)
#33008 (wallet: support bip388 policy with external signer by Sjors)
#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)

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.

ryanofsky · 2023-12-23T04:40:39Z

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 (comment)). But this seems like a very good start and very functional.

achow101 · 2024-01-04T20:03:16Z

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.

ryanofsky · 2024-01-08T17:41:01Z

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.

One use case for this is a multisig setup that starts from a blank wallet with an unused(KEY) descriptor (proposed in bitcoin#29136). A descriptor is then imported that contains both a key from this wallet and that of an external signer.

Sjors · 2025-06-03T05:49:59Z

re-ACK 6100108

Sjors · 2025-06-20T11:05:28Z

While working on #32784 I noticed we don't seem to be rotating unused(KEY) descriptors when encrypting the wallet, though I have to double check this...

Another thing I noticed is that createwalletdescriptor is not smart enough to just use the only available unused(KEY) descriptor.

One use case for this is a multisig setup that starts from a blank wallet with an unused(KEY) descriptor (proposed in bitcoin#29136). A descriptor is then imported that contains both a key from this wallet and that of an external signer.

achow101 · 2025-07-02T22:14:07Z

we don't seem to be rotating unused(KEY) descriptors when encrypting the wallet,

We only rotate the automatically generated descriptors, and unused(KEY) is not one of those.

Sjors · 2025-07-03T08:56:03Z

Another thing I noticed is that createwalletdescriptor is not smart enough to just use the only available unused(KEY) descriptor.

I opened a followup PR for this #32861.

(original ACK still stands)

One use case for this is a multisig setup that starts from a blank wallet with an unused(KEY) descriptor (proposed in bitcoin#29136). A descriptor is then imported that contains both a key from this wallet and that of an external signer.

src/wallet/rpc/wallet.cpp

src/script/descriptor.cpp

rkrux · 2025-07-07T13:08:14Z

src/script/descriptor.cpp

+        for (auto& pubkey : keys) {
+            if (pubkey->IsRange()) {
+                error = "unused(): key cannot be ranged";
+                return {};
+            }
+            ret.emplace_back(std::make_unique<UnusedDescriptor>(std::move(pubkey)));
+        }


In 10b6775 "descriptor: Add unused(KEY) descriptor"

For explicitness, loop can be replaced with an Assume on length being one.

- for (auto& pubkey : keys) { - if (pubkey->IsRange()) { - error = "unused(): key cannot be ranged"; - return {}; - } - ret.emplace_back(std::make_unique<UnusedDescriptor>(std::move(pubkey))); + Assume(keys.size() == 1); + auto& pubkey = keys[0]; + if (pubkey->IsRange()) { + error = "unused(): key cannot be ranged"; + return {}; } + ret.emplace_back(std::make_unique<UnusedDescriptor>(std::move(pubkey)));

We can have multiple keys if the key is multipath.

Ah right! Fine to keep it as-is in the parsing flow.

Via addhdkey RPC though, I don't suppose there is a way to get multipath keys added? (as only one hdkey can be generated or imported)

They can be imported with importdescriptors.

src/wallet/wallet.cpp

src/wallet/rpc/wallet.cpp

rkrux · 2025-07-07T13:56:19Z

src/wallet/rpc/wallet.cpp

+            if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+                throw JSONRPCError(RPC_WALLET_ERROR, "addhdkey is not available for wallets without private keys");


In 0d24f0a "wallet: Add addhdkey RPC"

Should we add a check for external signer wallet flag as well here?

External signer always has private keys disabled.

I'm regretting that assumption in the context of multisig :-)

rkrux · 2025-07-07T14:03:21Z

src/wallet/rpc/wallet.cpp

+            std::string desc_str = "unused(" + EncodeExtKey(hdkey) + ")";
+            FlatSigningProvider keys;
+            std::string error;
+            std::vector<std::unique_ptr<Descriptor>> descs = Parse(desc_str, keys, error, false);


In 0d24f0a "wallet: Add addhdkey RPC"

Shouldn't the descriptor checksum be added?

Edit: I see it's added automatically, still figuring out where it's coming from.

This call to Parse has require_checksum set to false so the checksum is not required.

test/functional/wallet_hd.py

rkrux · 2025-07-10T15:53:35Z

Concept ACK 6100108

I am looking forward to the PRs built on top of this one.

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

Sjors · 2025-07-25T07:07:34Z

re-ACK 439734e

(you can drop "Based on" from the PR description)

rkrux

ACK 439734e

Thanks for incorporating suggestions; TIL about the for-else pattern in Python.

git range-diff 6100108...439734e

rkrux · 2025-07-25T08:55:27Z

test/functional/wallet_hd.py

+            if len(x["descriptors"]) == 1 and x["descriptors"][0]["desc"].startswith("unused("):
+                break
+        else:
+            assert False, "Did not find HD key with no descriptors"


In 71b4900 "wallet: Add addhdkey RPC"

Nit if retouched:

- assert False, "Did not find HD key with no descriptors" + assert False, "Did not find any HD key with unused descriptor"

rkrux · 2025-07-25T08:55:48Z

src/script/descriptor.cpp

+        for (auto& pubkey : keys) {
+            if (pubkey->IsRange()) {
+                error = "unused(): key cannot be ranged";
+                return {};
+            }
+            ret.emplace_back(std::make_unique<UnusedDescriptor>(std::move(pubkey)));
+        }


Ah right! Fine to keep it as-is in the parsing flow.

Via addhdkey RPC though, I don't suppose there is a way to get multipath keys added? (as only one hdkey can be generated or imported)

DrahtBot added Wallet CI failed labels Dec 22, 2023

This was referenced Dec 23, 2023

wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors #29130

Merged

wallet: reenable sethdseed for descriptor wallets #29054

Closed

achow101 force-pushed the sethdseed-void-descriptor branch from 5338f09 to 18c2d9a Compare January 3, 2024 00:34

DrahtBot removed the CI failed label Jan 3, 2024

achow101 force-pushed the sethdseed-void-descriptor branch from 18c2d9a to 3e6a00d Compare January 4, 2024 19:03

achow101 force-pushed the sethdseed-void-descriptor branch 2 times, most recently from 42f1fc8 to 22c8984 Compare January 4, 2024 20:48

DrahtBot added CI failed and removed CI failed labels Jan 4, 2024

DrahtBot mentioned this pull request Jan 5, 2024

refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc #26840

Closed

DrahtBot added the CI failed label Jan 15, 2024

This was referenced Jan 16, 2024

Silent Payments: sending #28201

Draft

log, refactor: Allow log macros to accept context arguments #29256

Open

achow101 force-pushed the sethdseed-void-descriptor branch from b4ef67a to 6100108 Compare June 2, 2025 19:36

achow101 mentioned this pull request Jun 2, 2025

rpc: Use type-safe exception to pass RPC help #32660

Merged

Sjors mentioned this pull request Jun 20, 2025

wallet: derivehdkey RPC to get xpub at arbitrary path #32784

Draft

Sjors mentioned this pull request Jun 25, 2025

MuSig2 power Sjors/bitcoin#91

Open

Sjors mentioned this pull request Jul 3, 2025

Have createwalletdescriptor auto-detect an unused(KEY) #32861

Draft

rkrux reviewed Jul 7, 2025

View reviewed changes

This was referenced Jul 14, 2025

Silent Payments: Receiving #32966

Draft

wallet: support bip388 policy with external signer #33008

Draft

achow101 added 2 commits July 24, 2025 13:58

descriptor: Add unused(KEY) descriptor

1806299

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

test: Simple test for importing unused(KEY)

fb2eb22

achow101 force-pushed the sethdseed-void-descriptor branch from 6100108 to 8ee55e4 Compare July 24, 2025 21:04

achow101 added 4 commits July 24, 2025 14:18

wallet: Add addhdkey RPC

71b4900

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

3aff52c

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

844dd63

doc: Release note for addhdkey

439734e

achow101 force-pushed the sethdseed-void-descriptor branch from 8ee55e4 to 439734e Compare July 24, 2025 21:18

DrahtBot requested a review from rkrux July 25, 2025 07:07

rkrux approved these changes Jul 25, 2025

View reviewed changes

This was referenced Aug 5, 2025

wallet: prevent accidental unsafe older() import #33135

Open

BIP360: Includes the following: #33163

Closed

		if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
		throw JSONRPCError(RPC_WALLET_ERROR, "addhdkey is not available for wallets without private keys");

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

Are you sure you want to change the base?

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

Conversation

achow101 commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

ryanofsky commented Dec 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jan 4, 2024

Uh oh!

ryanofsky commented Jan 8, 2024

Uh oh!

Sjors commented Jun 3, 2025

Uh oh!

Sjors commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jul 2, 2025

Uh oh!

Sjors commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

rkrux Jul 7, 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!

Uh oh!

Uh oh!

Uh oh!

rkrux commented Jul 10, 2025

Uh oh!

Sjors commented Jul 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

achow101 commented Dec 22, 2023 •

edited

Loading

DrahtBot commented Dec 22, 2023 •

edited

Loading

ryanofsky commented Dec 23, 2023 •

edited

Loading

Sjors commented Jun 20, 2025 •

edited

Loading

Sjors commented Jul 3, 2025 •

edited

Loading

rkrux Jul 7, 2025 •

edited

Loading

Sjors commented Jul 25, 2025 •

edited

Loading