wallet: Move salvagewallet into wallettool #18918

achow101 · 2020-05-08T23:12:18Z

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}.

maflcko · 2020-05-09T00:02:37Z

test/functional/wallet_basic.py

@@ -404,8 +404,6 @@ def run_test(self):
            '-reindex',
            '-zapwallettxes=1',
            '-zapwallettxes=2',
-            # disabled until issue is fixed: https://github.com/bitcoin/bitcoin/issues/7463
-            # '-salvagewallet',


Would be nice to keep this test or at least document that salvagewallet will throw away your keys: #7379

In commit "wallet: remove -salvagewallet" (8ef1b45)

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.

I've added a placeholder test with a todo that mentions the issue in tool_wallet.py.

maflcko · 2020-05-09T00:03:27Z

Concept ACK

DrahtBot · 2020-05-09T07:22:33Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Refactor the classes in wallet/db.{cpp/h} #18971 (wallet: Refactor the classes in wallet/db.{cpp/h} by achow101)
wallet: upgradewallet fixes and additional tests #18836 (wallet: upgradewallet fixes and additional tests by achow101)
gui: Drop RecentRequestsTableModel dependency to WalletModel #18618 (gui: Drop RecentRequestsTableModel dependency to WalletModel by promag)
refactor: Remove CAddressBookData::destdata #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)

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.

laanwj · 2020-05-13T18:25:16Z

Concept ACK. Agree that this belongs in the wallet tool, not bitcoind.

jonasschnelli · 2020-05-13T18:34:47Z

Concept ACK, ... finally.
Follows the concept of #8745. There was already an attempt to add salvage to the wallet tool in #15307

ryanofsky

Started reviewing 67e30b2 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.

ryanofsky · 2020-05-15T19:46:04Z

test/functional/wallet_basic.py

@@ -404,8 +404,6 @@ def run_test(self):
            '-reindex',
            '-zapwallettxes=1',
            '-zapwallettxes=2',
-            # disabled until issue is fixed: https://github.com/bitcoin/bitcoin/issues/7463
-            # '-salvagewallet',


In commit "wallet: remove -salvagewallet" (8ef1b45)

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

ryanofsky · 2020-05-15T20:25:24Z

src/wallet/wallettool.cpp

+static const char *HEADER_END = "HEADER=END";
+/* End of key/value data */
+static const char *DATA_END = "DATA=END";
+
 static bool SalvageWallet(fs::path path)


In commit "walletool: Move salvage wallet implementation into wallettool" (67e30b2)

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

I've added a salvage.{cpp/h} and put the salvage stuff in there.

achow101 · 2020-05-15T23:49:43Z

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.

Empact · 2020-05-16T01:38:45Z

src/wallet/walletdb.cpp

@@ -924,7 +924,7 @@ bool WalletBatch::VerifyEnvironment(const fs::path& wallet_path, bilingual_str&

 bool WalletBatch::VerifyDatabaseFile(const fs::path& wallet_path, std::vector<bilingual_str>& warnings, bilingual_str& errorStr)
 {
-    return BerkeleyBatch::VerifyDatabaseFile(wallet_path, warnings, errorStr, WalletBatch::Recover);


WalletBatch::Recover two-argument version is unused as of commit: 1372aba

Nevermind, I see you got it in 61a1e57

src/wallet/salvage.cpp

Sjors

Concept ACK. Needs a release note.

Travis is unhappy (as is my local build):

In file included from ./wallet/scriptpubkeyman.h:16:
3519./wallet/walletdb.h:314:112: error: member access into incomplete type 'CWallet'
3520             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 b816866?

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.

src/wallet/db.cpp

jnewbery · 2020-05-18T15:53:19Z

Concept ACK

We need this exposed for BerkeleyBatch::Recover to be moved out.

…andalone Instead of having these be class static functions, just make them be standalone. Also removes WalletBatch::Recover which just passed through to BerkeleyBatch::Recover.

achow101 · 2020-05-25T17:21:01Z

but attempting to run bitcoin-wallet -regtest -wallet=test salvage returns DB_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.

jonatack · 2020-05-25T17:45:48Z

Thanks -- at 84ae057 the bitcoin-wallet salvage command now runs and exits. Yay! but it doesn't leave any feedback. A message that the wallet was successfully salvaged might be reassuring?

jonatack · 2020-05-25T17:54:09Z

ACK 84ae057 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.

jonatack · 2020-05-26T08:56:45Z

src/wallet/wallettool.cpp

+        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);


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.

Suggested change

tfm::format(std::cerr, "Failed to open wallet for salvage :%s\n", error_string.original);

tfm::format(std::cerr, "Failed to open wallet for salvage: %s\n", error_string.original);

Will fix if I have to push another change.

maflcko · 2020-05-26T11:51:35Z

re-ACK 84ae057 🏉

Show signature and timestamp

Signature:

Timestamp of file with hash c579e4ed85c7c252470d4ad60a04790a109bf7cc4aabcc2e2094bbf1b5c53880 -

practicalswift · 2020-05-26T18:12:09Z

Concept ACK: thanks for doing this! :)

