Fix some chainstate-init-order bugs.

[TheBlueMatt]

This does a number of things to clean up chainstate init order,
fixing some issues as it goes:

• 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 the writing of fTxIndex to LoadBlockIndex - this fixes a
  bug introduced in d6af06d 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.

• 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.

• Give 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.

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

• 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.

[TheBlueMatt]

Note that the two generic "Error initializing block database" strings really should be changed, but as we are already in string freeze for 0.15, they will have to stay as-is for now :(.

[TheBlueMatt]

More explicitly fixed the bug that I accidentally fixed by being more explicit - RewindBlockIndex MUST be called even if we start with -reindex-chainstate.

[TheBlueMatt]

This needs an 0.15 tag as it fixes some bugs.

[TheBlueMatt]

Rebased and added a fix for the issue @sipa pointed out at #10770 (comment)

[sipa]

I have difficulty parsing this sentence. Can you split it up?

[TheBlueMatt]

OK, rewrote the comment.

[sipa]

@TheBlueMatt When starting with -reindex, and then restart before completing without -reindex, it seems to start over from scratch.

[sipa]

ACK

@TheBlueMatt I retract my comment, everything seems to be working like before.

[sipa]

I'd like to see this in 0.15 as it at least turns an assert into an error message, in case your chainstate is out of sync with the block chain.

[TheBlueMatt]

This (or some subset of it) absolutely has to land for 15, it fixes several regressions currently on master, but its a bugfix, so doesn't need to land today (but isnt small, so should go sooner rather than later).

[theuni]

I haven't finished reviewing, but this also fixes a crash when using an invalid -wallet argument. For example: ./bitcoind -wallet='with spaces', which crashes for me on master.

[morcos]

utACK c93db42

Strongly advocate for merging this in the near term.
Although I have not gone through testing all sequences, I think this logic is far clearer and more straight forward than the existing logic and there are known bugs in the existing startup sequence.
With all the other changes to databases recently (rewinding, replaying, reindexing), I think it makes sense to have as much testing as possible with this new code rather than waste our time potentially finding more bugs with the old logic.

[promag]

In current master running bitcoind -regtest '-wallet=foo%.dat' gives:

./src/bitcoind -regtest '-wallet=foo%.dat'
Error: Invalid characters in -wallet filename
Segmentation fault (core dumped)

With this branch gives

Error: Invalid characters in -wallet filename
bitcoind: scheduler.cpp:200: void SingleThreadedSchedulerClient::EmptyQueue(): Assertion `!m_pscheduler->AreThreadsServicingQueue()' failed.
Aborted (core dumped)

Reporting this here because of c93db42.

[TheBlueMatt]

Hmm, I went ahead and added the fix for @promag's race here, feel free to tell me it should be in a different PR. (@theuni note that it was a race, which may explain why you didnt see it).

[TheBlueMatt]

@sipa hmm, re: restarting-mid-reindex: your original comment that it was broken was more correct than your later retraction. Fixed and cleaned that stuff up more with more comments :).

[promag]

@TheBlueMatt if it's not related with your refactor then IMO a new PR would be best.

[TheBlueMatt]

OK, removed the top two commits so this is just what it was that got ACKs. The two commits from the top are in #10919, which should likely also be taken for 15.

[sipa]

reutACK c93db42

[laanwj]

This error is no longer correct, this is "failed to load genesis block" now I guess?

[laanwj]

Would be better to use error("%s: ...", __func__) here as in other places, but in any case, the function name is wrong now.

[laanwj]

utACK apart from error message nits

[TheBlueMatt]

Fixed error printings in LoadGenesisBlock.

[morcos]

re-utACK c0025d0