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

This member is only accessed by the scheduler thread, so I don't think it needs to be guarded. I see that all access happens to be while cs_main is held, but I'm not sure that's a good reason to add this annotation.

That it is accessible by other threads is why it needs to be guarded -- that's what ensures that it is only accessed by one thread at a time (or ever). Right now, if it were accessed by other threads who'd locked cs_main, it would be safe -- all this does is document that.

Just because a variable is accessed under cs_main does not mean that it has to be protected by it. It would have to be if there was a relationship between m_stale_tip_check_time and some other variable that is already protected by cs_main and both have to be accessed together, but that is not the case.

This is the m_stale_tip_check_time access:

if (now > m_stale_tip_check_time) {
    ...
    m_stale_tip_check_time = now + STALE_CHECK_INTERVAL;
}

If that would be called by multiple threads it would need a mutex protection. Not necessary cs_main. I think it is best to either leave it as is (because it is only accessed by a single thread) or introduce a new mutex for m_stale_tip_check_time.

Consider also a future work that tries to split cs_main to improve concurrency - then things have to be chopped off from cs_main. Needlessly adding m_stale_tip_check_time under it will create more work in the future.

Just because a variable is accessed under cs_main does not mean that it has to be protected by it.

That's true of everything protected by cs_main -- if it weren't there would be no hope of replacing cs_main by some other scheme. The issue isn't whether guarding the variable in some other way would also be safe, it's about documenting some way in which we can guarantee that the use is safe, and have the compiler maintain that guarantee for us.

I see your point, but enforcing an unnecessary protection by cs_main seems like an abuse to me.

IMO we shouldn't try to have the compiler maintain a thread-safety guarantee for private class members that are not accessed by multiple threads. If a future change expands the variable usage to multiple threads, then that change should introduce a suitable protection.

Just because a variable is accessed under cs_main does not mean that it has to be protected by it.

Agree.

Mutex's purpose is to protect not an access to a variable, rather class's invariants (any thread must see an object in the consistent state). If we fail to clear describe a protected invariant, it would be better to avoid guarding by a mutex, no?

When I look at both m_stale_tip_check_time and m_initial_sync_finished and want to convince myself that there's no chance of one thread racing to access them while another is in the middle of modifying them, the way I convince myself of that is "oh, they're both used from precisely one function, and that function just locked cs_main".

It's certainly possible to look further, and say that CheckForStaleTipAndEvictPeers is only called via the scheduler thread; but that's more work, and it's not something that I can easily get the compiler to check for me. I could tell the compiler to check it for me with more work; but that involves adding thread-specific mutexes which reviewers here have objected to previously #21527 (review) and the whole point of breaking this PR out was to try to make some incremental progress in the meantime.

As above, this doesn't need cs_main.

In fact, I don't see the point of this variable at all. It's only used to prevent calling StartExtraBlockRelayPeers() more than once, but that function could be made idempotent to prevent logging multiple times by using std::atomic<bool>::exchange().

As above, if you have cs_main it is safe to access this. It is not unconditionally safe for anyone to access it (unlike if it were atomic or const), hence it should be guarded. This is attempting to be a minimal, documentation only PR, so removing the variable is out of scope.

As your commit log already says, I think it'd be better just to actually make this const and included as a ctor parameter (and also made private).

I guess I'm okay with that in a follow up, but this patchset's been stewing for about 15 months now, so I'm a bit skeptical about increasing the scope?

Same as m_permissionFlags, better make it const. It is even easier than m_permissionFlags because there are no fuzzer changes. Or should we fuzz m_prefer_evict too? It would be one-line extension to the fuzzer's ConsumeNode() to pass one more boolean parameter from ConsumeBool().

Agree to make these private data members (consider to prefer non-const), if they do not need to be public or protected for testing/fuzzing, e.g.

