From ae5485fa0d258f060a22c399a4e257eaf96e67b5 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Mon, 5 May 2025 20:23:28 -0700 Subject: [PATCH 1/6] refactor: Generalize derivation target calculation --- src/wallet/wallet.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1e7ad3fc6b8..782667cc35d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -579,16 +579,20 @@ void CWallet::UpgradeDescriptorCache() * derivation parameters (should take at least 100ms) and encrypt the master key. */ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyingMaterial& plain_master_key, CMasterKey& master_key) { - constexpr MillisecondsDouble target{100}; - auto start{SteadyClock::now()}; + constexpr MillisecondsDouble target_time{100}; CCrypter crypter; - crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod); - master_key.nDeriveIterations = static_cast(master_key.nDeriveIterations * target / (SteadyClock::now() - start)); + // Get the weighted average of iterations we can do in 100ms over 2 runs. + for (int i = 0; i < 2; i++){ + auto start_time{SteadyClock::now()}; + crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod); + auto elapsed_time{SteadyClock::now() - start_time}; - start = SteadyClock::now(); - crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod); - master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; + // target_iterations : elapsed_iterations :: target_time : elapsed_time + unsigned int target_iterations = master_key.nDeriveIterations * target_time / elapsed_time; + // Get the weighted average with previous runs. + master_key.nDeriveIterations = (i * master_key.nDeriveIterations + target_iterations) / (i + 1); + } if (master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) { master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; From 9a158725161f726eb6747811e9e9335eef83f616 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Mon, 5 May 2025 20:28:18 -0700 Subject: [PATCH 2/6] 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. --- src/wallet/wallet.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 782667cc35d..dae37e05349 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -584,9 +584,15 @@ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyin // Get the weighted average of iterations we can do in 100ms over 2 runs. for (int i = 0; i < 2; i++){ - auto start_time{SteadyClock::now()}; + auto start_time{NodeClock::now()}; crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod); - auto elapsed_time{SteadyClock::now() - start_time}; + auto elapsed_time{NodeClock::now() - start_time}; + + if (elapsed_time <= 0s) { + // We are probably in a test with a mocked clock. + master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; + break; + } // target_iterations : elapsed_iterations :: target_time : elapsed_time unsigned int target_iterations = master_key.nDeriveIterations * target_time / elapsed_time; From 51ac1abf6fdb6340110397a959681d52e29f73f7 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 5 Feb 2025 16:55:33 -0800 Subject: [PATCH 3/6] bench: Add wallet encryption benchmark --- src/bench/CMakeLists.txt | 1 + src/bench/wallet_encrypt.cpp | 82 ++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 src/bench/wallet_encrypt.cpp diff --git a/src/bench/CMakeLists.txt b/src/bench/CMakeLists.txt index 50b29a14667..e1d5e4097ff 100644 --- a/src/bench/CMakeLists.txt +++ b/src/bench/CMakeLists.txt @@ -77,6 +77,7 @@ if(ENABLE_WALLET) wallet_balance.cpp wallet_create.cpp wallet_create_tx.cpp + wallet_encrypt.cpp wallet_loading.cpp wallet_ismine.cpp wallet_migration.cpp diff --git a/src/bench/wallet_encrypt.cpp b/src/bench/wallet_encrypt.cpp new file mode 100644 index 00000000000..3d73081c7cc --- /dev/null +++ b/src/bench/wallet_encrypt.cpp @@ -0,0 +1,82 @@ +// Copyright (c) 2025-present The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace wallet { +static void WalletEncrypt(benchmark::Bench& bench, unsigned int key_count) +{ + auto test_setup = MakeNoLogFileContext(); + FastRandomContext rand{/*fDeterministic=*/true}; + + auto password{rand.randbytes(20)}; + SecureString secure_pass{password.begin(), password.end()}; + + WalletContext context; + context.args = &test_setup->m_args; + context.chain = test_setup->m_node.chain.get(); + + uint64_t create_flags = WALLET_FLAG_DESCRIPTORS; + auto database = CreateMockableWalletDatabase(); + auto wallet = TestCreateWallet(std::move(database), context, create_flags); + + { + LOCK(wallet->cs_wallet); + for (unsigned int i = 0; i < key_count; i++) { + CKey key = GenerateRandomKey(); + FlatSigningProvider keys; + std::string error; + std::vector> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /*require_checksum=*/false); + WalletDescriptor w_desc(std::move(desc.at(0)), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0); + Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false)); + } + } + + database = DuplicateMockDatabase(wallet->GetDatabase()); + + // reload the wallet for the actual benchmark + TestUnloadWallet(std::move(wallet)); + + // Setting a mock time is necessary to force default derive iteration count during + // wallet encryption. + SetMockTime(1); + + // This benchmark has a lot of overhead, this should be good enough to catch + // any regressions, but for an accurate measurement of how long wallet + // encryption takes, this should be reworked after something like + // https://github.com/bitcoin/bitcoin/pull/34208 is merged. + bench.batch(key_count).unit("key").run([&] { + wallet = TestLoadWallet(std::move(database), context); + + // Save a copy of the db before encrypting + database = DuplicateMockDatabase(wallet->GetDatabase()); + + wallet->EncryptWallet(secure_pass); + + for (const auto& [_, key] : wallet->mapMasterKeys){ + assert(key.nDeriveIterations == CMasterKey::DEFAULT_DERIVE_ITERATIONS); + } + + TestUnloadWallet(std::move(wallet)); + }); +} + +constexpr unsigned int KEY_COUNT = 2000; + +static void WalletEncryptDescriptors(benchmark::Bench& bench) { WalletEncrypt(bench, /*key_count=*/KEY_COUNT); } +BENCHMARK(WalletEncryptDescriptors); + +} // namespace wallet From 8c6fedaa81882be1e2db4a9a2291205d774fa4c6 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 31 Jan 2025 13:29:40 -0800 Subject: [PATCH 4/6] build: `lockedpool.cpp` kernel -> crypto Allows `crypto` functions and classes to use `secure_allocator`. --- src/crypto/CMakeLists.txt | 1 + src/kernel/CMakeLists.txt | 1 - src/util/CMakeLists.txt | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/crypto/CMakeLists.txt b/src/crypto/CMakeLists.txt index 92653ade5a7..4dc84664ea7 100644 --- a/src/crypto/CMakeLists.txt +++ b/src/crypto/CMakeLists.txt @@ -20,6 +20,7 @@ add_library(bitcoin_crypto STATIC EXCLUDE_FROM_ALL sha512.cpp siphash.cpp ../support/cleanse.cpp + ../support/lockedpool.cpp ) target_link_libraries(bitcoin_crypto diff --git a/src/kernel/CMakeLists.txt b/src/kernel/CMakeLists.txt index 244f0098fdf..cf2a0c047b1 100644 --- a/src/kernel/CMakeLists.txt +++ b/src/kernel/CMakeLists.txt @@ -54,7 +54,6 @@ add_library(bitcoinkernel ../script/solver.cpp ../signet.cpp ../streams.cpp - ../support/lockedpool.cpp ../sync.cpp ../txdb.cpp ../txgraph.cpp diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index fdf0081c263..3444fb09361 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -33,7 +33,6 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL ../random.cpp ../randomenv.cpp ../streams.cpp - ../support/lockedpool.cpp ../sync.cpp ) From d53852be316dd2322a7e621a81ee33e1b234c530 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 31 Jan 2025 11:48:20 -0800 Subject: [PATCH 5/6] crypto: Use `secure_allocator` for `AES256_ctx` --- src/crypto/aes.cpp | 15 +++++++++------ src/crypto/aes.h | 7 +++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/crypto/aes.cpp b/src/crypto/aes.cpp index 40df5690e0f..207a8e8a85d 100644 --- a/src/crypto/aes.cpp +++ b/src/crypto/aes.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include @@ -12,32 +13,34 @@ extern "C" { AES256Encrypt::AES256Encrypt(const unsigned char key[32]) { - AES256_init(&ctx, key); + ctx = allocator.allocate(1); + AES256_init(ctx, key); } AES256Encrypt::~AES256Encrypt() { - memset(&ctx, 0, sizeof(ctx)); + allocator.deallocate(ctx, 1); } void AES256Encrypt::Encrypt(unsigned char ciphertext[16], const unsigned char plaintext[16]) const { - AES256_encrypt(&ctx, 1, ciphertext, plaintext); + AES256_encrypt(ctx, 1, ciphertext, plaintext); } AES256Decrypt::AES256Decrypt(const unsigned char key[32]) { - AES256_init(&ctx, key); + ctx = allocator.allocate(1); + AES256_init(ctx, key); } AES256Decrypt::~AES256Decrypt() { - memset(&ctx, 0, sizeof(ctx)); + allocator.deallocate(ctx, 1); } void AES256Decrypt::Decrypt(unsigned char plaintext[16], const unsigned char ciphertext[16]) const { - AES256_decrypt(&ctx, 1, plaintext, ciphertext); + AES256_decrypt(ctx, 1, plaintext, ciphertext); } diff --git a/src/crypto/aes.h b/src/crypto/aes.h index 4eae9a3bf7f..191cffd9110 100644 --- a/src/crypto/aes.h +++ b/src/crypto/aes.h @@ -7,6 +7,7 @@ #ifndef BITCOIN_CRYPTO_AES_H #define BITCOIN_CRYPTO_AES_H +#include extern "C" { #include } @@ -18,7 +19,8 @@ static const int AES256_KEYSIZE = 32; class AES256Encrypt { private: - AES256_ctx ctx; + secure_allocator allocator; + AES256_ctx *ctx; public: explicit AES256Encrypt(const unsigned char key[32]); @@ -30,7 +32,8 @@ public: class AES256Decrypt { private: - AES256_ctx ctx; + secure_allocator allocator; + AES256_ctx *ctx; public: explicit AES256Decrypt(const unsigned char key[32]); From af0da2fce2146a005f54b900a1fd545a6278173a Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 31 Jan 2025 11:49:53 -0800 Subject: [PATCH 6/6] crypto: Use `secure_allocator` for `AES256CBC*::iv` --- src/crypto/aes.cpp | 6 ++++-- src/crypto/aes.h | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/crypto/aes.cpp b/src/crypto/aes.cpp index 207a8e8a85d..cd2b4f76c60 100644 --- a/src/crypto/aes.cpp +++ b/src/crypto/aes.cpp @@ -124,6 +124,7 @@ static int CBCDecrypt(const T& dec, const unsigned char iv[AES_BLOCKSIZE], const AES256CBCEncrypt::AES256CBCEncrypt(const unsigned char key[AES256_KEYSIZE], const unsigned char ivIn[AES_BLOCKSIZE], bool padIn) : enc(key), pad(padIn) { + iv = allocator.allocate(AES_BLOCKSIZE); memcpy(iv, ivIn, AES_BLOCKSIZE); } @@ -134,12 +135,13 @@ int AES256CBCEncrypt::Encrypt(const unsigned char* data, int size, unsigned char AES256CBCEncrypt::~AES256CBCEncrypt() { - memset(iv, 0, sizeof(iv)); + allocator.deallocate(iv, AES_BLOCKSIZE); } AES256CBCDecrypt::AES256CBCDecrypt(const unsigned char key[AES256_KEYSIZE], const unsigned char ivIn[AES_BLOCKSIZE], bool padIn) : dec(key), pad(padIn) { + iv = allocator.allocate(AES_BLOCKSIZE); memcpy(iv, ivIn, AES_BLOCKSIZE); } @@ -151,5 +153,5 @@ int AES256CBCDecrypt::Decrypt(const unsigned char* data, int size, unsigned char AES256CBCDecrypt::~AES256CBCDecrypt() { - memset(iv, 0, sizeof(iv)); + allocator.deallocate(iv, AES_BLOCKSIZE); } diff --git a/src/crypto/aes.h b/src/crypto/aes.h index 191cffd9110..617bb62de91 100644 --- a/src/crypto/aes.h +++ b/src/crypto/aes.h @@ -49,9 +49,10 @@ public: int Encrypt(const unsigned char* data, int size, unsigned char* out) const; private: + secure_allocator allocator; const AES256Encrypt enc; const bool pad; - unsigned char iv[AES_BLOCKSIZE]; + unsigned char *iv; }; class AES256CBCDecrypt @@ -62,9 +63,10 @@ public: int Decrypt(const unsigned char* data, int size, unsigned char* out) const; private: + secure_allocator allocator; const AES256Decrypt dec; const bool pad; - unsigned char iv[AES_BLOCKSIZE]; + unsigned char *iv; }; #endif // BITCOIN_CRYPTO_AES_H