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}.
403@@ -404,8 +404,6 @@ def run_test(self):
404 '-reindex',
405 '-zapwallettxes=1',
406 '-zapwallettxes=2',
407- # disabled until issue is fixed: https://github.com/bitcoin/bitcoin/issues/7463
408- # '-salvagewallet',
In commit “wallet: remove -salvagewallet” (8ef1b45b4d3de71214da1a6bc6e09f6e9a14d7d4)
Would be nice to keep this test or at least document that salvagewallet will throw away your keys: #7379 @MarcoFalke could you describe the test or comment you would add? It sounds there is a good suggestion here, but I don’t understand it’s asking for
@MarcoFalke could you describe the test or comment you would add? It sounds there is a good suggestion here, but I don’t understand it’s asking for
The new functionality has zero tests. I was hoping we could call wallet-tool salvage instead of bitcoind -salvagewallet here. Just because the test is commented out, doesn’t mean it should be removed.
If the test still fails occasionally with the wallet-tool, then it can be commented out again.
The behavior of salvage shouldn’t be any different, so it will probably still fail as the previous -salvagewallet did.
It’s also not clear to me what a salvage test should even look like.
tool_wallet.py.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
103@@ -103,11 +104,205 @@ static void WalletShowInfo(CWallet* wallet_instance)
104 tfm::format(std::cout, "Address Book: %zu\n", wallet_instance->m_address_book.size());
105 }
106
107+/* End of headers, beginning of key/value data */
108+static const char *HEADER_END = "HEADER=END";
109+/* End of key/value data */
110+static const char *DATA_END = "DATA=END";
111+
112 static bool SalvageWallet(fs::path path)
In commit “walletool: Move salvage wallet implementation into wallettool” (67e30b2b42b4d710739e6fe303cbca69be68032c)
Would be nice to move this to src/wallet/salvage.{h,cpp} so wallet tool file does not become monolithic and so salvage functionality can be more easily tested and improved and perhaps accessible to future RPC or GUI recovery features
Started reviewing 67e30b2b42b4d710739e6fe303cbca69be68032c but finding it difficult to compare old and new behavior with all the code moving and changing at the same time. Also wondering if the ReadKeyValue code duplication can be avoided, because it seems likely to doom this feature to obsolescence if updates to that function need to be mirrored here. It makes me wonder what the plan for salvage wallet is. Is the plan to wall it off, deprecate it, and remove it? It seems like -salvagewallet was implemented in a really convoluted way, but if we did want to implement in a sustainable way, we would still implement it as a load option and not as a standalone copy of loading code. (It could still be exposed through wallet tool either way.)
But overall this seems promising, and great to get rid of some of the convoluted callback & enum status code. I will spend more time with this and maybe try to come up with a MOVEONLY version that could be easier to review.
I’ve changed the final refactor commit into several smaller moves and changes. The final approach for salvage is also slightly different.
Instead of having all of the salvage logic wallettool.cpp, I’ve moved instead put it in a RecoverDatabaseFile function in new salvage.{cpp/h} files. Additionally, instead of copying ReadKeyValue, I’ve exposed it and CWalletScanState in walletdb.h so that they can be called from salvage.cpp. Lastly these changes are done in separate commits to make it easier to review.
923@@ -924,7 +924,7 @@ bool WalletBatch::VerifyEnvironment(const fs::path& wallet_path, bilingual_str&
924
925 bool WalletBatch::VerifyDatabaseFile(const fs::path& wallet_path, std::vector<bilingual_str>& warnings, bilingual_str& errorStr)
926 {
927- return BerkeleyBatch::VerifyDatabaseFile(wallet_path, warnings, errorStr, WalletBatch::Recover);
WalletBatch::Recover two-argument version is unused as of commit: 1372aba35fa91dd2fb4103ed7081a057f9794564
121+ bool fReadOK;
122+ {
123+ // Required in LoadKeyMetadata():
124+ LOCK(dummyWallet.cs_wallet);
125+ fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue,
126+ dummyWss, strType, strErr);
CWalletScanState isn’t used, how about exposing a more limited version of ReadKeyValue instead?
423- }
424- if (r == BerkeleyEnvironment::VerifyResult::RECOVER_FAIL)
425- {
426- errorStr = strprintf(_("%s corrupt, salvage failed"), walletFile);
427+ if (!env->Verify(walletFile)) {
428+ errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage"), walletFile);
Concept ACK. Needs a release note.
Travis is unhappy (as is my local build):
0In file included from ./wallet/scriptpubkeyman.h:16:
13519./wallet/walletdb.h:314:112: error: member access into incomplete type 'CWallet'
23520 CWalletScanState &wss, std::string& strType, std::string& strErr) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet);
#10540 makes the case for running in aggressive mode by default and having an option to run in aggressive mode. So maybe drop b816866e6756cb3800d51b8e943e5c408f5e6e0f?
The test problem described in #7463 (comment) seems to be specific to aggressive mode (cc @jnewbery). Perhaps we can at least add a test for non-aggressive mode? The original test wasn’t very interesting. Is there a way to programatically damage a wallet, or can we add a wallet payload that needs salvaging to the test suite? That’s useful for review as well.
103@@ -103,6 +104,11 @@ static void WalletShowInfo(CWallet* wallet_instance)
104 tfm::format(std::cout, "Address Book: %zu\n", wallet_instance->m_address_book.size());
105 }
106
107+static bool SalvageWallet(const fs::path path)
411@@ -419,18 +412,8 @@ bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::vector<bi
412 if (fs::exists(walletDir / walletFile))
413 {
414 std::string backup_filename;
422- walletFile, backup_filename, walletDir));
423- }
424- if (r == BerkeleyEnvironment::VerifyResult::RECOVER_FAIL)
425- {
426- errorStr = strprintf(_("%s corrupt, salvage failed"), walletFile);
427+ if (!env->Verify(walletFile)) {
verify result so we can log the error:
https://docs.oracle.com/cd/E17276_01/html/api_reference/C/dbverify.html
verify are meaningful.
347@@ -342,8 +348,55 @@ bool BerkeleyBatch::Recover(const fs::path& file_path, void *callbackDataIn, boo
348 return false;
349 }
350
351- std::vector<BerkeleyEnvironment::KeyValPair> salvagedData;
352- bool fSuccess = env->Salvage(newFilename, salvagedData);
353+ std::vector<KeyValPair> salvagedData;
354+ bool fSuccess = false; //env->Salvage(newFilename, salvagedData);
0wallet/walletdb.cpp:594:12: error: calling function 'ReadKeyValue' requires holding mutex 'pwallet->cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
11647 return ReadKeyValue(pwallet, ssKey, ssValue, dummy_wss, strType, strErr);
107+{
108+ CWallet dummy_wallet(nullptr, WalletLocation(), WalletDatabase::CreateDummy());
109+ std::string backup_filename;
110+ return WalletBatch::Recover(path, (void*)&dummy_wallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename);
111+}
112+
In commit “wallettool: Add a salvage command” (502024795a32c4600a294da10ad908f796e0cdac)
Note for reviewers: this is just a copy of existing CWallet::Verify code
removed in the next commit “wallet: remove -salvagewallet” (7116e0c73514438ba240827aaddf922d0c1c9d84)
202@@ -203,6 +203,10 @@ def test_getwalletinfo_on_different_wallet(self):
203 assert_equal(shasum_after, shasum_before)
204 self.log.debug('Wallet file shasum unchanged\n')
205
206+ def test_salvage(self):
207+ # TODO: We should implement a test for this. But such a test should be after we fix https://github.com/bitcoin/bitcoin/issues/7463
In commit “wallet: remove -salvagewallet” (7116e0c73514438ba240827aaddf922d0c1c9d84)
This is ok, but it’d be possible (and nice) to have a test that invokes salvagewallet tool doing basic checks like does it exit successfully.
66@@ -67,15 +67,6 @@ class BerkeleyEnvironment
67 fs::path Directory() const { return strPath; }
68
69 bool Verify(const std::string& strFile);
70- /**
71- * Salvage data from a file that Verify says is bad.
72- * fAggressive sets the DB_AGGRESSIVE flag (see berkeley DB->verify() method documentation).
73- * Appends binary key/value pairs to vResult, returns true if successful.
74- * NOTE: reads the entire database into memory, so cannot be used
75- * for huge databases.
In commit “Move BerkeleyEnvironment::Salvage into BerkeleyBatch::Recover” (dca31bd5bc820da889394d15a9337ef96262ee4c)
This comment (especially the part about loading to memory) makes code easier to understand and IMO would be good to move to the new location in the verify function. Skip if it is a pain though
Code review ACK 5276a811e1f42eea2a6c6b679a3f1b418e414d34. Beautiful job with this PR! Not only are the changes much easier to review than last time I looked at this (my --color-diff=dimmed_zebra pager thanks you), but the code cleanups are fantastic: getting rid of callbacks inside of callbacks and void pointers, and weird return values, and unused options, and randomly placed static methods, and methods that shouldn’t be methods, and so on.
Thank you for taking this on and being so responsive to suggestions.
To all reviewers who concepted ACKed here
Ok, I hereby commit to reviewing this when the previous review comments have been addressed.
139+ std::shared_ptr<CWallet> wallet_instance = LoadWallet(name, path);
140+ if (!wallet_instance) return false;
141+ WalletShowInfo(wallet_instance.get());
142+ wallet_instance->Flush(true);
143+ } else if (command == "salvage") {
144+ SalvageWallet(path);
in commit 7f6a5f3fff6e71108289ba9c9bfc5a938a081a79:
Why ignore the return value?
355+ * key/value pairs are appendedto salvagedData which are then written out to a new wallet file.
356+ * NOTE: reads the entire database into memory, so cannot be used
357+ * for huge databases.
358+ */
359+ std::vector<KeyValPair> salvagedData;
360+ bool fSuccess = false;
in commit 6c3afd67d2:
Why is this set all the way up here? The outcome is only needed to remember whether the end of the stream was reached or not. I think it should be defined after the last write to keyHex, but still before this code block:
Also, you could leave it uninitialized because every if branch will initialize it. If none does the initialization, at least a sanitizer could yell at us and hint at the bug.
0 bool fSuccess;
1 if (keyHex != DATA_END) {
2 LogPrintf("Salvage: WARNING: Unexpected end of file while reading salvage output.\n");
3 fSuccess = false;
4 } else {
5 fSuccess = (result == 0);
6 }
293@@ -294,4 +294,7 @@ class WalletBatch
294 //! Compacts BDB state so that wallet.dat is self-contained (if there are changes)
295 void MaybeCompactWalletDB();
296
297+//! Unserialize a give Key-Value pair and load it into the wallet
298+bool ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr);
in commit 6d05dff02b:
wallet is a reference to an object (like all other params in this function), so it should probably be passed as one.
Not sure about the lock annotation, but it is a bit confusing to lock cs_wallet, and then call a function that locks it again immediately after
0bool ReadKeyValue(CWallet& wallet, CDataStream& ssKey, CDataStream& ssValue, std::string& strType, std::string& strErr) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
CWallet is not yet defined.
908@@ -902,8 +909,7 @@ bool WalletBatch::RecoverKeysOnlyFilter(void *callbackData, CDataStream ssKey, C
909 {
in commit 6d05dff02be9e113ac6f835:
The now unused CWalletScanState dummyWss; a few lines up should now be removed.
ACK 94541050b8, only found style nits 💉
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 94541050b8, only found style nits 💉
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhGmQv+Mh8MY93W8aKewBmutaNXryt7hD5cskayCFIuYmD79NAthz5T6LRPVJJ6
8m/Um8cpR1riPqmxQ+dlU2lsxVEcCs4NcGDZxv2vXUjxITyhjVDYX106KJMTEnDNe
9uBvmsVjpFSE48Vi5gPgmzb2SDfmCSBCg//uWol/8gGmpMaX3ffdM5Xe9EjJfxC60
10lVqT8mAReuQhFItYymqN8M919mddxAwJSnpRpbt8lIZpyoq2xVEAWvh7Jq1udCae
11JGyWENYkkaDMBTPgbdfPvTJNlkvsBkyVsXicd42PXnT7N6d18w19LgXZYcClp5N4
12QNiY7izurFhAfW4ujiWJzYenPNGRFK7xlqy/t+BZbfVXaNNDB3gTmqp4FGQ13rSE
130vyFZQ69QGI5e3UnSBCFWCnYyDHpItkSQ2CyXMRCsnLBMAZjUWkfOhbhZHusQxEA
14DubuoJmYJt7T9xs9vqARAMEbjW6tEMQizZxl7GGXlOzzZmv3hNLdtM6V3U99U94S
1528quPr/u
16=Ghf5
17-----END PGP SIGNATURE-----
Timestamp of file with hash fd61ba44cfca59f816c1a6d9c69adc6b6e328cc4036fc94ab1ab7e6e91ee289d -
333@@ -410,100 +334,23 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, bilingual_str&
334 return true;
335 }
336
337-bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::vector<bilingual_str>& warnings, bilingual_str& errorStr, BerkeleyEnvironment::recoverFunc_type recoverFunc)
338+bool BerkeleyBatch::VerifyDatabaseFile(const fs::path& file_path, std::vector<bilingual_str>& warnings, bilingual_str& errorStr)
warnings argument is now unused.
355- }
356- if (r == BerkeleyEnvironment::VerifyResult::RECOVER_FAIL)
357- {
358- errorStr = strprintf(_("%s corrupt, salvage failed"), walletFile);
359+ if (!env->Verify(walletFile)) {
360+ errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup"), walletFile);
40+ return false;
41+ }
42+
43+ /**
44+ * Salvage data from a file. The DB_AGGRESSIVE flag is being used (see berkeley DB->verify() method documentation).
45+ * key/value pairs are appendedto salvagedData which are then written out to a new wallet file.
287@@ -294,4 +288,7 @@ class WalletBatch
288 //! Compacts BDB state so that wallet.dat is self-contained (if there are changes)
289 void MaybeCompactWalletDB();
290
291+//! Unserialize a give Key-Value pair and load it into the wallet
6@@ -7,6 +7,7 @@
7 #include <util/translation.h>
8 #include <wallet/wallet.h>
9 #include <wallet/walletutil.h>
10+#include <wallet/salvage.h>
144+ fSuccess = false;
145+ }
146+ ptxn->commit(0);
147+ pdbCopy->close(0);
148+
149+ return fSuccess;
RecoverDatabaseFile contains a fair amount of code to set and return bool fSuccess, but the value returned doesn’t appear to be used. Is this logic needed?
Concept ACK/almost ACK 94541050b84e16. Code review, builds are clean and tests are green, but attempting to run bitcoin-wallet -regtest -wallet=test salvage returns DB_ENV->dbrename: method not permitted before handle's open method. The same command with info runs fine. I’ve never used the salvage command before, so help using it is welcome. Functional tests would be really good here. A few comments below; feel free to ignore any nits.
Edit: re-attempt with a new wallet
0$ bitcoin-wallet -regtest -wallet=new create
1Topping up keypool...
2Wallet info
3===========
4Encrypted: no
5HD (hd seed available): yes
6Keypool Size: 2000
7Transactions: 0
8Address Book: 0
9$ bitcoin-wallet -regtest -wallet=new info
10Wallet info
11===========
12Encrypted: no
13HD (hd seed available): yes
14Keypool Size: 2000
15Transactions: 0
16Address Book: 0
17$ bitcoin-wallet -regtest -wallet=new salvage
18DB_ENV->dbrename: method not permitted before handle's open method
The only call to Salvage set fAggressive = true so remove that parameter
and always use DB_AGGRESSIVE
We need this exposed for BerkeleyBatch::Recover to be moved out.
Instead of having these be class static functions, just make them be
standalone. Also removes WalletBatch::Recover which just passed through
to BerkeleyBatch::Recover.
but attempting to run
bitcoin-wallet -regtest -wallet=test salvagereturnsDB_ENV->dbrename: method not permitted before handle's open method.
Fixed. Turns out we still have to go through the whole WalletDatbase::Create and VerifyEnvironment song and dance before recovery can be done.
Also added a basic functional test to check that salvage on a new wallet does not have issues. It does not check whether salvaged actually worked or did anything useful.
115+ WalletBatch::VerifyEnvironment(path, error_string);
116+ } catch (const fs::filesystem_error& e) {
117+ error_string = Untranslated(strprintf("Error loading wallet. %s", fsbridge::get_filesystem_error_message(e)));
118+ }
119+ if (!error_string.original.empty()) {
120+ tfm::format(std::cerr, "Failed to open wallet for salvage :%s\n", error_string.original);
Tests for the different exceptions would be great. Not sure on this, or on the need for an EOL \n in line 117 just above.
0 tfm::format(std::cerr, "Failed to open wallet for salvage: %s\n", error_string.original);
re-ACK 84ae0578b6 🏉
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3re-ACK 84ae0578b6 🏉
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjebwv/XkWDTfbKudR9e0H8atjyYFTG7ra0mC6wBPs6Apbj4CIhGGrrVeT/zEYu
8PUPAUO6/RJVD+nBPQTCajQ+/a7kkaRzR9W+Huu+1xXXJEbTzCME7Zld8uMMY750F
99X9VSKOR9JAqmaICejkZ6bk6lMY9HL2+VumYejhYKX0UTYiGAOspBX0hND68EFuR
10eSh+OF5BnFj0g89kiZpp8QNDZGXAQ94Tl9tOV5SSD8I941Ncwgnt0LG20hTGOZ04
11XN19zVW+f27yN/SuHO9powmwwIq/G3urif6eCnRxDHSGkc+9r/xa4SPXnv/SRcxn
12i82alb+6+4N0UoNIr3gPtaxZQ/fhGHiWO9Q8s+p+/Tk2gK4ssjobBwrc4uAMhVmN
13dmmQSQPmaR5Hi0PLKtiM3z98lXI29kbUxk/51vEhLH4sNs1i0TyEcwLgMcezO5/q
14RIYhBStpCVQtJ+LxVUzB0zHK+A/jCM9Q56aw7ZS0ly6LHwmVPD1VvzNDQKhCWUpm
15AANRPTgR
16=FqzM
17-----END PGP SIGNATURE-----
Timestamp of file with hash c579e4ed85c7c252470d4ad60a04790a109bf7cc4aabcc2e2094bbf1b5c53880 -
Labels
Wallet
Milestone
0.21.0