--- a/src/net.h
+++ b/src/net.h
@@ -339,6 +339,8 @@ class CNode
 {
     friend class CConnman;
     friend struct ConnmanTestMsg;
+private:
+    bool m_prefer_evict{false}; // This peer is preferred for eviction.
 
 public:
     const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
@@ -393,7 +395,6 @@ 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. (treated as const)
     bool HasPermission(NetPermissionFlags permission) const {
         return NetPermissions::HasFlag(m_permissionFlags, permission);
     }

Making data members private does not make them thread safe. This member is thread safe in the current code, despite the lack of a guarding mutex, because its value is fixed immediately after creation, before another thread can access it, and it does not change until it's destructed.

Yes, the point is to encapsulate as private if the data member participates in the object’s invariant, versus public (and const if thread-safe).

Edit: correcting myself, these don't seem to be invariants, so public and const seems fine.

The reason it's thread-safe is that it's (effectively) const; the reason it's (effectively) const is that its value is known at the time the connection is setup. This PR is only about documenting why data members are thread safe, not changing logic or encapsulation...

I don't really get the private suggestion -- it's a CNode data member, but it's not accessed by any CNode member functions, it's only used by CConnman::AttemptToEvictConnection. While CConnman is a friend class so could get away with that, it seems like a weird thing to do.

https://github.com/ajtowns/bitcoin/commits/202208-cnodeoptions has a commit with these actually made const for whatever that's worth. Again, it does more than just document the current state of the code, so I'm not adding it to this PR.

Made const in #25962

I don't think the comment is useful. Anyone reading the code should be able to check this for themselves (and not trust a code comment that claims this).

The comment is so the person reading the code knows what to check for.

m_serializer and m_deserializer do not have to be unique_ptr. I think const unique_ptr<T> m_member; is the same as T m_member; and the latter should be preferred because it is simpler, no?

Consider this:

<details> <summary>diff</summary>

diff --git i/src/net.cpp w/src/net.cpp
index e275c2964d..18110e553b 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -670,22 +670,22 @@ bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
     const auto time = GetTime<std::chrono::microseconds>();
     LOCK(cs_vRecv);
     m_last_recv = std::chrono::duration_cast<std::chrono::seconds>(time);
     nRecvBytes += msg_bytes.size();
     while (msg_bytes.size() > 0) {
         // absorb network data
-        int handled = m_deserializer->Read(msg_bytes);
+        int handled = m_deserializer.Read(msg_bytes);
         if (handled < 0) {
             // Serious header problem, disconnect from the peer.
             return false;
         }
 
-        if (m_deserializer->Complete()) {
+        if (m_deserializer.Complete()) {
             // decompose a transport agnostic CNetMessage from the deserializer
             bool reject_message{false};
-            CNetMessage msg = m_deserializer->GetMessage(time, reject_message);
+            CNetMessage msg = m_deserializer.GetMessage(time, reject_message);
             if (reject_message) {
                 // Message deserialization failed. Drop the message but don't disconnect the peer.
                 // store the size of the corrupt message
                 mapRecvBytesPerMsgType.at(NET_MESSAGE_TYPE_OTHER) += msg.m_raw_message_size;
                 continue;
             }
@@ -3042,14 +3042,14 @@ ServiceFlags CConnman::GetLocalServices() const
     return nLocalServices;
 }
 
 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
 
 CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion)
-    : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))},
-      m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())},
+    : m_deserializer{V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION)},
+      m_serializer{V1TransportSerializer()},
       m_sock{sock},
       m_connected{GetTime<std::chrono::seconds>()},
       addr(addrIn),
       addrBind(addrBindIn),
       m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn},
       m_inbound_onion(inbound_onion),
@@ -3094,13 +3094,13 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
         msg.data.size(),
         msg.data.data()
     );
 
     // make sure we use the appropriate network transport format
     std::vector<unsigned char> serializedHeader;
