Merge bitcoin/bitcoin#26532: wallet: bugfix, invalid crypted key "checksum_valid" set

13d9760829 test: load wallet, coverage for crypted keys (furszy)
373c99633e refactor: move DuplicateMockDatabase to wallet/test/util.h (furszy)
ee7a984f85 refactor: unify test/util/wallet.h with wallet/test/util.h (furszy)
cc5a5e8121 wallet: bugfix, invalid crypted key "checksum_valid" set (furszy)

Pull request description:

  At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed.

  Note:
  The first commit fixes the issue, the two commits in the middle are cleanups so `DuplicateMockDatabase`
  can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading
  process.

  Includes test coverage for the following scenarios:

  1) "All ckeys checksums valid" test:
  Loads an encrypted wallet with all the crypted keys with a valid checksum and
  verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.

      (we force a complete ckeys re-write if we find any missing crypted key checksum
  during the wallet loading process)

  2) "Missing checksum in one ckey" test:
  Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
  triggers a complete re-write of the crypted keys.

  3) "Invalid ckey checksum error" test:
  Verifies that loading up a ckey with an invalid checksum stops the wallet loading
  process with a corruption error.

  4) "Invalid ckey pubkey error" test:
  Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
  process with a corruption error.

ACKs for top commit:
  achow101:
    ACK 13d9760829
  aureleoules:
    ACK 13d9760829

Tree-SHA512: 9ea630ee4a355282fbeee61ca04737294382577bb4b2631f50e732568fdab8f72491930807fbda58206446c4f26200cdc34d8afa14dbe1241aec713887d06a0b

src/Makefile.test.include

@@ -199,8 +199,6 @@ FUZZ_WALLET_SRC += \
 endif # USE_SQLITE
 
 BITCOIN_TEST_SUITE += \
-  wallet/test/util.cpp \
-  wallet/test/util.h \
   wallet/test/wallet_test_fixture.cpp \
   wallet/test/wallet_test_fixture.h \
   wallet/test/init_test_fixture.cpp \

src/Makefile.test_util.include

@@ -18,8 +18,11 @@ TEST_UTIL_H = \
   test/util/str.h \
   test/util/transaction_utils.h \
   test/util/txmempool.h \
-  test/util/validation.h \
-  test/util/wallet.h
+  test/util/validation.h
+
+if ENABLE_WALLET
+TEST_UTIL_H += wallet/test/util.h
+endif # ENABLE_WALLET
 
 libtest_util_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
 libtest_util_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
@@ -33,6 +36,10 @@ libtest_util_a_SOURCES = \
   test/util/str.cpp \
   test/util/transaction_utils.cpp \
   test/util/txmempool.cpp \
-  test/util/validation.cpp \
-  test/util/wallet.cpp \
-  $(TEST_UTIL_H)
+  test/util/validation.cpp
+
+if ENABLE_WALLET
+libtest_util_a_SOURCES += wallet/test/util.cpp
+endif # ENABLE_WALLET
+
+libtest_util_a_SOURCES += $(TEST_UTIL_H)

src/bench/block_assemble.cpp

@@ -8,7 +8,6 @@
 #include <test/util/mining.h>
 #include <test/util/script.h>
 #include <test/util/setup_common.h>
-#include <test/util/wallet.h>
 #include <txmempool.h>
 #include <validation.h>
 

src/bench/wallet_balance.cpp

@@ -7,7 +7,7 @@
 #include <node/context.h>
 #include <test/util/mining.h>
 #include <test/util/setup_common.h>
-#include <test/util/wallet.h>
+#include <wallet/test/util.h>
 #include <validationinterface.h>
 #include <wallet/receive.h>
 #include <wallet/wallet.h>
@@ -20,6 +20,8 @@ using wallet::DBErrors;
 using wallet::GetBalance;
 using wallet::WALLET_FLAG_DESCRIPTORS;
 
+const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj";
+
 static void WalletBalance(benchmark::Bench& bench, const bool set_dirty, const bool add_mine)
 {
     const auto test_setup = MakeNoLogFileContext<const TestingSetup>();

src/bench/wallet_create_tx.cpp

@@ -9,9 +9,9 @@
 #include <kernel/chain.h>
 #include <node/context.h>
 #include <test/util/setup_common.h>
-#include <test/util/wallet.h>
 #include <validation.h>
 #include <wallet/spend.h>
+#include <wallet/test/util.h>
 #include <wallet/wallet.h>
 
 using wallet::CWallet;

src/bench/wallet_loading.cpp

@@ -7,7 +7,7 @@
 #include <node/context.h>
 #include <test/util/mining.h>
 #include <test/util/setup_common.h>
-#include <test/util/wallet.h>
+#include <wallet/test/util.h>
 #include <util/translation.h>
 #include <validationinterface.h>
 #include <wallet/context.h>
@@ -52,30 +52,6 @@ static void AddTx(CWallet& wallet)
     wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInactive{});
 }
 
-static std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database, DatabaseOptions& options)
-{
-    auto new_database = CreateMockWalletDatabase(options);
-
-    // Get a cursor to the original database
-    auto batch = database.MakeBatch();
-    batch->StartCursor();
-
-    // Get a batch for the new database
-    auto new_batch = new_database->MakeBatch();
-
-    // Read all records from the original database and write them to the new one
-    while (true) {
-        CDataStream key(SER_DISK, CLIENT_VERSION);
-        CDataStream value(SER_DISK, CLIENT_VERSION);
-        bool complete;
-        batch->ReadAtCursor(key, value, complete);
-        if (complete) break;
-        new_batch->Write(key, value);
-    }
-
-    return new_database;
-}
-
 static void WalletLoading(benchmark::Bench& bench, bool legacy_wallet)
 {
     const auto test_setup = MakeNoLogFileContext<TestingSetup>();

src/test/util/wallet.cpp

@@ -1,32 +0,0 @@
-// Copyright (c) 2019-2021 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#include <test/util/wallet.h>
-
-#include <key_io.h>
-#include <outputtype.h>
-#include <script/standard.h>
-#ifdef ENABLE_WALLET
-#include <util/check.h>
-#include <util/translation.h>
-#include <wallet/wallet.h>
-#endif
-
-using wallet::CWallet;
-
-const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj";
-
-#ifdef ENABLE_WALLET
-std::string getnewaddress(CWallet& w)
-{
-    constexpr auto output_type = OutputType::BECH32;
-    return EncodeDestination(getNewDestination(w, output_type));
-}
-
-CTxDestination getNewDestination(CWallet& w, OutputType output_type)
-{
-    return *Assert(w.GetNewDestination(output_type, ""));
-}
-
-#endif // ENABLE_WALLET

src/test/util/wallet.h

@@ -1,29 +0,0 @@
-// Copyright (c) 2019 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#ifndef BITCOIN_TEST_UTIL_WALLET_H
-#define BITCOIN_TEST_UTIL_WALLET_H
-
-#include <outputtype.h>
-#include <string>
-
-namespace wallet {
-class CWallet;
-} // namespace wallet
-
-// Constants //
-
-extern const std::string ADDRESS_BCRT1_UNSPENDABLE;
-
-// RPC-like //
-
-/** Import the address to the wallet */
-void importaddress(wallet::CWallet& wallet, const std::string& address);
-/** Returns a new encoded destination from the wallet (hardcoded to BECH32) */
-std::string getnewaddress(wallet::CWallet& w);
-/** Returns a new destination, of an specific type, from the wallet */
-CTxDestination getNewDestination(wallet::CWallet& w, OutputType output_type);
-
-
-#endif // BITCOIN_TEST_UTIL_WALLET_H

src/wallet/test/util.cpp

@@ -11,8 +11,6 @@
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
 
-#include <boost/test/unit_test.hpp>
-
 #include <memory>
 
 namespace wallet {
@@ -39,10 +37,46 @@ std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cc
     WalletRescanReserver reserver(*wallet);
     reserver.reserve();
     CWallet::ScanResult result = wallet->ScanForWalletTransactions(cchain.Genesis()->GetBlockHash(), /*start_height=*/0, /*max_height=*/{}, reserver, /*fUpdate=*/false, /*save_progress=*/false);
-    BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
-    BOOST_CHECK_EQUAL(result.last_scanned_block, cchain.Tip()->GetBlockHash());
-    BOOST_CHECK_EQUAL(*result.last_scanned_height, cchain.Height());
-    BOOST_CHECK(result.last_failed_block.IsNull());
+    assert(result.status == CWallet::ScanResult::SUCCESS);
+    assert(result.last_scanned_block == cchain.Tip()->GetBlockHash());
+    assert(*result.last_scanned_height == cchain.Height());
+    assert(result.last_failed_block.IsNull());
     return wallet;
 }
+
+std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database, DatabaseOptions& options)
+{
+    auto new_database = CreateMockWalletDatabase(options);
+
+    // Get a cursor to the original database
+    auto batch = database.MakeBatch();
+    batch->StartCursor();
+
+    // Get a batch for the new database
+    auto new_batch = new_database->MakeBatch();
+
+    // Read all records from the original database and write them to the new one
+    while (true) {
+        CDataStream key(SER_DISK, CLIENT_VERSION);
+        CDataStream value(SER_DISK, CLIENT_VERSION);
+        bool complete;
+        batch->ReadAtCursor(key, value, complete);
+        if (complete) break;
+        new_batch->Write(key, value);
+    }
+
+    return new_database;
+}
+
+std::string getnewaddress(CWallet& w)
+{
+    constexpr auto output_type = OutputType::BECH32;
+    return EncodeDestination(getNewDestination(w, output_type));
+}
+
+CTxDestination getNewDestination(CWallet& w, OutputType output_type)
+{
+    return *Assert(w.GetNewDestination(output_type, ""));
+}
+
 } // namespace wallet

