shutdown: fix crash on shutdown with reindex-chainstate

theuni commented at 10:53 pm on February 4, 2018: member

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

fanquake added this to the milestone 0.16.0 on Feb 4, 2018

MarcoFalke commented at 11:29 pm on February 4, 2018: member

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

theuni commented at 9:42 pm on February 5, 2018: member

@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.

MarcoFalke removed this from the milestone 0.16.0 on Feb 5, 2018

TheBlueMatt commented at 2:07 am on February 6, 2018: member

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

laanwj added the label GUI on Feb 6, 2018

laanwj added this to the milestone 0.16.1 on Feb 6, 2018

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

I prefer the fix in #12367 as much simpler.

MarcoFalke commented at 7:27 pm on February 6, 2018: member

Closing for now

MarcoFalke closed this on Feb 6, 2018

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

eklitzke commented at 11:13 pm on February 12, 2018: contributor

Without this change, I still get a segfault from bitcoind master (commit c997f8808256521397f1c003bb1e9896fee6eaa0) 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[#0](/bitcoin-bitcoin/0/)  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
 1[#1](/bitcoin-bitcoin/1/)  0x00007f514a479381 in __GI_abort () at abort.c:79
 2[#2](/bitcoin-bitcoin/2/)  0x00007f514a46f8fa in __assert_fail_base (fmt=0x7f514a5eac28 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
 3    assertion=assertion@entry=0x55efac7413fd "!hashBlock.IsNull()", file=file@entry=0x55efac7413f4 "txdb.cpp", line=line@entry=90, 
 4    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
 5[#3](/bitcoin-bitcoin/3/)  0x00007f514a46f972 in __GI___assert_fail (assertion=0x55efac7413fd "!hashBlock.IsNull()", file=0x55efac7413f4 "txdb.cpp", line=90, 
 6    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
 7[#4](/bitcoin-bitcoin/4/)  0x000055efac41d7e1 in CCoinsViewDB::BatchWrite (this=0x55efacf5e9d0, mapCoins=std::unordered_map with 0 elements, hashBlock=...) at txdb.cpp:90
 8[#5](/bitcoin-bitcoin/5/)  0x000055efac5c3aae in CCoinsViewBacked::BatchWrite (this=0x55efc1c0d150, mapCoins=std::unordered_map with 0 elements, hashBlock=...) at coins.cpp:28
 9[#6](/bitcoin-bitcoin/6/)  0x000055efac5c4c7a in CCoinsViewCache::Flush (this=0x55efacf6de10) at coins.cpp:204
10[#7](/bitcoin-bitcoin/7/)  0x000055efac4882a3 in FlushStateToDisk (chainparams=..., state=..., mode=FLUSH_STATE_ALWAYS, nManualPruneHeight=0) at validation.cpp:2099
11[#8](/bitcoin-bitcoin/8/)  0x000055efac4887ed in FlushStateToDisk () at validation.cpp:2118
12[#9](/bitcoin-bitcoin/9/)  0x000055efac1ad8c6 in Shutdown () at init.cpp:229
13[#10](/bitcoin-bitcoin/10/) 0x000055efac19706a in AppInit (argc=2, argv=0x7fff3ef9cb68) at bitcoind.cpp:174
14[#11](/bitcoin-bitcoin/11/) 0x000055efac19767a in main (argc=2, argv=0x7fff3ef9cb68) at bitcoind.cpp:186

MarcoFalke reopened this on Feb 13, 2018

MarcoFalke removed this from the milestone 0.16.1 on Feb 13, 2018

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

theuni force-pushed on Feb 13, 2018

fix possible shutdown assertion with -reindex-shutdown

Credit @eklitzke for reproducing.

ceaefdd5f3

theuni force-pushed on Feb 13, 2018

theuni commented at 5:58 am on February 13, 2018: member

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

0--- a/src/init.cpp
1+++ b/src/init.cpp
2@@ -1520,6 +1520,7 @@ bool AppInitMain()
3                         break;
4                     }
5                 }
6+                fRequestShutdown = true;
7
8                 if (!is_coinsview_empty) {
9                     uiInterface.InitMessage(_("Verifying blocks..."));

And running with:

0gdb --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 commented at 4:06 pm on February 13, 2018: member

@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 commented at 5:05 pm on February 13, 2018: none

You’re deferring to the wrong wumpus!

laanwj added the label Needs backport on Feb 15, 2018

laanwj commented at 1:40 pm on February 15, 2018: member

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 commented at 6:59 pm on February 15, 2018: member

@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 commented at 7:05 pm on February 15, 2018: member

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

laanwj commented at 9:16 pm on February 15, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12349/commits/ceaefdd5f362537a1908d0095059e4be788f3dee

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

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

(but not in this PR, obviously)

laanwj merged this on Feb 15, 2018

laanwj closed this on Feb 15, 2018

laanwj referenced this in commit 5eff1c748d on Feb 15, 2018

laanwj referenced this in commit ad10b90e50 on Feb 15, 2018

laanwj removed the label Needs backport on Feb 15, 2018

HashUnlimited referenced this in commit e1f154cb93 on Mar 16, 2018

markblundeberg referenced this in commit ea00f38e32 on May 9, 2019

jonspock referenced this in commit 962f61396c on Jun 5, 2019

PastaPastaPasta referenced this in commit a7bdf62871 on Mar 14, 2020

PastaPastaPasta referenced this in commit d6f22f9064 on Mar 14, 2020

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

PastaPastaPasta referenced this in commit 51187864b6 on Mar 14, 2020

PastaPastaPasta referenced this in commit 8095699575 on Mar 15, 2020

PastaPastaPasta referenced this in commit b447c5a0c6 on Mar 15, 2020

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

ckti referenced this in commit eee4da33f2 on Mar 28, 2021

MarcoFalke locked this on Sep 8, 2021

shutdown: fix crash on shutdown with reindex-chainstate #12349