shutdown: fix crash on shutdown with reindex-chainstate

[theuni]

Fixes the assertion error reported here: #12349 (comment)

[maflcko]

Tests need update (test_bitcoin: coins.cpp:205: bool CCoinsViewCache::Flush(): Assertion `cachedCoinsUsage == 0' failed.)

[theuni]

@TheBlueMatt pointed out that this shouldn't be new for 0.16, and I've now reproduced with 0.15.1.

So, this probably shouldn't be an 0.16 blocker.

[TheBlueMatt]

I ran into this a few times during testing, which was somewhat surprising so maybe it is somehow easier to hit now? Not sure why that would be...still, worth fixing if there's an easy fix for 0.16 (eaiser fix is probably to make the ActivateBestChain if (ShutdownRequested()) check a if (ShutdownRequested() && chainActive.Tip() != nullptr) check).

[TheBlueMatt]

I prefer the fix in #12367 as much simpler.

[maflcko]

Closing for now

[eklitzke]

Without this change, I still get a segfault from bitcoind master (commit c997f88) reliably. To reproduce:

• Run bitcoind -reindex-chainstate -daemon=0
• Immediately type Ctrl-c, and wait for bitcoind to stop
• bitcoind segfaults with this stack trace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007f514a479381 in __GI_abort () at abort.c:79
#2  0x00007f514a46f8fa in __assert_fail_base (fmt=0x7f514a5eac28 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x55efac7413fd "!hashBlock.IsNull()", file=file@entry=0x55efac7413f4 "txdb.cpp", line=line@entry=90, 
    function=function@entry=0x55efac741c40 <CCoinsViewDB::BatchWrite(std::unordered_map<COutPoint, CCoinsCacheEntry, SaltedOutpointHasher, std::equal_to<COutPoint>, std::allocator<std::pair<COutPoint const, CCoinsCacheEntry> > >&, uint256 const&)::__PRETTY_FUNCTION__> "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap&, const uint256&)") at assert.c:92
#3  0x00007f514a46f972 in __GI___assert_fail (assertion=0x55efac7413fd "!hashBlock.IsNull()", file=0x55efac7413f4 "txdb.cpp", line=90, 
    function=0x55efac741c40 <CCoinsViewDB::BatchWrite(std::unordered_map<COutPoint, CCoinsCacheEntry, SaltedOutpointHasher, std::equal_to<COutPoint>, std::allocator<std::pair<COutPoint const, CCoinsCacheEntry> > >&, uint256 const&)::__PRETTY_FUNCTION__> "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap&, const uint256&)") at assert.c:101
#4  0x000055efac41d7e1 in CCoinsViewDB::BatchWrite (this=0x55efacf5e9d0, mapCoins=std::unordered_map with 0 elements, hashBlock=...) at txdb.cpp:90
#5  0x000055efac5c3aae in CCoinsViewBacked::BatchWrite (this=0x55efc1c0d150, mapCoins=std::unordered_map with 0 elements, hashBlock=...) at coins.cpp:28
#6  0x000055efac5c4c7a in CCoinsViewCache::Flush (this=0x55efacf6de10) at coins.cpp:204
#7  0x000055efac4882a3 in FlushStateToDisk (chainparams=..., state=..., mode=FLUSH_STATE_ALWAYS, nManualPruneHeight=0) at validation.cpp:2099
#8  0x000055efac4887ed in FlushStateToDisk () at validation.cpp:2118
#9  0x000055efac1ad8c6 in Shutdown () at init.cpp:229
#10 0x000055efac19706a in AppInit (argc=2, argv=0x7fff3ef9cb68) at bitcoind.cpp:174
#11 0x000055efac19767a in main (argc=2, argv=0x7fff3ef9cb68) at bitcoind.cpp:186

[theuni]

Thanks for helping to reproduce, eklitzke! Indeed I managed to force this with this patch:

--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1520,6 +1520,7 @@ bool AppInitMain()
                         break;
                     }
                 }
+                fRequestShutdown = true;

                 if (!is_coinsview_empty) {
                     uiInterface.InitMessage(_("Verifying blocks..."));

And running with:

gdb --args ./bitcoind -connect=0 -testnet -reindex-chainstate

I pushed a simple change that I think is obviously safe. I believe @TheBlueMatt would prefer a more elegant fix, but imo this is a reasonable option for the 0.16 branch. I defer to @wumpus on backporting.

Note that at first glance it may make more sense to add the checks in init.cpp around the FlushStateToDisk()s, but the first is called without cs_main and I avoided messing with locks for the sake of simplicity.

[ryanofsky]

@TheBlueMatt, in IRC you wrote "we could revert to the initial suggestion of #12349 and just short-circuit flushing, but I really hate that" https://botbot.me/freenode/bitcoin-core-dev/msg/96765327/

Could you explain more? I haven't looked in detail, but at first glance this looks like a simple, obvious fix that doesn't rely on action at a distance like the one in #12367.

[wumpus]

You're deferring to the wrong wumpus!

[laanwj]

utACK

We absolutely should have tests for this! it's a bit silly to merge a fix for something (#12367) then need another fix for the same thing, within the same rc phase.

[theuni]

@laanwj Yea, this is a mess :(

As a small data point, though, we believed this to be qt only and couldn't hit it with rc3. It was @eklitzke's bitcoind backtrace that made it easy to reproduce.

I just tested with current master and can confirm that the bug and fix are still valid.

Any suggestions for testing? Only thing I can think of is an rpc test that runs bitcoind with something like -stopafterblockimport, but that'd only be testing a really specific code path.

[promag]

Tested ACK ceaefdd. Can't reproduce the crash with this change.

[laanwj]

utACK ceaefdd

Any suggestions for testing? Only thing I can think of is an rpc test that runs bitcoind with something like -stopafterblockimport, but that'd only be testing a really specific code path.

@TheBlueMatt's suggestion was pretty good

<BlueMatt> yes, one thing I did during testing was just make ShutdownRequested() start shutdown after being called N times
<BlueMatt> and just restart with an incrementing N
<BlueMatt> this is something we could automate, but would largely not have caught the qt issues

(but not in this PR, obviously)