Check for missing undo files at startup, making the check somewhat more ... #4515

rdponticelli commented at 7:48 PM on July 12, 2014: contributor

...efficient.

This is also an incremental step in making an index pruner for #4481

sipa commented at 8:12 PM on July 12, 2014: member

What if the first encountered block with file N doesn't have undo data, but the second one does? You'd skip checking it.

EDIT: fixed

in src/main.cpp:None in d000f7f0c7 outdated

2983 | @@ -2984,19 +2984,30 @@ bool static LoadBlockIndexDB()
2984 |  
2985 |      // Check presence of blk files
2986 |      LogPrintf("Checking all blk files are present...\n");
2987 | -    set<int> setBlkDataFiles;
2988 | +    map<int, bool> mapBlkDataFileReadable,mapBlkUndoFileReadable;

sipa commented at 6:04 PM on July 13, 2014:

Why the map and bool? Just a set for both suffices.

rdponticelli commented at 6:09 PM on July 13, 2014:

Because if we go the #4481 route, we'll have to expand the logic to better manage the cases when the files are missing without failing.

rdponticelli commented at 12:17 PM on July 16, 2014: contributor

Closing this, as it has been rebased, refactored and pushed as part of #4481.

rdponticelli closed this on Jul 16, 2014

rdponticelli commented at 10:13 PM on July 17, 2014: contributor

I simplified this code. Reopening.

rdponticelli reopened this on Jul 17, 2014

laanwj added the label UTXO Db and Indexes on Jul 31, 2014

in src/main.cpp:None in 18fe307de7 outdated

3006 | @@ -3025,6 +3007,50 @@ bool static LoadBlockIndexDB()
3007 |          DateTimeStrFormat("%Y-%m-%d %H:%M:%S", chainActive.Tip()->GetBlockTime()),
3008 |          Checkpoints::GuessVerificationProgress(chainActive.Tip()));
3009 |  
3010 | +    // Check presence of essential data
3011 | +    LogPrintf("Checking essential data is accesible...\n");

laanwj commented at 3:54 PM on August 4, 2014:

Now that this has been moved to the end of the function, consider factoring it out to a separate function (eg. CheckForBlockFiles()) and calling that separately. After all, this is not part of loading the block index DB.

in src/main.cpp:None in 8b8d083039 outdated

2984 | +}
2985 |  
2986 | +bool CheckBlockFiles()
2987 | +{
2988 | +    // Check presence of essential data
2989 | +    LogPrintf("Checking essential data is accesible...\n");

Diapolo commented at 8:32 AM on August 5, 2014:

Idea: Checking required block data is available...?

in src/main.cpp:None in 8b8d083039 outdated

3021 | +        else
3022 | +        {
3023 | +            // Block 0 doesn't have undo data.
3024 | +            if (pindex->nHeight)
3025 | +            {
3026 | +                LogPrintf("Missing undo for block: %i\n", pindex->nHeight);

Diapolo commented at 8:32 AM on August 5, 2014:

I would prepend an Error: and also log which file exactly is missing (also a suggestion for normal block files).

rdponticelli commented at 9:37 PM on August 5, 2014:

"Error:" prepended, but notice that this condition doesn't mean a file is missing, but that we don't have the bit HAVE_UNDO set in the index for this block. If the file were missing, it would be logged by the function called in line 3015. So, there's no need to log a file name here, IMHO.

in src/main.cpp:None in 43795dff47 outdated

3021 | +        else
3022 | +        {
3023 | +            // Block 0 doesn't have undo data.
3024 | +            if (pindex->nHeight)
3025 | +            {
3026 | +                LogPrintf("Error: Missing undo for block: %i\n", pindex->nHeight);

Diapolo commented at 6:06 AM on August 6, 2014:

Nit: undo data also fits better in here, but no need to add, if this is going to be merged soon.

rdponticelli commented at 5:52 PM on August 6, 2014: contributor

Feedback addressed, and some bugs were fixed, and some things improved along the way. Would this be mergeable?

rdponticelli commented at 2:10 PM on August 14, 2014: contributor

Code style fixed to adhere to current standards.

rdponticelli force-pushed on Aug 26, 2014

BitcoinPullTester commented at 1:39 PM on August 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4515_bac3df7ed9c9627d54c9887702182adf9adf21df/ 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.

rdponticelli closed this on Sep 18, 2014

Refactor startup checks onto a separate function. 7694ec98a9

Split the logic to check if a block file can be opened to a separate function. 19f870d867

Add undo data to checks to be sure all required info can be opened. 30ff651bc2

Scan active chain instead of map of blocks. 899cbc76b2

rdponticelli commented at 1:38 PM on November 20, 2014: contributor

I guess this is too late for 0.10, but reopening this anyway as I don't expect this code to change (a lot) any further, and it feels like it would make sense merging this independently of #4701.

rdponticelli reopened this on Nov 20, 2014

rdponticelli force-pushed on Nov 20, 2014

in src/main.cpp:None in 899cbc76b2

2897 | +    // Check presence of blk files
2898 | +    LogPrintf("Checking all blk files are present...\n");
2899 | +    set<int> setRequiredDataFilesAreOpenable;
2900 | +    for (CBlockIndex* pindex = chainActive.Tip(); pindex && pindex->pprev; pindex = pindex->pprev) {
2901 | +        if (pindex->nStatus & BLOCK_HAVE_DATA && pindex->nStatus & BLOCK_HAVE_UNDO) {
2902 | +            if (!setRequiredDataFilesAreOpenable.count(pindex->nFile)) {

laanwj commented at 1:20 PM on March 18, 2015:

It's not clear to me why you switched from the two-pass solution to a single loop, but from a readability viewpoint I'd prefer something like:

std::map<int,uint32_t> mapBlkDataFiles;
BOOST_FOREACH(const PAIRTYPE(uint256, CBlockIndex*)& item, mapBlockIndex)
{
    CBlockIndex* pindex = item.second;
    mapBlkDataFiles[pindex->nFile] |= pindex->nStatus;
}
for (std::map<int,uint32_t>::iterator it = mapBlkDataFiles.begin(); it != mapBlkDataFiles.end(); it++)
{
    if ((it->second & BLOCK_HAVE_DATA) && !BlockFileIsOpenable(it->first)) {
        LogPrintf("Error: Required block file %i can't be opened.\n", it->first);
        return false;
    }
    if ((it->second & BLOCK_HAVE_UNDO) && !UndoFileIsOpenable(it->first)) {
        LogPrintf("Error: Required undo file %i can't be opened.\n", it->first);
        return false;
    }
}

in src/main.cpp:None in 899cbc76b2

2872 | @@ -2891,6 +2873,48 @@ bool static LoadBlockIndexDB()
2873 |      return true;
2874 |  }
2875 |  
2876 | +bool BlockFileIsOpenable(int nFile)

laanwj commented at 1:21 PM on March 18, 2015:

These functions can be static

in src/main.h:None in 899cbc76b2

 172 | @@ -173,6 +173,8 @@ bool LoadExternalBlockFile(FILE* fileIn, CDiskBlockPos *dbp = NULL);
 173 |  bool InitBlockIndex();
 174 |  /** Load the block tree and coins database from disk */
 175 |  bool LoadBlockIndex();
 176 | +/** Check all required block files are present */

laanwj commented at 1:23 PM on March 18, 2015:

/** Check all required block and undo files are present */

laanwj commented at 5:04 PM on March 24, 2015: member

Looks like this is not necessary for the current autoprune (#5863), so I'm closing this. Let me know if I'm wrong.

laanwj closed this on Mar 24, 2015

MarcoFalke locked this on Sep 8, 2021