From b7e69953809b12c4153d480a8c0507851a9e408b Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 5 Feb 2025 14:32:21 -0800 Subject: [PATCH 1/5] wallet: EncryptWallet forced derivations Forcing a derivation count is useful for benchmarks, since otherwise `EncryptWallet` trying to normalize itself interferes with measurement. --- src/wallet/wallet.cpp | 28 ++++++++++++++++++---------- src/wallet/wallet.h | 2 +- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dd9dfcd516a..1cfc8520454 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -807,7 +807,7 @@ void CWallet::AddToSpends(const CWalletTx& wtx, WalletBatch* batch) AddToSpends(txin.prevout, wtx.GetHash(), batch); } -bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) +bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase, std::optional forceIterations) { if (IsCrypted()) return false; @@ -823,17 +823,25 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) GetStrongRandBytes(kMasterKey.vchSalt); CCrypter crypter; - constexpr MillisecondsDouble target{100}; - auto start{SteadyClock::now()}; - crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, 25000, kMasterKey.nDerivationMethod); - kMasterKey.nDeriveIterations = static_cast(25000 * target / (SteadyClock::now() - start)); - start = SteadyClock::now(); - crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); - kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast(kMasterKey.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; + // False by default, used by test or benchmarking code + if(forceIterations.has_value()) { + kMasterKey.nDeriveIterations = forceIterations.value(); + } + // Try to find an nDeriveIterations that takes about 100ms + else { + constexpr MillisecondsDouble target{100}; + auto start{SteadyClock::now()}; + crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, 25000, kMasterKey.nDerivationMethod); + kMasterKey.nDeriveIterations = static_cast(25000 * target / (SteadyClock::now() - start)); - if (kMasterKey.nDeriveIterations < 25000) - kMasterKey.nDeriveIterations = 25000; + start = SteadyClock::now(); + crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); + kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast(kMasterKey.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; + + if (kMasterKey.nDeriveIterations < 25000) + kMasterKey.nDeriveIterations = 25000; + } WalletLogPrintf("Encrypting Wallet with an nDeriveIterations of %i\n", kMasterKey.nDeriveIterations); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0afe71a40ef..052d8775b67 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -581,7 +581,7 @@ public: bool Unlock(const SecureString& strWalletPassphrase); bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); - bool EncryptWallet(const SecureString& strWalletPassphrase); + bool EncryptWallet(const SecureString& strWalletPassphrase, std::optional forceIterations = std::nullopt); void GetKeyBirthTimes(std::map &mapKeyBirth) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); unsigned int ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const; From 15fa9efd3c5dc42993ab63dd2809aaeeb5403848 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 5 Feb 2025 16:55:33 -0800 Subject: [PATCH 2/5] bench: Add wallet encryption benchmark --- src/bench/CMakeLists.txt | 1 + src/bench/wallet_encrypt.cpp | 104 +++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 src/bench/wallet_encrypt.cpp diff --git a/src/bench/CMakeLists.txt b/src/bench/CMakeLists.txt index c55bbb1e05f..8c1a9582c41 100644 --- a/src/bench/CMakeLists.txt +++ b/src/bench/CMakeLists.txt @@ -70,6 +70,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..3850e1fd861 --- /dev/null +++ b/src/bench/wallet_encrypt.cpp @@ -0,0 +1,104 @@ +// 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 + +namespace wallet { +static void WalletEncrypt(benchmark::Bench& bench, bool legacy_wallet, bool measure_overhead) +{ + auto test_setup = MakeNoLogFileContext(); + FastRandomContext random; + + auto password{random.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{0}; + if(!legacy_wallet) { + create_flags = WALLET_FLAG_DESCRIPTORS; + } + + auto database{CreateMockableWalletDatabase()}; + auto wallet{TestLoadWallet(std::move(database), context, create_flags)}; + + int key_count{0}; + + if(!legacy_wallet) { + // Add destinations + for(auto type : OUTPUT_TYPES) { + for(int i = 0; i < 10'000; i++) { + CMutableTransaction mtx; + mtx.vout.emplace_back(COIN, GetScriptForDestination(*Assert(wallet->GetNewDestination(type, "")))); + mtx.vin.emplace_back(); + wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}); + key_count++; + } + } + } + else { + LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM(); + /* legacy spkm */ + for(size_t i = 0; i < 10'000 * OUTPUT_TYPES.size(); i++) { + CKey key = GenerateRandomKey(); + CPubKey pubkey = key.GetPubKey(); + // Load key, scripts and create address book record + Assert(legacy_spkm->LoadKey(key, pubkey)); + CTxDestination dest{PKHash(pubkey)}; + Assert(wallet->SetAddressBook(dest, strprintf("legacy_%d", i), /*purpose=*/std::nullopt)); + + CMutableTransaction mtx; + mtx.vout.emplace_back(COIN, GetScriptForDestination(dest)); + mtx.vin.emplace_back(); + wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}); + key_count++; + } + } + + database = DuplicateMockDatabase(wallet->GetDatabase()); + + // reload the wallet for the actual benchmark + TestUnloadWallet(std::move(wallet)); + + bench.batch(key_count).unit("key").run([&] { + wallet = TestLoadWallet(std::move(database), context, create_flags); + + // Save a copy of the db before encrypting + database = DuplicateMockDatabase(wallet->GetDatabase()); + + // Skip actually encrypting wallet on the overhead measuring run, so we + // can subtract the overhead from the results. + if(!measure_overhead) { + wallet->EncryptWallet(secure_pass, 25000); + } + + // cleanup + TestUnloadWallet(std::move(wallet)); + }); +} + +static void WalletEncryptDescriptors(benchmark::Bench& bench) { WalletEncrypt(bench, /*legacy_wallet=*/false, /*measure_overhead=*/false); } +static void WalletEncryptLegacy(benchmark::Bench& bench) { WalletEncrypt(bench, /*legacy_wallet=*/true, /*measure_overhead=*/false); } + +BENCHMARK(WalletEncryptDescriptors, benchmark::PriorityLevel::HIGH); +BENCHMARK(WalletEncryptLegacy, benchmark::PriorityLevel::HIGH); + +static void WalletEncryptDescriptorsBenchOverhead(benchmark::Bench& bench) { WalletEncrypt(bench, /*legacy_wallet=*/false, /*measure_overhead=*/true); } +static void WalletEncryptLegacyBenchOverhead(benchmark::Bench& bench) { WalletEncrypt(bench, /*legacy_wallet=*/true, /*measure_overhead=*/true); } + +BENCHMARK(WalletEncryptDescriptorsBenchOverhead, benchmark::PriorityLevel::LOW); +BENCHMARK(WalletEncryptLegacyBenchOverhead, benchmark::PriorityLevel::LOW); + +} // namespace wallet From 721c9240fb44813318f309bf5c0020ec6f8c05ff Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 31 Jan 2025 13:29:40 -0800 Subject: [PATCH 3/5] build: Move `lockedpool.cpp` from util -> crypto Allows `crypto` functions and classes to use `secure_allocator`. --- src/crypto/CMakeLists.txt | 1 + src/util/CMakeLists.txt | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/CMakeLists.txt b/src/crypto/CMakeLists.txt index 03c6972dca0..e06dd435ecb 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/util/CMakeLists.txt b/src/util/CMakeLists.txt index 4999dbf13f0..585c5fe7590 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -32,7 +32,6 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL ../random.cpp ../randomenv.cpp ../streams.cpp - ../support/lockedpool.cpp ../sync.cpp ) From 28d15152f51aedfd83478f515e8c7b212d607ddc Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 31 Jan 2025 11:48:20 -0800 Subject: [PATCH 4/5] crypto: Use `secure_allocator` for `AES256_ctx` --- src/crypto/aes.cpp | 14 ++++++++------ src/crypto/aes.h | 7 +++++-- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/crypto/aes.cpp b/src/crypto/aes.cpp index 2afcbd16292..473115a25d5 100644 --- a/src/crypto/aes.cpp +++ b/src/crypto/aes.cpp @@ -12,32 +12,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 3a0011bee42..a8c637de647 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 15d8500f99012422be495b8e85e4e25e6a4419d8 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Fri, 31 Jan 2025 11:49:53 -0800 Subject: [PATCH 5/5] 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 473115a25d5..0237eb094a1 100644 --- a/src/crypto/aes.cpp +++ b/src/crypto/aes.cpp @@ -123,6 +123,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); } @@ -133,12 +134,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); } @@ -150,5 +152,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 a8c637de647..cce4684cbd0 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