Merge bitcoin/bitcoin#25174: net/net_processing: Add thread safety related annotations for CNode and Peer

9816dc96b7 net: note CNode members that are treated as const (Anthony Towns)
ef26f2f421 net: mark CNode unique_ptr members as const (Anthony Towns)
bbec32c9ad net: mark TransportSerializer/m_serializer as const (Anthony Towns)
06ebdc886f net/net_processing: add missing thread safety annotations (Anthony Towns)

Pull request description:

  Adds `GUARDED_BY` and `const` annotations to document how we currently ensure various members of `CNode` and `Peer` aren't subject to race conditions.

ACKs for top commit:
  MarcoFalke:
    review ACK 9816dc96b7 📍
  jonatack:
    utACK 9816dc96b7
  hebasto:
    ACK 9816dc96b7, I have reviewed the code and it looks OK. In particular, I verified the usage of variables which got `GUARDED_BY` annotations.

Tree-SHA512: fa95bca72435d79caadc736ee7687e505dbe8fbdb20690809e97666664a8d0dea39a7d17cf16f0437d7f5746b9ad98a466b26325d2913252c5d2b520b384b785

src/net.cpp

@@ -797,7 +797,8 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds
     return msg;
 }
 
-void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) {
+void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const
+{
     // create dbl-sha256 checksum
     uint256 hash = Hash(msg.data);
 
@@ -2722,7 +2723,9 @@ CNode::CNode(NodeId idIn,
              ConnectionType conn_type_in,
              bool inbound_onion,
              std::unique_ptr<i2p::sam::Session>&& i2p_sam_session)
-    : m_sock{sock},
+    : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))},
+      m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())},
+      m_sock{sock},
       m_connected{GetTime<std::chrono::seconds>()},
       addr{addrIn},
       addrBind{addrBindIn},
@@ -2745,9 +2748,6 @@ CNode::CNode(NodeId idIn,
     } else {
         LogPrint(BCLog::NET, "Added connection peer=%d\n", id);
     }
-
-    m_deserializer = std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION));
-    m_serializer = std::make_unique<V1TransportSerializer>(V1TransportSerializer());
 }
 
 bool CConnman::NodeFullyConnected(const CNode* pnode)

src/net.h

@@ -325,13 +325,13 @@ public:
 class TransportSerializer {
 public:
     // prepare message for transport (header construction, error-correction computation, payload encryption, etc.)
-    virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) = 0;
+    virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const = 0;
     virtual ~TransportSerializer() {}
 };
 
-class V1TransportSerializer  : public TransportSerializer {
+class V1TransportSerializer : public TransportSerializer {
 public:
-    void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) override;
+    void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const override;
 };
 
 /** Information about a peer */
@@ -341,10 +341,10 @@ class CNode
     friend struct ConnmanTestMsg;
 
 public:
-    std::unique_ptr<TransportDeserializer> m_deserializer;
-    std::unique_ptr<TransportSerializer> m_serializer;
+    const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
+    const std::unique_ptr<const TransportSerializer> m_serializer;
 
-    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None};
+    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester
 
     /**
      * Socket used for communication with the node.
@@ -368,7 +368,7 @@ public:
 
     RecursiveMutex cs_vProcessMsg;
     std::list<CNetMessage> vProcessMsg GUARDED_BY(cs_vProcessMsg);
-    size_t nProcessQueueSize{0};
+    size_t nProcessQueueSize GUARDED_BY(cs_vProcessMsg){0};
 
     RecursiveMutex cs_sendProcessing;
 
@@ -393,7 +393,7 @@ public:
      * from the wire. This cleaned string can safely be logged or displayed.
      */
     std::string cleanSubVer GUARDED_BY(m_subver_mutex){};
-    bool m_prefer_evict{false}; // This peer is preferred for eviction.
+    bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const)
     bool HasPermission(NetPermissionFlags permission) const {
         return NetPermissions::HasFlag(m_permissionFlags, permission);
     }

src/net_processing.cpp

@@ -289,7 +289,7 @@ struct Peer {
          *  non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the
          *  mempool to sort transactions in dependency order before relay, so
          *  this does not have to be sorted. */
-        std::set<uint256> m_tx_inventory_to_send;
+        std::set<uint256> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
         /** Whether the peer has requested us to send our complete mempool. Only
          *  permitted if the peer has NetPermissionFlags::Mempool. See BIP35. */
         bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
@@ -648,7 +648,7 @@ private:
     std::atomic<int> m_best_height{-1};
 
     /** Next time to check for stale tip */
-    std::chrono::seconds m_stale_tip_check_time{0s};
+    std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s};
 
     /** Whether this node is running in -blocksonly mode */
     const bool m_ignore_incoming_txs;
@@ -657,7 +657,7 @@ private:
 
     /** Whether we've completed initial sync yet, for determining when to turn
       * on extra block-relay-only peers. */
-    bool m_initial_sync_finished{false};
+    bool m_initial_sync_finished GUARDED_BY(cs_main){false};
 
     /** Protects m_peer_map. This mutex must not be locked while holding a lock
      *  on any of the mutexes inside a Peer object. */