Merge bitcoin/bitcoin#29117: wallettool: Always be able to dump a wallet's database

d83bea4 wallettool: Don't create CWallet when dumping DB (Andrew Chow)
40c80e3 wallettool: Don't unilaterally reset wallet_instance if loading error (Ava Chow)

Pull request description:

  bitcoin/bitcoin#29109 (comment) reports that a wallet with noncritical errors cannot be dumped with `bitcoin-wallet dump`. This was caused by an erroneous reset of the wallet pointer when the loading the wallet returns something other than `LOAD_OK`. Not all errors are errors that require aborting, so unilaterally resetting the pointer at that time is incorrect. The first commit resolves this issue.

  Furthermore, if a wallet has loading errors, that should not prevent the wallet tool from dumping the wallet. The wallet application logic should not get in the way of performing such a low level database operation, especially when it's primary usage is for debugging potentially corrupted wallets. The 2nd commit is taken from #28710 and changes the `dump` to stop at making a `WalletDatabase` rather than making a `CWallet` only to retrieve the underlying `WalletDatabase`.

ACKs for top commit:
  furszy:
    Code review ACK d83bea4
  BrandonOdiwuor:
    Code Review ACK d83bea4

Tree-SHA512: 425d712dfff1002bd81272aca0bae1016f9126a3c89506f8cb7cf0a0ec9f33d0c03b8d03896394f3a45c2998e59047e19218dfd08dc8a5f40e8625134e886b0f

Author: fanquake

src/wallet/dump.cpp

@@ -8,6 +8,7 @@
 #include <util/fs.h>
 #include <util/translation.h>
 #include <wallet/wallet.h>
+#include <wallet/walletdb.h>
 
 #include <algorithm>
 #include <fstream>
@@ -20,7 +21,7 @@ namespace wallet {
 static const std::string DUMP_MAGIC = "BITCOIN_CORE_WALLET_DUMP";
 uint32_t DUMP_VERSION = 1;
 
-bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
+bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error)
 {
     // Get the dumpfile
     std::string dump_filename = args.GetArg("-dumpfile", "");
@@ -44,7 +45,6 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
 
     HashWriter hasher{};
 
-    WalletDatabase& db = wallet.GetDatabase();
     std::unique_ptr<DatabaseBatch> batch = db.MakeBatch();
 
     bool ret = true;
@@ -90,9 +90,6 @@ bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error)
     cursor.reset();
     batch.reset();
 
-    // Close the wallet after we're done with it. The caller won't be doing this
-    wallet.Close();
-
     if (ret) {
         // Write the hash
         tfm::format(dump_file, "checksum,%s\n", HexStr(hasher.GetHash()));

src/wallet/dump.h

@@ -14,8 +14,9 @@ struct bilingual_str;
 class ArgsManager;
 
 namespace wallet {
-class CWallet;
-bool DumpWallet(const ArgsManager& args, CWallet& wallet, bilingual_str& error);
+class WalletDatabase;
+
+bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& error);
 bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs::path& wallet_path, bilingual_str& error, std::vector<bilingual_str>& warnings);
 } // namespace wallet
 

src/wallet/wallettool.cpp

@@ -68,7 +68,6 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa
     }
 
     if (load_wallet_ret != DBErrors::LOAD_OK) {
-        wallet_instance = nullptr;
         if (load_wallet_ret == DBErrors::CORRUPT) {
             tfm::format(std::cerr, "Error loading %s: Wallet corrupted", name);
             return nullptr;
@@ -194,10 +193,15 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
         DatabaseOptions options;
         ReadDatabaseArgs(args, options);
         options.require_existing = true;
-        const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options);
-        if (!wallet_instance) return false;
+        DatabaseStatus status;
         bilingual_str error;
-        bool ret = DumpWallet(args, *wallet_instance, error);
+        std::unique_ptr<WalletDatabase> database = MakeDatabase(path, options, status, error);
+        if (!database) {
+            tfm::format(std::cerr, "%s\n", error.original);
+            return false;
+        }
+
+        bool ret = DumpWallet(args, *database, error);
         if (!ret && !error.empty()) {
             tfm::format(std::cerr, "%s\n", error.original);
             return ret;