Solve chainActive-related locking issues #4058
Conversation
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 bitcoin#3997
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/55a1db4fa2cf62b9766ef382c1e14b3ecbdf67fe for binaries and test log. |
|
Is it really necessary for the 4 lock-functions (isLockedCoin,lockCoin,unlockCoin,listLockedCoins) to |
|
@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/ |
5 participants
to IsInitialBlockDownload.
Fixes #3997