locking for Misbehave() and other cs_main locking fixes#7942
locking for Misbehave() and other cs_main locking fixes#7942laanwj merged 2 commits intobitcoin:masterfrom
Conversation
ProcessMessage calls State(...) and Misbehaving(...) without holding the required lock; add LOCK(cs_main) blocks.
ActivateBestChain uses chainActive after releasing the lock; reorder operations to move all access to synchronized object into existing LOCK(cs_main) block.
|
hmph. peppering cs_main in the middle of functions sounds like a formula for lock inversion. Perhaps Misbehaving() should get its own lock internally and not depend on cs_main? |
|
A separate nodestate cs is definitely doable; there's a fair amount of code that would need to hold cs_main and nodestate, but no new locks ever need to be acquired within the scope of a cs_nodestate lock. I'll draft a refactor. |
kazcw
changed the title
kazcw
changed the title
kazcw
changed the title
|
I created a separate mutex, cs_misbehavior. cs_misbehavior is only locked either directly inside a cs_main lock or along with no other locks, so there shouldn't be any deadlocks (and DEBUG_LOCKORDER isn't complaining). cs_misbehavior is the sole mutex guarding the nMisbehavior and fShouldBan fields; modifications to the mapBlockSource container are guarded by both cs_main and cs_misbehavior so that either lock suffices to perform lookups safely. |
| LOCK(cs_vNodes); | ||
| BOOST_FOREACH(CNode* pnode, vNodes) { | ||
| if (chainActive.Height() > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) { | ||
| if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) { |
There was a problem hiding this comment.
You can use pindexNewTip->nHeight instead of creating an extra variable.
There was a problem hiding this comment.
You can use pindexNewTip->nHeight instead of creating an extra variable.
ping @kazcw
|
@kazcw I don't see your cs_misbehaviour, but I think the code here is fine as is. We certainly need a separate lock for node states, but I would delay that until the net refactors are done. |
|
utACK 719de56 |
|
utACK 719de56, and I concur that I do not see |
|
utACK 719de56 |
ProcessMessage calls State(...) and Misbehaving(...) without holding the required lock; add LOCK(cs_main) blocks. zcash: cherry picked from commit efb54ba zcash: bitcoin/bitcoin#7942
ActivateBestChain uses chainActive after releasing the lock; reorder operations to move all access to synchronized object into existing LOCK(cs_main) block. zcash: cherry picked from commit 719de56 zcash: bitcoin/bitcoin#7942
Bitcoin 0.13 locking PRs These are locking changes from upstream (bitcoin core) release 0.13, oldest to newest (when they were merged to the master branch). - bitcoin/bitcoin#7846 - bitcoin/bitcoin#7913 - bitcoin/bitcoin#8016 - second commit only; first commit, test changes, are already done - bitcoin/bitcoin#7942 This PR does not include: - bitcoin/bitcoin#8244 bitcoin/bitcoin@27f8126 - zcash requires locking `cs_main` in this instance (`getrawmempool()` calls `mempoolToJSON()`, which calls `chainActive.Height()`).
6 participants
In several places in main.cpp, synchronized data structures are accessed without cs_main being held.