Merge #19805: wallet: Avoid deserializing unused records when salvaging

0bbe26a1af2aab2287b18048f80b3f70e63e0044 wallet: filter for keys only before record deser in salvage (Andrew Chow)
544e12a4e81633d222574eec253a1ff292d3c4a5 walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow)

Pull request description:

  When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too.

  This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire.

ACKs for top commit:
  ryanofsky:
    Code review ACK 0bbe26a1af2aab2287b18048f80b3f70e63e0044. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in #19078, so I think we're covered
  laanwj:
    Code review ACK 0bbe26a1af2aab2287b18048f80b3f70e63e0044

Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01

3 files changed, 18 insertions, 6 deletions

diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp
index c0755db751..934e3d5c86 100644
--- a/src/wallet/salvage.cpp
+++ b/src/wallet/salvage.cpp
@@ -16,6 +16,11 @@ static const char *HEADER_END = "HEADER=END";
 static const char *DATA_END = "DATA=END";
 typedef std::pair<std::vector<unsigned char>, std::vector<unsigned char> > KeyValPair;
 
+static bool KeyFilter(const std::string& type)
+{
+    return WalletBatch::IsKeyType(type) || type == DBKeys::HDCHAIN;
+}
+
 bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
     std::string filename;
@@ -129,9 +134,9 @@ bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::v
         {
             // Required in LoadKeyMetadata():
             LOCK(dummyWallet.cs_wallet);
-            fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue, strType, strErr);
+            fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue, strType, strErr, KeyFilter);
         }
-        if (!WalletBatch::IsKeyType(strType) && strType != DBKeys::HDCHAIN) {
+        if (!KeyFilter(strType)) {
             continue;
         }
         if (!fReadOK)
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index f25acee1e9..962ea66fa0 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -263,13 +263,17 @@ public:
 
 static bool
 ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
-             CWalletScanState &wss, std::string& strType, std::string& strErr) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
+             CWalletScanState &wss, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn = nullptr) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
 {
     try {
         // Unserialize
         // Taking advantage of the fact that pair serialization
         // is just the two items serialized one after the other
         ssKey >> strType;
+        // If we have a filter, check if this matches the filter
+        if (filter_fn && !filter_fn(strType)) {
+            return true;
+        }
         if (strType == DBKeys::NAME) {
             std::string strAddress;
             ssKey >> strAddress;
@@ -668,11 +672,11 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
     return true;
 }
 
-bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr)
+bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn)
 {
     CWalletScanState dummy_wss;
     LOCK(pwallet->cs_wallet);
-    return ReadKeyValue(pwallet, ssKey, ssValue, dummy_wss, strType, strErr);
+    return ReadKeyValue(pwallet, ssKey, ssValue, dummy_wss, strType, strErr, filter_fn);
 }
 
 bool WalletBatch::IsKeyType(const std::string& strType)
diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h
index c13de01319..2548c17508 100644
--- a/src/wallet/walletdb.h
+++ b/src/wallet/walletdb.h
@@ -279,8 +279,11 @@ private:
 //! Compacts BDB state so that wallet.dat is self-contained (if there are changes)
 void MaybeCompactWalletDB();
 
+//! Callback for filtering key types to deserialize in ReadKeyValue
+using KeyFilterFn = std::function<bool(const std::string&)>;
+
 //! Unserialize a given Key-Value pair and load it into the wallet
-bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr);
+bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn = nullptr);
 
 /** Return whether a wallet database is currently loaded. */
 bool IsWalletLoaded(const fs::path& wallet_path);