-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Salvage wallet should not set the aggressive flag on Db::verify() #10540
Conversation
|
||
std::stringstream strDump; | ||
|
||
Db db(dbenv, 0); | ||
int result = db.verify(strFile.c_str(), NULL, &strDump, flags); | ||
if (result == DB_VERIFY_BAD) { | ||
LogPrintf("CDBEnv::Salvage: Database salvage found errors, all data may not be recoverable.\n"); | ||
if (!fAggressive) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: This branch was impossible to hit since Salvage() was never called with fAggressive set to false
Fair enough, but in that case I'd argue removing |
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. |
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.)
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. |
Excellent comment. Completely agree with this. Would make sense for
Yes, that would be a good additional strategy for non-encrypted keys. |
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:
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 Sounds good to me |
Currently, the salvagewallet function sets the
DB_AGGRESSIVE
flag when calling BDB'sDb::verify()
and then tries to load the output back into a database. Here's the documentation for that flag: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