wallet: refactor: various master key encryption cleanups #31398

theStack commented at 3:54 AM on December 1, 2024: contributor

This PR contains various cleanups around the wallet's master key encryption logic. The default/minimum key derivation rounds magic number of 25000 is hoisted into a constant (member of CMasterKey) and two new functions EncryptMasterKey/DecryptMasterKey are introduced in order to deduplicate code for the derivation round determination and master key en/decryption. Also, mentions of the never-implemented derivation method scrypt are removed from the wallet crypter header and both plain and encrypted master key instances are renamed to adapt to moderning coding style (hopefully improving readability).

DrahtBot commented at 3:54 AM on December 1, 2024: 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/31398.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg, achow101
Concept ACK	furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31774 (crypto: Use secure_allocator for AES256_ctx by davidgumberg)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

DrahtBot added the label Wallet on Dec 1, 2024

furszy commented at 6:04 PM on December 27, 2024: member

Concept ACK, nice code dedup.

in src/wallet/crypter.h:48 in 22e9869524 outdated

  41 | @@ -42,16 +42,19 @@ class CMasterKey
  42 |      //! Use this for more parameters to key derivation (currently unused)
  43 |      std::vector<unsigned char> vchOtherDerivationParameters;
  44 |  
  45 | +    //! Default/minimum number of key derivation rounds
  46 | +    // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
  47 | +    // ie slightly lower than the lowest hardware we need bother supporting
  48 | +    static constexpr int DEFAULT_DERIVE_ITERATIONS = 25000;

davidgumberg commented at 8:20 AM on January 25, 2025:

nit: this could be used in src/wallet/test/wallet_crypto_tests.cpp and src/wallet/test/fuzz/crypter.cpp also, although maybe the fuzzing range should actually go higher than this

davidgumberg commented at 8:49 AM on January 25, 2025:

    static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000;

theStack commented at 5:20 PM on January 27, 2025:

Good catch, done. Kept the fuzzing range as-is though since this PR is not meant to change any behaviour.

theStack commented at 5:21 PM on January 27, 2025:

Makes sense to match the type of the constant with the one of the variable that it gets assigned to (nDeriveIterations), done.

in src/wallet/crypter.h:37 in a6af5b45a1 outdated

  36 | @@ -37,11 +37,9 @@ class CMasterKey
  37 |      std::vector<unsigned char> vchCryptedKey;

davidgumberg commented at 8:33 AM on January 25, 2025:

nit: could also remove the mention of scrypt above on L26

theStack commented at 5:20 PM on January 27, 2025:

Done.

in src/wallet/wallet.cpp:592 in de9b54780b outdated

 581 | +    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
 582 | +    master_key.nDeriveIterations = static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start));
 583 | +
 584 | +    start = SteadyClock::now();
 585 | +    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
 586 | +    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;

davidgumberg commented at 9:03 AM on January 25, 2025:

just a remark feel-free-to-ignore: I tried to think of a way to deduplicate this adjustment logic using a lambda here, but it turned out worse :)

    CCrypter crypter;

    auto adjust_iterations = [&](unsigned int current_iterations) -> unsigned int {
        auto start{SteadyClock::now()};

        if (!crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, current_iterations, master_key.nDerivationMethod)) {
            return false;
        }

        return current_iterations * target / (SteadyClock::now() - start);
    };

    master_key.nDeriveIterations = adjust_iterations(master_key.nDeriveIterations);
    master_key.nDeriveIterations = (master_key.nDeriveIterations + adjust_iterations(master_key.nDeriveIterations)) / 2;

in src/wallet/wallet.cpp:587 in de9b54780b outdated

 576 | +{
 577 | +    constexpr MillisecondsDouble target{100};
 578 | +    auto start{SteadyClock::now()};
 579 | +    CCrypter crypter;
 580 | +
 581 | +    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);

davidgumberg commented at 9:08 AM on January 25, 2025:

Should the return value of the 'benchmark' runs of SetKeyFromPassphrase be checked and an early return if false, or does it not matter until the real SetKeyFromPassphrase call below?

theStack commented at 5:22 PM on January 27, 2025:

Good question. Left it as-is for now, since this is a refactoring PR, but maybe something to consider for a follow-up PR.

in src/wallet/wallet.cpp:596 in de9b54780b outdated

 585 | +    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
 586 | +    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
 587 | +
 588 | +    if (master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) {
 589 | +        master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS;
 590 | +    }

davidgumberg commented at 9:12 AM on January 25, 2025:

nit, maybe:

master_key.nDeriveIterations = std::max(CMasterKey::DEFAULT_DERIVE_ITERATIONS, master_key.nDeriveIterations);

theStack commented at 5:27 PM on January 27, 2025:

Left this as-is for ease of review. Even though the suggestion would deduplicate the use of the constant, I find the std::max construct slightly harder to grasp than the two lines now (but maybe that's me, happy to change).

davidgumberg commented at 5:54 PM on January 27, 2025:

Agreed, it is confusing to use std::max when we are really interested in setting a minimum value.

davidgumberg commented at 9:12 AM on January 25, 2025: contributor

Concept ACK, left a few non-blocking nits

wallet: doc: remove mentions of unavailable scrypt derivation method

These comments are there since wallet encryption was first introduced
(see commit 4e87d341f75f13bbd7d108c31c03886fbc4df56f, PR #352), but scrypt
was actually never implemented as a derivation method.

62c209f50d

wallet: refactor: introduce `CMasterKey::DEFAULT_DERIVE_ITERATIONS` constant

This gets rid of the magic number used in both the `CMasterKey` ctor
and the wallet encryption / passphrase change methods.

a6d9b415aa

wallet: refactor: dedup master key encryption / derivation rounds setting 846545947c

wallet: refactor: dedup master key decryption 5a92077fd5

scripted-diff: wallet: rename plain and encrypted master key variables

-BEGIN VERIFY SCRIPT-
sed -i s/_vMasterKey/plain_master_key/g ./src/wallet/wallet.cpp
sed -i s/kMasterKey/master_key/g ./src/wallet/wallet.cpp
sed -i "s/const MasterKeyMap::value_type& pMasterKey/const auto\& \[_, master_key\]/g" ./src/wallet/wallet.cpp
sed -i s/pMasterKey\.second/master_key/g ./src/wallet/wallet.cpp
sed -i "s/MasterKeyMap::value_type& pMasterKey/auto\& \[master_key_id, master_key\]/g" ./src/wallet/wallet.cpp
sed -i s/pMasterKey\.first/master_key_id/g ./src/wallet/wallet.cpp
-END VERIFY SCRIPT-

a8333fc9ff

theStack force-pushed on Jan 27, 2025

theStack commented at 5:27 PM on January 27, 2025: contributor

@davidgumberg: Thanks for the detailed review! I took most of your suggestions (see above) and rebased on master.

davidgumberg commented at 6:31 PM on January 27, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/31398/commits/a8333fc9ff9adaa97a1f9024f5783cc071777150

$ git range-diff dbc8ba1..bda3a48  0a931a9..a8333fc | sed 's/^ \{4\}//' # first four spaces removed to improve github diff highlighting

</summary>

1:  a6af5b45a1 ! 1:  62c209f50d wallet: doc: remove mentions of unavailable scrypt derivation method
@@ Commit message
     was actually never implemented as a derivation method.
 
  ## src/wallet/crypter.h ##
+@@ src/wallet/crypter.h: const unsigned int WALLET_CRYPTO_IV_SIZE = 16;
+  * derived using derivation method nDerivationMethod
+  * (0 == EVP_sha512()) and derivation iterations nDeriveIterations.
+  * vchOtherDerivationParameters is provided for alternative algorithms
+- * which may require more parameters (such as scrypt).
++ * which may require more parameters.
+  *
+  * Wallet Private Keys are then encrypted using AES-256-CBC
+  * with the double-sha256 of the public key as the IV, and the
 @@ src/wallet/crypter.h: public:
      std::vector<unsigned char> vchCryptedKey;
      std::vector<unsigned char> vchSalt;
2:  22e9869524 ! 2:  a6d9b415aa wallet: refactor: introduce `CMasterKey::DEFAULT_DERIVE_ITERATIONS` constant
@@ src/wallet/crypter.h: public:
 +    //! Default/minimum number of key derivation rounds
 +    // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
 +    // ie slightly lower than the lowest hardware we need bother supporting
-+    static constexpr int DEFAULT_DERIVE_ITERATIONS = 25000;
++    static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000;
 +
      SERIALIZE_METHODS(CMasterKey, obj)
      {
@@ src/wallet/crypter.h: public:
          vchOtherDerivationParameters = std::vector<unsigned char>(0);
      }
 
+ ## src/wallet/test/fuzz/crypter.cpp ##
+@@ src/wallet/test/fuzz/crypter.cpp: FUZZ_TARGET(crypter, .init = initialize_crypter)
+         // Limiting the value of rounds since it is otherwise uselessly expensive and causes a timeout when fuzzing.
+         crypt.SetKeyFromPassphrase(/*key_data=*/secure_string,
+                                    /*salt=*/ConsumeFixedLengthByteVector(fuzzed_data_provider, WALLET_CRYPTO_SALT_SIZE),
+-                                   /*rounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 25000),
++                                   /*rounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, CMasterKey::DEFAULT_DERIVE_ITERATIONS),
+                                    /*derivation_method=*/derivation_method);
+     }
+ 
+
+ ## src/wallet/test/wallet_crypto_tests.cpp ##
+@@ src/wallet/test/wallet_crypto_tests.cpp: static void TestEncrypt(const CCrypter& crypt, const std::span<const unsigned ch
+ BOOST_AUTO_TEST_CASE(passphrase) {
+     // These are expensive.
+ 
+-    TestCrypter::TestPassphrase("0000deadbeef0000"_hex_u8, "test", 25000,
++    TestCrypter::TestPassphrase("0000deadbeef0000"_hex_u8, "test", CMasterKey::DEFAULT_DERIVE_ITERATIONS,
+                                 "fc7aba077ad5f4c3a0988d8daa4810d0d4a0e3bcb53af662998898f33df0556a"_hex_u8,
+                                 "cf2f2691526dd1aa220896fb8bf7c369"_hex_u8);
+ 
+@@ src/wallet/test/wallet_crypto_tests.cpp: BOOST_AUTO_TEST_CASE(passphrase) {
+ BOOST_AUTO_TEST_CASE(encrypt) {
+     constexpr std::array<uint8_t, WALLET_CRYPTO_SALT_SIZE> salt{"0000deadbeef0000"_hex_u8};
+     CCrypter crypt;
+-    crypt.SetKeyFromPassphrase("passphrase", salt, 25000, 0);
++    crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0);
+     TestCrypter::TestEncrypt(crypt, "22bcade09ac03ff6386914359cfe885cfeb5f77ff0d670f102f619687453b29d"_hex_u8);
+ 
+     for (int i = 0; i != 100; i++)
+@@ src/wallet/test/wallet_crypto_tests.cpp: BOOST_AUTO_TEST_CASE(encrypt) {
+ BOOST_AUTO_TEST_CASE(decrypt) {
+     constexpr std::array<uint8_t, WALLET_CRYPTO_SALT_SIZE> salt{"0000deadbeef0000"_hex_u8};
+     CCrypter crypt;
+-    crypt.SetKeyFromPassphrase("passphrase", salt, 25000, 0);
++    crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0);
+ 
+     // Some corner cases the came up while testing
+     TestCrypter::TestDecrypt(crypt,"795643ce39d736088367822cdc50535ec6f103715e3e48f4f3b1a60a08ef59ca"_hex_u8);
+
  ## src/wallet/wallet.cpp ##
 @@ src/wallet/wallet.cpp: bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
                  crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod);
3:  de9b54780b = 3:  846545947c wallet: refactor: dedup master key encryption / derivation rounds setting
4:  ba9f582332 = 4:  5a92077fd5 wallet: refactor: dedup master key decryption
5:  bda3a48190 = 5:  a8333fc9ff scripted-diff: wallet: rename plain and encrypted master key variables

DrahtBot requested review from furszy on Jan 27, 2025

in src/wallet/crypter.h:60 in a6d9b415aa outdated

  58 | -        // ie slightly lower than the lowest hardware we need bother supporting
  59 | -        nDeriveIterations = 25000;
  60 | +        nDeriveIterations = DEFAULT_DERIVE_ITERATIONS;
  61 |          nDerivationMethod = 0;
  62 |          vchOtherDerivationParameters = std::vector<unsigned char>(0);
  63 |      }

davidgumberg commented at 6:36 PM on January 27, 2025:

nit: this constructor should probably be replaced with default initializers

-    unsigned int nDerivationMethod;
-    unsigned int nDeriveIterations;
+    unsigned int nDerivationMethod{0};
+    unsigned int nDeriveIterations{DEFAULT_DERIVE_ITERATIONS};
     //! Use this for more parameters to key derivation (currently unused)
-    std::vector<unsigned char> vchOtherDerivationParameters;
+    std::vector<unsigned char> vchOtherDerivationParameters{0};

     SERIALIZE_METHODS(CMasterKey, obj)
     {
         READWRITE(obj.vchCryptedKey, obj.vchSalt, obj.nDerivationMethod, obj.nDeriveIterations, obj.vchOtherDerivationParameters);
     }
-
-    CMasterKey()
-    {
-        nDeriveIterations = DEFAULT_DERIVE_ITERATIONS;
-        nDerivationMethod = 0;
-        vchOtherDerivationParameters = std::vector<unsigned char>(0);
-    }
 };

(https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures)

in src/wallet/crypter.h:43 in 62c209f50d outdated

  41 |      unsigned int nDerivationMethod;
  42 |      unsigned int nDeriveIterations;
  43 | -    //! Use this for more parameters to key derivation,
  44 | -    //! such as the various parameters to scrypt
  45 | +    //! Use this for more parameters to key derivation (currently unused)
  46 |      std::vector<unsigned char> vchOtherDerivationParameters;

furszy commented at 9:18 PM on March 13, 2025:

In 62c209f50d9c33: What about inlining this into the (un)serialization method? If it is not used, we don't need to keep the field in memory.

theStack commented at 10:42 PM on March 17, 2025:

The memory consumption of an empty vector is negligible (24 bytes on my system [1]) so this is not a big deal, but happy to address this on the next push.

[1] according to

    std::vector<unsigned char> empty_vec;
    printf("%ld\n", sizeof(empty_vec) + memusage::DynamicUsage(empty_vec));

in src/wallet/wallet.cpp:665 in a8333fc9ff

 667 |                  }
 668 | -                WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", pMasterKey.second.nDeriveIterations);
 669 | +                WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", master_key.nDeriveIterations);
 670 |  
 671 | -                WalletBatch(GetDatabase()).WriteMasterKey(pMasterKey.first, pMasterKey.second);
 672 | +                WalletBatch(GetDatabase()).WriteMasterKey(master_key_id, master_key);

furszy commented at 9:27 PM on March 13, 2025:

Not related to this PR but if WriteMasterKey() fails then this will still succeed when it shouldn't.

theStack commented at 10:47 PM on March 17, 2025:

Good point, ignoring the return values of database writes seems to be a really bad idea. Should be addressed in a (non-refactor) followup.

in src/wallet/wallet.cpp:864 in a8333fc9ff

 869 |              delete encrypted_batch;
 870 |              encrypted_batch = nullptr;
 871 |              return false;
 872 |          }
 873 | -        encrypted_batch->WriteMasterKey(nMasterKeyMaxID, kMasterKey);
 874 | +        encrypted_batch->WriteMasterKey(nMasterKeyMaxID, master_key);

furszy commented at 9:27 PM on March 13, 2025:

Same as above; not related to this PR but if WriteMasterKey() fails then this will still succeed when it shouldn't.

DrahtBot requested review from furszy on Mar 13, 2025

in src/wallet/crypter.h:48 in a8333fc9ff

  46 |      std::vector<unsigned char> vchOtherDerivationParameters;
  47 |  
  48 | +    //! Default/minimum number of key derivation rounds
  49 | +    // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
  50 | +    // ie slightly lower than the lowest hardware we need bother supporting
  51 | +    static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000;

yancyribbens commented at 5:57 PM on March 14, 2025:

    static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25'000;

in src/wallet/crypter.h:46 in a8333fc9ff

  44 | -    //! such as the various parameters to scrypt
  45 | +    //! Use this for more parameters to key derivation (currently unused)
  46 |      std::vector<unsigned char> vchOtherDerivationParameters;
  47 |  
  48 | +    //! Default/minimum number of key derivation rounds
  49 | +    // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M

yancyribbens commented at 5:57 PM on March 14, 2025:

    // 25,000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M

theStack commented at 10:40 PM on March 17, 2025: contributor

Thanks for the reviews! The suggestions (https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1931016480, #31398 (review), #31398 (review) and #31398 (review)) make sense, but since they are all nits (IMHO), I'll address them if I have to retouch.

achow101 commented at 11:25 PM on April 29, 2025: member

ACK a8333fc9ff9adaa97a1f9024f5783cc071777150

achow101 merged this on Apr 29, 2025

achow101 closed this on Apr 29, 2025

theStack deleted the branch on Apr 29, 2025