Fix some chainstate-init-order bugs.

TheBlueMatt commented at 0:06 am on July 7, 2017: member

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 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.
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 force-pushed on Jul 7, 2017

TheBlueMatt commented at 0:07 am on July 7, 2017: member

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 force-pushed on Jul 7, 2017

fanquake added the label Refactoring on Jul 7, 2017

TheBlueMatt force-pushed on Jul 7, 2017

TheBlueMatt commented at 1:33 am on July 7, 2017: member

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 commented at 5:45 pm on July 10, 2017: member

This needs an 0.15 tag as it fixes some bugs.

TheBlueMatt force-pushed on Jul 16, 2017

TheBlueMatt commented at 0:05 am on July 16, 2017: member

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

in src/validation.cpp:3875 in ccc35ffdc0 outdated

3872+        if (!ret) return false;
3873+        needs_init = mapBlockIndex.empty();
3874+    }
3875+
3876+    if (needs_init) {
3877+        // Note that LoadBlockIndexDB may have set fReindex if we shut down

sipa commented at 0:42 am on July 16, 2017:

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

TheBlueMatt commented at 10:44 pm on July 16, 2017:

OK, rewrote the comment.

sipa commented at 2:22 am on July 16, 2017: member

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

TheBlueMatt force-pushed on Jul 16, 2017

sipa commented at 5:18 am on July 17, 2017: member

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

sipa commented at 5:48 pm on July 17, 2017: member

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 commented at 5:49 pm on July 17, 2017: member

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

sipa added this to the milestone 0.15.0 on Jul 17, 2017

theuni commented at 6:10 pm on July 19, 2017: member

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 commented at 6:33 pm on July 20, 2017: member

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.

laanwj added this to the "Blockers" column in a project

promag commented at 4:03 pm on July 21, 2017: member

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

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

With this branch gives

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

Reporting this here because of c93db42.

TheBlueMatt commented at 5:55 pm on July 23, 2017: member

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 commented at 10:11 pm on July 23, 2017: member

@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 commented at 11:08 pm on July 23, 2017: member

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

TheBlueMatt force-pushed on Jul 24, 2017

TheBlueMatt commented at 6:58 pm on July 24, 2017: member

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 commented at 1:42 am on July 25, 2017: member

reutACK c93db429f909e75fbeacd2419fb8434350e3b674

laanwj added the label Bug on Jul 25, 2017

in src/validation.cpp:3935 in c93db429f9 outdated

3957+            return error("LoadBlockIndex(): writing genesis block to disk failed");
3958+        CBlockIndex *pindex = AddToBlockIndex(block);
3959+        if (!ReceivedBlockTransactions(block, state, pindex, blockPos, chainparams.GetConsensus()))
3960+            return error("LoadBlockIndex(): genesis block not accepted");
3961+    } catch (const std::runtime_error& e) {
3962+        return error("LoadBlockIndex(): failed to initialize block database: %s", e.what());

laanwj commented at 10:44 am on July 27, 2017:

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

in src/validation.cpp:3928 in c93db429f9 outdated

3950+        // Start new block file
3951+        unsigned int nBlockSize = ::GetSerializeSize(block, SER_DISK, CLIENT_VERSION);
3952+        CDiskBlockPos blockPos;
3953+        CValidationState state;
3954+        if (!FindBlockPos(state, blockPos, nBlockSize+8, 0, block.GetBlockTime()))
3955+            return error("LoadBlockIndex(): FindBlockPos failed");

laanwj commented at 10:44 am on July 27, 2017:

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

laanwj commented at 11:19 am on July 27, 2017: member

utACK apart from error message nits

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.

eda888e573

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.

b0f32497b8

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.

ff3a21919d

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.

138569722c

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.

c0025d0a92

TheBlueMatt force-pushed on Jul 27, 2017

TheBlueMatt commented at 7:03 pm on July 27, 2017: member

Fixed error printings in LoadGenesisBlock.

morcos commented at 2:38 am on July 28, 2017: member

re-utACK c0025d0

in src/test/test_bitcoin.cpp:75 in c0025d0a92

71@@ -72,7 +72,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
72         pblocktree = new CBlockTreeDB(1 << 20, true);
73         pcoinsdbview = new CCoinsViewDB(1 << 23, true);
74         pcoinsTip = new CCoinsViewCache(pcoinsdbview);
75-        if (!InitBlockIndex(chainparams)) {
76+        if (!LoadGenesisBlock(chainparams)) {

sdaftuar commented at 3:38 pm on July 31, 2017:

The string on the next line ought to be changed to reflect the new function name.

in src/validation.cpp:3901 in eda888e573 outdated

3923-            if (!ReceivedBlockTransactions(block, state, pindex, blockPos, chainparams.GetConsensus()))
3924-                return error("LoadBlockIndex(): genesis block not accepted");
3925-        } catch (const std::runtime_error& e) {
3926-            return error("LoadBlockIndex(): failed to initialize block database: %s", e.what());
3927-        }
3928+    try {

sdaftuar commented at 5:28 pm on July 31, 2017:

The comment at line 3900 now seems outdated; perhaps it makes more sense to move it to init.cpp:1422?

in src/validation.cpp:3897 in eda888e573 outdated

3897-    if (chainActive.Genesis() != NULL)
3898+    // Check whether we're already initialized by checking for genesis in
3899+    // mapBlockIndex. Note that we can't use chainActive here, since it is
3900+    // set based on the coins db, not the block index db, which is the only
3901+    // thing loaded at this point.
3902+    if (mapBlockIndex.count(chainparams.GenesisBlock().GetHash()))

sdaftuar commented at 5:39 pm on July 31, 2017:

style nit: curly braces for this if

in src/validation.cpp:3558 in b0f32497b8 outdated

3556 
3557     // Load pointer to end of best chain
3558     BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock());
3559     if (it == mapBlockIndex.end())
3560-        return;
3561+        return false;

sdaftuar commented at 6:04 pm on July 31, 2017:

style nit: perhaps add the curly braces here, while you’re at it

in src/init.cpp:1432 in 138569722c outdated

1426@@ -1437,10 +1427,34 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1427                     break;
1428                 }
1429 
1430+                // At this point blocktree args are consistent with what's on disk.
1431+                // If we're not mid-reindex (based on disk + args), add a genesis block on disk.
1432+                // This is called again in ThreadImport in the reindex completes.

sdaftuar commented at 6:25 pm on July 31, 2017:

“in the reindex completes” -> “after the reindex completes”?

sdaftuar approved

sdaftuar commented at 6:45 pm on July 31, 2017: member

utACK, left some nits.

Is it worth backporting the RewindBlockIndex bugfix to the 0.13 or 0.14 branch? I could imagine someone upgrading from 0.13.0 to the latest 0.13 release after segwit activates. Or to the latest 0.14 if the 0.15.0 release comes out after activation?

laanwj commented at 10:50 am on August 1, 2017: member

I’m going to merge this now - sorry @sdaftuar, we should fix your minor nits later, but we have to make some progress with merging for 0.15

laanwj merged this on Aug 1, 2017

laanwj closed this on Aug 1, 2017

laanwj referenced this in commit bd924241e7 on Aug 1, 2017

sdaftuar commented at 5:22 pm on August 1, 2017: member

@laanwj Agreed, I don’t think those nits should have held this up.

fanquake removed this from the "Blockers" column in a project

laanwj referenced this in commit c1c671feb1 on Aug 7, 2017

PastaPastaPasta referenced this in commit 950051209d on Sep 16, 2019

PastaPastaPasta referenced this in commit dbd4993084 on Sep 18, 2019

PastaPastaPasta referenced this in commit 953a6b5504 on Sep 18, 2019

PastaPastaPasta referenced this in commit 203b90d57f on Sep 20, 2019

PastaPastaPasta referenced this in commit d5fdf62faa on Sep 20, 2019

barrystyle referenced this in commit 25f442ab48 on Jan 22, 2020

barrystyle referenced this in commit 7be49d120c on Jan 22, 2020

furszy referenced this in commit 2518433d52 on Mar 3, 2021

MarcoFalke locked this on Sep 8, 2021

Fix some chainstate-init-order bugs. #10758