Part of #35645
Adds a key label record to the wallet database so master keys can be identified by a label.
Also adds three new RPC calls, setkeylabel, getkeylabel and listkeylabels which allow to set, read and list all key labels for master keys.
Also adds four new interfaces so the GUI can consume those functionality too and list in, for example, a multisig setup labels associated to the keys. See https://gist.github.com/pseudoramdom/4e14d21a93323217cfe016885285b9aa for a visual example.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36126.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | jeanpablojp |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible typos and grammar issues:
This augments does not replace per-address labels set with setlabel. -> This augments, not replaces, per-address labels set with setlabel. [Grammar is broken; the intended meaning is unclear as written.]usefull -> useful [Misspelling.]getkeylabe -> getkeylabel [Misspelled RPC name in documentation.]get the the label -> get the label [Duplicate word.]<sup>2026-08-31 12:19:10</sup>
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/33377440526/job/99441988368</sub>
<sub>LLM reason (✨ experimental): CI failed because IWYU detected missing/incorrect includes (failure generated from the IWYU check).</sub>
<details><summary>Hints</summary>
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.
</details>
Introduce a new `keylabel` wallet database record that maps a master key
fingerprint to an arbitrary label string. This is the persistence layer
for per-key labels, stored separately from the per-address name records.
Add WalletBatch::WriteKeyLabel/EraseKeyLabel, mirroring other Write/Erase functions.
The record is keyed by the fingerprint so that a single label covers every
address derived from the same root across all descriptor types.
Add CWallet::m_key_labels, a map from master key fingerprint to label that
mirrors the new `keylabel` DB records. Per-key labels augment, do not
replace, the per-address labels in m_address_book and never affect the
change/receive heuristic in CAddressBookData::IsChange().
Adds accesors for adding and removing key labels.
Also adds DecodeKeyFingerprnt helper function that will be used
for multiple RPC introduced in future commits.
567 | }, 568 | }; 569 | } 570 | 571 | +// Parse an 8-hex-character (4-byte) master key fingerprint from an RPC string. 572 | +static KeyFingerprint DecodeKeyFingerprint(const std::string& hex)
The labels will really help in identifying cosigners, especially. Just 2 questions regarding setting them (setkeylabel)
Yes to both questions :) I don't see why any of those two would be a problem. But I am not closed to change that behavior if we agree on it.
Concept ACK
The RPCs call CWallet directly, so the four new interfaces::Wallet methods end up with no caller and no test. The mining RPCs go through interfaces::Mining. Worth the same here?
Built the ten commits and ran the new test.
585 | +RPCMethod setkeylabel() 586 | +{ 587 | + return RPCMethod{ 588 | + "setkeylabel", 589 | + "Sets the label associated with a master key fingerprint.\n" 590 | + "Every address corresponding to a key derived from that master key inherits the label.\n"
With both cosigners labelled, getaddressinfo on a wsh(sortedmulti(...)) address returns no key label. Neither does tr(k,sortedmulti_a(...)) with k itself labelled, since GetKeyForDestination gives up once a merkle root exists. Plain tr(k) works. Is single-key the intended scope, or should this follow the descriptor's origins?
In my opinion it should keep in the scope of the single-key. If the user wants to have a label on the cases you mentioned, they should add it at the descriptor level. This new key label is not labeling descriptors but master keys.
2562 | + const auto it = m_key_labels.find(fingerprint); 2563 | + if (it == m_key_labels.end()) return std::nullopt; 2564 | + return it->second; 2565 | +} 2566 | + 2567 | +std::optional<std::string> CWallet::GetKeyLabelForDest(const CTxDestination& dest) const
If single-key is the intended scope, getaddressinfo already has meta->key_origin.fingerprint in hand when it pushes hdmasterfingerprint, and reusing it there leaves this helper with no caller. I tried that and the functional tests touching getaddressinfo still pass.
485 | @@ -486,6 +486,22 @@ class WalletImpl : public Wallet 486 | auto spk_man = m_wallet->GetScriptPubKeyMan(OutputType::BECH32M, /*internal=*/false); 487 | return spk_man != nullptr; 488 | } 489 | + bool setKeyLabel(KeyFingerprint fingerprint, std::string label) override {
An empty label here writes a blank record, while setkeylabel with an empty string removes it. Since delKeyLabel already exists, shouldn't the two agree?
555 | labels.push_back(address_book_entry->GetLabel()); 556 | } 557 | + // Per-key labels (keyed by master fingerprint) augment the per-address label 558 | + // above. A single address can therefore carry both its own address-book label 559 | + // and the label inherited from the corresponding master key. 560 | + const std::optional<std::string> key_label{pwallet->GetKeyLabelForDest(dest)};
With HW set on the key, the key label lands at index 0 on a change address, and disappears on an address whose own label is also HW. Both cases return ["HW"] and nothing in the response tells them apart. That label doesn't show up in listlabels either, and getaddressesbylabel answers -11 No addresses with label. Wouldn't a key_labels object keyed by fingerprint be simpler, and cover the multisig case too?
44 | + change = wallet.getrawchangeaddress() 45 | + assert_equal(wallet.getaddressinfo(change)["labels"], ["my hardware wallet"]) 46 | + 47 | + # Augments, does not replace, per-address labels. 48 | + wallet.setlabel(addr, "receiving") 49 | + assert_equal(wallet.getaddressinfo(addr)["labels"], ["receiving", "my hardware wallet"])
If the array shape stays, this branch has no assertion. I tried these lines, they pass on the head and fail if the suppression goes.
assert_equal(wallet.getaddressinfo(addr)["labels"], ["receiving", "my hardware wallet"])
# A key label equal to the address label is not repeated.
wallet.setlabel(addr, "my hardware wallet")
assert_equal(wallet.getaddressinfo(addr)["labels"], ["my hardware wallet"])
wallet.setlabel(addr, "receiving")
2567 | +std::optional<std::string> CWallet::GetKeyLabelForDest(const CTxDestination& dest) const 2568 | +{ 2569 | + const CScript script_pub_key = GetScriptForDestination(dest); 2570 | + const std::set<ScriptPubKeyMan*>& spk_mans = GetScriptPubKeyMans(script_pub_key); 2571 | + if (spk_mans.empty()) return std::nullopt; 2572 | + ScriptPubKeyMan* spk_man = *spk_mans.begin();
Could this avoid picking a label by set order? Two descriptors declaring different origins over the same scripts both import fine, and the winner flipped between runs that imported in the same order. The ambiguity predates the PR for hdmasterfingerprint, but that is a hex string and a label is a name someone chose.
The RPCs call CWallet directly, so the four new interfaces::Wallet methods end up with no caller and no test. The mining RPCs go through interfaces::Mining. Worth the same here?
No, for the wallet we decided in the past to not mix the RPC and the interfaces. See previous discussions in #34861 (comment) and #35436 (comment) (+ responses below).