When reindexing check for file before trying to open #4875

pull sdaftuar wants to merge 2 commits into bitcoin:master from sdaftuar:fix-reindex-loop changing 3 files +11 −2

sdaftuar commented at 7:27 PM on September 8, 2014: member

Fix for #4693 -- check for each file before trying to open, so an error is only logged if the file exists but is unreadable.

I thought about refactoring the code so that the way you map from a file number to a path is done in one function only, but the assumption that block files look like "blkNNNNN.dat" is in several places, including comments and debug messages, so wasn't sure such a refactor was warranted compared with this simple fix.
laanwj commented at 11:37 AM on September 10, 2014: member
Thanks for addressing this!

I thought about refactoring the code so that the way you map from a file number to a path is done in one function only, but the assumption that block files look like "blkNNNNN.dat" is in several places

I've looked at the code and tend to agree. One sane way to factor this out may be:
```
boost::filesystem::path GetBlockPosFilename(const CDiskBlockPos &pos, const char *prefix)
{
    return GetDataDir() / "blocks" / strprintf("%s%05u.dat", prefix, pos.nFile);
}
```
This could then be used in OpenDiskFile and here. I wouldn't bother changing it in comments or logging (where you don't want the entire path etc... they are much less important when the files are moved, and the file name is much less likely to change than the path).
When reindexing check for file before trying to open (refactored) ec7eb0fa80
sdaftuar commented at 6:01 PM on September 10, 2014: member

Thanks for the feedback; I redid it the way I think you suggested (and squashed the commits).

However, I wasn't sure whether it might make more sense to make the GetBlockPosFilename function a static member function of CDiskBlockPos rather than add this as a global function? Not sure what makes the most sense for the overall code design.
laanwj commented at 7:25 AM on September 11, 2014: member

@sdaftuar It's fine like this. This is more akin to the rest of the code. ut ACK
sipa commented at 5:21 PM on September 12, 2014: member

ut ACK
jgarzik commented at 1:31 AM on September 15, 2014: contributor

ut ACK
Eliminate extra assignment f7e36370f3

in src/main.cpp:None in ec7eb0fa80 outdated

2792 | @@ -2793,6 +2793,12 @@ FILE* OpenUndoFile(const CDiskBlockPos &pos, bool fReadOnly) {
2793 |      return OpenDiskFile(pos, "rev", fReadOnly);
2794 |  }
2795 |  
2796 | +boost::filesystem::path GetBlockPosFilename(const CDiskBlockPos &pos, const char *prefix)
2797 | +{
2798 | +    boost::filesystem::path path = GetDataDir() / "blocks" / strprintf("%s%05u.dat", prefix, pos.nFile);

sipa commented at 5:12 AM on September 15, 2014:

just use

return GetDataDir() / "blocks" / strprintf("%s%05u.dat", prefix, pos.nFile);

BitcoinPullTester commented at 2:11 PM on September 15, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4875_f7e36370f36ca806076cdcf0cd3ed0c35fd38c42/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
sipa merged this on Sep 16, 2014
sipa closed this on Sep 16, 2014
sipa referenced this in commit 765f398436 on Sep 16, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4693 `-reindex` always shows an error after reindex