src/wallet/test/util.h

@@ -5,19 +5,32 @@
 #ifndef BITCOIN_WALLET_TEST_UTIL_H
 #define BITCOIN_WALLET_TEST_UTIL_H
 
+#include <script/standard.h>
 #include <memory>
 
 class ArgsManager;
 class CChain;
 class CKey;
+enum class OutputType;
 namespace interfaces {
 class Chain;
 } // namespace interfaces
 
 namespace wallet {
 class CWallet;
+struct DatabaseOptions;
+class WalletDatabase;
 
 std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, ArgsManager& args, const CKey& key);
+
+// Creates a copy of the provided database
+std::unique_ptr<WalletDatabase> DuplicateMockDatabase(WalletDatabase& database, DatabaseOptions& options);
+
+/** Returns a new encoded destination from the wallet (hardcoded to BECH32) */
+std::string getnewaddress(CWallet& w);
+/** Returns a new destination, of an specific type, from the wallet */
+CTxDestination getNewDestination(CWallet& w, OutputType output_type);
+
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_TEST_UTIL_H

src/wallet/test/walletload_tests.cpp

@@ -2,6 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or https://www.opensource.org/licenses/mit-license.php.
 
+#include <wallet/test/util.h>
 #include <wallet/wallet.h>
 #include <test/util/setup_common.h>
 
@@ -50,5 +51,139 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup)
     }
 }
 
