Fix more init bugs.

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

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.

fanquake added this to the milestone 0.15.0 on Jul 25, 2017

laanwj added the label Bug on Jul 25, 2017

TheBlueMatt force-pushed on Jul 26, 2017

morcos commented at 5:07 pm on July 27, 2017: member

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.

Always wait for threadGroup to exit in bitcoind shutdown

This resolves a possible-assert-on-shutdown race introduced in
1f668b646806f94acd851acdbd9939c24e0492d3 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

efac91e654

Fix resume-of-reindex-after-restart

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.

fce3f4f492

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

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.

13ab353829

Fix some broken init-time prints/constants e7539f8649

TheBlueMatt force-pushed on Aug 1, 2017

TheBlueMatt commented at 9:02 pm on August 1, 2017: member

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

in src/bitcoind.cpp:179 in efac91e654 outdated

175@@ -176,9 +176,7 @@ bool AppInit(int argc, char* argv[])
176     if (!fRet)
177     {
178         Interrupt(threadGroup);
179-        // threadGroup.join_all(); was left out intentionally here, because we didn't re-test all of
180-        // the startup-failure cases to make sure they don't result in a hang due to some
181-        // thread-blocking-waiting-for-another-thread-during-startup case
182+        threadGroup.join_all();

ryanofsky commented at 3:52 pm on August 3, 2017:

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

in src/init.cpp:1418 in fce3f4f492 outdated

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

ryanofsky commented at 4:19 pm on August 3, 2017:

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

ryanofsky commented at 4:57 pm on August 3, 2017: member

Low-confidence utACK e7539f864984740b80efc44e1a8970f4353ff066. 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.

sdaftuar commented at 6:13 pm on August 3, 2017: member

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

but utACK for the rest.

laanwj commented at 7:04 am on August 7, 2017: member

utACK e7539f8

laanwj merged this on Aug 7, 2017

laanwj closed this on Aug 7, 2017

laanwj referenced this in commit c1c671feb1 on Aug 7, 2017

laanwj referenced this in commit 2507fd5556 on Aug 8, 2017

codablock referenced this in commit eeabdb36cd on Sep 27, 2017

codablock referenced this in commit 33bbf1e2be on Oct 12, 2017

codablock referenced this in commit 426e3e0fb5 on Oct 26, 2017

codablock referenced this in commit da5217f43a on Oct 26, 2017

codablock referenced this in commit 83945a6380 on Oct 26, 2017

codablock referenced this in commit 7e3e9e433b on Oct 30, 2017

codablock referenced this in commit bff8b16aa8 on Oct 31, 2017

codablock referenced this in commit ceb0d19b2d on Oct 31, 2017

codablock referenced this in commit 64755903e2 on Oct 31, 2017

codablock referenced this in commit 4cd19913dc on Oct 31, 2017

UdjinM6 referenced this in commit bea88da7aa on Nov 8, 2017

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 7be49d120c on Jan 22, 2020

furszy referenced this in commit 0e264e2607 on Jun 28, 2021

DrahtBot locked this on Sep 8, 2021

Fix more init bugs. #10919