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 NACK | achow101, rkrux |
| Concept ACK | jeanpablojp, Eunovo |
| Approach ACK | vicjuma |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--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>
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?
Originally I answered saying single-key is the intended scope, but after thinking it deeply multi-sig makes sense too. Added a per-object key label to getaddressinfo so it takes into account multi-keys scripts.
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.
good catch, fixed :)
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?
IMO the interface consumer is the one who should take care of disable empty labels, or make an empty label call DelKeyLabel.
At max it could return false if label is an empty string.
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?
Fixed
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")
added thx!
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.
resolving as that function does not exist anymore
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).
0 | @@ -0,0 +1,9 @@ 1 | +RPC 2 | +--- 3 | +- A new RPC, `setkeylabel` allows to set a label per master key fingerprint. This is usefull for example to identify participants of a multi signature setup, tag hardware wallets, etc. If an empty label is provided, the previous label is removed. 4 | + 5 | +- A new RPC, `getkeylabe` allows to get the the label associated to a master key by its fingerprint.
There are a bunch of typos (see the drahtbot LLM comment #36126 (comment)), which break git grep when searching or at least create confusion or follow-ups. So those should be fixed, I'd say.
Proof of humanity 😉 Hehe fixed thanks :)
563 | @@ -554,6 +564,148 @@ RPCMethod getaddressinfo() 564 | } 565 | ret.pushKV("labels", std::move(labels)); 566 | 567 | + // Per-key labels keyed by master fingerprint for every key corresponding to this address.
Regarding multisigs and cosigners, the getaddressinfo returns all the keys for multiple key addresses in the key_labels returned json field
<img width="2690" height="1430" alt="Image" src="https://github.com/user-attachments/assets/1d0371c3-dfda-4b01-896e-aa7cc498099f" />
Tested the added RPCs, functioning as expected.
<img width="1960" height="534" alt="Image" src="https://github.com/user-attachments/assets/9becb7c0-8150-4532-bebc-aa46abd9a387" />
Approach ACK
509 | + * across all descriptor types, which makes it a natural unit for labelling 510 | + * e.g. a hardware wallet or a multisig participant. 511 | + * These labels augment, do not replace, the per-address labels in m_address_book 512 | + * and never affect the change/receive heuristic in CAddressBookData::IsChange(). 513 | + */ 514 | + std::map<KeyFingerprint, std::string> m_key_labels GUARDED_BY(cs_wallet);
https://github.com/bitcoin/bitcoin/pull/36126/commits/5af8c85bd05d69c8ff61e1c3ed0d2e2a9d4a3072:
I think m_key_labels should not be public. The CWallet class is responsible for keeping it in sync with the DB; making it public leaves it open to modification.
done
503 | @@ -504,6 +504,15 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati 504 | std::map<CTxDestination, CAddressBookData> m_address_book GUARDED_BY(cs_wallet); 505 | const CAddressBookData* FindAddressBookEntry(const CTxDestination&, bool allow_change = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); 506 | 507 | + /** Per-key labels keyed by the key's master fingerprint. 508 | + * A single fingerprint covers every key/address derived from the same root
https://github.com/bitcoin/bitcoin/pull/36126/commits/5af8c85bd05d69c8ff61e1c3ed0d2e2a9d4a3072:
why not "same master key" instead of "same root"?
done
603 | + 604 | + if (label.empty()) { 605 | + if (!pwallet->DelKeyLabel(fingerprint)) { 606 | + throw JSONRPCError(RPC_WALLET_ERROR, "Error: failed to clear per-key label"); 607 | + } 608 | + } else if (!pwallet->SetKeyLabel(fingerprint, label)) {
https://github.com/bitcoin/bitcoin/pull/36126/commits/e2c160236b9bd4c76a0ff624d7303b889c78d312:
This RPC silently overwrites any existing label that the user might have set during a previous multisig setup.
Yes, and that is intentional. setlabel works the same way for addresses. I would say that this is a UX concern which should mostly matter for a GUI not an RPC interface.
Concept ACK https://github.com/bitcoin/bitcoin/pull/36126/commits/8b47c46832341b85e505d27548c99474b385f392:
Looks good; left some comments. I also think the new per-key label should be added to the output of gethdkeys.
Force pushed to address some nits and also re:
I also think the new per-key label should be added to the output of
gethdkeys
Done :)
NACK
Unless there was some discussion that I missed, this is not at all what we discussed for key labeling.
The key labeling that was discussed is for the GUI only to show friendly names for keys during the multisig setup. Once the setup is complete, the labels would not be shown again, and hence do not need to be stored in the wallet.
Also, DO NOT USE FINGERPRINTS, THEY ARE COLLIDABLE.
Shouldn't the key labels be persisted so that at signing time the user can see what keys have and haven't signed? I don't see a good reason to not persist key labels when it seems useful for UX.
Concept ~0 (tending towards NACK)
Maybe there's some benefit in showing labels in the GUI but I don't see much benefit in providing RPCs for setting or viewing key labels (both separately and within the context of address(es) RPCs). An organic use case would be the user creating a multi-sig wallet (2-2 or 2-3 or 3-5) and not be importing descriptors separately in that wallet later, so all the addresses of the wallet would involve the keys used to create the wallet.
I think we should keep it very simple while implementing a guided multi-sig flow and not make the user go through many steps.
I don't see much benefit in providing RPCs for setting or viewing key labels (both separately and within the context of address(es) RPCs)
If we are finally adding some type of key labels, then there should be a way of adding, removing and viewing them using the RPC interface. Because if not, then we are adding something invisible for anyone not using the GUI.
I think we should keep it very simple while implementing a guided multi-sig flow and not make the user go through many steps.
I agree in that, but I don't think this makes the user go through many steps. The RPC is not an interface thought or designed for a user itself but for other applications to consume, normally users should stick to the GUI (if a good one is provided). That is why this PR also adds an interface for the GUI to consume, the GUI should take care of make all this process-easy.
I think the debate is more about the concept and if we actually want to store key labels, or not. Are they useful to identify multisig participants, see who is missing to sign, who already signed, etc (https://github.com/bitcoin/bitcoin/pull/36126#issuecomment-5592872052). Or if that is unnecessary information that the users can handle outside the wallet itself (https://github.com/bitcoin/bitcoin/pull/36126#issuecomment-5575897296).
A more immediate need is for the end-users to have a guided multi-sig flow in the GUI that's easy to follow. Otherwise there already are (though a bit convoluted) ways for the end-users to setup multi-sig in the wallet. Adding key labels may be beneficial for the end-users using the GUI, I don't see why we need to prioritise for applications (RPCs) right now.
Adding the key label is an additional step in a workflow that the GUI doesn't even have right now. That's why my preference to keep things straightforward and get a working multi-sig GUI flow first.
It is not mandatory to have multiple people participating in the multi-sig. It is not mandatory to have different signing devices as well in the multi-sig. An end user can just want to have multiple keys created by the core wallet to be participating in the multi-sig wallet and they might not want to have to label each key. And in this scenario, they might not care which keys have signed as long as there are enough signatures.
There's a cost to maintaining features over time, and I would rather us not to have to bear the cost of something that might not be used as much. And hence, my reluctance to add this functionality at this stage.
I don't see why we need to prioritise for applications (RPCs) right now.
This PR is not prioritizing RPCs, it is adding an interface for the GUI. At the same time it adds the RPC interface to also consume that functionality as I don't see the reason to have multiple PRs for it.
Adding the key label is an additional step in a workflow that the GUI doesn't even have right now. That's why my preference to keep things straightforward and get a working multi-sig GUI flow first.
Didn't understand that from your first message, I agree, this is not a must to allow multisig with the GUI.
As said, we should first discuss about if this should be added in the first place as seems that there isn't a consensus regarding the #35645 proposal. I though there was and that's why I opened this PR. IMHO it is a useful feature (it's something I've had before in other wallets and appreciated having, see p.e. Sparrow Wallet) and worth the, in my opinion, not to much burden to maintain.
But at the same time, the PR is not in a rush to get merged, so, for me, it is ok to stay open for a while while prioritizing other multisig PRs.
Will mark as draft for now until we decide if we want to include labels or not.
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.