Merge #19457: wallet: Cleanup wallettool salvage and walletdb extraneous declarations

0e279fe walletdb: Remove unused static functions from walletdb.h (Andrew Chow)
9f536d4 wallettool: Have RecoverDatabaseFile return errors and warnings (Andrew Chow)
06e263a Call RecoverDatabaseFile directly from wallettool (Andrew Chow)

Pull request description:

  Followup to #19324 addressing some comments.

  Removes the `SalvageWallet` function in wallettool and instead directly calls `RecoverDatabaseFile` as suggested in bitcoin/bitcoin#19324 (comment)

  Removes the `LogPrintf`s and `tfm::format`s in `RecoverDatabaseFile` as noted in bitcoin/bitcoin#19324 (comment)

  Removes the declarations of `VerifyEnvironment` and `VerifyDatabaseFile` that were forgotten in `walletdb.h` as noted in bitcoin/bitcoin#19324 (comment)

ACKs for top commit:
  meshcollider:
    Code review ACK 0e279fe
  ryanofsky:
    Code review ACK 0e279fe, just dropped last commit

Tree-SHA512: ffd01f30536c2eab4bf40ba363c3ea916ecef3c8f0c5262040b40498776ffb00f95240204a40e38415d6931800851d0a3fa63ee91efc1d329b60ac317da0363d

Author: MarcoFalke

src/wallet/salvage.cpp

@@ -16,14 +16,12 @@ 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;
 
-bool RecoverDatabaseFile(const fs::path& file_path)
+bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
     std::string filename;
     std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
 
-    bilingual_str open_err;
-    if (!env->Open(open_err)) {
-        tfm::format(std::cerr, "%s\n", open_err.original);
+    if (!env->Open(error)) {
         return false;
     }
 
@@ -39,11 +37,9 @@ bool RecoverDatabaseFile(const fs::path& file_path)
 
     int result = env->dbenv->dbrename(nullptr, filename.c_str(), nullptr,
                                        newFilename.c_str(), DB_AUTO_COMMIT);
-    if (result == 0)
-        LogPrintf("Renamed %s to %s\n", filename, newFilename);
-    else
+    if (result != 0)
     {
-        LogPrintf("Failed to rename %s to %s\n", filename, newFilename);
+        error = strprintf(Untranslated("Failed to rename %s to %s"), filename, newFilename);
         return false;
     }
 
@@ -60,10 +56,10 @@ bool RecoverDatabaseFile(const fs::path& file_path)
     Db db(env->dbenv.get(), 0);
     result = db.verify(newFilename.c_str(), nullptr, &strDump, DB_SALVAGE | DB_AGGRESSIVE);
     if (result == DB_VERIFY_BAD) {
-        LogPrintf("Salvage: Database salvage found errors, all data may not be recoverable.\n");
+        warnings.push_back(Untranslated("Salvage: Database salvage found errors, all data may not be recoverable."));
     }
     if (result != 0 && result != DB_VERIFY_BAD) {
-        LogPrintf("Salvage: Database salvage failed with result %d.\n", result);
+        error = strprintf(Untranslated("Salvage: Database salvage failed with result %d."), result);
         return false;
     }
 
@@ -87,7 +83,7 @@ bool RecoverDatabaseFile(const fs::path& file_path)
                 break;
             getline(strDump, valueHex);
             if (valueHex == DATA_END) {
-                LogPrintf("Salvage: WARNING: Number of keys in data does not match number of values.\n");
+                warnings.push_back(Untranslated("Salvage: WARNING: Number of keys in data does not match number of values."));
                 break;
             }
             salvagedData.push_back(make_pair(ParseHex(keyHex), ParseHex(valueHex)));
@@ -96,18 +92,17 @@ bool RecoverDatabaseFile(const fs::path& file_path)
 
     bool fSuccess;
     if (keyHex != DATA_END) {
-        LogPrintf("Salvage: WARNING: Unexpected end of file while reading salvage output.\n");
+        warnings.push_back(Untranslated("Salvage: WARNING: Unexpected end of file while reading salvage output."));
         fSuccess = false;
     } else {
         fSuccess = (result == 0);
     }
 
     if (salvagedData.empty())
     {
-        LogPrintf("Salvage(aggressive) found no records in %s.\n", newFilename);
+        error = strprintf(Untranslated("Salvage(aggressive) found no records in %s."), newFilename);
         return false;
     }
-    LogPrintf("Salvage(aggressive) found %u records\n", salvagedData.size());
 
     std::unique_ptr<Db> pdbCopy = MakeUnique<Db>(env->dbenv.get(), 0);
     int ret = pdbCopy->open(nullptr,               // Txn pointer
@@ -117,7 +112,7 @@ bool RecoverDatabaseFile(const fs::path& file_path)
                             DB_CREATE,          // Flags
                             0);
     if (ret > 0) {
-        LogPrintf("Cannot create database file %s\n", filename);
+        error = strprintf(Untranslated("Cannot create database file %s"), filename);
         pdbCopy->close(0);
         return false;
     }
@@ -141,7 +136,7 @@ bool RecoverDatabaseFile(const fs::path& file_path)
         }
         if (!fReadOK)
         {
-            LogPrintf("WARNING: WalletBatch::Recover skipping %s: %s\n", strType, strErr);
+            warnings.push_back(strprintf(Untranslated("WARNING: WalletBatch::Recover skipping %s: %s"), strType, strErr));
             continue;
         }
         Dbt datKey(&row.first[0], row.first.size());

