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), ensure that these hash preimages are cleared out by using secure_allocator for the std::vector<unsigned char> instances. 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.)

   The first commit contains the minimal-diff fix. The second commit is a code deduplication refactor; it could be dropped and picked up in a separate PR if preferred (there may be some bikeshedding about naming and style).

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

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	furszy
   Concept ACK	achow101, Bortlesboat
   Approach ACK	w0xlt

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   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:

   0auto 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:

   0const 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:

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

Contributors
theStack DrahtBot achow101 w0xlt theuni Bortlesboat furszy

Review Requested
achow101 w0xlt Bortlesboat