Fix more init bugs. #10919

TheBlueMatt · 2017-07-24T18:58:02Z

This is a follow-on to #10758 to help move 10758 along. The first fixes a regression in master that was partially fixed in 10758, the second I'm not sure if its a regression or not, but its clearly a bug that should be fixed.

morcos · 2017-07-27T17:07:09Z

utACK 818bb29

Similar to #10758, I think its hard to reason about all the possible edge cases here, but this is a step in the right direction and I think on balance better code than not having it.

This resolves a possible-assert-on-shutdown race introduced in 1f668b6 when early shutdown occurs. Previously this was not done to avoid any cases where the threadGroup might not exit due to a blocking thread, but at this point the threadGroup isn't used all that much, plus Qt already does this, and its good to keep their init/shutdown consistent. For those curious, the threadGroup is only used in a few places: * Its used to run the CCheckQueues in script validation, but these use the boost mutex/condition variable primitives, so they respect the interrupt pretty trivially. * Its used for the import thread, which should exit rather quickly as mostly it just calls LoadExternalBlockFile, which has an interruption_point right before each block loaded. * Its used in the scheduler thread, which is only used for: * validationinterface has an effectively-dummy reference to it. * wallet compaction, which should not last long * addr/banlist dumping from CConnman, which should also be fast

This more clearly uses fReindex vs fReset to make sure we're not clearing our coinsdb needlessly when restarting after a reindex. It also makes it so that restarting after shutting down mid-reindex isn't treates specially at all during txdb loading code, as it shouldn't be.

@morcos

This fixes a few cases where we should be treating a restart-after- coinsviewdb-reset identically to a just-reset-coinsviewdb. Thanks to @morcos for identifying the bug.

TheBlueMatt · 2017-08-01T21:02:49Z

Rebased and addressed some of the comments from @sdaftuar at #10758.

ryanofsky

Low-confidence utACK e7539f8. Changes make sense, though I'm not extremely familiar with this code, so I don't have a lot of confidence that they couldn't potentially cause problems.

ryanofsky · 2017-08-03T15:52:21Z

src/bitcoind.cpp

-        // threadGroup.join_all(); was left out intentionally here, because we didn't re-test all of
-        // the startup-failure cases to make sure they don't result in a hang due to some
-        // thread-blocking-waiting-for-another-thread-during-startup case
+        threadGroup.join_all();


In commit "Always wait for threadGroup to exit in bitcoind shutdown"

This is a nice change and seems safe given what you describe in your commit message. I guess we should look out for shutdown issues with #10286, though, since after #10286, the part about "validationinterface has an effectively-dummy reference to [CScheduler]" will no longer be true

ryanofsky · 2017-08-03T16:19:55Z

src/init.cpp

@@ -1414,6 +1414,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
                // LoadBlockIndex will load fTxIndex from the db, or set it if
                // we're reindexing. It will also load fHavePruned if we've
                // ever removed a block file from disk.
+                // Note that it also sets fReindex based on the disk flag!
+                // From here on out fReindex and fReset mean something different!


In commit "Fix resume-of-reindex-after-restart"

Maybe make this more specific. You could say "From here on out fReindex may be true even if fReset is false (indicating the node was shut down mid-reindex)."

sdaftuar · 2017-08-03T18:13:20Z

I'm not sure of the first commit:
efac91e Always wait for threadGroup to exit in bitcoind shutdown

but utACK for the rest.

laanwj · 2017-08-07T07:04:13Z

utACK e7539f8

e7539f8 Fix some broken init-time prints/constants (Matt Corallo) 13ab353 Check for empty coinsview instead of just-reset coinsview in init (Matt Corallo) fce3f4f Fix resume-of-reindex-after-restart (Matt Corallo) efac91e Always wait for threadGroup to exit in bitcoind shutdown (Matt Corallo) Pull request description: This is a follow-on to #10758 to help move 10758 along. The first fixes a regression in master that was partially fixed in 10758, the second I'm not sure if its a regression or not, but its clearly a bug that should be fixed. Tree-SHA512: aca7b97a97dca66e1a218a33cc6f4aa002292ff1bb0af64e35b81fbaa91b9504f2605375808b43e93a63fc73634ad079b30ef6c9f4ba338d3b5f72d816dfeaff

861f9a2 Skip remainder of init if upgrade is cancelled (Matt Corallo) Pull request description: Based on #10919. Without this, if you cancel upgrade, you get a needless error: ERROR: VerifyDB(): *** irrecoverable inconsistency in block data at Tree-SHA512: aa47665682c6605ada376f1c100ce17cf8c4312427929eb2e75306f2199b47cbcdb4e0d98d5efcfefff03947b2c0fcbd3aab487a4ed14d50607df685c91a03d0

861f9a2 Skip remainder of init if upgrade is cancelled (Matt Corallo) Pull request description: Based on bitcoin#10919. Without this, if you cancel upgrade, you get a needless error: ERROR: VerifyDB(): *** irrecoverable inconsistency in block data at Tree-SHA512: aa47665682c6605ada376f1c100ce17cf8c4312427929eb2e75306f2199b47cbcdb4e0d98d5efcfefff03947b2c0fcbd3aab487a4ed14d50607df685c91a03d0