+bool HasAnyRecordOfType(WalletDatabase& db, const std::string& key)
+{
+    std::unique_ptr<DatabaseBatch> batch = db.MakeBatch(false);
+    BOOST_CHECK(batch->StartCursor());
+    while (true) {
+        CDataStream ssKey(SER_DISK, CLIENT_VERSION);
+        CDataStream ssValue(SER_DISK, CLIENT_VERSION);
+        bool complete;
+        BOOST_CHECK(batch->ReadAtCursor(ssKey, ssValue, complete));
+        if (complete) break;
+        std::string type;
+        ssKey >> type;
+        if (type == key) return true;
+    }
+    return false;
+}
+
+BOOST_FIXTURE_TEST_CASE(wallet_load_verif_crypted_key_checksum, TestingSetup)
+{
+    // The test duplicates the db so each case has its own db instance.
+    int NUMBER_OF_TESTS = 4;
+    std::vector<std::unique_ptr<WalletDatabase>> dbs;
+    CKey first_key;
+    auto get_db = [](std::vector<std::unique_ptr<WalletDatabase>>& dbs) {
+        std::unique_ptr<WalletDatabase> db = std::move(dbs.back());
+        dbs.pop_back();
+        return db;
+    };
+
+    {   // Context setup.
+        // Create and encrypt legacy wallet
+        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", m_args, CreateMockWalletDatabase()));
+        LOCK(wallet->cs_wallet);
+        auto legacy_spkm = wallet->GetOrCreateLegacyScriptPubKeyMan();
+        BOOST_CHECK(legacy_spkm->SetupGeneration(true));
+
+        // Get the first key in the wallet
+        CTxDestination dest = *Assert(legacy_spkm->GetNewDestination(OutputType::LEGACY));
+        CKeyID key_id = GetKeyForDestination(*legacy_spkm, dest);
+        BOOST_CHECK(legacy_spkm->GetKey(key_id, first_key));
+
+        // Encrypt the wallet and duplicate database
+        BOOST_CHECK(wallet->EncryptWallet("encrypt"));
+        wallet->Flush();
+
+        DatabaseOptions options;
+        for (int i=0; i < NUMBER_OF_TESTS; i++) {
+            dbs.emplace_back(DuplicateMockDatabase(wallet->GetDatabase(), options));
+        }
+    }
+
+    {
+        // First test case:
+        // Erase all the crypted keys from db and unlock the wallet.
+        // The wallet will only re-write the crypted keys to db if any checksum is missing at load time.
+        // So, if any 'ckey' record re-appears on db, then the checksums were not properly calculated, and we are re-writing
+        // the records every time that 'CWallet::Unlock' gets called, which is not good.
+
+        // Load the wallet and check that is encrypted
+        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", m_args, get_db(dbs)));
+        BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::LOAD_OK);
+        BOOST_CHECK(wallet->IsCrypted());
+        BOOST_CHECK(HasAnyRecordOfType(wallet->GetDatabase(), DBKeys::CRYPTED_KEY));
+
+        // Now delete all records and check that the 'Unlock' function doesn't re-write them
+        BOOST_CHECK(wallet->GetLegacyScriptPubKeyMan()->DeleteRecords());
+        BOOST_CHECK(!HasAnyRecordOfType(wallet->GetDatabase(), DBKeys::CRYPTED_KEY));
+        BOOST_CHECK(wallet->Unlock("encrypt"));
+        BOOST_CHECK(!HasAnyRecordOfType(wallet->GetDatabase(), DBKeys::CRYPTED_KEY));
+    }
+
+    {
+        // Second test case:
+        // Verify that loading up a 'ckey' with no checksum triggers a complete re-write of the crypted keys.
+        std::unique_ptr<WalletDatabase> db = get_db(dbs);
+        {
+            std::unique_ptr<DatabaseBatch> batch = db->MakeBatch(false);
+            std::pair<std::vector<unsigned char>, uint256> value;
+            BOOST_CHECK(batch->Read(std::make_pair(DBKeys::CRYPTED_KEY, first_key.GetPubKey()), value));
+
+            const auto key = std::make_pair(DBKeys::CRYPTED_KEY, first_key.GetPubKey());
+            BOOST_CHECK(batch->Write(key, value.first, /*fOverwrite=*/true));
+        }
+
+        // Load the wallet and check that is encrypted
+        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", m_args, std::move(db)));
+        BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::LOAD_OK);
+        BOOST_CHECK(wallet->IsCrypted());
+        BOOST_CHECK(HasAnyRecordOfType(wallet->GetDatabase(), DBKeys::CRYPTED_KEY));
+
+        // Now delete all ckey records and check that the 'Unlock' function re-writes them
+        // (this is because the wallet, at load time, found a ckey record with no checksum)
+        BOOST_CHECK(wallet->GetLegacyScriptPubKeyMan()->DeleteRecords());
+        BOOST_CHECK(!HasAnyRecordOfType(wallet->GetDatabase(), DBKeys::CRYPTED_KEY));
+        BOOST_CHECK(wallet->Unlock("encrypt"));
+        BOOST_CHECK(HasAnyRecordOfType(wallet->GetDatabase(), DBKeys::CRYPTED_KEY));
+    }
+
+    {
+        // Third test case:
+        // Verify that loading up a 'ckey' with an invalid checksum throws an error.
+        std::unique_ptr<WalletDatabase> db = get_db(dbs);
+        {
+            std::unique_ptr<DatabaseBatch> batch = db->MakeBatch(false);
+            std::vector<unsigned char> crypted_data;
+            BOOST_CHECK(batch->Read(std::make_pair(DBKeys::CRYPTED_KEY, first_key.GetPubKey()), crypted_data));
+
+            // Write an invalid checksum
+            std::pair<std::vector<unsigned char>, uint256> value = std::make_pair(crypted_data, uint256::ONE);
+            const auto key = std::make_pair(DBKeys::CRYPTED_KEY, first_key.GetPubKey());
+            BOOST_CHECK(batch->Write(key, value, /*fOverwrite=*/true));
+        }
+
+        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", m_args, std::move(db)));
+        BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::CORRUPT);
+    }
+
+    {
+        // Fourth test case:
+        // Verify that loading up a 'ckey' with an invalid pubkey throws an error
+        std::unique_ptr<WalletDatabase> db = get_db(dbs);
+        {
+            CPubKey invalid_key;
+            BOOST_ASSERT(!invalid_key.IsValid());
+            const auto key = std::make_pair(DBKeys::CRYPTED_KEY, invalid_key);
+            std::pair<std::vector<unsigned char>, uint256> value;
+            BOOST_CHECK(db->MakeBatch(false)->Write(key, value, /*fOverwrite=*/true));
+        }
+
+        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", m_args, std::move(db)));
+        BOOST_CHECK_EQUAL(wallet->LoadWallet(), DBErrors::CORRUPT);
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet

src/wallet/walletdb.cpp

@@ -482,7 +482,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
             if (!ssValue.eof()) {
                 uint256 checksum;
                 ssValue >> checksum;
-                if ((checksum_valid = Hash(vchPrivKey) != checksum)) {
+                if (!(checksum_valid = Hash(vchPrivKey) == checksum)) {
                     strErr = "Error reading wallet database: Encrypted key corrupt";
                     return false;
                 }