pindexFirst->prev can overstep bounds in GetNextWorkRequired #3290

Regarding the "Mac Corruption Bug" as discussed here:

https://bitcointalk.org/index.php?topic=337294

I noticed a number of people quoting this kind of error:

Assertion failed: (pindexFirst), function GetNextWorkRequired, file ../litecoin/src/main.cpp, line 1149.

The relevant code in Bitcoin is in src/main.cpp:1026 (as of commit d980f9b7d687a1e60eecf3691b592d9806a30f4a):

// Go back by what we want to be 14 days worth of blocks
const CBlockIndex* pindexFirst = pindexLast;
for (int i = 0; pindexFirst && i < nInterval-1; i++)
    pindexFirst = pindexFirst->pprev;
assert(pindexFirst);

I assume that the error is due to the # of pindexFirst->pprev links being less than nInterval-1. In that scenario, this code would walk pindexFirst all the way back until pprev is NULL. The code then is faulty in that it is overstepping the walking back of pindexFirst by one. Recommended code fix is to change the test as below:

// Go back by what we want to be 14 days worth of blocks
const CBlockIndex* pindexFirst = pindexLast;
for (int i = 0; pindexFirst->pprev && i < nInterval-1; i++)
    pindexFirst = pindexFirst->pprev;
assert(pindexFirst);

The check for pindexLast being NULL is done in line 1031, so we are safe to check pindexFirst->pprev.

laanwj: edited title, as this is not about the mac corruption bug

I think this fix makes sense, unless it's supposed to be a fatal error to end up in GetWorkRequired without at least two weeks of blocks.

This fix will just mask another problem.

The first part of the function determines whether a retarget is necessary, which is only when the parent's block height + 1 is a multiple of 2016.

The assertion fails if we end up at NULL by going back 2015 blocks, which should always be safe, as the earliest height at which a reorg is necessary, is at height 2015.

My guess is that either the CBlockIndex tree is total garbage, or we have a block with height == -1.

This is a very common way that database corruption is detected.

Maybe we could make an blockdberror_assert() function that offers to reindex when it is triggered.

The Mac leveldb corruption bug has been solved.

There is already another issue for improving leveldb error reporting/offer rebuilds instead of assertion errors: #2202 . Closing this one.