Add a consistency check for the block chain data structures #5900
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
070b25f to
2a8bb41
Compare
This adds a -checkblockindex (defaulting to true for regtest), which occasionally does a full consistency check for mapBlockIndex, setBlockIndexCandidates, chainActive, and mapBlocksUnlinked.
|
Ready for review. Ping @morcos. |
|
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); Not a huge issue for a hidden troubleshooting option, though enabling this by default on regtest could slow down the RPC tests. |
|
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? |
|
@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. |
|
@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. |
| CBlockIndex* pindexFirstNotTreeValid = NULL; // Oldest ancestor of pindex which does not have BLOCK_VALID_TREE (regardless of being valid or not). | ||
| CBlockIndex* pindexFirstNotChainValid = NULL; // Oldest ancestor of pindex which does not have BLOCK_VALID_CHAIN (regardless of being valid or not). | ||
| CBlockIndex* pindexFirstNotScriptsValid = NULL; // Oldest ancestor of pindex which does not have BLOCK_VALID_SCRIPTS (regardless of being valid or not). | ||
| while (pindex != NULL) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
This sounds correct to me. Feel free to PR an extra check for this?
laanwj
added a commit
that referenced
this pull request
Apr 1, 2015
3fcfbc8 Add a consistency check for the block chain data structures (Pieter Wuille)
laanwj
added a commit
that referenced
this pull request
Apr 1, 2015
|
Belated ACK |
dexX7
added a commit
to dexX7/bitcoin
that referenced
this pull request
Apr 27, 2015
d8ac901 doc: improve credits in release notes (Gregory Maxwell) bf8ad0d update release notes for 0.10.1rc3 (Wladimir J. van der Laan) 139cd81 Cap nAttempts penalty at 8 and switch to pow instead of a division loop. (Gregory Maxwell) bac6fca Set nSequenceId when a block is fully linked (Suhas Daftuar) 323de27 Initialization: setup environment before starting QT tests (dexX7) 7494e09 Initialization: setup environment before starting tests (dexX7) df45564 Initialization: set fallback locale as environment variable (dexX7) 57d1f46 Fix CheckBlockIndex for reindex. (mrbandrews) eae305f Fix missing lock in submitblock (Matt Corallo) 34127c7 doc: update release notes pre rc2 (Wladimir J. van der Laan) 1c62e84 Keep mempool consistent during block-reorgs (Gavin Andresen) 149c1d8 doc: Credit Jonas Nick in release notes (Wladimir J. van der Laan) bf1cc80 Docs: Use new Bitcoin.org download URLs (David A. Harding) 9e1cc16 doc: add historical release notes for 0.10.0 (Wladimir J. van der Laan) fe31225 update release notes for bitcoin#5953/bitcoin#5900 (Wladimir J. van der Laan) a1f425b Add a consistency check for the block chain data structures (Pieter Wuille) ae1479a update release notes after bitcoin#5941 (Wladimir J. van der Laan) aa587d4 Scale up addrman (Pieter Wuille) 0c6f334 Always use a 50% chance to choose between tried and new entries (Pieter Wuille) 214154e Do not bias outgoing connections towards fresh addresses (Pieter Wuille) 2218d4b Simplify hashing code (Pieter Wuille) cf0218f Make addrman's bucket placement deterministic. (Pieter Wuille) b788994 Switch addrman key from vector to uint256 (Pieter Wuille) 90bef66 No notable changes for minor release (Wladimir J. van der Laan) 4635a4c Translations update from transifex (Wladimir J. van der Laan) 0eccf0a Add commits (up to now) to release notes (Wladimir J. van der Laan) 78f64ef don't trickle for whitelisted nodes (Ruben de Vries) a316622 Clean out release notes for 0.10.1 (Wladimir J. van der Laan)
dexX7
added a commit
to OmniLayer/omnicore
that referenced
this pull request
May 16, 2015
16f4560 doc: small amandment to release notes (Wladimir J. van der Laan) ff32503 Release notes 0.10.2 (Wladimir J. van der Laan) da65606 Avoid crash on start in TestBlockValidity with gen=1. (Gregory Maxwell) 49e4d14 Translations update (Wladimir J. van der Laan) d7e7727 Preparations for 0.10.2 release (Wladimir J. van der Laan) 424ae66 don't imbue boost::filesystem::path with locale "C" on windows (Jonas Schnelli) 824c011 wallet: fix boost::get usage with boost 1.58 (Cory Fields) ebc0e41 qt: translation update for next 0.10 point release (Wladimir J. van der Laan) d8ac901 doc: improve credits in release notes (Gregory Maxwell) bf8ad0d update release notes for 0.10.1rc3 (Wladimir J. van der Laan) 139cd81 Cap nAttempts penalty at 8 and switch to pow instead of a division loop. (Gregory Maxwell) bac6fca Set nSequenceId when a block is fully linked (Suhas Daftuar) 323de27 Initialization: setup environment before starting QT tests (dexX7) 7494e09 Initialization: setup environment before starting tests (dexX7) df45564 Initialization: set fallback locale as environment variable (dexX7) 57d1f46 Fix CheckBlockIndex for reindex. (mrbandrews) eae305f Fix missing lock in submitblock (Matt Corallo) 34127c7 doc: update release notes pre rc2 (Wladimir J. van der Laan) 1c62e84 Keep mempool consistent during block-reorgs (Gavin Andresen) 149c1d8 doc: Credit Jonas Nick in release notes (Wladimir J. van der Laan) bf1cc80 Docs: Use new Bitcoin.org download URLs (David A. Harding) 9e1cc16 doc: add historical release notes for 0.10.0 (Wladimir J. van der Laan) fe31225 update release notes for bitcoin#5953/bitcoin#5900 (Wladimir J. van der Laan) a1f425b Add a consistency check for the block chain data structures (Pieter Wuille) ae1479a update release notes after bitcoin#5941 (Wladimir J. van der Laan) aa587d4 Scale up addrman (Pieter Wuille) 0c6f334 Always use a 50% chance to choose between tried and new entries (Pieter Wuille) 214154e Do not bias outgoing connections towards fresh addresses (Pieter Wuille) 2218d4b Simplify hashing code (Pieter Wuille) cf0218f Make addrman's bucket placement deterministic. (Pieter Wuille) b788994 Switch addrman key from vector to uint256 (Pieter Wuille) 90bef66 No notable changes for minor release (Wladimir J. van der Laan) 4635a4c Translations update from transifex (Wladimir J. van der Laan) 0eccf0a Add commits (up to now) to release notes (Wladimir J. van der Laan) 78f64ef don't trickle for whitelisted nodes (Ruben de Vries) a316622 Clean out release notes for 0.10.1 (Wladimir J. van der Laan)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a -checkblockindex (defaulting to true for regtest), which occasionally does a full consistency check for mapBlockIndex, setBlockIndexCandidates, chainActive, and mapBlocksUnlinked.