-    pnode->m_serializer->prepareForTransport(msg, serializedHeader);
+    pnode->m_serializer.prepareForTransport(msg, serializedHeader);
     size_t nTotalSize = nMessageSize + serializedHeader.size();
 
     size_t nBytesSent = 0;
     {
         LOCK(pnode->cs_vSend);
         bool optimisticSend(pnode->vSendMsg.empty());
diff --git i/src/net.h w/src/net.h
index 5c43c5bfb8..f99a33a7d1 100644
--- i/src/net.h
+++ w/src/net.h
@@ -410,14 +410,14 @@ public:
 class CNode
 {
     friend class CConnman;
     friend struct ConnmanTestMsg;
 
 public:
-    const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
-    const std::unique_ptr<const TransportSerializer> m_serializer;
+    V1TransportDeserializer m_deserializer; // Used only by SocketHandler thread
+    const V1TransportSerializer m_serializer;
 
     NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester
     std::atomic<ServiceFlags> nServices{NODE_NONE};
 
     /**
      * Socket used for communication with the node.
diff --git i/src/test/util/net.cpp w/src/test/util/net.cpp
index 62b770753a..c6d0c185d1 100644
--- i/src/test/util/net.cpp
+++ w/src/test/util/net.cpp
@@ -30,13 +30,13 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_by
     }
 }
 
 bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const
 {
     std::vector<uint8_t> ser_msg_header;
-    node.m_serializer->prepareForTransport(ser_msg, ser_msg_header);
+    node.m_serializer.prepareForTransport(ser_msg, ser_msg_header);
 
     bool complete;
     NodeReceiveMsgBytes(node, ser_msg_header, complete);
     NodeReceiveMsgBytes(node, ser_msg.data, complete);
     return complete;
 }

</details>

They need to be pointers because they're initialised to a subclass of the type they are.

The type they are can be changed: V1TransportDeserializer m_deserializer;, as in the diff above. The polymorphism is not needed - we don't use it via a pointer to the base class.

Before:

const std::unique_ptr<TransportDeserializer> m_deserializer;
m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(...))}

VS

After:

V1TransportDeserializer m_deserializer;
m_deserializer{V1TransportDeserializer(...)}

It looks like a pure simplification.

For context, the unique ptr to a deserializer object was added in #16202. The intention was to use a polymorphism so the deserializer could be upgraded to a p2p v2 (BIP 324) deserializer later.

I tend to agree with @vasild that we should change this to be simpler. There's been almost no progress on p2p v2 in the intervening years, and it's easy enough to change this back to a polymorphism later if required.

The draft p2p v2 patches make use of the polymorphism, so removing it now seems like it makes things worse to me. Even if it is a good idea, it's fine for a followup, and doesn't improve thread safety so isn't necessary here.

Ok, unique_ptr or not is not so much related to thread-safety, kind of scope creep for this PR.

Instead of just a comment, consider actually making it const:

<details> <summary>diff</summary>

diff --git i/src/net.cpp w/src/net.cpp
index e275c2964d..7bcaa6feef 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -1237,15 +1237,15 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
                              addr,
                              CalculateKeyedNetGroup(addr),
                              nonce,
                              addr_bind,
                              /*addrNameIn=*/"",
                              ConnectionType::INBOUND,
-                             inbound_onion);
+                             inbound_onion,
+                             permissionFlags);
     pnode->AddRef();
-    pnode->m_permissionFlags = permissionFlags;
     pnode->m_prefer_evict = discouraged;
     m_msgproc->InitializeNode(pnode);
 
     LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString());
 
     {
@@ -3041,15 +3041,16 @@ ServiceFlags CConnman::GetLocalServices() const
 {
     return nLocalServices;
 }
 
 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
 
-CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion)
+CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, NetPermissionFlags permissoin_flags)
     : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))},
       m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())},
+      m_permissionFlags{permissoin_flags},
       m_sock{sock},
       m_connected{GetTime<std::chrono::seconds>()},
       addr(addrIn),
       addrBind(addrBindIn),
       m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn},
       m_inbound_onion(inbound_onion),
diff --git i/src/net.h w/src/net.h
index 5c43c5bfb8..8701e7502a 100644
--- i/src/net.h
+++ w/src/net.h
@@ -413,13 +413,13 @@ class CNode
     friend struct ConnmanTestMsg;
 
 public:
     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}; // treated as const outside of fuzz tester
+    const NetPermissionFlags m_permissionFlags;
     std::atomic<ServiceFlags> nServices{NODE_NONE};
 
     /**
      * Socket used for communication with the node.
      * May not own a Sock object (after `CloseSocketDisconnect()` or during tests).
      * `shared_ptr` (instead of `unique_ptr`) is used to avoid premature close of
@@ -582,13 +582,13 @@ public:
     std::atomic<std::chrono::microseconds> m_last_ping_time{0us};
 
     /** Lowest measured round-trip time. Used as an inbound peer eviction
      * criterium in CConnman::AttemptToEvictConnection. */
     std::atomic<std::chrono::microseconds> m_min_ping_time{std::chrono::microseconds::max()};
 
