Add a consistency check for the block chain data structures #5900

pull sipa wants to merge 1 commits into bitcoin:master from sipa:checkblockindex changing 6 files +153 −12

sipa commented at 5:30 PM on March 14, 2015: member

This adds a -checkblockindex (defaulting to true for regtest), which occasionally does a full consistency check for mapBlockIndex, setBlockIndexCandidates, chainActive, and mapBlocksUnlinked.
sipa force-pushed on Mar 14, 2015
laanwj added the label UTXO Db and Indexes on Mar 16, 2015
sipa force-pushed on Mar 23, 2015
sipa force-pushed on Mar 23, 2015
sipa force-pushed on Mar 23, 2015
sipa force-pushed on Mar 26, 2015

Add a consistency check for the block chain data structures

This adds a -checkblockindex (defaulting to true for regtest), which occasionally
does a full consistency check for mapBlockIndex, setBlockIndexCandidates, chainActive, and
mapBlocksUnlinked.

3fcfbc8ac5

sipa force-pushed on Mar 27, 2015
sipa commented at 11:31 PM on March 27, 2015: member

Ready for review.

Ping @morcos.

laanwj commented at 10:54 AM on March 30, 2015: member

I wanted to do a full sync with this enabled, but the extra checking is slow, even at the lower blocks it takes about a second per block (executing the test twice, sometimes it happens more often);

2015-03-30 10:51:32 Checked block index in 645ms
2015-03-30 10:51:32 Checked block index in 565ms
2015-03-30 10:51:32 UpdateTip: new best=00000000bc55e94d87fc6f2565abb2b781cf0b588b2c65f5fcb91e38a69ec93e  height=8189  log2_work=44.99967  tx=8273  date=2009-03-21 01:25:02 progress=0.000058  cache=23
2015-03-30 10:51:33 Checked block index in 647ms
2015-03-30 10:51:34 Checked block index in 565ms
2015-03-30 10:51:34 UpdateTip: new best=000000002136a553f9bf7ba9979781548453aaf836e275febf273cc1c67dd3fe  height=8190  log2_work=44.999846  tx=8274  date=2009-03-21 01:27:49 progress=0.000058  cache=24
2015-03-30 10:51:34 Checked block index in 665ms
2015-03-30 10:51:35 Checked block index in 576ms
2015-03-30 10:51:35 UpdateTip: new best=000000007b8668d5592a7121834c43237d7cfd28104f6276eebedab94901ff66  height=8191  log2_work=45.000022  tx=8275  date=2009-03-21 01:43:09 progress=0.000058  cache=25
2015-03-30 10:51:36 Checked block index in 653ms

Not a huge issue for a hidden troubleshooting option, though enabling this by default on regtest could slow down the RPC tests.

sdaftuar commented at 9:40 PM on March 31, 2015: member

I've been looking at this in the context of #5863 and so far this code has found one bug we introduced there, in LoadBlockIndexDB (we have an incorrect test for adding things to mapBlocksUnlinked). Still working my way through to determine if this catches any other bugs.

I've also started to identify the tests/assumptions introduced in this code which would need to change to support autoprune, would it be helpful to discuss those here?
sipa commented at 1:34 AM on April 1, 2015: member

@laanwj A full sync is probably not very interesting, as it's mostly a single linear synchronization run.

We could perhaps have -checkblockindex=[float] to only run the test with the specified frequency, or have it only check blocks with height above a certain number or not in the main chain.
laanwj commented at 7:07 AM on April 1, 2015: member

@sipa OK, I don't think that needs to be changed, likely this is based on a misunderstanding. I was assuming it was just another level of checking that can be enabled relatively innocuously during normal use, but it has too much impact for that.

Could use a little bit of documentation (maybe in -help-debug?) about when and how to use this mode.
in src/main.cpp:None in 3fcfbc8ac5
```
3248 | +    CBlockIndex* pindexFirstInvalid = NULL; // Oldest ancestor of pindex which is invalid.
3249 | +    CBlockIndex* pindexFirstMissing = NULL; // Oldest ancestor of pindex which does not have BLOCK_HAVE_DATA.
3250 | +    CBlockIndex* pindexFirstNotTreeValid = NULL; // Oldest ancestor of pindex which does not have BLOCK_VALID_TREE (regardless of being valid or not).
3251 | +    CBlockIndex* pindexFirstNotChainValid = NULL; // Oldest ancestor of pindex which does not have BLOCK_VALID_CHAIN (regardless of being valid or not).
3252 | +    CBlockIndex* pindexFirstNotScriptsValid = NULL; // Oldest ancestor of pindex which does not have BLOCK_VALID_SCRIPTS (regardless of being valid or not).
3253 | +    while (pindex != NULL) {
```
sdaftuar commented at 2:59 PM on April 1, 2015:

It looks like there aren't any explicit consistency checks for BLOCK_VALID_TRANSACTIONS. As I understand it there are two properties:

BLOCK_VALID_TRANSACTIONS implies all parents are at least TREE. We get this test implicitly since we already check that for everything TREE or greater, all its parents are at least TREE, so I don't think an additional test is needed for this condition.

If pindex and all parents are at least BLOCK_VALID_TRANSACTIONS then nChainTx is set. I don't believe this consistency check is performed anywhere -- seems like this is worth adding? I think right now the only check on nChainTx is based on whether pindexFirstMissing is set (a few lines down).
sipa commented at 5:56 PM on April 1, 2015:

This sounds correct to me. Feel free to PR an extra check for this?

sdaftuar commented at 7:38 PM on April 1, 2015:

Yep -- please see #5959
laanwj merged this on Apr 1, 2015
laanwj closed this on Apr 1, 2015
laanwj referenced this in commit 3e8a1f2725 on Apr 1, 2015
laanwj referenced this in commit 2b7636c3d6 on Apr 1, 2015
laanwj referenced this in commit fe3122580e on Apr 1, 2015
morcos commented at 3:01 AM on April 2, 2015: member

Belated ACK
dexX7 referenced this in commit 88d6c5eb95 on Apr 27, 2015
dexX7 referenced this in commit 0dcd7f203f on May 16, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#5953 Add a consistency check for the block chain data structures (0.10)