Merge bitcoin/bitcoin#24914: wallet: Load database records in a particular order

3c83b1d884 doc: Add release note for wallet loading changes (Andrew Chow)
2636844f53 walletdb: Remove loading code where the database is iterated (Andrew Chow)
cd211b3b99 walletdb: refactor decryption key loading (Andrew Chow)
31c033e5ca walletdb: refactor defaultkey and wkey loading (Andrew Chow)
c978c6d39c walletdb: refactor active spkm loading (Andrew Chow)
6fabb7fc99 walletdb: refactor tx loading (Andrew Chow)
abcc13dd24 walletdb: refactor address book loading (Andrew Chow)
405b4d9147 walletdb: Refactor descriptor wallet records loading (Andrew Chow)
30ab11c497 walletdb: Refactor legacy wallet record loading into its own function (Andrew Chow)
9e077d9b42 salvage: Remove use of ReadKeyValue in salvage (Andrew Chow)
ad779e9ece walletdb: Refactor hd chain loading to its own function (Andrew Chow)
72c2a54ebb walletdb: Refactor encryption key loading to its own function (Andrew Chow)
3ccde4599b walletdb: Refactor crypted key loading to its own function (Andrew Chow)
7be10adff3 walletdb: Refactor key reading and loading to its own function (Andrew Chow)
52932c5adb walletdb: Refactor wallet flags loading (Andrew Chow)
01b35b55a1 walletdb: Refactor minversion loading (Andrew Chow)

Pull request description:

  Currently when we load a wallet, we just iterate through all of the records in the database and add them completely statelessly. However we have some records which do rely on other records being loaded before they are. To deal with this, we use `CWalletScanState` to hold things temporarily until all of the records have been read and then we load the stateful things.

  However this can be slow, and with some future improvements, can cause some pretty drastic slowdowns to retain this pattern. So this PR changes the way we load records by choosing to load the records in a particular order. This lets us do things such as loading a descriptor record, then finding and loading that descriptor's cache and key records. In the future, this will also let us use `IsMine` when loading transactions as then `IsMine` will actually be working as we now always load keys and descriptors before transactions.

  In order to get records of a specific type, this PR includes some refactors to how we do database cursors. Functionality is also added to retrieve a cursor that will give us records beginning with a specified prefix.

  Lastly, one thing that iterating the entire database let us do was to find unknown records. However even if unknown records were found, we would not do anything with this information except output a number in a log line. With this PR, we would no longer be aware of any unknown records. This does not change functionality as we don't do anything with unknown records, and having unknown records is not an error. Now we would just not be aware that unknown records even exist.

ACKs for top commit:
  MarcoFalke:
    re-ACK 3c83b1d884 🍤
  furszy:
    reACK 3c83b1d8
  ryanofsky:
    Code review ACK 3c83b1d884. Just Marco's suggested error handling fixes since last review

Tree-SHA512: 15fa56332fb2ce4371db468a0c674ee7a3a8889c8cee9f428d06a7d1385d17a9bf54bcb0ba885c87736841fe6a5c934594bcf4476a473616510ee47862ef30b4

doc/release-notes-24914.md

@@ -0,0 +1,9 @@
+Wallet
+------
+
+- Wallet loading has changed in this release. Wallets with some corrupted records that could be
+  previously loaded (with warnings) may no longer load. For example, wallets with corrupted
+  address book entries may no longer load. If this happens, it is recommended
+  load the wallet in a previous version of Bitcoin Core and import the data into a new wallet.
+  Please also report an issue to help improve the software and make wallet loading more robust
+  in these cases.

src/wallet/salvage.cpp