-    CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion);
+    CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, NetPermissionFlags permission_flags = NetPermissionFlags::None);
     CNode(const CNode&) = delete;
     CNode& operator=(const CNode&) = delete;
 
     NodeId GetId() const {
         return id;
     }
diff --git i/src/test/fuzz/util.cpp w/src/test/fuzz/util.cpp
index 033c6e18d5..5bbaf903c0 100644
--- i/src/test/fuzz/util.cpp
+++ w/src/test/fuzz/util.cpp
@@ -233,13 +233,12 @@ bool FuzzedSock::IsConnected(std::string& errmsg) const
 }
 
 void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, PeerManager& peerman, CNode& node) noexcept
 {
     const bool successfully_connected{fuzzed_data_provider.ConsumeBool()};
     const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
-    const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
     const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max());
     const bool relay_txs{fuzzed_data_provider.ConsumeBool()};
 
     const CNetMsgMaker mm{0};
 
     CSerializedNetMsg msg_version{
@@ -268,13 +267,12 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
     assert(node.nVersion == version);
     assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
     assert(node.nServices == remote_services);
     CNodeStateStats statestats;
     assert(peerman.GetNodeStateStats(node.GetId(), statestats));
     assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
-    node.m_permissionFlags = permission_flags;
     if (successfully_connected) {
         CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)};
         (void)connman.ReceiveMsgFrom(node, msg_verack);
         node.fPauseSend = false;
         connman.ProcessMessagesOnce(node);
         {
diff --git i/src/test/fuzz/util.h w/src/test/fuzz/util.h
index 580105e442..4d3568ff81 100644
--- i/src/test/fuzz/util.h
+++ w/src/test/fuzz/util.h
@@ -297,23 +297,25 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
     const uint64_t keyed_net_group = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
     const uint64_t local_host_nonce = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
     const CAddress addr_bind = ConsumeAddress(fuzzed_data_provider);
     const std::string addr_name = fuzzed_data_provider.ConsumeRandomLengthString(64);
     const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES);
     const bool inbound_onion{conn_type == ConnectionType::INBOUND ? fuzzed_data_provider.ConsumeBool() : false};
+    const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
     if constexpr (ReturnUniquePtr) {
         return std::make_unique<CNode>(node_id,
                                        local_services,
                                        sock,
                                        address,
                                        keyed_net_group,
                                        local_host_nonce,
                                        addr_bind,
                                        addr_name,
                                        conn_type,
-                                       inbound_onion);
+                                       inbound_onion,
+                                       permission_flags);
     } else {
         return CNode{node_id,
                      local_services,
                      sock,
                      address,
                      keyed_net_group,

</details>

Apart from the feedback already given, this also seems arbitrary. If you want to document symbols that happen to be under the cs_main scope, why only document this one, and not everything else, like m_highest_fast_announce ...

Maybe leave this hunk for a separate pull request? Otherwise I am not sure if this will move forward.

m_highest_fast_announce is a member of PeerManagerImpl, not CNode or Peer, so is out of scope for this PR...

Question, are both guards useful here? (All but one of the other instances of PT_GUARDED_BY don't cumulate both annotations.)

-    std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex) PT_GUARDED_BY(m_sock_mutex);
+    std::unique_ptr<i2p::sam::Session> m_i2p_sam_session PT_GUARDED_BY(m_sock_mutex);
 };

No, the original annotation was correct; for some reason I thought PT_GUARDED_BY added some checks GUARDED_BY doesn't have, but that doesn't seem to be the case.

Changing from GUARDED_BY to PT_GUARDED_BY alone would allow you to do m_i2p_sam_session = nullptr without holding the lock, or if (m_i2p_sam_session != nullptr) { LOCK(m_sock_mutex); m_i2p_sam_session->foo(); } which would be racy.