#26036 introduced a new
mutex (NetEventsInterface::g_msgproc_mutex) to document the fact that our
message processing code is single threaded (msghand thread). Any
PeerManagerImpl or Peer members that are only ever accessed from that
single thread should be annotated as GUARDED_BY(g_msgproc_mutex), to avoid
bugs where those members are accessed by other threads as well (in which case
they need to be guarded by a different mutex).
CNodeState is documented to only have validation specific members and is
therefore entirely guarded by cs_main. However, not all members are
validation specific, and the ones that aren’t should be moved to Peer.
#26140 is a simple refactor
PR that moves some of the CNodeState members that are not validation
relevant to Peer and annotates them as guarded by g_msgproc_mutex.
Which threads access state relevant for message processing
(PeerManagerImpl, Peer, etc.)? (Hint: have a look at the developer
for a list of all threads)
What is the difference between CNodeState and Peer? How would you decide
where to store new per-peer state? (Bonus points if you also mention CNode
in your answer)
The PR moves nUnconnectingHeaders, m_headers_sync_timeout,
fPreferHeaders and m_recently_announced_invs from CNodeState to
Peer. Multiple other members of CNodeState are also not validation
specific and should also move to Peer. Which members are that and why is
it not as trivial to move those in comparison to the ones that this PR
Why does the PR rename nUnconnectingHeaders and
<svav> cs_main is a recursive mutex which is used to ensure that validation is carried out in an atomic way. It guards access to validation specific variables (such as CChainState and CNode) or mempool variables (in net_processing). The lock of cs_main is in validation.cpp.
<dergoegge> svav: I don't think that CNode is guarded by cs_main (and it totally shouldn't be). That SO answer is over a year old, so it might be out of date. Although i also can't remember CNode being guarded by cs_main.
<LarryRuane> if a thread acquires (locks) a mutex and then that same thread tries to lock it again, it will block... with a recursive mutex, the same thread can lock the mutex multiple times (simultaneously), and it's only unlocked on the last unlock
<stickies-v> dergoegge: the docs state that CNodeState's members are protected by cs_main, but I can't see that enforced in code anywhere. Do the docs mean that they _should_ only be accessed with a lock on cs_main?
<dergoegge> My prepared answer for this question: cs_main is our main validation mutex lock, used to guard any validation state from parallel access by different threads. It is also used to guard non-validation state which we should change (like this week's PR does)
<dergoegge> stickies-v: yes thats what the docs mean, we could add annotations (i.e. GUARDED_BY(cs_main)) to get the compiler to check (i think i did that at some point privately just to check that cs_main is always held when they're accessed)
<stickies-v> dergoegge: okay cool thanks, that's what I thought. The docs just make it sound like the locking is already taken care of, instead of highlighting that it absolutely needs to be done - which I think is a bit confusing
<dergoegge> PeerManager is registered as a CValidationInterface so it receives validation callbacks which are scheduled on the scheduler thread (e.g. BlockConnected, BlockDisconnected, NewPowValidBlock)
<stickies-v> oh. good point, I only checked the direct calls, not the entire stacks. that seems like a very difficult thing to verify if you're not reasonably familiar with all the functions called, or is there a trick?
<LarryRuane> dergoegge: I think currently, some of CNodeState is not relevant to validation, and so better belongs in Peer.. Peer has non-validation stuff, CNodeState should have validation stuff -- did I get that right? :)
<stickies-v> intuitively, I'd say CNode mostly deals with communication between nodes, CNodeState with validating what we get from a node, and Peer to deal with policy and checking if peers are behaving correctly. Is that a fair approximation?
<pablomartin> dergoegge: about... where to store new peer-peer state? CNode (defined in net.h, used by m_nodes(CConnman) and covered by m_nodes_mutex) is concerned with the connection state of the peer.