wallet, rpc: Implements set key label functionality #36126

pull polespinasa wants to merge 11 commits into bitcoin:master from polespinasa:2026-08-28-setkeylabel changing 11 files +452 −3
  1. polespinasa commented at 9:23 AM on August 31, 2026: member

    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.

  2. DrahtBot commented at 9:23 AM on August 31, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    Concept NACK achow101
    Concept ACK jeanpablojp, Eunovo
    Approach ACK vicjuma

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36070 (wallet: Add deriveHDKey interface by PraneethGunas)
    • #35998 (wallet: Handle or explicitly ignore WalletBatch write failures by achow101)
    • #35436 (wallet: Add addHDkey interface by pseudoramdom)
    • #32895 (wallet: Prepare for future upgrades by recording versions of last client to open and decrypt 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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. polespinasa force-pushed on Aug 31, 2026
  4. DrahtBot added the label CI failed on Aug 31, 2026
  5. DrahtBot commented at 9:35 AM on August 31, 2026: contributor

    <!--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>

  6. polespinasa force-pushed on Aug 31, 2026
  7. walletdb: add keylabel record to wallet database
    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.
    bf3a2eb4b4
  8. polespinasa force-pushed on Aug 31, 2026
  9. polespinasa force-pushed on Aug 31, 2026
  10. polespinasa force-pushed on Aug 31, 2026
  11. DrahtBot removed the label CI failed on Aug 31, 2026
  12. in src/wallet/rpc/addresses.cpp:567 in 0009793fb9 outdated
     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)
    


    vicjuma commented at 2:02 PM on August 31, 2026:

    The labels will really help in identifying cosigners, especially. Just 2 questions regarding setting them (setkeylabel)

    1. Should it allow pre-labelling cosigners' fingerprints (not yet known to the wallet)? - currently it does does
    2. Are two different fingerprints allowed to have the same label? - currently it does

    polespinasa commented at 5:49 PM on August 31, 2026:

    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.

  13. jeanpablojp commented at 4:10 PM on August 31, 2026: contributor

    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.

  14. in src/wallet/rpc/addresses.cpp:585 in 0009793fb9 outdated
     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"
    


    jeanpablojp commented at 4:10 PM on August 31, 2026:

    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?


    polespinasa commented at 8:27 PM on August 31, 2026:

    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.

  15. in src/wallet/wallet.cpp:2567 in 0009793fb9
    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
    


    jeanpablojp commented at 4:10 PM on August 31, 2026:

    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.


    polespinasa commented at 6:17 PM on August 31, 2026:

    good catch, fixed :)

  16. in src/wallet/interfaces.cpp:489 in 0009793fb9 outdated
     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 {
    


    jeanpablojp commented at 4:10 PM on August 31, 2026:

    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?


    polespinasa commented at 6:23 PM on August 31, 2026:

    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.

  17. in src/wallet/rpc/addresses.cpp:555 in 0009793fb9
     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)};
    


    jeanpablojp commented at 4:10 PM on August 31, 2026:

    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?


    polespinasa commented at 8:27 PM on August 31, 2026:

    Fixed

  18. in test/functional/wallet_keylabels.py:49 in 0009793fb9 outdated
      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"])
    


    jeanpablojp commented at 4:10 PM on August 31, 2026:

    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")
    

    polespinasa commented at 6:30 PM on August 31, 2026:

    added thx!

  19. in src/wallet/wallet.cpp:2572 in 0009793fb9
    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();
    


    jeanpablojp commented at 4:10 PM on August 31, 2026:

    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.


    polespinasa commented at 6:39 PM on August 31, 2026:

    resolving as that function does not exist anymore

  20. polespinasa commented at 5:53 PM on August 31, 2026: member

    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).

  21. polespinasa force-pushed on Aug 31, 2026
  22. polespinasa force-pushed on Aug 31, 2026
  23. DrahtBot added the label CI failed on Aug 31, 2026
  24. DrahtBot removed the label CI failed on Aug 31, 2026
  25. polespinasa force-pushed on Aug 31, 2026
  26. polespinasa force-pushed on Aug 31, 2026
  27. polespinasa force-pushed on Aug 31, 2026
  28. DrahtBot added the label CI failed on Aug 31, 2026
  29. polespinasa force-pushed on Aug 31, 2026
  30. polespinasa force-pushed on Aug 31, 2026
  31. DrahtBot removed the label CI failed on Aug 31, 2026
  32. in doc/release-notes-36126.md:5 in 64e1fc6d9b
       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.
    


    maflcko commented at 5:39 AM on September 1, 2026:

    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.


    polespinasa commented at 7:28 AM on September 1, 2026:

    Proof of humanity 😉 Hehe fixed thanks :)

  33. polespinasa force-pushed on Sep 1, 2026
  34. polespinasa force-pushed on Sep 1, 2026
  35. DrahtBot added the label CI failed on Sep 1, 2026
  36. DrahtBot removed the label CI failed on Sep 1, 2026
  37. in src/wallet/rpc/addresses.cpp:567 in 8b47c46832 outdated
     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.
    


    vicjuma commented at 2:37 PM on September 1, 2026:

    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" />

  38. in src/wallet/rpc/addresses.cpp:1 in 8b47c46832 outdated


    vicjuma commented at 2:41 PM on September 1, 2026:

    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" />

  39. vicjuma commented at 2:41 PM on September 1, 2026: contributor

    Approach ACK

  40. in src/wallet/wallet.h:514 in 5af8c85bd0 outdated
     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);
    


    Eunovo commented at 9:08 AM on September 2, 2026:

    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.


    polespinasa commented at 10:37 AM on September 4, 2026:

    done

  41. in src/wallet/wallet.h:508 in 5af8c85bd0 outdated
     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
    


    Eunovo commented at 9:08 AM on September 2, 2026:

    polespinasa commented at 10:39 AM on September 4, 2026:

    done

  42. in src/wallet/rpc/addresses.cpp:608 in e2c160236b outdated
     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)) {
    


    Eunovo commented at 10:01 AM on September 2, 2026:

    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.


    polespinasa commented at 10:47 AM on September 4, 2026:

    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.

  43. Eunovo commented at 10:11 AM on September 2, 2026: contributor

    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.

  44. polespinasa force-pushed on Sep 4, 2026
  45. wallet, walletdb: add per-key label state and load keylabel records
    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.
    21c5f0c0e6
  46. rpc, wallet: add setkeylabel RPC
    Also adds DecodeKeyFingerprnt helper function that will be used
    for multiple RPC introduced in future commits.
    175ec80346
  47. wallet: Add a setkeylabel() and delkeylabel() interface for the wallet 7c27585175
  48. rpc, wallet: add getkeylabel RPC 5ce06f6e19
  49. wallet Add getKeyLabel() interface for the wallet fac6dba5bc
  50. rpc, wallet: add listkeylabels RPC 6f658a26d2
  51. wallet: Add getKeyLabels() interface for the wallet 4b1b8d4946
  52. rpc, wallet: add key labels to getaddressinfo RPC 09a314d8be
  53. test: add functional test for wallet key labels 9b196faac2
  54. rpc, wallet: gethdkeys report keys fingerprint and labels if any 2f1a3d69b2
  55. polespinasa force-pushed on Sep 4, 2026
  56. DrahtBot added the label CI failed on Sep 4, 2026
  57. polespinasa commented at 11:12 AM on September 4, 2026: member

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

  58. DrahtBot removed the label CI failed on Sep 4, 2026
  59. achow101 commented at 9:43 PM on September 7, 2026: member

    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.

  60. davidgumberg commented at 10:41 PM on September 8, 2026: contributor

    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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-09-09 07:56 UTC