src/wallet/salvage.h

@@ -9,6 +9,8 @@
 #include <fs.h>
 #include <streams.h>
 
-bool RecoverDatabaseFile(const fs::path& file_path);
+struct bilingual_str;
+
+bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::vector<bilingual_str>& warnings);
 
 #endif // BITCOIN_WALLET_SALVAGE_H

src/wallet/walletdb.h

@@ -261,10 +261,6 @@ class WalletBatch
     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);
-    /* verifies the database environment */
-    static bool VerifyEnvironment(const fs::path& wallet_path, bilingual_str& errorStr);
-    /* verifies the database file */
-    static bool VerifyDatabaseFile(const fs::path& wallet_path, bilingual_str& errorStr);
 
     //! write the hdchain model (external chain child index counter)
     bool WriteHDChain(const CHDChain& chain);

src/wallet/wallettool.cpp

@@ -104,27 +104,6 @@ static void WalletShowInfo(CWallet* wallet_instance)
     tfm::format(std::cout, "Address Book: %zu\n", wallet_instance->m_address_book.size());
 }
 
-static bool SalvageWallet(const fs::path& path)
-{
-    // Create a Database handle to allow for the db to be initialized before recovery
-    std::unique_ptr<WalletDatabase> database = CreateWalletDatabase(path);
-
-    // Initialize the environment before recovery
-    bilingual_str error_string;
-    try {
-        database->Verify(error_string);
-    } catch (const fs::filesystem_error& e) {
-        error_string = Untranslated(strprintf("Error loading wallet. %s", fsbridge::get_filesystem_error_message(e)));
-    }
-    if (!error_string.original.empty()) {
-        tfm::format(std::cerr, "Failed to open wallet for salvage :%s\n", error_string.original);
-        return false;
-    }
-
-    // Perform the recovery
-    return RecoverDatabaseFile(path);
-}
-
 bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
 {
     fs::path path = fs::absolute(name, GetWalletDir());
@@ -147,7 +126,18 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
             WalletShowInfo(wallet_instance.get());
             wallet_instance->Close();
         } else if (command == "salvage") {
-            return SalvageWallet(path);
+            bilingual_str error;
+            std::vector<bilingual_str> warnings;
+            bool ret = RecoverDatabaseFile(path, error, warnings);
+            if (!ret) {
+                for (const auto warning : warnings) {
+                    tfm::format(std::cerr, "%s\n", warning.original);
+                }
+                if (!error.empty()) {
+                    tfm::format(std::cerr, "%s\n", error.original);
+                }
+            }
+            return ret;
         }
     } else {
         tfm::format(std::cerr, "Invalid command: %s\n", command);