Merge #18918: wallet: Move salvagewallet into wallettool

84ae0578b6c68dda145ca65fef510ce0fdac0d7b Add release notes about salvage changes (Andrew Chow)
ea337f2d0318a860f695698cfb3aa91c03ded858 Move RecoverKeysOnlyFilter into RecoverDataBaseFile (Andrew Chow)
9ea2d258b46e8a9776100633585ed0feede5c2a4 Move RecoverDatabaseFile and RecoverKeysOnlyFilter into salvage.{cpp/h} (Andrew Chow)
b426c7764d26e280e1f814cf36e050743c45cd12 Make BerkeleyBatch::Recover and WalletBatch::RecoverKeysOnlyFilter standalone (Andrew Chow)
2741774214168eb287c7066d6823afe5e570381d Expose a version of ReadKeyValue and use it in RecoverKeysOnlyFilter (Andrew Chow)
ced95d0e43389fe62b5d30fcc7c42dbca0e88242 Move BerkeleyEnvironment::Salvage into BerkeleyBatch::Recover (Andrew Chow)
07250b8dcebe2b97ed0fd900ad35cba4091b8ecf walletdb: remove fAggressive from Salvage (Andrew Chow)
8ebcbc85c652665b78dcfd2ad55fa67cafd42c73 walletdb: don't automatically salvage when corruption is detected (Andrew Chow)
d321046f4bb4887742699c586755a21f3a2edbe1 wallet: remove -salvagewallet (Andrew Chow)
cdd955e580dff99f3fa440494ed2b348f7f094af Add basic test for bitcoin-wallet salvage (Andrew Chow)
c87770915b88d195d264b58111c64142b1965cfa wallettool: Add a salvage command (Andrew Chow)

Pull request description:

  Removes the `-salvagewallet` startup option and adds a `salvage` command to the `bitcoin-wallet` tool. As such, `-salvagewallet` is removed. Additionally, the automatic salvage that is done if the wallet file fails to load is removed.

  Lastly the salvage code entirely is moved out entirely into `bitcoin-wallet` from `walletdb.{cpp/h}` and `db.{cpp/h}`.

ACKs for top commit:
  jonatack:
    ACK 84ae0578b6c68dda145ca65fef510ce0fdac0d7b feedback taken, and compared to my previous review, the bitcoin-wallet salvage command now seems to run and it exits without raising. The new test passes at both 9454105 and 84ae057 so as a sanity check I'd agree there is room for improvement, if possible.
  MarcoFalke:
    re-ACK 84ae0578b6 🏉
  Empact:
    Code Review ACK https://github.com/bitcoin/bitcoin/pull/18918/commits/84ae0578b6c68dda145ca65fef510ce0fdac0d7b
  ryanofsky:
    Code review ACK 84ae0578b6c68dda145ca65fef510ce0fdac0d7b. Lot of small changes since previous review: added verify step before salvage, added basic test in new commit, removed unused scanstate variable and warnings parameter, tweaked various comments and strings, moved fsuccess variable declaration
  meshcollider:
    Concept / light code review ACK 84ae0578b6c68dda145ca65fef510ce0fdac0d7b

Tree-SHA512: 05be116b56ecade1c58faca1728c8fe4b78f0a082dbc2544a3f7507dd155f1f4f39070bd1fe90053444384337bc48b97149df5c1010230d78f8ecc08e69d93af

1 files changed, 9 insertions, 41 deletions

diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 331408ef48..e7adbfea77 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -672,6 +672,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
     return true;
 }
 
+bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr)
+{
+    CWalletScanState dummy_wss;
+    LOCK(pwallet->cs_wallet);
+    return ReadKeyValue(pwallet, ssKey, ssValue, dummy_wss, strType, strErr);
+}
+
 bool WalletBatch::IsKeyType(const std::string& strType)
 {
     return (strType == DBKeys::KEY ||
@@ -976,53 +983,14 @@ void MaybeCompactWalletDB()
     fOneThread = false;
 }
 
-//
-// Try to (very carefully!) recover wallet file if there is a problem.
-//
-bool WalletBatch::Recover(const fs::path& wallet_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& out_backup_filename)
-{
-    return BerkeleyBatch::Recover(wallet_path, callbackDataIn, recoverKVcallback, out_backup_filename);
-}
-
-bool WalletBatch::Recover(const fs::path& wallet_path, std::string& out_backup_filename)
-{
-    // recover without a key filter callback
-    // results in recovering all record types
-    return WalletBatch::Recover(wallet_path, nullptr, nullptr, out_backup_filename);
-}
-
-bool WalletBatch::RecoverKeysOnlyFilter(void *callbackData, CDataStream ssKey, CDataStream ssValue)
-{
-    CWallet *dummyWallet = reinterpret_cast<CWallet*>(callbackData);
-    CWalletScanState dummyWss;
-    std::string strType, strErr;
-    bool fReadOK;
-    {
-        // Required in LoadKeyMetadata():
-        LOCK(dummyWallet->cs_wallet);
-        fReadOK = ReadKeyValue(dummyWallet, ssKey, ssValue,
-                               dummyWss, strType, strErr);
-    }
-    if (!IsKeyType(strType) && strType != DBKeys::HDCHAIN) {
-        return false;
-    }
-    if (!fReadOK)
-    {
-        LogPrintf("WARNING: WalletBatch::Recover skipping %s: %s\n", strType, strErr);
-        return false;
-    }
-
-    return true;
-}
-
 bool WalletBatch::VerifyEnvironment(const fs::path& wallet_path, bilingual_str& errorStr)
 {
     return BerkeleyBatch::VerifyEnvironment(wallet_path, errorStr);
 }
 
-bool WalletBatch::VerifyDatabaseFile(const fs::path& wallet_path, std::vector<bilingual_str>& warnings, bilingual_str& errorStr)
+bool WalletBatch::VerifyDatabaseFile(const fs::path& wallet_path, bilingual_str& errorStr)
 {
-    return BerkeleyBatch::VerifyDatabaseFile(wallet_path, warnings, errorStr, WalletBatch::Recover);
+    return BerkeleyBatch::VerifyDatabaseFile(wallet_path, errorStr);
 }
 
 bool WalletBatch::WriteDestData(const std::string &address, const std::string &key, const std::string &value)