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

18 files changed, 237 insertions, 273 deletions

diff --git a/doc/release-notes-18918.md b/doc/release-notes-18918.md
new file mode 100644
index 0000000000..f57a62eeb7
--- /dev/null
+++ b/doc/release-notes-18918.md
@@ -0,0 +1,3 @@
+# Wallet
+
+The `-salvagewallet` startup option has been removed. A new `salvage` command has been added to the `bitcoin-wallet` tool which performs the salvage operations that `-salvagewallet` did.
diff --git a/src/Makefile.am b/src/Makefile.am
index 882d83e0b8..2b004691fd 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -248,6 +248,7 @@ BITCOIN_CORE_H = \
   wallet/ismine.h \
   wallet/load.h \
   wallet/rpcwallet.h \
+  wallet/salvage.h \
   wallet/scriptpubkeyman.h \
   wallet/wallet.h \
   wallet/walletdb.h \
@@ -356,6 +357,7 @@ libbitcoin_wallet_a_SOURCES = \
   wallet/load.cpp \
   wallet/rpcdump.cpp \
   wallet/rpcwallet.cpp \
+  wallet/salvage.cpp \
   wallet/scriptpubkeyman.cpp \
   wallet/wallet.cpp \
   wallet/walletdb.cpp \
diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp
index 7f9439788a..b420463c00 100644
--- a/src/bitcoin-wallet.cpp
+++ b/src/bitcoin-wallet.cpp
@@ -31,6 +31,7 @@ static void SetupWalletToolArgs()
 
     gArgs.AddArg("info", "Get wallet info", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
     gArgs.AddArg("create", "Create new wallet file", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
+    gArgs.AddArg("salvage", "Attempt to recover private keys from a corrupt wallet", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
 }
 
 static bool WalletAppInit(int argc, char* argv[])
diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp
index 1b2bd83a4c..4ed28b0623 100644
--- a/src/wallet/db.cpp
+++ b/src/wallet/db.cpp
@@ -268,21 +268,14 @@ BerkeleyEnvironment::BerkeleyEnvironment()
     fMockDb = true;
 }
 
-BerkeleyEnvironment::VerifyResult BerkeleyEnvironment::Verify(const std::string& strFile, recoverFunc_type recoverFunc, std::string& out_backup_filename)
+bool BerkeleyEnvironment::Verify(const std::string& strFile)
 {
     LOCK(cs_db);
     assert(mapFileUseCount.count(strFile) == 0);
 
     Db db(dbenv.get(), 0);
     int result = db.verify(strFile.c_str(), nullptr, nullptr, 0);
-    if (result == 0)
-        return VerifyResult::VERIFY_OK;
-    else if (recoverFunc == nullptr)
-        return VerifyResult::RECOVER_FAIL;
-
-    // Try to recover:
-    bool fRecovered = (*recoverFunc)(fs::path(strPath) / strFile, out_backup_filename);
-    return (fRecovered ? VerifyResult::RECOVER_OK : VerifyResult::RECOVER_FAIL);
+    return result == 0;
 }
 
 BerkeleyBatch::SafeDbt::SafeDbt()
@@ -324,75 +317,6 @@ BerkeleyBatch::SafeDbt::operator Dbt*()
     return &m_dbt;
 }
 
-bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& newFilename)
-{
-    std::string filename;
-    std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
-
-    // Recovery procedure:
-    // move wallet file to walletfilename.timestamp.bak
-    // Call Salvage with fAggressive=true to
-    // get as much data as possible.
-    // Rewrite salvaged data to fresh wallet file
-    // Set -rescan so any missing transactions will be
-    // found.
-    int64_t now = GetTime();
-    newFilename = strprintf("%s.%d.bak", filename, now);
-
-    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
-    {
-        LogPrintf("Failed to rename %s to %s\n", filename, newFilename);
-        return false;
-    }
-
-    std::vector<BerkeleyEnvironment::KeyValPair> salvagedData;
-    bool fSuccess = env->Salvage(newFilename, true, salvagedData);
-    if (salvagedData.empty())
-    {
-        LogPrintf("Salvage(aggressive) found no records in %s.\n", 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
-                            filename.c_str(),   // Filename
-                            "main",             // Logical db name
-                            DB_BTREE,           // Database type
-                            DB_CREATE,          // Flags
-                            0);
-    if (ret > 0) {
-        LogPrintf("Cannot create database file %s\n", filename);
-        pdbCopy->close(0);
-        return false;
-    }
-
-    DbTxn* ptxn = env->TxnBegin();
-    for (BerkeleyEnvironment::KeyValPair& row : salvagedData)
-    {
-        if (recoverKVcallback)
-        {
-            CDataStream ssKey(row.first, SER_DISK, CLIENT_VERSION);
-            CDataStream ssValue(row.second, SER_DISK, CLIENT_VERSION);
-            if (!(*recoverKVcallback)(callbackDataIn, ssKey, ssValue))
-                continue;
-        }
-        Dbt datKey(&row.first[0], row.first.size());
-        Dbt datValue(&row.second[0], row.second.size());
-        int ret2 = pdbCopy->put(ptxn, &datKey, &datValue, DB_NOOVERWRITE);
-        if (ret2 > 0)
-            fSuccess = false;
-    }
-    ptxn->commit(0);
-    pdbCopy->close(0);
-
-    return fSuccess;
-}
-
 bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, bilingual_str& errorStr)
 {
     std::string walletFile;
@@ -410,7 +334,7 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, bilingual_str&
     return true;
 }
 
-bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::vector<bilingual_str>& warnings, bilingual_str& errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc)
+bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, bilingual_str& errorStr)
 {
     std::string walletFile;
     std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, walletFile);
@@ -418,19 +342,8 @@ bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::vector<bi
 
     if (fs::exists(walletDir / walletFile))
     {
-        std::string backup_filename;
-        BerkeleyEnvironment::VerifyResult r = env->Verify(walletFile, recoverFunc, backup_filename);
-        if (r == BerkeleyEnvironment::VerifyResult::RECOVER_OK)
-        {
-            warnings.push_back(strprintf(_("Warning: Wallet file corrupt, data salvaged!"
-                                     " Original %s saved as %s in %s; if"
-                                     " your balance or transactions are incorrect you should"
-                                     " restore from a backup."),
-                walletFile, backup_filename, walletDir));
-        }
-        if (r == BerkeleyEnvironment::VerifyResult::RECOVER_FAIL)
-        {
-            errorStr = strprintf(_("%s corrupt, salvage failed"), walletFile);
+        if (!env->Verify(walletFile)) {
+            errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), walletFile);
             return false;
         }
     }
@@ -438,72 +351,6 @@ bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::vector<bi
     return true;
 }
 
