Don't use fixed "wallet.bak"-filename during salvagewallet

jonasschnelli commented at 10:01 am on May 26, 2017: contributor

Simple ~~fix~~ change.

At the moment, during salvagewallet/recover, we rename the wallet to wallet.<timestamp>.bak regardless of a possible custom wallet filename (-wallet=). This fix is “required” for multiwallet via #8694.

jonasschnelli added the label Wallet on May 26, 2017

laanwj commented at 8:30 am on June 1, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10457/commits/f4823abad5bd2c9a556f3ebf2b87160c94f693ed.

If I read correctly this will change the name to <something>.dat.<timestamp>.bak but that’s okay.

laanwj approved

jnewbery commented at 6:49 pm on June 2, 2017: member

Concept ACK. Not having this blocks multi-wallet.

I’d be a lot happier if the -salvagewallet test was being run. It’s currently commented out of wallet.py because of intermittent failure (#7463). I’ll spend some time seeing if I can fix those failures.

sipa commented at 6:23 pm on June 3, 2017: member

Concept ACK

laanwj commented at 12:31 pm on June 5, 2017: member

It’s currently commented out of wallet.py because of intermittent failure (#7463)

Yes I narrowed it down to a berkeleydb issue last time, and wasn’t sure how to proceed:

#7463 (comment)

MarcoFalke commented at 4:51 pm on June 5, 2017: member

utACK f4823abad5bd2c9a556f3ebf2b87160c94f693ed (uncommented the salvagewallet check and run in a loop without issues)

jnewbery commented at 4:57 pm on June 5, 2017: member

uncommented the salvagewallet check and run in a loop without issues

I can reproduce this by running wallet.py 50 times through test_runner.py. From debug.log:

02017-06-05 15:46:54.510473 CDBEnv::Salvage: Database salvage found errors, all data may not be recoverable.
12017-06-05 15:46:54.510639 CDBEnv::Salvage: WARNING: Number of keys in data does not match number of values.
22017-06-05 15:46:54.510648 CDBEnv::Salvage: WARNING: Unexpected end of file while reading salvage output.
32017-06-05 15:46:54.510657 Salvage(aggressive) found 102 records

jnewbery commented at 5:11 pm on June 6, 2017: member

Tested ACK with one small nit:

Please update the comment at the top of CDB::Recover() from: // move wallet file to wallet.timestamp.bak

to: // move wallet file to <wallet_name>.<timestamp>.bak

Otherwise looks good. I’ve tested f4823abad5bd2c9a556f3ebf2b87160c94f693ed and it renames the wallet backups correctly.

laanwj commented at 1:34 pm on June 12, 2017: member

Needs rebase after #8694

jnewbery commented at 1:46 pm on June 12, 2017: member

I think this is a much cleaner solution than was implemented in #8694. There’s no need to pass around the wallet name just so it can be printed in a warning message. I think it’s worth rebasing this on master and reverting the changes in https://github.com/bitcoin/bitcoin/pull/8694/commits/b823a4c9f6836c802803dbd265cb7451fb93b8e7

Revert "wallet: Include actual backup filename in recovery warning message"

This reverts commit b823a4c9f6836c802803dbd265cb7451fb93b8e7.

35d770e87e

Don't use fixed "wallet.bak"-filename during salvagewallet e9d09f23e3

jonasschnelli force-pushed on Jun 12, 2017

jonasschnelli commented at 7:57 pm on June 12, 2017: contributor

Changed: This now reverts https://github.com/bitcoin/bitcoin/commit/b823a4c9f6836c802803dbd265cb7451fb93b8e7 and adds the identical simple fix for the static wallet name.

ryanofsky commented at 8:24 pm on June 12, 2017: member

PR title and summary are misleading because now this PR only does code cleanup without actually changing the filename.

Also, I prefer b823a4c9f6836c802803dbd265cb7451fb93b8e7 to this PR, because it prints a more meaningful log message, and I don’t think it is a big deal that the recover function now takes 2 arguments instead of 1.

I think if you wanted to clean up this code, you would just call the recover function directly or through std::function instead through a raw function pointer.

jnewbery commented at 5:49 pm on June 14, 2017: member

Tested ACK e9d09f23e3a097605730988f2dcb818a57dee745. I think this is definitely an improvement.

in src/wallet/db.cpp:273 in e9d09f23e3

272             warningStr = strprintf(_("Warning: Wallet file corrupt, data salvaged!"
273                                      " Original %s saved as %s in %s; if"
274                                      " your balance or transactions are incorrect you should"
275                                      " restore from a backup."),
276-                                   walletFile, backup_filename, dataDir);
277+                                   walletFile, "{walletfilename}.{timestamp}.bak", dataDir);

luke-jr commented at 6:51 pm on June 14, 2017:

Why show a filename at all, if it’s not even going to tell you which one?

jonasschnelli commented at 8:19 am on June 30, 2017:

I think it won’t hurt. But no strong opinion.

in src/wallet/db.h:58 in e9d09f23e3

54@@ -55,8 +55,7 @@ class CDBEnv
55     enum VerifyResult { VERIFY_OK,
56                         RECOVER_OK,
57                         RECOVER_FAIL };
58-    typedef bool (*recoverFunc_type)(const std::string& strFile, std::string& out_backup_filename);

laanwj commented at 6:52 am on July 26, 2017:

Not sure what the state of this PR is, but I just saw this change: isn’t it clearer to define a type here, instead of make the function type definition in-line in the parameter definition?

laanwj commented at 8:24 am on July 26, 2017:

And maybe use a std::function, that’s more c++11 than a function pointer.

ryanofsky commented at 7:32 am on July 26, 2017: member

PR description and title are misleading and need to be updated (especially now that PR description will go into git history) to say this is only changing the Wallet file corrupt, data salvaged! error message (by removing the recovered filename), and making a slight code simplification.

This PR is not fixing any current bug. It’s just temporarily reintroducing a bug that was fixed a while ago, and then applying a slightly simpler fix. IMO, squashing commit history in this PR would be a good way to make it less confusing and avoid temporarily reintroducing a bug.

luke-jr commented at 9:02 am on July 26, 2017: member

I think it’s better to show the actual filename, but so long as this is refactored cleanly (ie, keep the typedef), I don’t care to argue against it.

jnewbery commented at 9:20 am on July 26, 2017: member

I’m marginally for this change because it’s a worthwhile simplification to not pass the wallet name around simply for printing in an error message. I agree with @ryanofsky that PR comment should be changed and commits squashed.

laanwj commented at 1:25 pm on October 2, 2017: member

Closing this for now. I think the change in itself is worthwhile, but the diff itself seems to be controversial, the PR seems to be abandoned for a while, and it seems low priority.

laanwj closed this on Oct 2, 2017

DrahtBot locked this on Sep 8, 2021

Don’t use fixed “wallet.bak”-filename during salvagewallet #10457