Empact · 2020-05-26T18:35:05Z

Code Review ACK 84ae057

ryanofsky

Code review ACK 84ae057. 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 84ae057

84ae057 Add release notes about salvage changes (Andrew Chow) ea337f2 Move RecoverKeysOnlyFilter into RecoverDataBaseFile (Andrew Chow) 9ea2d25 Move RecoverDatabaseFile and RecoverKeysOnlyFilter into salvage.{cpp/h} (Andrew Chow) b426c77 Make BerkeleyBatch::Recover and WalletBatch::RecoverKeysOnlyFilter standalone (Andrew Chow) 2741774 Expose a version of ReadKeyValue and use it in RecoverKeysOnlyFilter (Andrew Chow) ced95d0 Move BerkeleyEnvironment::Salvage into BerkeleyBatch::Recover (Andrew Chow) 07250b8 walletdb: remove fAggressive from Salvage (Andrew Chow) 8ebcbc8 walletdb: don't automatically salvage when corruption is detected (Andrew Chow) d321046 wallet: remove -salvagewallet (Andrew Chow) cdd955e Add basic test for bitcoin-wallet salvage (Andrew Chow) c877709 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 84ae057 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 84ae057 🏉 Empact: Code Review ACK bitcoin@84ae057 ryanofsky: Code review ACK 84ae057. 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 84ae057 Tree-SHA512: 05be116b56ecade1c58faca1728c8fe4b78f0a082dbc2544a3f7507dd155f1f4f39070bd1fe90053444384337bc48b97149df5c1010230d78f8ecc08e69d93af