-/* End of headers, beginning of key/value data */
-static const char *HEADER_END = "HEADER=END";
-/* End of key/value data */
-static const char *DATA_END = "DATA=END";
-
-bool BerkeleyEnvironment::Salvage(const std::string& strFile, bool fAggressive, std::vector<BerkeleyEnvironment::KeyValPair>& vResult)
-{
-    LOCK(cs_db);
-    assert(mapFileUseCount.count(strFile) == 0);
-
-    u_int32_t flags = DB_SALVAGE;
-    if (fAggressive)
-        flags |= DB_AGGRESSIVE;
-
-    std::stringstream strDump;
-
-    Db db(dbenv.get(), 0);
-    int result = db.verify(strFile.c_str(), nullptr, &strDump, flags);
-    if (result == DB_VERIFY_BAD) {
-        LogPrintf("BerkeleyEnvironment::Salvage: Database salvage found errors, all data may not be recoverable.\n");
-        if (!fAggressive) {
-            LogPrintf("BerkeleyEnvironment::Salvage: Rerun with aggressive mode to ignore errors and continue.\n");
-            return false;
-        }
-    }
-    if (result != 0 && result != DB_VERIFY_BAD) {
-        LogPrintf("BerkeleyEnvironment::Salvage: Database salvage failed with result %d.\n", result);
-        return false;
-    }
-
-    // Format of bdb dump is ascii lines:
-    // header lines...
-    // HEADER=END
-    //  hexadecimal key
-    //  hexadecimal value
-    //  ... repeated
-    // DATA=END
-
-    std::string strLine;
-    while (!strDump.eof() && strLine != HEADER_END)
-        getline(strDump, strLine); // Skip past header
-
-    std::string keyHex, valueHex;
-    while (!strDump.eof() && keyHex != DATA_END) {
-        getline(strDump, keyHex);
-        if (keyHex != DATA_END) {
-            if (strDump.eof())
-                break;
-            getline(strDump, valueHex);
-            if (valueHex == DATA_END) {
-                LogPrintf("BerkeleyEnvironment::Salvage: WARNING: Number of keys in data does not match number of values.\n");
-                break;
-            }
-            vResult.push_back(make_pair(ParseHex(keyHex), ParseHex(valueHex)));
-        }
-    }
-
-    if (keyHex != DATA_END) {
-        LogPrintf("BerkeleyEnvironment::Salvage: WARNING: Unexpected end of file while reading salvage output.\n");
-        return false;
-    }
-
-    return (result == 0);
-}
-
-
 void BerkeleyEnvironment::CheckpointLSN(const std::string& strFile)
 {
     dbenv->txn_checkpoint(0, 0, 0);
diff --git a/src/wallet/db.h b/src/wallet/db.h
index 37f96a1a96..54ce144ffc 100644
--- a/src/wallet/db.h
+++ b/src/wallet/db.h
@@ -66,26 +66,7 @@ public:
     bool IsDatabaseLoaded(const std::string& db_filename) const { return m_databases.find(db_filename) != m_databases.end(); }
     fs::path Directory() const { return strPath; }
 
-    /**
-     * Verify that database file strFile is OK. If it is not,
-     * call the callback to try to recover.
-     * This must be called BEFORE strFile is opened.
-     * Returns true if strFile is OK.
-     */
-    enum class VerifyResult { VERIFY_OK,
-                        RECOVER_OK,
-                        RECOVER_FAIL };
-    typedef bool (*recoverFunc_type)(const fs::path& file_path, std::string& out_backup_filename);
-    VerifyResult Verify(const std::string& strFile, recoverFunc_type recoverFunc, std::string& out_backup_filename);
-    /**
-     * Salvage data from a file that Verify says is bad.
-     * fAggressive sets the DB_AGGRESSIVE flag (see berkeley DB->verify() method documentation).
-     * Appends binary key/value pairs to vResult, returns true if successful.
-     * NOTE: reads the entire database into memory, so cannot be used
-     * for huge databases.
-     */
-    typedef std::pair<std::vector<unsigned char>, std::vector<unsigned char> > KeyValPair;
-    bool Salvage(const std::string& strFile, bool fAggressive, std::vector<KeyValPair>& vResult);
+    bool Verify(const std::string& strFile);
 
     bool Open(bool retry);
     void Close();
@@ -245,7 +226,6 @@ public:
 
     void Flush();
     void Close();
-    static bool Recover(const fs::path& file_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& out_backup_filename);
 
     /* flush the wallet passively (TRY_LOCK)
        ideal to be called periodically */
@@ -253,7 +233,7 @@ public:
     /* verifies the database environment */
     static bool VerifyEnvironment(const fs::path& file_path, bilingual_str& errorStr);
     /* verifies the database file */
-    static bool VerifyDatabaseFile(const fs::path& file_path, std::vector<bilingual_str>& warnings, bilingual_str& errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc);
+    static bool VerifyDatabaseFile(const fs::path& file_path, bilingual_str& errorStr);
 
     template <typename K, typename T>
     bool Read(const K& key, T& value)
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 6f973aab1c..3885eb6185 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -54,7 +54,6 @@ void WalletInit::AddWalletOptions() const
     gArgs.AddArg("-paytxfee=<amt>", strprintf("Fee (in %s/kB) to add to transactions you send (default: %s)",
                                                             CURRENCY_UNIT, FormatMoney(CFeeRate{DEFAULT_PAY_TX_FEE}.GetFeePerK())), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     gArgs.AddArg("-rescan", "Rescan the block chain for missing wallet transactions on startup", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
-    gArgs.AddArg("-salvagewallet", "Attempt to recover private keys from a corrupt wallet on startup", ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     gArgs.AddArg("-spendzeroconfchange", strprintf("Spend unconfirmed change when sending transactions (default: %u)", DEFAULT_SPEND_ZEROCONF_CHANGE), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     gArgs.AddArg("-txconfirmtarget=<n>", strprintf("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)", DEFAULT_TX_CONFIRM_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
     gArgs.AddArg("-wallet=<path>", "Specify wallet database path. Can be specified multiple times to load multiple wallets. Path is interpreted relative to <walletdir> if it is not absolute, and will be created if it does not exist (as a directory containing a wallet.dat file and log files). For backwards compatibility this will also accept names of existing data files in <walletdir>.)", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::WALLET);
@@ -89,16 +88,6 @@ bool WalletInit::ParameterInteraction() const
         LogPrintf("%s: parameter interaction: -blocksonly=1 -> setting -walletbroadcast=0\n", __func__);
     }
 
-    if (gArgs.GetBoolArg("-salvagewallet", false)) {
-        if (is_multiwallet) {
-            return InitError(strprintf(Untranslated("%s is only allowed with a single wallet file"), "-salvagewallet"));
-        }
-        // Rewrite just private keys: rescan to find transactions
-        if (gArgs.SoftSetBoolArg("-rescan", true)) {
-            LogPrintf("%s: parameter interaction: -salvagewallet=1 -> setting -rescan=1\n", __func__);
-        }
-    }
-
     bool zapwallettxes = gArgs.GetBoolArg("-zapwallettxes", false);
     // -zapwallettxes implies dropping the mempool on startup
     if (zapwallettxes && gArgs.SoftSetBoolArg("-persistmempool", false)) {
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
index 16f3699d37..8df3e78215 100644
--- a/src/wallet/load.cpp
+++ b/src/wallet/load.cpp
@@ -37,11 +37,6 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
 
     chain.initMessage(_("Verifying wallet(s)...").translated);
 
-    // Parameter interaction code should have thrown an error if -salvagewallet
-    // was enabled with more than wallet file, so the wallet_files size check
-    // here should have no effect.
-    bool salvage_wallet = gArgs.GetBoolArg("-salvagewallet", false) && wallet_files.size() <= 1;
-
     // Keep track of each wallet absolute path to detect duplicates.
     std::set<fs::path> wallet_paths;
 
@@ -55,7 +50,7 @@ bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wal
 
         bilingual_str error_string;
         std::vector<bilingual_str> warnings;
-        bool verify_success = CWallet::Verify(chain, location, salvage_wallet, error_string, warnings);
+        bool verify_success = CWallet::Verify(chain, location, error_string, warnings);
         if (!warnings.empty()) chain.initWarning(Join(warnings, Untranslated("\n")));
         if (!verify_success) {
             chain.initError(error_string);
diff --git a/src/wallet/load.h b/src/wallet/load.h
index 5a62e29303..e24b1f2e69 100644
--- a/src/wallet/load.h
+++ b/src/wallet/load.h
@@ -16,8 +16,6 @@ class Chain;
 } // namespace interfaces
 
 //! Responsible for reading and validating the -wallet arguments and verifying the wallet database.
-//! This function will perform salvage on the wallet if requested, as long as only one wallet is
-//! being loaded (WalletInit::ParameterInteraction() forbids -salvagewallet, -zapwallettxes or -upgradewallet with multiwallet).
 bool VerifyWallets(interfaces::Chain& chain, const std::vector<std::string>& wallet_files);
 
 //! Load wallet databases.
diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp
new file mode 100644
index 0000000000..70067ebef0
--- /dev/null
+++ b/src/wallet/salvage.cpp
@@ -0,0 +1,150 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <fs.h>
+#include <streams.h>
+#include <wallet/salvage.h>
+#include <wallet/wallet.h>
+#include <wallet/walletdb.h>
+
+/* End of headers, beginning of key/value data */
+static const char *HEADER_END = "HEADER=END";
+/* End of key/value data */
+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)
+{
+    std::string filename;
+    std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
+
+    // Recovery procedure:
+    // move wallet file to walletfilename.timestamp.bak
+    // Call Salvage with fAggressive=true to
+    // get as much data as possible.
+    // Rewrite salvaged data to fresh wallet file
+    // Set -rescan so any missing transactions will be
+    // found.
+    int64_t now = GetTime();
+    std::string newFilename = strprintf("%s.%d.bak", filename, now);
+
+    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
+    {
+        LogPrintf("Failed to rename %s to %s\n", filename, newFilename);
+        return false;
+    }
+
+    /**
+     * Salvage data from a file. The DB_AGGRESSIVE flag is being used (see berkeley DB->verify() method documentation).
+     * key/value pairs are appended to salvagedData which are then written out to a new wallet file.
+     * NOTE: reads the entire database into memory, so cannot be used
+     * for huge databases.
+     */
+    std::vector<KeyValPair> salvagedData;
+
+    std::stringstream strDump;
+
+    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");
+    }
+    if (result != 0 && result != DB_VERIFY_BAD) {
+        LogPrintf("Salvage: Database salvage failed with result %d.\n", result);
+        return false;
+    }
+
+    // Format of bdb dump is ascii lines:
+    // header lines...
+    // HEADER=END
+    //  hexadecimal key
+    //  hexadecimal value
+    //  ... repeated
+    // DATA=END
+
+    std::string strLine;
+    while (!strDump.eof() && strLine != HEADER_END)
+        getline(strDump, strLine); // Skip past header
+
+    std::string keyHex, valueHex;
+    while (!strDump.eof() && keyHex != DATA_END) {
+        getline(strDump, keyHex);
+        if (keyHex != DATA_END) {
+            if (strDump.eof())
+                break;
+            getline(strDump, valueHex);
+            if (valueHex == DATA_END) {
+                LogPrintf("Salvage: WARNING: Number of keys in data does not match number of values.\n");
+                break;
+            }
+            salvagedData.push_back(make_pair(ParseHex(keyHex), ParseHex(valueHex)));
+        }
+    }
+
+    bool fSuccess;
+    if (keyHex != DATA_END) {
+        LogPrintf("Salvage: WARNING: Unexpected end of file while reading salvage output.\n");
+        fSuccess = false;
+    } else {
+        fSuccess = (result == 0);
+    }
+
+    if (salvagedData.empty())
+    {
+        LogPrintf("Salvage(aggressive) found no records in %s.\n", 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
+                            filename.c_str(),   // Filename
+                            "main",             // Logical db name
+                            DB_BTREE,           // Database type
+                            DB_CREATE,          // Flags
+                            0);
+    if (ret > 0) {
+        LogPrintf("Cannot create database file %s\n", filename);
+        pdbCopy->close(0);
+        return false;
+    }
+
+    DbTxn* ptxn = env->TxnBegin();
+    CWallet dummyWallet(nullptr, WalletLocation(), WalletDatabase::CreateDummy());
+    for (KeyValPair& row : salvagedData)
+    {
+        /* Filter for only private key type KV pairs to be added to the salvaged wallet */
+        CDataStream ssKey(row.first, SER_DISK, CLIENT_VERSION);
+        CDataStream ssValue(row.second, SER_DISK, CLIENT_VERSION);
+        std::string strType, strErr;
+        bool fReadOK;
+        {
+            // Required in LoadKeyMetadata():
+            LOCK(dummyWallet.cs_wallet);
+            fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue, strType, strErr);
+        }
+        if (!WalletBatch::IsKeyType(strType) && strType != DBKeys::HDCHAIN) {
+            continue;
+        }
+        if (!fReadOK)
+        {
+            LogPrintf("WARNING: WalletBatch::Recover skipping %s: %s\n", strType, strErr);
+            continue;
+        }
+        Dbt datKey(&row.first[0], row.first.size());
+        Dbt datValue(&row.second[0], row.second.size());
+        int ret2 = pdbCopy->put(ptxn, &datKey, &datValue, DB_NOOVERWRITE);
+        if (ret2 > 0)
+            fSuccess = false;
+    }
+    ptxn->commit(0);
+    pdbCopy->close(0);
+
+    return fSuccess;
+}
diff --git a/src/wallet/salvage.h b/src/wallet/salvage.h
new file mode 100644
index 0000000000..e361930f5e
--- /dev/null
+++ b/src/wallet/salvage.h
@@ -0,0 +1,14 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_WALLET_SALVAGE_H
+#define BITCOIN_WALLET_SALVAGE_H
+
+#include <fs.h>
+#include <streams.h>
+
+bool RecoverDatabaseFile(const fs::path& file_path);
+
+#endif // BITCOIN_WALLET_SALVAGE_H
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 255770552f..7824563254 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -153,7 +153,7 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
     try {
-        if (!CWallet::Verify(chain, location, false, error, warnings)) {
+        if (!CWallet::Verify(chain, location, error, warnings)) {
             error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error;
             return nullptr;
         }
@@ -195,7 +195,7 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString&
     }
 
     // Wallet::Verify will check if we're trying to create a wallet with a duplicate name.
-    if (!CWallet::Verify(chain, location, false, error, warnings)) {
+    if (!CWallet::Verify(chain, location, error, warnings)) {
         error = Untranslated("Wallet file verification failed.") + Untranslated(" ") + error;
         return WalletCreationStatus::CREATION_FAILED;
     }
@@ -3650,7 +3650,7 @@ std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
     return values;
 }
 
-bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, bool salvage_wallet, bilingual_str& error_string, std::vector<bilingual_str>& warnings)
+bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error_string, std::vector<bilingual_str>& warnings)
 {
     // Do some checking on wallet path. It should be either a:
     //
@@ -3690,16 +3690,7 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b
         return false;
     }
 
-    if (salvage_wallet) {
-        // Recover readable keypairs:
-        CWallet dummyWallet(&chain, WalletLocation(), WalletDatabase::CreateDummy());
-        std::string backup_filename;
-        if (!WalletBatch::Recover(wallet_path, (void *)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) {
-            return false;
-        }
-    }
-
-    return WalletBatch::VerifyDatabaseFile(wallet_path, warnings, error_string);
+    return WalletBatch::VerifyDatabaseFile(wallet_path, error_string);
 }
 
 std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags)
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 9d312e8ee5..29d04a0cba 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1136,7 +1136,7 @@ public:
     bool MarkReplaced(const uint256& originalHash, const uint256& newHash);
 
     //! Verify wallet naming and perform salvage on the wallet if required
-    static bool Verify(interfaces::Chain& chain, const WalletLocation& location, bool salvage_wallet, bilingual_str& error_string, std::vector<bilingual_str>& warnings);
+    static bool Verify(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error_string, std::vector<bilingual_str>& warnings);
 
     /* Initializes the wallet, returns a new CWallet instance or a null pointer in case of an error */
     static std::shared_ptr<CWallet> CreateWalletFromFile(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector<bilingual_str>& warnings, uint64_t wallet_creation_flags = 0);
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)
diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h
index a2788ed6c4..b95ed24d12 100644
--- a/src/wallet/walletdb.h
+++ b/src/wallet/walletdb.h
@@ -261,18 +261,12 @@ public:
     DBErrors FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWalletTx>& vWtx);
     DBErrors ZapWalletTx(std::list<CWalletTx>& vWtx);
     DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut);
