fix crash with -reindex and empty datadir #2242

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:fix_reindex changing 1 files +14 −3

Diapolo commented at 3:24 PM on January 30, 2013: none
- check for existance of /blocks/ dir and verify if it is non-empty, disable -reindex, if these checks fail
- print "Reindexing aborted" to log, if OpenBlockFile() in ThreadImport() fails
Intended to fix #2239

fix crash with -reindex and empty datadir

- check for existance of /blocks/ dir and verify if it is non-empty,
  disable -reindex, if these checks fail
- print "Reindexing aborted" to log, if OpenBlockFile() in ThreadImport()
  fails

e5a83b70ad

in src/init.cpp:None in b49cbe53eb outdated
```
 354 | @@ -355,16 +355,23 @@ void ThreadImport(void *data) {
 355 |          while (!fRequestShutdown) {
 356 |              CDiskBlockPos pos(nFile, 0);
 357 |              FILE *file = OpenBlockFile(pos, true);
 358 | -            if (!file)
 359 | +            if (!file) {
 360 | +                // problem opening block file
```
sipa commented at 3:54 PM on January 30, 2013:

fReindex=false is set immediately after the while loop anyway, just break suffices. I prefer not changing fReindex without writing it to the block index.

Diapolo commented at 6:13 PM on January 30, 2013:

The change here is intended to give "Reindexing aborted" in the log (nothing more in the end, as I found it weird to read Reindexing finished after a crash).

I understand your wish to link setting fReindex with writing the reindex-state to block index. But if fRequestShutdown is getting true we also don't write it?

Edit: What about using nFile = -1 as a check that OpenBlockFile() failed ;)? Then no need to set fReindex = false here.

Diapolo commented at 6:35 PM on January 30, 2013:

Changed this to use the flag nFile = -1 now.
in src/init.cpp:None in b49cbe53eb outdated
```
 746 | @@ -740,7 +747,13 @@ bool AppInit2()
 747 |  
 748 |      // ********************************************************* Step 7: load block chain
 749 |  
 750 | +    filesystem::path blocksDir = GetDataDir() / "blocks";
 751 | +
 752 |      fReindex = GetBoolArg("-reindex");
 753 | +    if (fReindex && !filesystem::exists(blocksDir)) {
```
sipa commented at 3:55 PM on January 30, 2013:

Is this necessary? it will fail if the directory exists but is empty, for example. Trying to do a reindex shouldn't hurt.

Diapolo commented at 6:15 PM on January 30, 2013:

This check prevents the crash for me, you are right perhaps it should use ::is_empty(). If you think there is no crash or problem, did you try -reindex with empty data dir yet?

Diapolo commented at 6:36 PM on January 30, 2013:

Reworked to now check if the dir exists and if it's empty.

in src/init.cpp:None in e5a83b70ad

 354 | @@ -355,8 +355,10 @@ void ThreadImport(void *data) {
 355 |          while (!fRequestShutdown) {
 356 |              CDiskBlockPos pos(nFile, 0);
 357 |              FILE *file = OpenBlockFile(pos, true);
 358 | -            if (!file)
 359 | +            if (!file) {
 360 | +                nFile = -1; // use -1 as a flag for reindexing aborted

sipa commented at 7:23 PM on January 30, 2013:

That will cause it to always report aborted, no? We iterate until we can't open any file anymore.

in src/init.cpp:None in e5a83b70ad

 744 | @@ -740,7 +745,14 @@ bool AppInit2()
 745 |  
 746 |      // ********************************************************* Step 7: load block chain
 747 |  
 748 | +    filesystem::path blocksDir = GetDataDir() / "blocks";
 749 | +
 750 |      fReindex = GetBoolArg("-reindex");
 751 | +    // don't try to reindex, if blocks dir is missing or empty

sipa commented at 7:24 PM on January 30, 2013:

I don't think this is the correct solution. Whether the directory exist or not shouldn't ever cause a crash. If it does, that's a bug elsewhere.

Diapolo closed this on Jan 30, 2013
DrahtBot locked this on Sep 8, 2021

Contributors

Diapolo

sipa

Linked (view graph)

#2239 Qt: specifiying -reindex with empty datadir leads to crash