Summary: Partial backport (1/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@c877709 Test Plan: ninja all check-all bitcoin-wallet --help | grep -C1 salvage Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8588

Summary: Partial backport (2/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@cdd955e Depends on D8588. Test Plan: ninja check-functional Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8589

Summary: Partial backport (3/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@d321046 Depends on D8589. Test Plan: ninja all check-functional Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8590

Summary: Partial (4/11) backport of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@8ebcbc8 Depends on D8590. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8591

Summary: ``` The only call to Salvage set fAggressive = true so remove that parameter and always use DB_AGGRESSIVE ``` Partial backport (5/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@07250b8 Depends on D8591. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8592

Summary: Partial backport (6/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@ced95d0 Depends on D8592. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8593

Summary: ``` We need this exposed for BerkeleyBatch::Recover to be moved out. ``` Partial backport (7/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@2741774 Depends on D8593. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8594

…andalone Summary: ``` Instead of having these be class static functions, just make them be standalone. Also removes WalletBatch::Recover which just passed through to BerkeleyBatch::Recover. ``` Partial backport (8/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@b426c77 Depends on D8594. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8595

Summary: Partial backport (9/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@9ea2d25 Depends on D8595. The DBKeys declaration are added to the `walletdb.h` file to make them available for `salvage.cpp`, see D7020. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8596

Summary: Partial backport (10/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@ea337f2 Depends on D8596. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8597

Summary: Completes backport (11/11) of core [[bitcoin/bitcoin#18918 | PR18918]]: bitcoin/bitcoin@84ae057 Depends on D8597. Test Plan: Read it. Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8598

fanquake added the Wallet label May 8, 2020

maflcko reviewed May 9, 2020

View reviewed changes

achow101 force-pushed the wallettool-salvagewallet branch from a7274c4 to d099709 Compare May 12, 2020 17:31

DrahtBot added the Needs rebase label May 13, 2020

achow101 mentioned this pull request May 14, 2020

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

8 tasks

achow101 force-pushed the wallettool-salvagewallet branch from d099709 to 67e30b2 Compare May 14, 2020 00:06

DrahtBot removed the Needs rebase label May 14, 2020

ryanofsky reviewed May 15, 2020

View reviewed changes

achow101 force-pushed the wallettool-salvagewallet branch from 67e30b2 to 7360e74 Compare May 15, 2020 23:46

achow101 force-pushed the wallettool-salvagewallet branch from 7360e74 to 2dc91a8 Compare May 15, 2020 23:52

Empact reviewed May 16, 2020

View reviewed changes

src/wallet/salvage.cpp Outdated Show resolved Hide resolved

This was referenced May 16, 2020

wallet: Keep inactive seeds after sethdseed and derive keys from them as needed #17681

Merged

gui: Drop RecentRequestsTableModel dependency to WalletModel #18618

Closed

Sjors reviewed May 18, 2020

View reviewed changes

src/wallet/db.cpp Outdated Show resolved Hide resolved

Sjors mentioned this pull request May 18, 2020

-salvagewallet seems to be causing more issues than it fixes, it should be removed #10991

Closed

achow101 force-pushed the wallettool-salvagewallet branch 2 times, most recently from 31c3f68 to e3fe339 Compare May 18, 2020 16:48

achow101 added 5 commits May 25, 2020 12:59

Expose a version of ReadKeyValue and use it in RecoverKeysOnlyFilter

2741774

We need this exposed for BerkeleyBatch::Recover to be moved out.

Make BerkeleyBatch::Recover and WalletBatch::RecoverKeysOnlyFilter st…

b426c77

…andalone Instead of having these be class static functions, just make them be standalone. Also removes WalletBatch::Recover which just passed through to BerkeleyBatch::Recover.

Move RecoverDatabaseFile and RecoverKeysOnlyFilter into salvage.{cpp/h}

9ea2d25

Move RecoverKeysOnlyFilter into RecoverDataBaseFile

ea337f2

Add release notes about salvage changes

84ae057

achow101 force-pushed the wallettool-salvagewallet branch from 9454105 to 84ae057 Compare May 25, 2020 17:16

jonatack reviewed May 26, 2020

View reviewed changes

ryanofsky approved these changes May 26, 2020

View reviewed changes

meshcollider reviewed May 27, 2020

View reviewed changes

meshcollider merged commit 520e435 into bitcoin:master May 27, 2020

jnewbery mentioned this pull request May 27, 2020

[RFC] Long term plan for wallet command-line args #13044

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	tfm::format(std::cerr, "Failed to open wallet for salvage :%s\n", error_string.original);
	tfm::format(std::cerr, "Failed to open wallet for salvage: %s\n", error_string.original);

wallet: Move salvagewallet into wallettool #18918

wallet: Move salvagewallet into wallettool #18918

Uh oh!

Conversation

achow101 commented May 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 9, 2020

Uh oh!

DrahtBot commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented May 13, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented May 18, 2020

Uh oh!

achow101 commented May 25, 2020

Uh oh!

jonatack commented May 25, 2020

Uh oh!

jonatack commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 26, 2020

Uh oh!

practicalswift commented May 26, 2020

Uh oh!

Empact commented May 26, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 May 15, 2020 •

edited

Loading

DrahtBot commented May 9, 2020 •

edited

Loading

laanwj commented May 13, 2020 •

edited

Loading

Sjors left a comment •

edited

Loading

jonatack commented May 25, 2020 •

edited

Loading

jonatack May 26, 2020 •

edited

Loading