crypto: Use secure_allocator for AES256_ctx #31774

pull davidgumberg wants to merge 6 commits into bitcoin:master from davidgumberg:1-31-25-aes-secure-alloc changing 8 files +123 −21
  1. davidgumberg commented at 8:10 pm on January 31, 2025: contributor

    Fixes #31744

    Reuse secure_allocator for AES256_ctx in the aes 256 encrypters and decrypters and the iv of AES256CBC encrypters and decrypters. These classes are relevant to CCrypter, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (AES256_ctx & iv) they might be able to decrypt a user’s wallet.

    Presently the secure_allocator tries to protect sensitive data with mlock() on POSIX systems and VirtualLock() on Windows to prevent memory being paged to disk, and by zero’ing out memory contents on deallocation with memory_cleanse() which is similar to OPENSSL_cleanse() by scaring compilers away from optimizing memset calls on non-Windows systems, and using SecureZeroMemory() on Windows.

  2. DrahtBot commented at 8:10 pm on January 31, 2025: contributor

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

    Code Coverage & Benchmarks

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, theStack, achow101
    Stale ACK sipa

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28690 (build: Introduce internal kernel library by sedited)

    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.

  3. DrahtBot added the label Utils/log/libs on Jan 31, 2025
  4. davidgumberg force-pushed on Jan 31, 2025
  5. DrahtBot added the label CI failed on Jan 31, 2025
  6. DrahtBot commented at 9:32 pm on January 31, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36501336640

    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.

  7. DrahtBot removed the label CI failed on Jan 31, 2025
  8. theStack commented at 3:51 pm on February 4, 2025: contributor

    Concept ACK on clearing out the ctx/iv members

    I’m wondering if a minimum-diff fix which simply replaces the memset calls in the dtors with memory_cleanse would be largely sufficient here? In #31166 (comment) one argument for not needing secure allocators was the short-lived nature of the secrets. Looking at the only usage in the wallet, this would imho apply here too (en/decrypting might take a while for larger inputs, but I still wouldn’t classify that as long-lived):

    https://github.com/bitcoin/bitcoin/blob/94ca99ac51dddbee79d6409ebcc43b1119b0aca9/src/wallet/crypter.cpp#L85-L91 https://github.com/bitcoin/bitcoin/blob/94ca99ac51dddbee79d6409ebcc43b1119b0aca9/src/wallet/crypter.cpp#L102-L108

    The reasoning might be a bit loose as there might be other uses of these classes in the future where the situation is different. Curious about other opinions.

  9. sipa commented at 4:35 pm on February 4, 2025: member

    I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

    It may be better to:

    • Somehow make the AES256CBC classes members of CCrypter, surviving an individual encryption/decryption (e.g. by adding a reset function that can get called for each encryption/decryption, resetting the CBC state, but letting the key material survive).
    • Follow @theStack’s suggestion of not locking the memory, but just securely wiping it after each operation.
  10. davidgumberg force-pushed on Feb 8, 2025
  11. davidgumberg force-pushed on Feb 8, 2025
  12. DrahtBot commented at 0:50 am on February 8, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36882999841

    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.

  13. DrahtBot added the label CI failed on Feb 8, 2025
  14. davidgumberg force-pushed on Feb 8, 2025
  15. davidgumberg force-pushed on Feb 8, 2025
  16. davidgumberg commented at 1:49 am on February 8, 2025: contributor

    I’m wondering if a minimum-diff fix which simply replaces the memset calls in the dtors with memory_cleanse would be largely sufficient here? In #31166 (comment) one argument for not needing secure allocators was the short-lived nature of the secrets.

    It seems right to me that this structure is always short lived, but I also felt in #31166 secure_allocator should have been used over just memory_cleanse(). I feel that the alloc/dealloc strategy for secrets in memory should be reused and applied universally unless there is a good reason not to.

    I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

    I added a benchmark for WalletEncrypt() which I ran a few times and it appears that the performance impact is neglible on my machine (Ryzen 7900x, Fedora 40), if the benchmark I wrote is actually representative:

    wallet type branch EncryptWallet() (ns/key) benchmark overhead (ns/key) normalized value (total - overhead) flamegraph
    Descriptor https://github.com/bitcoin/bitcoin/pull/31774/commits/15d8500f99012422be495b8e85e4e25e6a4419d8 42,106.55 36,235.03 5,871.53 link
    Descriptor master 42,050.82 36,362.40 5,688.42 link
    Legacy https://github.com/bitcoin/bitcoin/pull/31774/commits/15d8500f99012422be495b8e85e4e25e6a4419d8 17,067.82 2,779.16 14,288.6 link
    Legacy master 16,812.63 2,757.26 14,055.57 link

    But if the short-lived nature of the key material makes just using memory_cleanse() a good-enough solution over having to reason about whether the benchmark is sufficiently correct, and whether the larger / more complicated diff required to reuse secure_allocator is worth the review effort / risk, I’m happy to change it.

  17. DrahtBot removed the label CI failed on Feb 8, 2025
  18. laanwj commented at 1:27 pm on March 27, 2025: member

    I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

    This may still be true, though FWIW the idea behind the LockedPool is that it reduces the amount of locking/unlocking operations by mapping and locking memory in blocks, not every time it’s requested.

    But agree that for short-lived key material, it’s less important, there is little to no chance of it ending up in swap anyway.

  19. DrahtBot added the label Needs rebase on Apr 30, 2025
  20. davidgumberg force-pushed on May 1, 2025
  21. DrahtBot removed the label Needs rebase on May 1, 2025
  22. DrahtBot added the label CI failed on May 1, 2025
  23. DrahtBot commented at 5:42 am on May 1, 2025: contributor

    🚧 At least one of the CI tasks failed. Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/41468173464 LLM reason (✨ experimental): The CI failure is caused by an assertion failure in wallet creation during the bench_sanity_check test.

    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.

  24. davidgumberg force-pushed on May 1, 2025
  25. davidgumberg commented at 5:44 am on May 1, 2025: contributor
    Rebased to address merge conflict, dropped legacy wallet encryption benchmark, and reduced number of keys since the benchmark was taking an unreasonably long time.
  26. DrahtBot removed the label CI failed on May 1, 2025
  27. in src/wallet/wallet.cpp:590 in 8342cdd1e4 outdated 
    588-    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
589-    master_key.nDeriveIterations = static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start));
590+    if (force_iterations.has_value()) {
591+        master_key.nDeriveIterations = force_iterations.value();
592+    }
593+    else {
    sipa commented at 12:52 pm on May 1, 2025:
    Coding style nit: } else {.
    davidgumberg commented at 1:16 am on May 2, 2025:
    Thanks, fixed.
  28. in src/bench/wallet_encrypt.cpp:61 in 6cbd528a53 outdated 
    56+        // Save a copy of the db before encrypting
57+        database = DuplicateMockDatabase(wallet->GetDatabase());
58+
59+        // Skip actually encrypting wallet on the overhead measuring run, so we
60+        // can subtract the overhead from the results.
61+        if(!measure_overhead) {
    sipa commented at 12:54 pm on May 1, 2025:
    Coding style nit: space after if.
    davidgumberg commented at 1:17 am on May 2, 2025:
    Thanks, fixed.
  29. sipa commented at 12:55 pm on May 1, 2025: member
    utACK 591764f6170d6f74d7eebb5fec1cbf5b912098a4, just coding style nits
  30. DrahtBot requested review from theStack on May 1, 2025
  31. davidgumberg force-pushed on May 2, 2025
  32. davidgumberg commented at 1:17 am on May 2, 2025: contributor
    Rebase to address style feedback.
  33. theStack approved
  34. theStack commented at 11:30 am on May 4, 2025: contributor
    Code-review ACK 3d7d2c37f7fe10e77d50c8b8fa4d6c74ad52a3c6
  35. DrahtBot requested review from sipa on May 4, 2025
  36. furszy commented at 3:09 pm on May 4, 2025: member
    I don’t see why the first commit is necessary. Couldn’t you just mock the time so that it’s fixed and the number of derivation rounds always stays at the default value?
  37. davidgumberg commented at 11:36 pm on May 5, 2025: contributor

    I don’t see why the first commit is necessary. Couldn’t you just mock the time so that it’s fixed and the number of derivation rounds always stays at the default value?

    Maybe I’m not thinking creatively enough, but I think because the timing takes place entirely inside of EncryptWallet()->EncryptMasterKey, from the wallet_encrypt.cpp benchmark, I could only mock the clock so that it’s static during the derivation “benchmark”1 runs, so the difference between the end time and the start time of a run will be 0 which will result in dividing by 0: https://github.com/bitcoin/bitcoin/blob/68ac9f116c0228a277f18f60ba2278b56356e6ac/src/wallet/wallet.cpp#L581-L589

    1. The ones that decide how many derivations to perform based on hardware speed. ↩︎

  38. furszy commented at 11:54 pm on May 5, 2025: member

    so the difference between the end time and the start time of a run will be 0 which will result in dividing by 0:

    Yes, it seems simpler and more scoped to address that inside the encryption function (which could arguably be considered a ‘fix’, since we shouldn’t be dividing by zero regardless of whether the clock time is messed up) than to add a test-only field to the wallet and another function arg just just for this.

  39. davidgumberg commented at 0:11 am on May 6, 2025: contributor

    since we shouldn’t be dividing by zero regardless of whether the clock time is messed up

    AFAIK, std::chrono::steady_clock, unlike the system clock, cannot fail to move forward, and this will never happen.

    than to add a test-only field to the wallet and another function arg just just for this.

    I agree that adding a test-only field and arg is very bad, and would like to avoid it, but it seems to me worse to imbue secret meaning into the benchmark by handling a time difference of 0 in this special way which appears on the surface to be error-handling, but is really a “side-channel” for test code to sneak an extra parameter into EncryptWallet, but I don’t feel strongly enough about which of those is worse, so I’ll implement your approach.

  40. davidgumberg force-pushed on May 6, 2025
  41. davidgumberg force-pushed on May 6, 2025
  42. DrahtBot added the label CI failed on May 6, 2025
  43. DrahtBot commented at 4:03 am on May 6, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/41695411207 LLM reason (✨ experimental): The CI failure is due to issues identified by lint-includes.py and the all_python_linters check.

    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.

  44. davidgumberg force-pushed on May 6, 2025
  45. davidgumberg commented at 4:26 am on May 6, 2025: contributor

    Refactored to address @furszy feedback to avoid adding a test-only parameter to CWallet, and added a somewhat opinionated refactor (https://github.com/bitcoin/bitcoin/pull/31774/commits/7176b26cde7cbaffdd92af9c25f85f8e5233e78a) of CWallet::EncryptMasterKey() since I was touching the iteration measuring stuff anyways.

    I don’t like the idea of using a for loop that iterates once with 0 and once with 1, but it’s shorter, I believe it is easier to understand how the weighted average calculation is working, and this version generalizes to any number of measurement runs.

  46. DrahtBot removed the label CI failed on May 6, 2025
  47. DrahtBot added the label CI failed on May 16, 2025
  48. DrahtBot removed the label CI failed on May 21, 2025
  49. achow101 requested review from achow101 on Oct 22, 2025
  50. sipa commented at 6:22 pm on November 3, 2025: member
    utACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd
  51. DrahtBot requested review from theStack on Nov 3, 2025
  52. theStack approved
  53. theStack commented at 3:00 pm on November 4, 2025: contributor
    Code-review ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd
  54. in src/wallet/wallet.cpp:592 in 7176b26cde outdated 
    594+        auto elapsed_time{SteadyClock::now() - start_time};
595 
596-    start = SteadyClock::now();
597-    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
598-    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
599+        // target_iterations : elapsed_iterations :: target_time : elapsed_time
    furszy commented at 1:36 pm on November 10, 2025:
    In 7176b26cde7cbaffdd92af9c25f85f8e5233e78a: nano nit: I’m not sure how helpful this comment is.
    achow101 commented at 4:46 pm on November 10, 2025:

    In 7176b26cde7cbaffdd92af9c25f85f8e5233e78a “refactor: Generalize derivation target calculation”

    This comment does not seem helpful, I’m not sure what information it is trying to convey.

    davidgumberg commented at 5:11 pm on November 10, 2025:

    https://en.wikipedia.org/wiki/Ratio#Notation_and_terminology (archive link)

    This comment explains why the assignment below is correct. This can be read as “target_iterations is to elapsed_iterations as target_time is to elapsed_time”, this is mostly equivalent to: $\frac{\text{targetiterations}}{\text{elapsediterations}} = \frac{\text{targettime}}{\text{elapsedtime}}$

    davidgumberg commented at 5:16 pm on November 10, 2025:

    I responded here: #31774 (review)

    I’ll mark this resolved and we can discuss there.

  55. in src/wallet/wallet.cpp:586 in 7176b26cde outdated 
    585+    constexpr MillisecondsDouble target_time{100};
586     CCrypter crypter;
587 
588-    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
589-    master_key.nDeriveIterations = static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start));
590+    // Get the weighted average of iterations we can do in 100ms over 2 runs.
    furszy commented at 1:38 pm on November 10, 2025:
    In https://github.com/bitcoin/bitcoin/commit/7176b26cde7cbaffdd92af9c25f85f8e5233e78a: Pedantic ultra-nano nit: We finish comments with a dot only if they span more than one line.
    davidgumberg commented at 5:20 pm on November 10, 2025:

    Really? I couldn’t find this in doc/ and I find a lot of counter-examples when doing:

    0git grep -n -C 2 "// .*\." '*.cpp' '*.h'
  56. in src/wallet/wallet.cpp:594 in 7176b26cde outdated 
    596-    start = SteadyClock::now();
597-    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
598-    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
599+        // target_iterations : elapsed_iterations :: target_time : elapsed_time
600+        unsigned int target_iterations = master_key.nDeriveIterations * target_time / elapsed_time;
601+        // Get the weighted average with previous runs.
    furszy commented at 1:47 pm on November 10, 2025:
    In https://github.com/bitcoin/bitcoin/commit/7176b26cde7cbaffdd92af9c25f85f8e5233e78a: nano nit: you are already mentioning this above the for-loop line.
    davidgumberg commented at 5:13 pm on November 10, 2025:
    Fair enough, I think this is adding a little more information than the comment above the for loop, but I’ll remove when/if retouching.
  57. furszy commented at 2:01 pm on November 10, 2025: member

    ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd

    No need to tackle the nano nits I left.

  58. in src/bench/wallet_encrypt.cpp:46 in 26442fec57 outdated 
    41+            mtx.vout.emplace_back(COIN, GetScriptForDestination(*Assert(wallet->GetNewDestination(type, ""))));
42+            mtx.vin.emplace_back();
43+            wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{});
44+            key_count++;
45+        }
46+    }
    achow101 commented at 4:47 pm on November 10, 2025:

    In 26442fec572da263765c576c080186c84cee1615 “bench: Add wallet encryption benchmark”

    This part of setup is unnecessary. Encryption does not encrypt destinations or transactions. Generating new destinations does not result in more things to be encrypted.

    If the idea was to have a bunch of keys to be encrypted for the benchmark, then several independent descriptors would need to be imported.

    davidgumberg commented at 0:13 am on December 10, 2025:
    Thanks for catching this, I’ve fixed this with importing multiple descriptors as you suggest in https://github.com/bitcoin/bitcoin/pull/31774/commits/8abee2b4bb1ac463a9ec200aa32b9c5276f0dcac
  59. in src/bench/wallet_encrypt.cpp:65 in 26442fec57 outdated 
    60+        database = DuplicateMockDatabase(wallet->GetDatabase());
