net processing: Move remaining globals into PeerManagerImpl #24543

Remove this blank line above UpdateLastBlockAnnounceTime() (since the declaration should be grouped with the other "Implement PeerManager" method declarations)

Move this comment to be a doxygen comment in the header file (function-level comments should be with the function declaration).

What do you think about an extra commit that moves the declaration of CNodeState up here, rather than having a forward declaration?

What do you think about also removing:

} // namespace

namespace {

?

This function is only called in one place and the logic could easily be inlined directly into the version message processing. I don't think having this extra function adds any value. What do you think?

Perhaps rename to m_most_recent_block_mutex in the scripted diff

Note that this call to ForEachNode() is removed in #21160 (not suggesting you do anything differently here).

It may be better to move this code inline in one commit, separate from changing nPreferredDownload from a global to a member variable, so you're not mixing move-only changes and refactor changes in the same commit.

This code could also be cleaned up in a couple of ways now that it's inline (but that doesn't need to happen in this PR):

• m_num_preferred_download_peers -= state->fPreferredDownload; can be removed, since version processing can only happen once for each peer, and fPreferredDownload is initialized to false, so this line is always a no-op.
• // Whether this node should be marked as a preferred download node. is redundant with the // Potentially mark this peer as a preferred download peer. comment above and can be removed.

Good idea, I split the inlining and clean ups into a separate commit.

To avoid the code duplication here, you can define this in terms of the State() const function, and cast away constness:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 877bca4c0c..6cadef2a37 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -555,8 +555,10 @@ private:
     /** Map maintaining per-node state. */
     std::map<NodeId, CNodeState> m_node_states GUARDED_BY(cs_main);
 
-    CNodeState* State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    /** Get a pointer to a const CNodeState, used when not mutating the CNodeState object.*/
     const CNodeState* State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    /** Get a pointer to a mutable CNodeState.*/
+    CNodeState* State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
     uint32_t GetFetchFlags(const CNode& pfrom) const EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -798,20 +800,17 @@ private:
     bool SetupAddressRelay(const CNode& node, Peer& peer);
 };
 
-CNodeState* PeerManagerImpl::State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
-    std::map<NodeId, CNodeState>::iterator it = m_node_states.find(pnode);
+    std::map<NodeId, CNodeState>::const_iterator it = m_node_states.find(pnode);
     if (it == m_node_states.end())
         return nullptr;
     return &it->second;
 }
 
-const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+CNodeState* PeerManagerImpl::State(NodeId pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
-    std::map<NodeId, CNodeState>::const_iterator it = m_node_states.find(pnode);
-    if (it == m_node_states.end())
-        return nullptr;
-    return &it->second;
+    return const_cast<CNodeState*>(std::as_const(*this).State(pnode));
 }

See https://stackoverflow.com/a/47369227/933705. Generally const_cast is discouraged, but this is one instance where it's considered acceptable.

You can remove the UpdateLastBlockAnnounceTime() declaration on line 39 (it's now no longer used).

Done, good catch!

I think the commit [net processing] Inline and simplify UpdatePreferredDownload could have slightly more explanation in the commit log (explaining that this logic can only be hit once per peer, so fPreferredDownload will always be false, and the m_num_preferred_download_peers -= state->fPreferredDownload; line is always a no-op).

This comment could be more explicit in referring to "block announced using BIP 152 high-bandwidth mode" or similar. I don't think "fast announced blocks" is commonly understood terminology.

Good point, adjusted the comment