wallet: Remove mapMasterKeys and enforce that only one encryption key can exist #36031

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:single-encryption-key changing 6 files +26 −40
  1. achow101 commented at 11:41 PM on August 19, 2026: member

    mapMasterKeys was introduced in #352 without a clear rationale. Likely it was to allow the user to have multiple passphrases by encrypting the wallet's encryption key with different passphrases. However, this functionality was never implemented, and current code enforces in a few places (inconsistently) that there is only one encryption key.

    By removing mapMasterKeys and replacing it with a single m_encryption_key, we can remove this confusion and simplify encryption key handling. No one should have a wallet that has more than one encryption key, and the ID of that key should be 1.

    The format of the database record remains unchanged. If a wallet somehow has an encryption key with an ID other than 1, the record will stay the same and the id is stored. Otherwise, new encryption keys always have an ID of 1.

    If a wallet has more than one encryption key, this becomes a corruption error because it should never happen outside of someone doing something weird with their wallet.


    I asked Matt for his rationale for adding mapMasterKeys and his response was

    I have no idea I barely knew how to code when I wrote that shit.

  2. DrahtBot added the label Wallet on Aug 19, 2026
  3. DrahtBot commented at 11:42 PM on August 19, 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/36031.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    Concept ACK jeanpablojp, theStack, rkrux, w0xlt, polespinasa
    Approach ACK vicjuma
    Stale ACK davidgumberg

    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:

    • #36397 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36397.svg"></sub> (scripted-diff: [wallet] Remove trailing newlines from one-line log messages by maflcko)
    • #30343 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/30343.svg"></sub> (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)

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

    LLM Linter (✨ experimental)

    Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • WriteIC(std::make_pair(DBKeys::MASTER_KEY, kMasterKey.m_id), kMasterKey, true) in src/wallet/walletdb.cpp

    <sup>2026-09-22 20:23:21</sup>

  4. DrahtBot added the label CI failed on Aug 20, 2026
  5. DrahtBot commented at 12:59 AM on August 20, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/32314344744/job/96263457089</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU reported an include-fixing issue (“Failure generated from IWYU”) in src/bench/wallet_encrypt.cpp, causing the CI test script to exit with code 1.</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. jeanpablojp commented at 6:45 PM on August 22, 2026: contributor

    Concept ACK, keeping a single key makes sense. One thing inline.

  7. in src/wallet/walletdb.cpp:410 in dc1d640c5d outdated
     409 | -        if(pwallet->mapMasterKeys.contains(nID))
     410 | -        {
     411 | -            strErr = strprintf("Error reading wallet database: duplicate CMasterKey id %u", nID);
     412 | +        kMasterKey.m_id = nID;
     413 | +
     414 | +        if (pwallet->m_encryption_key.has_value()) {
    


    jeanpablojp commented at 6:45 PM on August 22, 2026:

    Tried this out. With v0.19.1, I created a wallet with blank=true and ran encryptwallet, unloadwallet, loadwallet, and encryptwallet again, ending up with two mkey records. The second encryptwallet succeeded because IsCrypted() back then read an in-memory flag that loading an mkey never set. v28.2 opens that wallet and leaves both mkey records in place. On master, migratewallet converts it to a descriptor wallet but carries both records over. On this branch, migratewallet rejects the legacy wallet and loadwallet rejects the already migrated descriptor wallet with Wallet corrupted. Neither failure writes to the file. I don't know how common these wallets are.


    achow101 commented at 11:00 PM on August 24, 2026:

    That's interesting.

    I think this scenario is pretty unlikely. It requires you to create a blank wallet, encrypt it, and then unload it, then reload, then encrypt again. The expected usage of blank wallets was that you would immediately put stuff into that wallet and it would no longer become blank. There's really no reason to have a blank wallet that you would reload. This was a also a fairly niche feature when introduced (and kinda still is) and it looks like this behavior was present for 0.18 and 0.19, so I expect that the number of affected users is approximately 0.

    I should also note that once the wallet becomes not blank, even with the multiple encryption keys, only one of those keys would actually be used and be the correct one for that wallet. If we had some way to figure out which key that was, this change would still mostly hold.

    But I don't think that this is worth addressing as I expect no one to actually be affected by it. If it is problem, I have a half-baked solution that should work, but I feel like it's a bit complicated and premature to be adding that in when we don't know if anyone is affected by this.

  8. theStack commented at 4:30 PM on August 24, 2026: contributor

    Concept ACK

  9. in src/wallet/crypter.h:45 in dc1d640c5d outdated
      40 | @@ -41,6 +41,8 @@ class CMasterKey
      41 |      unsigned int nDeriveIterations;
      42 |      //! Use this for more parameters to key derivation (currently unused)
      43 |      std::vector<unsigned char> vchOtherDerivationParameters;
      44 | +    //! Id of this CMasterKey. Only used for writing the key of the database record. Retained for backwards compatibility, otherwise unused.
      45 | +    uint32_t m_id{1};
    


    vicjuma commented at 9:08 PM on August 24, 2026:

    Clearly backward compatible as its value is independent of the encryption operations

    Tweak

    src/wallet/crypter.h:45  uint32_t m_id{10000};
    

    Results

    <img width="2708" height="1520" alt="Image" src="https://github.com/user-attachments/assets/4a642ee9-75fc-47af-bdbe-ccee66fcc4eb" />

  10. in src/wallet/wallet.cpp:862 in dc1d640c5d outdated
     858 | @@ -865,14 +859,14 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
     859 |  
     860 |      {
     861 |          LOCK2(m_relock_mutex, cs_wallet);
     862 | -        mapMasterKeys[++nMasterKeyMaxID] = master_key;
     863 | +        m_encryption_key = master_key;
    


    vicjuma commented at 9:23 PM on August 24, 2026:

    Confirmed the assertion via a functional test

    def test_nonstandard_master_key_id(self, passphrase, do_wallet_tool):
            self.log.info("Test that a master key with a non-standard ID remains unchanged")
            current_node = self.nodes[0]
            wallet_name = "pr36031_non_standard_id"
            current_node.createwallet(wallet_name=wallet_name)
            pr36031_non_standard_id = current_node.get_wallet_rpc(wallet_name)
            pr36031_non_standard_id.encryptwallet(passphrase)
            pr36031_non_standard_id.unloadwallet()
            dumpfile_path = current_node.datadir_path / f"{pr36031_non_standard_id}.dump"
            do_wallet_tool(f"-wallet={wallet_name}", f"-dumpfile={dumpfile_path}", "dump")
            mkey_prefix = "046d6b6579"
            ''' Patch the mkey record's ID from 1 (01000000, 4-byte LE) to 42 (2a000000)'''
            with open(dumpfile_path, "r") as f:
                dump_content = f.readlines()
            dump_content = dump_content[:-1]
            mkey_idx = next(i for i, l in enumerate(dump_content) if l.startswith(F"{mkey_prefix}"))
            key_hex, value_hex = dump_content[mkey_idx].strip().split(",")
    
            ''''asserts that the value is not 1 as changed in `src/wallet/crypter.h:45  uint32_t m_id{10000};` '''
            assert_not_equal(key_hex[-8:], "01000000")
            dump_content[mkey_idx] = f"{key_hex[:-8]}2a000000,{value_hex}\n"
            with open(dumpfile_path, "w") as f:
                contents = "".join(dump_content)
                f.write(contents)
                checksum = hash256(contents.encode())
                f.write(f"checksum,{checksum.hex()}\n")
            wallet_name_new = "pr36031_non_standard_id_new"
            do_wallet_tool(f"-wallet={wallet_name_new}", f"-dumpfile={dumpfile_path}", "createfromdump")
            current_node.loadwallet(wallet_name_new)
            pr36031_non_standard_id_new = current_node.get_wallet_rpc(wallet_name_new)
            receive = pr36031_non_standard_id_new.getnewaddress()
            current_node.generatetoaddress(101, receive, called_by_framework=True)
    
            with WalletUnlock(pr36031_non_standard_id_new, passphrase):
                destination = pr36031_non_standard_id_new.getnewaddress()
    
                utxo = pr36031_non_standard_id_new.listunspent()[0]
    
                raw = pr36031_non_standard_id_new.createrawtransaction(
                    [{"txid": utxo["txid"], "vout": utxo["vout"]}],
                    {destination: 1.0},
                )
    
                signed = pr36031_non_standard_id_new.signrawtransactionwithwallet(raw)
    
                assert_equal(signed["complete"], True)
            pr36031_non_standard_id_new.unloadwallet()
    
            redump_path = current_node.datadir_path / "pr36031_non_standard_id_new.dump"
            do_wallet_tool(f"-wallet={wallet_name_new}", f"-dumpfile={redump_path}", "dump")
            with open(redump_path, "r") as f:
                mkey_line = next(l for l in f if l.startswith(f"{mkey_prefix}"))
    
            ''''asserts that the above operations were actually done using the tweaked mkey '''
            assert mkey_line.split(",")[0].endswith("2a000000")
    

    The test passes with the above change output

    <img width="2708" height="1520" alt="Image" src="https://github.com/user-attachments/assets/7dbecfd9-e8b8-440c-9a41-4f5f9391dddc" />

  11. vicjuma commented at 9:24 PM on August 24, 2026: contributor

    Approach ACK

  12. achow101 force-pushed on Aug 24, 2026
  13. DrahtBot removed the label CI failed on Aug 25, 2026
  14. rkrux commented at 9:07 AM on August 26, 2026: contributor

    Strong Concept ACK 94af5c903604486dbcb8bc1fb4c7cb0ce9044cb9

    I don't see a good reason to have multiple keys or passphrases to encrypt the wallet going forward. And the corresponding code cleanup is necessary for long term wallet maintainability and usability.

  15. w0xlt commented at 6:23 PM on September 8, 2026: contributor

    Concept ACK

  16. in src/wallet/walletdb.cpp:1215 in 94af5c9036 outdated
    1211 | @@ -1214,13 +1212,11 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
    1212 |      // Removing the mkey records is only safe if there are no *ckey records.
    1213 |      if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) {
    1214 |          pwallet->WalletLogPrintf("Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys.\n");
    1215 | -        for (const auto& [id, _] : pwallet->mapMasterKeys) {
    1216 | -            if (!EraseMasterKey(id)) {
    1217 | -                pwallet->WalletLogPrintf("Error: Unable to remove extraneous encryption key '%u'. Wallet corrupt.\n", id);
    1218 | -                return DBErrors::CORRUPT;
    1219 | -            }
    1220 | +        if (!EraseMasterKey(pwallet->m_encryption_key->m_id)) {
    


    davidgumberg commented at 11:51 PM on September 8, 2026:

    High-level out-of-scope remark:

    It would be wise to just rename these records instead of deleting them.

  17. davidgumberg commented at 11:52 PM on September 8, 2026: contributor

    crACK https://github.com/bitcoin/bitcoin/commit/94af5c903604486dbcb8bc1fb4c7cb0ce9044cb9

    From a high-level this change is safe:

    the first two commits are basically ~refactors, but there is a slight behavior change in eea102293c2494c110efe46b974f6623b93bbfdc which is that multiple encryption keys in a wallet file will result in failing to load after only this commit.

    In the main commit (94af5c903604486dbcb8bc1fb4c7cb0ce9044cb9) it seems to me that the worst that can happen is that some wallet edge case has not been taken into account and will fail to load on startup, which is fine.

  18. DrahtBot requested review from rkrux on Sep 8, 2026
  19. DrahtBot requested review from jeanpablojp on Sep 8, 2026
  20. DrahtBot requested review from theStack on Sep 8, 2026
  21. DrahtBot requested review from vicjuma on Sep 8, 2026
  22. crypter: Store the CMasterKey ID in the CMasterKey itself f0773b18de
  23. walletdb: Make WriteMasterKey take only a CMasterKey d60681a53a
  24. wallet: Hold only one encryption key in memory
    mapMasterKeys was introduced in #352 without a clear rationale. Likely
    it was to allow the user to have multiple passphrases by encrypting the
    wallet's encryption key with different passphrases. However, this
    functionality was never implemented, and current code enforces in a few
    places that there is only one encryption key.
    
    By removing mapMasterKeys and replacing it with a single
    m_encryption_key, we can remove this confusion and simplify encryption
    key handling. No one should have a wallet that has more than one
    encrypion key.
    8e3e423921
  25. achow101 force-pushed on Sep 22, 2026
  26. in src/wallet/walletdb.h:243 in d60681a53a
     238 | @@ -239,7 +239,7 @@ class WalletBatch
     239 |      bool WriteKeyMetadata(const CKeyMetadata& meta, const CPubKey& pubkey, bool overwrite);
     240 |      bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata &keyMeta);
     241 |      bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector<unsigned char>& vchCryptedSecret, const CKeyMetadata &keyMeta);
     242 | -    bool WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey);
     243 | +    bool WriteMasterKey(const CMasterKey& kMasterKey);
     244 |      bool EraseMasterKey(unsigned int id);
    


    polespinasa commented at 4:18 PM on October 1, 2026:

    in d60681a53ab3fd4fd2e03b6bee0d85bbb35c3f1c walletdb: Make WriteMasterKey take only a CMasterKey

    Can't this function also take the kMasterKey so it is consistent with WriteMasterKey??

  27. in src/wallet/wallet.cpp:615 in 8e3e423921
     611 | @@ -612,11 +612,7 @@ util::Expected<void, WalletError> CWallet::Unlock(const SecureString& strWalletP
     612 |  
     613 |      {
     614 |          LOCK(cs_wallet);
     615 | -        for (const auto& [_, master_key] : mapMasterKeys)
     616 | -        {
     617 | -            if (!DecryptMasterKey(strWalletPassphrase, master_key, plain_master_key)) {
     618 | -                continue; // try another master key
     619 | -            }
     620 | +        if (DecryptMasterKey(strWalletPassphrase, *m_encryption_key, plain_master_key)) {
    


    polespinasa commented at 4:25 PM on October 1, 2026:

    in 8e3e4239213798602ef0ea5551fa2a3f10623789 wallet: Hold only one encryption key in memory

    Isn't this undefined behavior if m_encryption_key is empty? I've checked and I think all callers have some kind of guard. I think an assert(m_encryption_key.hasvalue()) would make the function safer. Or as the function is Expected maybe return an error if it does not have value.

  28. in src/bench/wallet_encrypt.cpp:80 in 8e3e423921
      78 | @@ -78,9 +79,7 @@ static void WalletEncrypt(benchmark::Bench& bench, unsigned int key_count)
      79 |          .run([&] {
      80 |              wallet->EncryptWallet(secure_pass);
    


    polespinasa commented at 4:29 PM on October 1, 2026:

    8e3e4239213798602ef0ea5551fa2a3f10623789 wallet: Hold only one encryption key in memory

    Not introduced by this PR but, this return is silently ignored. Maybe worth ensure it returns true?

  29. polespinasa commented at 4:31 PM on October 1, 2026: member

    concept ACK

    from 8e3e4239213798602ef0ea5551fa2a3f10623789 wallet: Hold only one encryption key in memory:

    No one should have a wallet that has more than one encrypion key.

    What if someone does? IIUC think this PR makes the wallet unloadable.


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-10-15 17:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me