e7539f8 Fix some broken init-time prints/constants (Matt Corallo) 13ab353 Check for empty coinsview instead of just-reset coinsview in init (Matt Corallo) fce3f4f Fix resume-of-reindex-after-restart (Matt Corallo) efac91e Always wait for threadGroup to exit in bitcoind shutdown (Matt Corallo) Pull request description: This is a follow-on to bitcoin#10758 to help move 10758 along. The first fixes a regression in master that was partially fixed in 10758, the second I'm not sure if its a regression or not, but its clearly a bug that should be fixed. Tree-SHA512: aca7b97a97dca66e1a218a33cc6f4aa002292ff1bb0af64e35b81fbaa91b9504f2605375808b43e93a63fc73634ad079b30ef6c9f4ba338d3b5f72d816dfeaff

73d7288 [Doc] Mention reindex-chainstate in the release notes (random-zebra) c45c0ec Skip MoneySupply update during reindex/reindex-chainstate (random-zebra) efe193f Scan for better chains in ThreadImport (random-zebra) 0aceea3 Make ProcessNewBlock dbp const and update comment (Pieter Wuille) 8255e82 [Cleanup] Remove fVerifyingBlocks global flag (random-zebra) 30c7aed Add -reindex-chainstate that does not rebuild block index (random-zebra) 79d5cb2 Verify DB with original default check-level 3 (random-zebra) ed96859 Reduce default number of blocks to check at startup (Pieter Wuille) d4516fa Log/report in 10% steps during VerifyDB (Jonas Schnelli) cce2c9e doc: Mention dbcache increase in release notes (random-zebra) 11dc022 [GUI] Update default db cache size in the options model (random-zebra) 9b7f764 Bump `-dbcache` default to 300MiB (Wladimir J. van der Laan) Pull request description: This was attempted already in #1864 and #1907. Third time's a charm. Now the speed is acceptable and (unlike what was happening in #1907) the process can be interrupted. Further, as it is performed in ThreadImport, the GUI is open and functional during the chainstate reindex, instead of being seemingly stuck at the splash screen. This PR also includes a few improvements coming from bitcoin#10919, bitcoin#8273, bitcoin#8136 and bitcoin#8611. Here's a quick comparison of the data I had testing here (empty wallet with GUI, empty pivx.conf / default values). The reindexes have been performed with 0 network connections. <table> <tr> <th>Mainnet (height=2847011)</th><th>Master</th><th>this PR</th> </tr><tr> <td>online sync</td><td>8 hr 13 min</td><td>6 hr 42 min</td> </tr><tr> <td>-reindex (offline)</td><td>7 hr 14 min</td><td>6 hr 21 min</td> </tr><tr> <td>-reindex-chainstate (offline)</td><td>N/A</td><td>6 hr 13 min</td> </tr><tr> <td>startup (synced node)</td><td>100 sec</td><td>83 sec</td> </tr><tr> <td>RAM (synced node)</td><td>1.65 GiB</td><td>1.5 GiB</td> </tr><tr> <td>utxo_cache (synced node)</td><td>28.9MiB</td><td>39.5MiB</td> </tr> </table> ACKs for top commit: furszy: re-ACK 73d7288, tested again. Tree-SHA512: fb828d89692ccb6105eae5411b70a3c7a129d98d3101121cf604171de1b818a8c68b3771cc69fcf0a04642fe1327ee7ce360a72ecd7ef019b1614bd83c081de5

TheBlueMatt mentioned this pull request Jul 24, 2017

Fix some chainstate-init-order bugs. #10758

Merged

fanquake added this to the 0.15.0 milestone Jul 25, 2017

laanwj added the Bug label Jul 25, 2017

TheBlueMatt force-pushed the 2017-07-init-bugs branch 2 times, most recently from 479efd7 to 818bb29 Compare July 26, 2017 21:09

TheBlueMatt added 4 commits August 1, 2017 16:35

Check for empty coinsview instead of just-reset coinsview in init

13ab353

This fixes a few cases where we should be treating a restart-after- coinsviewdb-reset identically to a just-reset-coinsviewdb. Thanks to @morcos for identifying the bug.

Fix some broken init-time prints/constants

e7539f8

TheBlueMatt force-pushed the 2017-07-init-bugs branch from 818bb29 to e7539f8 Compare August 1, 2017 21:02

ryanofsky reviewed Aug 3, 2017

View reviewed changes

TheBlueMatt mentioned this pull request Aug 7, 2017

Fix upgrade cancel warnings #10998

Merged

laanwj merged commit e7539f8 into bitcoin:master Aug 7, 2017

random-zebra mentioned this pull request May 16, 2021

[Core] Add reindex-chainstate startup option PIVX-Project/PIVX#2391

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix more init bugs. #10919

Fix more init bugs. #10919

Uh oh!

TheBlueMatt commented Jul 24, 2017 •

edited

Loading

Uh oh!

morcos commented Jul 27, 2017

Uh oh!

TheBlueMatt commented Aug 1, 2017

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Aug 3, 2017

Uh oh!

ryanofsky Aug 3, 2017

Uh oh!

sdaftuar commented Aug 3, 2017

Uh oh!

laanwj commented Aug 7, 2017 •

edited

Loading

Uh oh!

Uh oh!

Fix more init bugs. #10919

Fix more init bugs. #10919

Uh oh!

Conversation

TheBlueMatt commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morcos commented Jul 27, 2017

Uh oh!

TheBlueMatt commented Aug 1, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Aug 3, 2017

Uh oh!

laanwj commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Jul 24, 2017 •

edited

Loading

laanwj commented Aug 7, 2017 •

edited

Loading