[WIP] Salvage wallet should not set the aggressive flag on Db::verify()

jnewbery commented at 3:08 pm on June 6, 2017: member

Currently, the salvagewallet function sets the DB_AGGRESSIVE flag when calling BDB’s Db::verify() and then tries to load the output back into a database. Here’s the documentation for that flag:

Output all the key/data pairs in the file that can be found. By default, Db::verify() does not assume corruption. For example, if a key/data pair on a page is marked as deleted, it is not then written to the output file. When DB_AGGRESSIVE is specified, corruption is assumed, and any key/data pair that can be found is written. In this case, key/data pairs that are corrupted or have been deleted may appear in the output (even if the file being salvaged is in no way corrupt), and the output will almost certainly require editing before being loaded into a database.

We should not be setting the DB_AGGRESSIVE flag and then immediately trying to load the output into a database.

For more technical details see #7463 (comment).

Running with aggressive mode can lead to full database corruption and loss of private keys (#7379). The wallet is backed up to wallet.timestamp.bak, but the original wallet.dat file will lose some or all of its key-value pairs. That’s obviously a very serious usability issue for users. Many will not know about the existence of the wallet.timestamp.bak backup files.

I think in almost all cases it’s better to run Db::verify() without the aggressive flag set. There perhaps should be a tool to run Db::verify() with the aggressive flag. If the database has been badly corrupted, it may be the only way to recover key-value pairs. However, this should be done in an external tool as the output will almost certainly require editing before being reloaded. #8745 (bitcoin-wallet-tool) is the right place for this.

This PR removes the DB_AGGRESSIVE flag from calls to Db::verify(). It also re-enables the -salvagewallet incidental test in wallet.py. I’ve also included a commit which cleans up that test, which can be omitted if it’s not wanted.

I’ve run wallet.py 100 times and not yet hit the salvagewallet issue in #7463. That was intermittent, so I may just be getting lucky, but I think this is an improvement.

Fixes #7463

[wallet] don't set aggressive flag to salvage wallet b8e50c6e52

[tests] cleanup wallet.py b90c905d73

[tests] re-enable salvagewallet test 7a4e314d93

in src/wallet/db.cpp:307 in 7a4e314d93

302 
303     Db db(dbenv, 0);
304     int result = db.verify(strFile.c_str(), NULL, &strDump, flags);
305     if (result == DB_VERIFY_BAD) {
306         LogPrintf("CDBEnv::Salvage: Database salvage found errors, all data may not be recoverable.\n");
307-        if (!fAggressive) {

jnewbery commented at 3:09 pm on June 6, 2017:

Note for reviewers: This branch was impossible to hit since Salvage() was never called with fAggressive set to false

fanquake added the label Wallet on Jun 6, 2017

laanwj commented at 3:18 pm on June 6, 2017: member

-salvagewallet is meant to “salvage” - do everything it can - to get something usable from corrupted wallets. I think a better approach would be to try with non-aggressive first, and only if that fails try aggressively. But disabling aggressive mode completely is throwing away the baby with the bathwater and neutering this mode completely.

#8745 (bitcoin-wallet-tool) is the right place for this.

Fair enough, but in that case I’d argue removing -salvagewallet mode from bitcoind completely. After this pull it loses much of its utility.

jnewbery commented at 3:43 pm on June 6, 2017: member

neutering this mode completely.

That doesn’t seem to be true from my testing. Running verify without DB_AGGRESSIVE does remove corruption from the wallet.dat file and make the balance available. Perhaps that’s only because -salvagewallet is also causing a rescan to happen.

Do you have experience of -salvagewallet recovering a wallet where running a non-aggressive verify would not have recovered it? I have evidence of -salvagewallet making the situation worse, but we don’t have any directed tests for salvagewallet, so I haven’t seen any examples of it making the situation better. The documentation “the output will almost certainly require editing before being loaded into a database.” makes me think that -salvagewallet is very unlikely to actually work.

gmaxwell commented at 4:59 pm on June 6, 2017: contributor

I was going to make pretty much exactly the same post that Wladimir made: Salvage is supposed to be used on a corrupted file– thats the whole point, perhaps we should do both, if you think that we shouldn’t be working on corrupted files there perhaps we should remove it. (I’d be pretty game to remove it: I think it’s mistakenly used by users FAR too often even though it’s something of a nuclear option.)

Do you have experience of -salvagewallet recovering a wallet where running a non-aggressive verify would not have recovered it?

Yes, I have had wallets that were corrupted that needed agressive mode to extract the keys. I’ve also seen aggressive mode fail to read keys in totally non-corrupted wallets…

Keys in the wallet are stored with this terrible der private key encoding that is obvious from orbit… if we had an extra tool it could also scan just looking for that encoding as https://bitcointalk.org/index.php?topic=25091.0 does– which would allow recovering files that BDB won’t do anything with too.

But in all cases the recovery should probably try all reading methods available and union them.

laanwj commented at 11:19 am on June 7, 2017: member

But in all cases the recovery should probably try all reading methods available and union them.

Excellent comment. Completely agree with this. Would make sense for -salvagewallet to try various techniques, then combine the result.

Keys in the wallet are stored with this terrible der private key encoding that is obvious from orbit…

Yes, that would be a good additional strategy for non-encrypted keys.

jnewbery renamed this:
~~Salvage wallet should not set the aggressive flag on Db::verify()~~
[WIP] Salvage wallet should not set the aggressive flag on Db::verify()
on Jun 7, 2017

jnewbery commented at 1:31 pm on June 7, 2017: member

Thanks for the great feedback @laanwj and @gmaxwell . Sounds like there are plenty of improvements we can make here. There are at least three recovery techniques we can use:

attempt to salvage non-aggressively
attempt to salvage aggressively
do a memory scan for DER encoded private keys in the .dat file.

Those operations should be done by a separate tool and not loaded immediately into bitcoind, since the output could be worse than simply reading the .dat file, and can lead to loss of keys. I’ll put this on hold until we have #8745 merged.

jnewbery closed this on Jun 7, 2017

laanwj commented at 3:45 pm on June 7, 2017: member

@jnewbery Sounds good to me

MarcoFalke locked this on Sep 8, 2021

[WIP] Salvage wallet should not set the aggressive flag on Db::verify() #10540