Locking robustness fixes

[laanwj]

• Add required locks in tests (fixes Unit tests failing with -DDEBUG_LOCKORDER #4084)
• Get required locks up-front in GUI updates and use a try_lock to skip when lock already held (fixes Deadlock on exit after low disk space during syncing #4083)
• Add missing AssertLockHeld in ConnectBlock (missed this externally-callable function in Solve chainActive-related locking issues  #4058)

[laanwj]

Having fixed this I get a different exception (not related to locking) on shutdown now sometimes when I try to reproduce #4083,

bitcoin-qt: main.cpp:1737: bool ConnectBlock(CBlock&, CValidationState&, CBlockIndex*, CCoinsViewCache&, bool): Assertion `hashPrevBlock == view.GetBestBlock()' failed.

Traceback:

#4  0x00005555556c5afa in ConnectBlock (block=..., state=..., 
    pindex=pindex@entry=0x7fff78853bc0, view=..., 
    fJustCheck=fJustCheck@entry=false) at main.cpp:1737
#5  0x00005555556c7f96 in ConnectTip (pindexNew=0x7fff78853bc0, state=...)
    at main.cpp:2001
#6  ActivateBestChain (state=...) at main.cpp:2110
#7  0x00005555556c9508 in AddToBlockIndex (block=..., state=..., pos=...)
    at main.cpp:2173
#8  0x00005555556c98ff in AcceptBlock (block=..., state=..., dbp=dbp@entry=0x0)
    at main.cpp:2438
#9  0x00005555556ca8b0 in ProcessBlock (state=..., 
    pfrom=pfrom@entry=0x7fff84000d80, pblock=pblock@entry=0x7fff8effc5e0, 
    dbp=dbp@entry=0x0) at main.cpp:2557
#10 0x00005555556cf4ac in ProcessMessage (pfrom=pfrom@entry=0x7fff84000d80, 
    strCommand=..., vRecv=...) at main.cpp:3815
#11 0x00005555556d0cc2 in ProcessMessages (pfrom=0x7fff84000d80)
    at main.cpp:4127
#12 0x000055555572d714 in operator() (a0=<optimized out>, this=<optimized out>)
    at /usr/include/boost/function/function_template.hpp:767
#13 m_invoke (connectionBody=..., this=<optimized out>)
    at /usr/include/boost/signals2/detail/signal_template.hpp:368
#14 operator() (connectionBody=..., this=<optimized out>)
    at /usr/include/boost/signals2/detail/signal_template.hpp:345
#15 dereference (this=0x7fff8effcb30)
    at /usr/include/boost/signals2/detail/slot_call_iterator.hpp:82
:_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode*, boost::function<bool(CNode*)> >, boost::signals2::mutex> > >, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode*, boost::function<bool(CNode*)> >, boost::signals2::mutex> > > (f=...)
    at /usr/include/boost/iterator/iterator_facade.hpp:514
#17 operator* (this=0x7fff8effcb30)
    at /usr/include/boost/iterator/iterator_facade.hpp:639
... (lots of boost::signal stuff)
#21 0x000055555571803d in operator() (arg1=0x7fff84000d80, 
    this=0x555555eca778 <g_signals+24>)
    at /usr/include/boost/signals2/detail/signal_template.hpp:695
#22 ThreadMessageHandler () at net.cpp:1540
#23 0x000055555571ffb3 in TraceThread<void (*)()> (
    name=0x555555b43561 "msghand", 
    func=0x555555717d90 <ThreadMessageHandler()>) at util.h:576

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b39a07dc42ab6ba746a25206969fb81913146f1f for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

[rdponticelli]

That looks like the same issue I had when I ran master with 55a1db4 reverted.

[laanwj]

I think I know what happens:

• CheckDiskSpace calls StartShutdown and returns false.
  • This error propagates up the call chain, causing ProcessBlock to fail, leaving the view in an inconsistent state.
• After ProcessBlock fails, the message loop continues as normal (see https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L3812 ).

Somehow it manages to process another message before the GUI thread detects that a shutdown is requested.

The solution here would be to either not leave the view in an inconsistent state when failing, change the assertion to a normal error, and/or make sure that all message loops are exited when a fatal exception (such as disk full) occurs.

In any case, it is not a problem with the changes in this pull, but a problem that existed before.

[gavinandresen]

ACK