Make explicitly requested salvage operations keep going when there is an error. #2410

Merged
merged 1 commit into from May 7, 2013

Conversation

Projects
None yet
5 participants
Member

gmaxwell commented Mar 24, 2013

In my tests corrupted wallets would often result in BDB dropping an error
just due to duplicate records being found, which appears harmless.

@gmaxwell gmaxwell Make explicitly requested salvage operations keep going when there is…
… an error.

In my tests corrupted wallets would often result in BDB dropping an error
just due to duplicate records being found, which appears harmless.
14c9d11

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/14c9d116be476d08dd18f2e9f4a8ed251a6bbf79 for binaries and test log.

@Diapolo Diapolo commented on the diff Mar 25, 2013

src/db.cpp
{
- printf("ERROR: db salvage failed\n");
+ printf("Error: Salvage found errors, all data may not be recoverable.\n");
+ if (!fAggressive)
+ {
+ printf("Error: Rerun with aggressive mode to ignore errors and continue.\n");
+ return false;
+ }
+ }
+ if (result != 0 && result != DB_VERIFY_BAD)
+ {
+ printf("ERROR: db salvage failed: %d\n",result);
@Diapolo

Diapolo Mar 25, 2013

You managed to use Error above 2 times, can you update this also ;)?

@sipa

sipa Apr 12, 2013

Owner

@Diapolo What do you mean?

@Diapolo

Diapolo Apr 12, 2013

Was just another nit about how it is written ;).
Error vs. ERROR

Diapolo commented Mar 25, 2013

How can I start aggressive mode? Or is the client doing this for me, if "normal" mode fails?

Owner

sipa commented Apr 7, 2013

This is one change for which I'd like to see a test plan, and what behaviour changes compared to before...

Owner

sipa commented Apr 20, 2013

It seems there are several issues with salvaging wallets, and some can perhaps be fixed. This may seem pointless, as we want to move away from BDB for wallets anyway, but we'll still need conversion tools (either built-in, or as separate applications) that can reliably import wallets.

First of all, we're not dealing at all with environment errors, which actually seem to be the most common. As wallets are detached from the env at shutdown anyway, they really serve no purpose except crash recovery. And even when the detaching failed, almost always the actual data is written to the wallet file anyway. If opening an environment fails, I wonder whether we can just play safe, and delete database/* before trying anything else. In addition, I think the database dir could just be deleted after a clean detach of the wallet too. That would reduce problems when files get moved without their environment.

Then regarding the wallet database file itself, I've frequently seen people who were unable to open their wallet file, while a simple db4.8_dump old.dat | db4.8_load new.dat fixed the problem completely. That suggests to me that we should try the salvaging code, without removing all transactions, and only when importing fails or corrupted keys are found, do an implicit rescan. Even then, I'm not sure there's a need for completely resetting the other entries.

Owner

sipa commented May 4, 2013

Ok, I've only tested this superficially, but the worst case side effect seems to be continuing a salvage operation that would fail anyway.

ACK.

@gavinandresen gavinandresen added a commit that referenced this pull request May 7, 2013

@gavinandresen gavinandresen Merge pull request #2410 from gmaxwell/salvageharder
Make explicitly requested salvage operations keep going when there is an error.
e5ddaf5

@gavinandresen gavinandresen merged commit e5ddaf5 into bitcoin:master May 7, 2013

@laudney laudney pushed a commit to reddcoin-project/reddcoin that referenced this pull request Mar 19, 2014

@gavinandresen gavinandresen Merge pull request #2410 from gmaxwell/salvageharder
Make explicitly requested salvage operations keep going when there is an error.
48089c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment