Remove redundant locks:
FindNode(...) is locking cs_vNodes internallySetAddressBook(...) is locking cs_wallet internallyDelAddressBook(...) is locking cs_wallet internallyNote to reviewers: From what I can tell these locks are redundantly held from a data integrity perspective (guarding specific variables), and they do not appear to be needed from a data consistency perspective (ensuring a consistent state at the right points). Review thoroughly and please let me know if I'm mistaken :-)
The changes to net.cpp remove 2/3 LOCKs introduced in 3c37dc40d39e1a1e56b6b0d3e660626a78656d4f, part of #9626.
"This also ensures that if we return a CNode* from FindNode, we are still holding cs_vNodes if we use it for anything aside from existance-checking, fixing a stupid-unlikely race where it might be deleted out from under us."
@fanquake Thanks for reviewing. Good point. Would it be possible to state that locking requirement more explicitly using annotations (see #11226)? Perhaps one way to do it would be if the two uses were separated: CNode* FindNode(…) would require holding cs_vNodes (which would be clearly stated using EXCLUSIVE_LOCKS_REQUIRED(cs_vNodes)) whereas say bool NodeExists(…) wouldn't? Worth doing?
398 | @@ -399,7 +399,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo 399 | // In that case, drop the connection that was just created, and return the existing CNode instead. 400 | // Also store the name we used to connect in that CNode, so that future FindNode() calls to that 401 | // name catch this early. 402 | - LOCK(cs_vNodes);
Yes, see @fanquake's comment above regarding 3c37dc4 :-)
Alright 😄
2550 | @@ -2552,7 +2551,6 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats) 2551 | 2552 | bool CConnman::DisconnectNode(const std::string& strNode) 2553 | { 2554 | - LOCK(cs_vNodes);
Same as above.
* SetAddressBook(...) is locking cs_wallet internally
* DelAddressBook(...) is locking cs_wallet internally
Anyone willing to review? :-)
Any measurable speedup by removing those? Just asking.
@MarcoFalke I would assume no. My goal with this PR was to achieve correct locking, so any measurable speed-up would be an unintended bonus :-)
utACK d6f3a737361d8fb562f08763f613a6529b870d1e. This could help performance since it releases the lock while writing to disk. Anyway it's an obvious improvement for code simplicity & consistency.
utACK d6f3a73, indeed makes no sense to do this locking internally as well as externally, and having the locking contained internally is better.