@@ -18,11 +18,6 @@ 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;
-}
-
 class DummyCursor : public DatabaseCursor
 {
     Status Next(DataStream& key, DataStream& value) override { return Status::FAIL; }
@@ -186,17 +181,24 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil
     {
         /* Filter for only private key type KV pairs to be added to the salvaged wallet */
         DataStream ssKey{row.first};
-        CDataStream ssValue(row.second, SER_DISK, CLIENT_VERSION);
+        DataStream ssValue(row.second);
         std::string strType, strErr;
-        bool fReadOK;
-        {
-            // Required in LoadKeyMetadata():
-            LOCK(dummyWallet.cs_wallet);
-            fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue, strType, strErr, KeyFilter);
-        }
-        if (!KeyFilter(strType)) {
+
+        // We only care about KEY, MASTER_KEY, CRYPTED_KEY, and HDCHAIN types
+        ssKey >> strType;
+        bool fReadOK = false;
+        if (strType == DBKeys::KEY) {
+            fReadOK = LoadKey(&dummyWallet, ssKey, ssValue, strErr);
+        } else if (strType == DBKeys::CRYPTED_KEY) {
+            fReadOK = LoadCryptedKey(&dummyWallet, ssKey, ssValue, strErr);
+        } else if (strType == DBKeys::MASTER_KEY) {
+            fReadOK = LoadEncryptionKey(&dummyWallet, ssKey, ssValue, strErr);
+        } else if (strType == DBKeys::HDCHAIN) {
+            fReadOK = LoadHDChain(&dummyWallet, ssValue, strErr);
+        } else {
             continue;
         }
+
         if (!fReadOK)
         {
             warnings.push_back(strprintf(Untranslated("WARNING: WalletBatch::Recover skipping %s: %s"), strType, strErr));

src/wallet/wallet.cpp

@@ -2929,7 +2929,7 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
         else if (nLoadWalletRet == DBErrors::NONCRITICAL_ERROR)
         {
             warnings.push_back(strprintf(_("Error reading %s! All keys read correctly, but transaction data"
-                                           " or address book entries might be missing or incorrect."),
+                                           " or address metadata may be missing or incorrect."),
                 walletFile));
         }
         else if (nLoadWalletRet == DBErrors::TOO_NEW) {

src/wallet/walletdb.h

@@ -42,19 +42,21 @@ struct WalletContext;
 
 static const bool DEFAULT_FLUSHWALLET = true;
 
-/** Error statuses for the wallet database */
-enum class DBErrors
+/** Error statuses for the wallet database.
+ * Values are in order of severity. When multiple errors occur, the most severe (highest value) will be returned.
+ */
+enum class DBErrors : int
 {
-    LOAD_OK,
-    CORRUPT,
-    NONCRITICAL_ERROR,
-    TOO_NEW,
-    EXTERNAL_SIGNER_SUPPORT_REQUIRED,
-    LOAD_FAIL,
-    NEED_REWRITE,
-    NEED_RESCAN,
-    UNKNOWN_DESCRIPTOR,
-    UNEXPECTED_LEGACY_ENTRY
+    LOAD_OK = 0,
+    NEED_RESCAN = 1,
+    NEED_REWRITE = 2,
+    EXTERNAL_SIGNER_SUPPORT_REQUIRED = 3,
+    NONCRITICAL_ERROR = 4,
+    TOO_NEW = 5,
+    UNKNOWN_DESCRIPTOR = 6,
+    LOAD_FAIL = 7,
+    UNEXPECTED_LEGACY_ENTRY = 8,
+    CORRUPT = 9,
 };
 
 namespace DBKeys {
@@ -276,8 +278,6 @@ public:
     DBErrors LoadWallet(CWallet* pwallet);
     DBErrors FindWalletTxHashes(std::vector<uint256>& tx_hashes);
     DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut);
-    /* Function to determine if a certain KV/key-type is a key (cryptographical key) type */
-    static bool IsKeyType(const std::string& strType);
 
     //! write the hdchain model (external chain child index counter)
     bool WriteHDChain(const CHDChain& chain);
@@ -300,11 +300,10 @@ private:
 //! Compacts BDB state so that wallet.dat is self-contained (if there are changes)
 void MaybeCompactWalletDB(WalletContext& context);
 
-//! 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, DataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr, const KeyFilterFn& filter_fn = nullptr);
+bool LoadKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::string& strErr);
+bool LoadCryptedKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::string& strErr);
+bool LoadEncryptionKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::string& strErr);
+bool LoadHDChain(CWallet* pwallet, DataStream& ssValue, std::string& strErr);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_WALLETDB_H