61+
62+        // Skip actually encrypting wallet on the overhead measuring run, so we
63+        // can subtract the overhead from the full benchmark results to get
64+        // encryption performance.
65+        if (!measure_overhead) {
    achow101 commented at 4:54 pm on November 10, 2025:

    In 26442fec572da263765c576c080186c84cee1615 “bench: Add wallet encryption benchmark”

    We’ve generally not had “measure overhead” in the benchmarks. If this is because encryption is actually a very small amount of time, I would suggest generating significantly more keys that need to actually be encrypted.

    davidgumberg commented at 0:18 am on December 10, 2025:

    Unfortunately, even after generating more keys, there is still significant overhead, and this overhead is not fixed, it scales with the number of keys…

    Even though total - overhead isn’t something we’ve measured in any existing benchmarks, it’s probably a good idea for some benchmarks. It won’t make a difference for catching extreme regressions, but for smaller performance regressions / improvements, the setup overhead in each run adds noise that makes changes harder to measure. Ideally there would be some way to achieve this with nanobench, but for now this hasn’t been implemented and the maintainer of nanobench has suggested this approach: https://github.com/martinus/nanobench/issues/86#issuecomment-1423165976

  60. glozow commented at 3:53 pm on December 9, 2025: member
    IIRC we’re waiting on @davidgumberg to address the comments from @achow101 ’s reviews #31774 (review) here?
  61. davidgumberg force-pushed on Dec 9, 2025
  62. davidgumberg force-pushed on Dec 9, 2025
  63. DrahtBot added the label CI failed on Dec 9, 2025
  64. DrahtBot commented at 10:36 pm on December 9, 2025: contributor

    🚧 At least one of the CI tasks failed. Task Windows-cross to x86_64, msvcrt: https://github.com/bitcoin/bitcoin/actions/runs/20080403030/job/57605821707 LLM reason (✨ experimental): Linker failure from multiple definitions in lockedpool.cpp (duplicate symbols across modules).

    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.

  65. davidgumberg force-pushed on Dec 9, 2025
  66. davidgumberg commented at 0:12 am on December 10, 2025: contributor 
    0git range-diff 68ac9f1..8bdcd12 9890058..e54690a

    Rebased against master to fix a silent merge conflict with lockedpool.cpp being moved from util -> kernel in aed38ea, and addressed @achow101’s comment about the previous benchmark only measuring encryption of a single key, I’ve updated the benchmarks to test encryption of 2000 keys, the performance impact is still negligible:

    branch EncryptWallet (total - overhead) total ns/key overhead ns/key
    master + benchmarks (8abee2b) 192,652.62 ns 293,553.20 ns 104,682.68 ns
    branch (e54690a) 192,772.32 ns 294,794.46 ns 104,329.09 ns

    Benchmark details

    Master + benchmarks (8abee2b)

    0git checkout 8abee2b
1cmake -B build -DBUILD_BENCH=ON && cmake --build build -j $(nproc)
2./build/bin/bench_bitcoin --filter="WalletEncrypt.*" -min-time=12000
    ns/key key/s err% ins/key cyc/key IPC bra/key miss% total benchmark
    293,553.20 3,406.54 0.1% 6,848,134.51 1,258,326.47 5.442 151,429.70 0.9% 12.91 WalletEncryptDescriptors
    104,682.68 9,552.68 0.1% 2,329,994.28 448,813.08 5.191 74,084.65 0.9% 13.02 WalletEncryptDescriptorsBenchOverhead

    Branch (e54690a)

    0git checkout e54690a
1cmake -B build -DBUILD_BENCH=ON && cmake --build build -j $(nproc)
2./build/bin/bench_bitcoin --filter="WalletEncrypt.*" -min-time=12000
    ns/key key/s err% ins/key cyc/key IPC bra/key miss% total benchmark
    294,794.46 3,392.19 0.3% 6,855,246.26 1,264,321.32 5.422 153,125.52 0.9% 12.97 WalletEncryptDescriptors
    104,329.09 9,585.05 0.1% 2,330,778.18 447,414.99 5.209 74,203.06 0.8% 12.94 WalletEncryptDescriptorsBenchOverhead
  67. DrahtBot removed the label CI failed on Dec 10, 2025
  68. sedited requested review from sipa on Jan 13, 2026
  69. sedited requested review from theStack on Mar 4, 2026
  70. sedited requested review from furszy on Mar 4, 2026
  71. in src/bench/wallet_encrypt.cpp:40 in 8abee2b4bb 
    35+
36+    {
37+        LOCK(wallet->cs_wallet);
38+        for (unsigned int i = 0; i < key_count; i++) {
39+            CKey key;
40+            key.MakeNewKey(/*fCompressed=*/true);
    theStack commented at 5:35 pm on March 6, 2026:

    nit:

    0            CKey key = GenerateRandomKey();
    davidgumberg commented at 11:55 pm on March 10, 2026:
    Thanks, I’ve taken this.
  72. theStack approved
  73. theStack commented at 5:57 pm on March 6, 2026: contributor
    re-ACK e54690ab9b53996d7ec893671e10b6d582cd6a1d
  74. in src/wallet/wallet.cpp:540 in e35f7b8ae0 
    539     CCrypter crypter;
540 
541-    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
542-    master_key.nDeriveIterations = static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start));
543+    // Get the weighted average of iterations we can do in 100ms over 2 runs.
544+    for (int i = 0; i <= 1; i++){
    furszy commented at 8:26 pm on March 6, 2026:
    nano nit: you are the first person I know that writes i <= 1 instead of i < 2.
    davidgumberg commented at 11:55 pm on March 10, 2026:
    Okay, I’ll take this as I don’t feel very strong, but my intention was to make it obvious to a reader that this loop was only run twice.
  75. in src/bench/wallet_encrypt.cpp:85 in 8abee2b4bb 
    80+
81+static void WalletEncryptDescriptors(benchmark::Bench& bench) { WalletEncrypt(bench, /*key_count=*/KEY_COUNT, /*measure_overhead=*/false); }
82+BENCHMARK(WalletEncryptDescriptors, benchmark::PriorityLevel::HIGH);
83+
84+static void WalletEncryptDescriptorsBenchOverhead(benchmark::Bench& bench) { WalletEncrypt(bench, /*key_count=*/KEY_COUNT, /*measure_overhead=*/true); }
85+BENCHMARK(WalletEncryptDescriptorsBenchOverhead, benchmark::PriorityLevel::LOW);
    furszy commented at 8:35 pm on March 6, 2026:

    I would remove this one, and focus on improving the benchmark code (or/and the benchmark framework) to be able to benchmark what we want and nothing else. Having an extra benchmark only to measure the overhead of another benchmark’s setup and teardown smells quite bad.

    We can do the improvements in a follow-up. But I wouldn’t leave this overhead bench here, most likely no one will ever run it again.

    davidgumberg commented at 11:55 pm on March 10, 2026:
    OK, I’ve removed it and added a comment explaining in the benchmark.
  76. DrahtBot requested review from furszy on Mar 6, 2026
  77. fanquake referenced this in commit 544c15ff4e on Mar 10, 2026
  78. davidgumberg force-pushed on Mar 10, 2026
  79. davidgumberg commented at 0:22 am on March 11, 2026: contributor

    Pushed an update to address feedback from @theStack, @furszy @achow101

  80. davidgumberg force-pushed on Mar 11, 2026
  81. davidgumberg commented at 0:37 am on March 11, 2026: contributor

    Rebased to fix two silent merge conflicts:

  82. DrahtBot added the label CI failed on Mar 11, 2026
  83. DrahtBot commented at 0:37 am on March 11, 2026: contributor

    🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/22929713476/job/66548336774 LLM reason (✨ experimental): Build failure in src/wallet/wallet.cpp.o causing the CI to fail.

    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.

  84. refactor: Generalize derivation target calculation ae5485fa0d
  85. davidgumberg force-pushed on Mar 11, 2026
  86. wallet: Make encryption derivation clock mockable 
    Adds a special case where if the elapsed time during measurement of DKF
performance is 0, the default derive iterations are used so that
behavior is stable for testing and benchmarks.
    9a15872516
  87. bench: Add wallet encryption benchmark 51ac1abf6f
  88. build: `lockedpool.cpp` kernel -> crypto 
    Allows `crypto` functions and classes to use `secure_allocator`.
    8c6fedaa81
  89. crypto: Use `secure_allocator` for `AES256_ctx` d53852be31
  90. crypto: Use `secure_allocator` for `AES256CBC*::iv` af0da2fce2
  91. davidgumberg force-pushed on Mar 11, 2026
  92. DrahtBot removed the label CI failed on Mar 11, 2026
  93. furszy commented at 2:02 pm on March 11, 2026: member

    utACK af0da2fce2146a005f54b900a1fd545a6278173a

    Thanks.

  94. DrahtBot requested review from theStack on Mar 11, 2026
  95. theStack approved
  96. theStack commented at 5:42 pm on March 11, 2026: contributor

    re-ACK af0da2fce2146a005f54b900a1fd545a6278173a

    (as per $ git range-diff e54690ab...af0da2fc)

  97. achow101 commented at 8:27 pm on March 13, 2026: member
    ACK af0da2fce2146a005f54b900a1fd545a6278173a
  98. achow101 merged this on Mar 13, 2026
  99. achow101 closed this on Mar 13, 2026


davidgumberg DrahtBot theStack sipa laanwj furszy achow101 glozow


sipa

Labels
Utils/log/libs

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-03-23 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me