Fix two fast-shutdown bugs

TheBlueMatt commented at 6:45 pm on February 6, 2018: member

The second commit is a much simpler alternative fix for the issue fixed in #12349. To test I made ShutdownRequested() always StartShutdown() after a certain number of calls, which turned up one other hang, fixed in the first commit.

MarcoFalke added this to the milestone 0.16.0 on Feb 6, 2018

laanwj added the label GUI on Feb 6, 2018

in src/init.cpp:1652 in 9afc8a71d6 outdated

1644@@ -1645,8 +1645,8 @@ bool AppInitMain()
1645     // Wait for genesis block to be processed
1646     {
1647         WaitableLock lock(cs_GenesisWait);
1648-        while (!fHaveGenesis) {
1649-            condvar_GenesisWait.wait(lock);
1650+        while (!fHaveGenesis && !ShutdownRequested()) {
1651+            condvar_GenesisWait.wait_for(lock, std::chrono::milliseconds(500));

ryanofsky commented at 6:49 pm on February 6, 2018:

In commit “Fix fast-shutdown hang on ThreadImport+GenesisWait”

Can you add comment explaining why timeout is needed here and wait() is not sufficient?

laanwj commented at 7:28 pm on February 6, 2018:

So this means that if ShutdownRequested() is set it will fall out of this loop, without a genesis block. Don’t we also have to make sure that AppInitMain() immediately exits (with false) in that case? Looks like an error waiting to happen if it proceeds the initialization without genesis block.

in src/validation.cpp:2635 in b1fa6237d4 outdated

2626@@ -2630,6 +2627,9 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
2627         }
2628 
2629         if (nStopAtHeight && pindexNewTip && pindexNewTip->nHeight >= nStopAtHeight) StartShutdown();
2630+
2631+        if (ShutdownRequested())

ryanofsky commented at 7:00 pm on February 6, 2018:

In commit “Fix fast-shutdown crash if genesis block was not loaded”

Would add short comment here like “Only allow shutting down after calling ActivateBestChainStep to avoid problems with shutdown code assuming there is a known best block.”

It’d seem cleaner to just fix the broken flush code (and also probably easier to write a unit test for), but I realise you are trying to implement a quick fix.

ryanofsky commented at 7:02 pm on February 6, 2018: member

utACK b1fa6237d47a291adfbe4562ec001146411f63f9

laanwj added the label Validation on Feb 6, 2018

TheBlueMatt force-pushed on Feb 6, 2018

TheBlueMatt commented at 7:30 pm on February 6, 2018: member

Added comments at @ryanofsky’s request. Also finished testing successive exit-after-N-ShutdownRequested()-calls tests with empty-datadir-start, genesis-block-only-datadir-start, start-with-some-blocks, reindex and reindex-chainstate and turned up no additional issues.

ryanofsky commented at 7:45 pm on February 6, 2018: member

utACK d307a81add95fd83cb3cbaa25109d824d9a94cae. Just new comments since my previous review. (Thanks!)

It does seem to me that #12349 is a more robust and straightforward fix for null hashblock issue, because it’s fixing the problem directly where it occurs, instead of reordering faraway code to work around it. But it’s understandable if you want to fix the problem more quickly and not have to update any tests :)

laanwj commented at 7:49 pm on February 6, 2018: member

@TheBlueMatt You haven’t addressed my comment it seems.

Fix fast-shutdown hang on ThreadImport+GenesisWait

If the user somehow manages to get into ShutdownRequested before
ThreadImport gets to ActivateBestChain() we may hang waiting on
condvar_GenesisWait forever. A simple wait_for and
ShutdownRequested resolves this case.

1c9394ad47

Fix fast-shutdown crash if genesis block was not loaded

If the ShutdownRequested() check at the top of ActivateBestChain()
returns false during initial genesis block load we will fail an
assertion in UTXO DB flush as the best block hash IsNull(). To work
around this, we move the check until after one round of
ActivateBestChainStep(), ensuring the genesis block gets connected.

dd2de47c62

TheBlueMatt force-pushed on Feb 6, 2018

TheBlueMatt commented at 8:14 pm on February 6, 2018: member

@laanwj ah, thanks, github seems to have eaten your comment (I can only see it in the email feed)…fixed anyway.

MarcoFalke added the label Needs backport on Feb 6, 2018

jonasschnelli commented at 5:46 am on February 7, 2018: contributor

utACK dd2de47c6288654abb2c3eef29edcd1cc5f39fc9

dcousens approved

laanwj commented at 9:29 am on February 7, 2018: member

utACK dd2de47

meshcollider commented at 10:04 pm on February 7, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/12367/commits/dd2de47c6288654abb2c3eef29edcd1cc5f39fc9

laanwj merged this on Feb 8, 2018

laanwj closed this on Feb 8, 2018

laanwj referenced this in commit 7217ea2cc8 on Feb 8, 2018

laanwj referenced this in commit 09fc859ef0 on Feb 8, 2018

laanwj referenced this in commit 0f207c488a on Feb 8, 2018

laanwj removed the label Needs backport on Feb 8, 2018

HashUnlimited referenced this in commit fef061c38f on Mar 16, 2018

HashUnlimited referenced this in commit aa80513c11 on Mar 16, 2018

ccebrecos referenced this in commit 1c4c45bd12 on Sep 14, 2018

ccebrecos referenced this in commit df4a32a5cd on Sep 14, 2018

PastaPastaPasta referenced this in commit a7bdf62871 on Mar 14, 2020

PastaPastaPasta referenced this in commit 9eecffff60 on Mar 14, 2020

PastaPastaPasta referenced this in commit 8095699575 on Mar 15, 2020

ckti referenced this in commit 3615ef0c41 on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021

Fix two fast-shutdown bugs #12367