Fixes #3997
4201 | @@ -4186,6 +4202,10 @@ bool SendMessages(CNode* pto, bool fSendTrickle) 4202 | } 4203 | } 4204 | 4205 | + TRY_LOCK(cs_main, lockMain); // Acquire cs_main for IsInitialBlockDownload() and CNodeState()
As IsInitialBlockDownload gets its own LOCK(cs_main), is this necessary? (I really prefer to keep the amount of time cs_main is locked minimal, as it's likely the most common cause for latency in processing)
It's not strictly necessary (it would get the lock either way), however, what is the use of a TRY_LOCK if a few lines before we do a non-try LOCK() of the same lock? That's why I moved the TRY_LOCK up, so that if the try fails it won't do IsInitialBlockDownload either...
Good point.
I agree that keeping the time that cs_main is locked down would be good.It's even worse than the Big Kernel Lock :) Finer-grained locking would be in order.
In the meantime: We could do TRY_LOCK and release the lock again multiple times?
Let's do that in a different PR, if needed.
Hmm - although it improves things, this is not yet complete. There are places left in the GUI code where chainActive is used directly without acquiring the appropriate lock. Will add a commit to fix these.
Edit: fixed, should be complete now
All functions that use ChainActive but do not aquire the cs_main
lock themselves, need to be called with the cs_main lock held.
This commit adds assertions to all externally callable functions
that use chainActive or chainMostWork.
This will flag usages when built with -DDEBUG_LOCKORDER.
- In wallet and GUI code LOCK cs_main as well as cs_wallet when
necessary
- In main.cpp SendMessages move the TRY_LOCK(cs_main) up, to encompass the call
to IsInitialBlockDownload.
- Make ActivateBestChain, AddToBlockIndex, IsInitialBlockDownload,
InitBlockIndex acquire the cs_main lock
Fixes #3997
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/55a1db4fa2cf62b9766ef382c1e14b3ecbdf67fe 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.
Is it really necessary for the 4 lock-functions (isLockedCoin,lockCoin,unlockCoin,listLockedCoins) to acquire a cs_main lock?
@cozz It's better to err on the safe side. If it happens that the cs_wallet lock is acquired before the cs_main lock it will result in a deadlock.
I've added cs_main locks everywhere before that the cs_wallet lock is taken except if I could clearly see within the function that the main lock is not necessary (for example, for address book manipulation).
(This does make the issue visible that almost everything in the wallet code depends on the cs_main lock)
ACK.
Some of the wallet function may survive with smaller locks (in particular, just locking the mempool rather than whole of main), but let's do that later.
http://nightly.bitcoin.it/0.9.99.0-20140420-4a102fa/ https://github.com/nightlybitcoin/bitcoin/commits/0.9.99.0-20140420-4a102fa This nightly build and later will contain this PR to more easily expose it to expose it to public testing.