Page MenuHomePhabricator

Fix some chainstate-init-order bugs.
ClosedPublic

Authored by deadalnix on Oct 9 2018, 18:18.

Details

Summary
  1. Fix some LoadChainTip-related init-order bugs.
  • Move the writing of fTxIndex to LoadBlockIndex - this fixes a bug introduced in d6af06d68aae985436cbc942f0d11078041d121b where InitBlockIndex was writing to fTxIndex which had not yet been checked (because LoadChainTip hadn't yet initialized the chainActive, which would otherwise have resulted in InitBlockIndex being a NOP), allowing you to modify -txindex without reindex, potentially corrupting your chainstate!
  • Rename InitBlockIndex to LoadGenesisBlock, which is now a more natural name for it. Also check mapBlockIndex instead of chainActive, fixing a bug where we'd write the genesis block out on every start.
  1. More user-friendly error message if UTXO DB runs ahead of block DB

This gives LoadChainTip a return value - allowing it to indicate that
the UTXO DB ran ahead of the block DB. This just provides a nicer
error message instead of the previous mysterious
assert(!setBlockIndexCandidates.empty()) error.

This also calls ActivateBestChain in case we just loaded the genesis
block in LoadChainTip, avoiding relying on the ActivateBestChain
in ThreadImport before continuing init process.

  1. Call RewindBlockIndex even if we're about to run -reindex-chainstate

RewindBlockIndex works over both chainActive - disconnecting blocks
from the tip that need witness verification - and mapBlockIndex -
requiring redownload of blocks missing witness data.

It should never have been the case that the second half is skipped
if we're about to run -reindex-chainstate.

  1. Order chainstate init more logically.
  • Order chainstate init more logically - first all of the blocktree-related loading, then coinsdb, then pcoinsTip/chainActive. Only create objects as needed.
  • More clearly document exactly what is and isn't called in -reindex and -reindex-chainstate both with comments noting calls as no-ops and by adding if guards.
  • Move LoadGenesisBlock further down in init. This is a more logical location for it, as it is after all of the blockindex-related loading and checking, but before any of the UTXO-related loading and checking.
  • Move all of the VerifyDB()-related stuff into a -reindex + -reindex-chainstate if guard. It couldn't do anything useful as chainActive.Tip() would be null at this point anyway.
  1. Fix segfault when shutting down before fully loading

This was introduced by 3192975f1d177aa9f0bbd823c6387cfbfa943610.
It can be triggered easily when canceling DB upgrade from
pre-per-utxo.

This is a backport of Core PR10758

Depends on D1906 and D1905

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fixinitchainstate
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3557
Build 5190: Bitcoin ABC Buildbot (legacy)
Build 5189: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Oct 9 2018, 23:42
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/init.cpp
246 ↗(On Diff #5367)

Any reason the logic is duplicated here?

2063 ↗(On Diff #5367)

in -> if

2136 ↗(On Diff #5367)

Looks like a merge error switching from constant MAX_FUTURE_BLOCK_TIME to magic numbers.

src/validation.cpp
4428 ↗(On Diff #5367)

Confused about this, it says FlushStateToDisk is skipped, but calls it anyway?

This revision now requires changes to proceed.Oct 9 2018, 23:42
deadalnix added inline comments.
src/init.cpp
246 ↗(On Diff #5367)

Not sure, but the logic is there in the PR as well. Either double check locking, or some code cruft.

src/validation.cpp
4428 ↗(On Diff #5367)

I assume this corresponds to skip in the case the tip is null ?

jasonbcox added inline comments.
src/validation.cpp
4428 ↗(On Diff #5367)

I suppose so.

This revision is now accepted and ready to land.Oct 12 2018, 20:57
This revision was automatically updated to reflect the committed changes.