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

3 files changed, 9 insertions, 7 deletions

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)