Fix two fast-shutdown bugs #12367

pull TheBlueMatt wants to merge 2 commits into bitcoin:master from TheBlueMatt:2018-02-wait-genesis-exit changing 2 files +16 −5

TheBlueMatt commented at 6:45 PM on February 6, 2018: contributor

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.
TheBlueMatt cross-referenced this on Feb 6, 2018 from issue shutdown: fix crash on shutdown with reindex-chainstate by theuni
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: contributor

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: contributor

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: contributor

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: contributor

@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
ryanofsky cross-referenced this on Feb 8, 2018 from issue Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection by ryanofsky
TheBlueMatt cross-referenced this on Mar 2, 2018 from issue Allow abort of ConnectBlock() when shutdown requested. by rebroad
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
str4d cross-referenced this on Apr 12, 2021 from issue Sync backports by str4d
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.16.0

Linked (view graph)

#9719 Allow abort of ConnectBlock() when shutdown requested.#11640 Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection #12349 shutdown: fix crash on shutdown with reindex-chainstate