walletdb: hash pubkey/privkey in one shot to avoid leaking secret data #34759

1. 
   theStack commented at 5:05 PM on March 6, 2026: contributor

   In several places in the wallet DB module, byte strings containing serialized public keys and secret keys are created in order to be hashed. To avoid sensitive data lingering in memory (and potentially leaking), don't store the preimage, but hash both public key and secret key in one shot, using the overloaded Hash function: https://github.com/bitcoin/bitcoin/blob/d198635fa2d48b7618789aedf112783935015d77/src/hash.h#L82-L88

   See e.g. #31166 and #31774 for similarly themed PRs (Note that in #31166 we used the explicit memory_cleanse approach though, as changing the allocator was not possible.)

2. 
   DrahtBot commented at 5:05 PM on March 6, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy, rkrux, theuni, davidgumberg
   Concept ACK	achow101, Bortlesboat
   Approach ACK	w0xlt

   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.

   <!--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::KEY, vchPubKey), std::make_pair(vchPrivKey, keypair_hash), false)] in src/wallet/walletdb.cpp
   • [WriteIC(std::make_pair(DBKeys::WALLETDESCRIPTORKEY, std::make_pair(desc_id, pubkey)), std::make_pair(privkey, keypair_hash), false)] in src/wallet/walletdb.cpp

   ^{2026-03-08 16:59:39}

3. 
   achow101 commented at 9:31 PM on March 6, 2026: member

   Concept ACK

4. 
   w0xlt commented at 10:07 PM on March 6, 2026: contributor

   Approach ACK

5. 
   theuni commented at 10:10 PM on March 6, 2026: member

   I think these can just be:

   auto hash = Hash(vchPubKey, vchPrivKey);
   

   ?

   That uses spans and does away with the copying altogether, so no need for the secure allocators.

6. 
   Bortlesboat commented at 10:51 PM on March 7, 2026: none

   Code review

   Concept ACK on clearing secret data from hash preimage buffers.

   I agree with theuni that the two-argument Hash() overload in src/hash.h is the better approach here. Both CPubKey (has data()/size()) and CPrivKey (which is std::vector<unsigned char, secure_allocator<unsigned char>>) are span-compatible via MakeUCharSpan, so all four sites can be reduced to:

   const auto keypair_hash = Hash(vchPubKey, vchPrivKey);
   

   This avoids copying the private key bytes into a temporary buffer entirely — the hasher reads directly from the source objects via spans. No temporary buffer means no need for secure_allocator on the intermediate vector, and the HashPubKeyPrivKey helper becomes unnecessary. The result is both more secure (no copy of secret data exists to cleanse) and simpler.

   Will re-review once updated.

7. walletdb: hash pubkey/privkey in one shot to avoid leaking secret data

   Avoid storing the privkey in a vector, which could linger in memory
   and potentially leak sensitive data. An alternative approach is to
   use `secure_allocator` for the `std::vector` instances, but this
   commit has the advantage of also deduplicating code at the same shot.
   
   Thanks to @theuni for suggesting this.

   501a3dd4ad
8. theStack force-pushed on Mar 8, 2026
9. theStack renamed this:
   walletdb: clear out hash preimage vectors containing secret data
   walletdb: hash pubkey/privkey in one shot to avoid leaking secret data
   on Mar 8, 2026
10. 
    theStack commented at 5:06 PM on March 8, 2026: contributor

    @theuni:

    I think these can just be:

    auto hash = Hash(vchPubKey, vchPrivKey);
    

    This is a neat solution, killing two birds with one stone (security fix and reducing LOC). Applied, and updated the PR title / description accordingly.

    One small drawback is that it needs slightly more review effort than the previous fix solution, due to being more implicit, but I think it's worth it. The previous solution can still be found here, in case anyone is interested: https://github.com/theStack/bitcoin/tree/202603-walletdb-clear_out_secret_data-previous (the first commit could be used as trivial backport, in the unlikely case that the one in this PR one causes problems).

11. 
    furszy commented at 5:21 PM on March 8, 2026: member

    ACK 501a3dd4ad4a545a05663a78cec61575966045c7

12. DrahtBot requested review from Bortlesboat on Mar 8, 2026
13. DrahtBot requested review from w0xlt on Mar 8, 2026
14. DrahtBot requested review from achow101 on Mar 8, 2026
15. 
    rkrux commented at 2:34 PM on March 9, 2026: contributor

    ACK 501a3dd

    Nit: can remove the strikethrough portions in the PR description, the old solution is gone now - specially the last paragraph.

16. theuni approved
17. 
    theuni commented at 4:34 PM on March 9, 2026: member

    ACK 501a3dd4ad4a545a05663a78cec61575966045c7

18. in src/wallet/walletdb.cpp:122 in 501a3dd4ad

     122 | -    vchKey.insert(vchKey.end(), vchPubKey.begin(), vchPubKey.end());
     123 | -    vchKey.insert(vchKey.end(), vchPrivKey.begin(), vchPrivKey.end());
     124 | +    const auto keypair_hash = Hash(vchPubKey, vchPrivKey);
     125 |  
     126 | -    return WriteIC(std::make_pair(DBKeys::KEY, vchPubKey), std::make_pair(vchPrivKey, Hash(vchKey)), false);
     127 | +    return WriteIC(std::make_pair(DBKeys::KEY, vchPubKey), std::make_pair(vchPrivKey, keypair_hash), false);
    

    ---

    ---

    

    davidgumberg commented at 10:52 PM on March 9, 2026:

    Just a note: this function is for legacy wallet key entries and is only used currently in benchmark code

19. in src/wallet/walletdb.cpp:220 in 501a3dd4ad

     216 | @@ -220,12 +217,9 @@ bool WalletBatch::EraseActiveScriptPubKeyMan(uint8_t type, bool internal)
     217 |  bool WalletBatch::WriteDescriptorKey(const uint256& desc_id, const CPubKey& pubkey, const CPrivKey& privkey)
     218 |  {
     219 |      // hash pubkey/privkey to accelerate wallet load
     220 | -    std::vector<unsigned char> key;
     221 | -    key.reserve(pubkey.size() + privkey.size());
     222 | -    key.insert(key.end(), pubkey.begin(), pubkey.end());
     223 | -    key.insert(key.end(), privkey.begin(), privkey.end());
     224 | +    const auto keypair_hash = Hash(pubkey, privkey);
    

    ---

    ---

    

    davidgumberg commented at 10:54 PM on March 9, 2026:

    Just a note: this function is used by wallets that are not encrypted on disk anyways, but still best practice to avoid allocating a copy of private key data.

20. 
    davidgumberg commented at 11:16 PM on March 9, 2026: contributor

    crACK https://github.com/bitcoin/bitcoin/commit/501a3dd4ad4a545a05663a78cec61575966045c7

    Although, AFAICT these functions are only used either in test code or in reading and writing records that are not encrypted on disk, avoiding the copy here is more readable, more performant, and more secure so seems like a pretty good idea!

21. fanquake merged this on Mar 10, 2026
22. fanquake closed this on Mar 10, 2026

    ---

23. theStack deleted the branch on Mar 10, 2026

Contributors

theStackDrahtBotachow101w0xlttheuniBortlesboatfurszyrkruxdavidgumberg

Review Requested
achow101w0xltBortlesboat