-    /* Try to (very carefully!) recover wallet database (with a possible key type filter) */
-    static bool Recover(const fs::path& wallet_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& out_backup_filename);
-    /* Recover convenience-function to bypass the key filter callback, called when verify fails, recovers everything */
-    static bool Recover(const fs::path& wallet_path, std::string& out_backup_filename);
-    /* Recover filter (used as callback), will only let keys (cryptographical keys) as KV/key-type pass through */
-    static bool RecoverKeysOnlyFilter(void *callbackData, CDataStream ssKey, CDataStream ssValue);
     /* 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, std::vector<bilingual_str>& warnings, bilingual_str& errorStr);
+    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);
@@ -292,4 +286,7 @@ private:
 //! Compacts BDB state so that wallet.dat is self-contained (if there are changes)
 void MaybeCompactWalletDB();
 
+//! 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);
+
 #endif // BITCOIN_WALLET_WALLETDB_H
diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp
index 522efaa884..be07c28503 100644
--- a/src/wallet/wallettool.cpp
+++ b/src/wallet/wallettool.cpp
@@ -5,6 +5,7 @@
 #include <fs.h>
 #include <util/system.h>
 #include <util/translation.h>
+#include <wallet/salvage.h>
 #include <wallet/wallet.h>
 #include <wallet/walletutil.h>
 
@@ -103,6 +104,27 @@ 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 = WalletDatabase::Create(path);
+
+    // Initialize the environment before recovery
+    bilingual_str error_string;
+    try {
+        WalletBatch::VerifyEnvironment(path, 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());
@@ -113,7 +135,7 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
             WalletShowInfo(wallet_instance.get());
             wallet_instance->Flush(true);
         }
-    } else if (command == "info") {
+    } else if (command == "info" || command == "salvage") {
         if (!fs::exists(path)) {
             tfm::format(std::cerr, "Error: no wallet file at %s\n", name);
             return false;
@@ -123,10 +145,15 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
             tfm::format(std::cerr, "%s\nError loading %s. Is wallet being used by other process?\n", error.original, name);
             return false;
         }
-        std::shared_ptr<CWallet> wallet_instance = LoadWallet(name, path);
-        if (!wallet_instance) return false;
-        WalletShowInfo(wallet_instance.get());
-        wallet_instance->Flush(true);
+
+        if (command == "info") {
+            std::shared_ptr<CWallet> wallet_instance = LoadWallet(name, path);
+            if (!wallet_instance) return false;
+            WalletShowInfo(wallet_instance.get());
+            wallet_instance->Flush(true);
+        } else if (command == "salvage") {
+            return SalvageWallet(path);
+        }
     } else {
         tfm::format(std::cerr, "Invalid command: %s\n", command);
         return false;
diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py
index 039ce7daee..524e1593ba 100755
--- a/test/functional/tool_wallet.py
+++ b/test/functional/tool_wallet.py
@@ -203,6 +203,14 @@ class ToolWalletTest(BitcoinTestFramework):
         assert_equal(shasum_after, shasum_before)
         self.log.debug('Wallet file shasum unchanged\n')
 
+    def test_salvage(self):
+        # TODO: Check salvage actually salvages and doesn't break things. https://github.com/bitcoin/bitcoin/issues/7463
+        self.log.info('Check salvage')
+        self.start_node(0, ['-wallet=salvage'])
+        self.stop_node(0)
+
+        self.assert_tool_output('', '-wallet=salvage', 'salvage')
+
     def run_test(self):
         self.wallet_path = os.path.join(self.nodes[0].datadir, self.chain, 'wallets', 'wallet.dat')
         self.test_invalid_tool_commands_and_args()
@@ -211,7 +219,7 @@ class ToolWalletTest(BitcoinTestFramework):
         self.test_tool_wallet_info_after_transaction()
         self.test_tool_wallet_create_on_existing_wallet()
         self.test_getwalletinfo_on_different_wallet()
-
+        self.test_salvage()
 
 if __name__ == '__main__':
     ToolWalletTest().main()
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index 9e295af330..797c903dd3 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -404,8 +404,6 @@ class WalletTest(BitcoinTestFramework):
             '-reindex',
             '-zapwallettxes=1',
             '-zapwallettxes=2',
-            # disabled until issue is fixed: https://github.com/bitcoin/bitcoin/issues/7463
-            # '-salvagewallet',
         ]
         chainlimit = 6
         for m in maintenance:
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
index 580a61f9f3..ff9ff34185 100755
--- a/test/functional/wallet_multiwallet.py
+++ b/test/functional/wallet_multiwallet.py
@@ -122,10 +122,6 @@ class MultiWalletTest(BitcoinTestFramework):
         self.nodes[0].assert_start_raises_init_error(['-zapwallettxes=1', '-wallet=w1', '-wallet=w2'], "Error: -zapwallettxes is only allowed with a single wallet file")
         self.nodes[0].assert_start_raises_init_error(['-zapwallettxes=2', '-wallet=w1', '-wallet=w2'], "Error: -zapwallettxes is only allowed with a single wallet file")
 
-        self.log.info("Do not allow -salvagewallet with multiwallet")
-        self.nodes[0].assert_start_raises_init_error(['-salvagewallet', '-wallet=w1', '-wallet=w2'], "Error: -salvagewallet is only allowed with a single wallet file")
-        self.nodes[0].assert_start_raises_init_error(['-salvagewallet=1', '-wallet=w1', '-wallet=w2'], "Error: -salvagewallet is only allowed with a single wallet file")
-
         # if wallets/ doesn't exist, datadir should be the default wallet dir
         wallet_dir2 = data_dir('walletdir')
         os.rename(wallet_dir(), wallet_dir2)