locking for Misbehave() and other cs_main locking fixes #7942

pull kazcw wants to merge 2 commits into bitcoin:master from kazcw:locking changing 1 files +22 −3

kazcw commented at 1:19 AM on April 26, 2016: contributor

In several places in main.cpp, synchronized data structures are accessed without cs_main being held.

lock cs_main for State/Misbehaving

ProcessMessage calls State(...) and Misbehaving(...) without holding the
required lock; add LOCK(cs_main) blocks.

efb54ba065

lock cs_main for chainActive

ActivateBestChain uses chainActive after releasing the lock; reorder operations
to move all access to synchronized object into existing LOCK(cs_main) block.

719de56ab2

gmaxwell commented at 1:28 AM on April 26, 2016: contributor

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?
kazcw commented at 5:24 AM on April 26, 2016: contributor

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 renamed this:
~~lock cs_main for State/Misbehaving/chainActive~~
[WIP] lock cs_main for State/Misbehaving/chainActive
on Apr 26, 2016
jonasschnelli added the label Refactoring on Apr 26, 2016
kazcw renamed this:
~~[WIP] lock cs_main for State/Misbehaving/chainActive~~
[WIP] locking for CNodeState/chainActive
on Apr 26, 2016
kazcw renamed this:
~~[WIP] locking for CNodeState/chainActive~~
locking for Misbehave() and other cs_main locking fixes
on Apr 27, 2016
kazcw commented at 1:46 AM on April 27, 2016: contributor

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.

in src/main.cpp:None in 719de56ab2

2949 | @@ -2948,7 +2950,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
2950 |                  {
2951 |                      LOCK(cs_vNodes);
2952 |                      BOOST_FOREACH(CNode* pnode, vNodes) {
2953 | -                        if (chainActive.Height() > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) {
2954 | +                        if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : nBlockEstimate)) {

sipa commented at 6:52 AM on May 17, 2016:

You can use pindexNewTip->nHeight instead of creating an extra variable.

jonasschnelli commented at 7:47 PM on June 2, 2016:

You can use pindexNewTip->nHeight instead of creating an extra variable.

ping @kazcw

sipa commented at 2:41 PM on June 2, 2016: member

@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.
sipa commented at 2:46 PM on June 2, 2016: member

utACK 719de56ab2c8e5bc6ce9f67c7bf159adc242d49b
dcousens commented at 3:03 PM on June 2, 2016: contributor

utACK 719de56, and I concur that I do not see cs_misbehavior
laanwj commented at 1:29 PM on June 3, 2016: member

utACK 719de56 Going to merge this, the nit can be done later.
laanwj merged this on Jun 3, 2016
laanwj closed this on Jun 3, 2016
laanwj referenced this in commit c141c14c9f on Jun 3, 2016
LarryRuane referenced this in commit 111eedf7bf on Feb 20, 2021
LarryRuane referenced this in commit c3646bdf88 on Feb 20, 2021
zkbot referenced this in commit d95a957841 on Apr 1, 2021
zkbot referenced this in commit 2d3b58c993 on Apr 1, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8147 CNodeState encapsulation, lock reduction