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

ajtowns commented at 10:35 pm on May 19, 2022: contributor

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

ajtowns added the label Refactoring on May 19, 2022

ajtowns commented at 10:36 pm on May 19, 2022: contributor

This breaks out the easy parts from #24474, which in turn was broken out from #21527.

ajtowns requested review from hebasto on May 19, 2022

ajtowns requested review from jnewbery on May 19, 2022

ajtowns requested review from vasild on May 19, 2022

in src/net_processing.cpp:651 in 5df0bd1e8d outdated

598@@ -599,14 +599,14 @@ class PeerManagerImpl final : public PeerManager
599     std::atomic<int> m_best_height{-1};
600 
601     /** Next time to check for stale tip */
602-    std::chrono::seconds m_stale_tip_check_time{0s};
603+    std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s};

jnewbery commented at 7:24 am on May 20, 2022:

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.

ajtowns commented at 3:15 pm on May 20, 2022:

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.

vasild commented at 9:40 am on May 24, 2022:

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:

0if (now > m_stale_tip_check_time) {
1    ...
2    m_stale_tip_check_time = now + STALE_CHECK_INTERVAL;
3}

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.

ajtowns commented at 5:21 pm on May 26, 2022:

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.

vasild commented at 7:31 am on May 27, 2022:

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.

hebasto commented at 2:11 pm on July 18, 2022:

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?

ajtowns commented at 12:20 pm on August 29, 2022:

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.

in src/net_processing.cpp:660 in 5df0bd1e8d outdated

606     const bool m_ignore_incoming_txs;
607 
608     /** Whether we've completed initial sync yet, for determining when to turn
609       * on extra block-relay-only peers. */
610-    bool m_initial_sync_finished{false};
611+    bool m_initial_sync_finished GUARDED_BY(cs_main){false};

jnewbery commented at 7:28 am on May 20, 2022:

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().

ajtowns commented at 3:17 pm on May 20, 2022:

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.

in src/net.h:396 in 5df0bd1e8d outdated

465@@ -466,7 +466,7 @@ class CNode
466      * from the wire. This cleaned string can safely be logged or displayed.
467      */
468     std::string cleanSubVer GUARDED_BY(m_subver_mutex){};
469-    bool m_prefer_evict{false}; // This peer is preferred for eviction.
470+    bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const)

jnewbery commented at 7:29 am on May 20, 2022:

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).

ajtowns commented at 3:58 pm on May 20, 2022:

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?

vasild commented at 10:26 am on May 24, 2022:

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().

jonatack commented at 10:36 am on August 29, 2022:

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.

 0--- a/src/net.h
 1+++ b/src/net.h
 2@@ -339,6 +339,8 @@ class CNode
 3 {
 4     friend class CConnman;
 5     friend struct ConnmanTestMsg;
 6+private:
 7+    bool m_prefer_evict{false}; // This peer is preferred for eviction.
 8 
 9 public:
10     const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
11@@ -393,7 +395,6 @@ public:
12      * from the wire. This cleaned string can safely be logged or displayed.
13      */
14     std::string cleanSubVer GUARDED_BY(m_subver_mutex){};
15-    bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const)
16     bool HasPermission(NetPermissionFlags permission) const {
17         return NetPermissions::HasFlag(m_permissionFlags, permission);
18     }

ajtowns commented at 11:51 am on August 29, 2022:

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.

jonatack commented at 12:03 pm on August 29, 2022:

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.

ajtowns commented at 12:48 pm on August 29, 2022:

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.

ajtowns commented at 8:18 am on August 31, 2022:

Made const in #25962

in src/net.h:344 in 5df0bd1e8d outdated

412@@ -413,10 +413,10 @@ class CNode
413     friend struct ConnmanTestMsg;
414 
415 public:
416-    std::unique_ptr<TransportDeserializer> m_deserializer;
417-    std::unique_ptr<TransportSerializer> m_serializer;
418+    const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread

jnewbery commented at 7:30 am on May 20, 2022:

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).

ajtowns commented at 3:17 pm on May 20, 2022:

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

jnewbery commented at 7:31 am on May 20, 2022: member

concept ACK

DrahtBot commented at 9:25 am on May 20, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

w0xlt commented at 4:36 pm on May 20, 2022: contributor

Approach ACK.

in src/net.h:345 in 5df0bd1e8d outdated

412@@ -413,10 +413,10 @@ class CNode
413     friend struct ConnmanTestMsg;
414 
415 public:
416-    std::unique_ptr<TransportDeserializer> m_deserializer;
417-    std::unique_ptr<TransportSerializer> m_serializer;
418+    const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
419+    const std::unique_ptr<const TransportSerializer> m_serializer;

vasild commented at 9:51 am on May 24, 2022:

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:

  0diff --git i/src/net.cpp w/src/net.cpp
  1index e275c2964d..18110e553b 100644
  2--- i/src/net.cpp
  3+++ w/src/net.cpp
  4@@ -670,22 +670,22 @@ bool CNode::ReceiveMsgBytes(Span<const uint8_t> msg_bytes, bool& complete)
  5     const auto time = GetTime<std::chrono::microseconds>();
  6     LOCK(cs_vRecv);
  7     m_last_recv = std::chrono::duration_cast<std::chrono::seconds>(time);
  8     nRecvBytes += msg_bytes.size();
  9     while (msg_bytes.size() > 0) {
 10         // absorb network data
 11-        int handled = m_deserializer->Read(msg_bytes);
 12+        int handled = m_deserializer.Read(msg_bytes);
 13         if (handled < 0) {
 14             // Serious header problem, disconnect from the peer.
 15             return false;
 16         }
 17 
 18-        if (m_deserializer->Complete()) {
 19+        if (m_deserializer.Complete()) {
 20             // decompose a transport agnostic CNetMessage from the deserializer
 21             bool reject_message{false};
 22-            CNetMessage msg = m_deserializer->GetMessage(time, reject_message);
 23+            CNetMessage msg = m_deserializer.GetMessage(time, reject_message);
 24             if (reject_message) {
 25                 // Message deserialization failed. Drop the message but don't disconnect the peer.
 26                 // store the size of the corrupt message
 27                 mapRecvBytesPerMsgType.at(NET_MESSAGE_TYPE_OTHER) += msg.m_raw_message_size;
 28                 continue;
 29             }
 30@@ -3042,14 +3042,14 @@ ServiceFlags CConnman::GetLocalServices() const
 31     return nLocalServices;
 32 }
 33 
 34 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
 35 
 36 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)
 37-    : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))},
 38-      m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())},
 39+    : m_deserializer{V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION)},
 40+      m_serializer{V1TransportSerializer()},
 41       m_sock{sock},
 42       m_connected{GetTime<std::chrono::seconds>()},
 43       addr(addrIn),
 44       addrBind(addrBindIn),
 45       m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn},
 46       m_inbound_onion(inbound_onion),
 47@@ -3094,13 +3094,13 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
 48         msg.data.size(),
 49         msg.data.data()
 50     );
 51 
 52     // make sure we use the appropriate network transport format
 53     std::vector<unsigned char> serializedHeader;
 54-    pnode->m_serializer->prepareForTransport(msg, serializedHeader);
 55+    pnode->m_serializer.prepareForTransport(msg, serializedHeader);
 56     size_t nTotalSize = nMessageSize + serializedHeader.size();
 57 
 58     size_t nBytesSent = 0;
 59     {
 60         LOCK(pnode->cs_vSend);
 61         bool optimisticSend(pnode->vSendMsg.empty());
 62diff --git i/src/net.h w/src/net.h
 63index 5c43c5bfb8..f99a33a7d1 100644
 64--- i/src/net.h
 65+++ w/src/net.h
 66@@ -410,14 +410,14 @@ public:
 67 class CNode
 68 {
 69     friend class CConnman;
 70     friend struct ConnmanTestMsg;
 71 
 72 public:
 73-    const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
 74-    const std::unique_ptr<const TransportSerializer> m_serializer;
 75+    V1TransportDeserializer m_deserializer; // Used only by SocketHandler thread
 76+    const V1TransportSerializer m_serializer;
 77 
 78     NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester
 79     std::atomic<ServiceFlags> nServices{NODE_NONE};
 80 
 81     /**
 82      * Socket used for communication with the node.
 83diff --git i/src/test/util/net.cpp w/src/test/util/net.cpp
 84index 62b770753a..c6d0c185d1 100644
 85--- i/src/test/util/net.cpp
 86+++ w/src/test/util/net.cpp
 87@@ -30,13 +30,13 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_by
 88     }
 89 }
 90 
 91 bool ConnmanTestMsg::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const
 92 {
 93     std::vector<uint8_t> ser_msg_header;
 94-    node.m_serializer->prepareForTransport(ser_msg, ser_msg_header);
 95+    node.m_serializer.prepareForTransport(ser_msg, ser_msg_header);
 96 
 97     bool complete;
 98     NodeReceiveMsgBytes(node, ser_msg_header, complete);
 99     NodeReceiveMsgBytes(node, ser_msg.data, complete);
100     return complete;
101 }

ajtowns commented at 5:21 pm on May 26, 2022:

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

vasild commented at 7:10 am on May 27, 2022:

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:

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

VS

After:

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

It looks like a pure simplification.

jnewbery commented at 10:33 am on May 27, 2022:

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.

ajtowns commented at 3:06 pm on May 27, 2022:

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.

vasild commented at 12:12 pm on June 2, 2022:

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

in src/net.h:347 in 5df0bd1e8d outdated

417-    std::unique_ptr<TransportSerializer> m_serializer;
418+    const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
419+    const std::unique_ptr<const TransportSerializer> m_serializer;
420 
421-    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None};
422+    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester

vasild commented at 10:14 am on May 24, 2022:

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

  0diff --git i/src/net.cpp w/src/net.cpp
  1index e275c2964d..7bcaa6feef 100644
  2--- i/src/net.cpp
  3+++ w/src/net.cpp
  4@@ -1237,15 +1237,15 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
  5                              addr,
  6                              CalculateKeyedNetGroup(addr),
  7                              nonce,
  8                              addr_bind,
  9                              /*addrNameIn=*/"",
 10                              ConnectionType::INBOUND,
 11-                             inbound_onion);
 12+                             inbound_onion,
 13+                             permissionFlags);
 14     pnode->AddRef();
 15-    pnode->m_permissionFlags = permissionFlags;
 16     pnode->m_prefer_evict = discouraged;
 17     m_msgproc->InitializeNode(pnode);
 18 
 19     LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString());
 20 
 21     {
 22@@ -3041,15 +3041,16 @@ ServiceFlags CConnman::GetLocalServices() const
 23 {
 24     return nLocalServices;
 25 }
 26 
 27 unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }
 28 
 29-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)
 30+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)
 31     : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))},
 32       m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())},
 33+      m_permissionFlags{permissoin_flags},
 34       m_sock{sock},
 35       m_connected{GetTime<std::chrono::seconds>()},
 36       addr(addrIn),
 37       addrBind(addrBindIn),
 38       m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn},
 39       m_inbound_onion(inbound_onion),
 40diff --git i/src/net.h w/src/net.h
 41index 5c43c5bfb8..8701e7502a 100644
 42--- i/src/net.h
 43+++ w/src/net.h
 44@@ -413,13 +413,13 @@ class CNode
 45     friend struct ConnmanTestMsg;
 46 
 47 public:
 48     const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread
 49     const std::unique_ptr<const TransportSerializer> m_serializer;
 50 
 51-    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester
 52+    const NetPermissionFlags m_permissionFlags;
 53     std::atomic<ServiceFlags> nServices{NODE_NONE};
 54 
 55     /**
 56      * Socket used for communication with the node.
 57      * May not own a Sock object (after `CloseSocketDisconnect()` or during tests).
 58      * `shared_ptr` (instead of `unique_ptr`) is used to avoid premature close of
 59@@ -582,13 +582,13 @@ public:
 60     std::atomic<std::chrono::microseconds> m_last_ping_time{0us};
 61 
 62     /** Lowest measured round-trip time. Used as an inbound peer eviction
 63      * criterium in CConnman::AttemptToEvictConnection. */
 64     std::atomic<std::chrono::microseconds> m_min_ping_time{std::chrono::microseconds::max()};
 65 
 66-    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);
 67+    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);
 68     CNode(const CNode&) = delete;
 69     CNode& operator=(const CNode&) = delete;
 70 
 71     NodeId GetId() const {
 72         return id;
 73     }
 74diff --git i/src/test/fuzz/util.cpp w/src/test/fuzz/util.cpp
 75index 033c6e18d5..5bbaf903c0 100644
 76--- i/src/test/fuzz/util.cpp
 77+++ w/src/test/fuzz/util.cpp
 78@@ -233,13 +233,12 @@ bool FuzzedSock::IsConnected(std::string& errmsg) const
 79 }
 80 
 81 void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, PeerManager& peerman, CNode& node) noexcept
 82 {
 83     const bool successfully_connected{fuzzed_data_provider.ConsumeBool()};
 84     const ServiceFlags remote_services = ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS);
 85-    const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
 86     const int32_t version = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max());
 87     const bool relay_txs{fuzzed_data_provider.ConsumeBool()};
 88 
 89     const CNetMsgMaker mm{0};
 90 
 91     CSerializedNetMsg msg_version{
 92@@ -268,13 +267,12 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman,
 93     assert(node.nVersion == version);
 94     assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
 95     assert(node.nServices == remote_services);
 96     CNodeStateStats statestats;
 97     assert(peerman.GetNodeStateStats(node.GetId(), statestats));
 98     assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn()));
 99-    node.m_permissionFlags = permission_flags;
100     if (successfully_connected) {
101         CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)};
102         (void)connman.ReceiveMsgFrom(node, msg_verack);
103         node.fPauseSend = false;
104         connman.ProcessMessagesOnce(node);
105         {
106diff --git i/src/test/fuzz/util.h w/src/test/fuzz/util.h
107index 580105e442..4d3568ff81 100644
108--- i/src/test/fuzz/util.h
109+++ w/src/test/fuzz/util.h
110@@ -297,23 +297,25 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
111     const uint64_t keyed_net_group = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
112     const uint64_t local_host_nonce = fuzzed_data_provider.ConsumeIntegral<uint64_t>();
113     const CAddress addr_bind = ConsumeAddress(fuzzed_data_provider);
114     const std::string addr_name = fuzzed_data_provider.ConsumeRandomLengthString(64);
115     const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES);
116     const bool inbound_onion{conn_type == ConnectionType::INBOUND ? fuzzed_data_provider.ConsumeBool() : false};
117+    const NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS);
118     if constexpr (ReturnUniquePtr) {
119         return std::make_unique<CNode>(node_id,
120                                        local_services,
121                                        sock,
122                                        address,
123                                        keyed_net_group,
124                                        local_host_nonce,
125                                        addr_bind,
126                                        addr_name,
127                                        conn_type,
128-                                       inbound_onion);
129+                                       inbound_onion,
130+                                       permission_flags);
131     } else {
132         return CNode{node_id,
133                      local_services,
134                      sock,
135                      address,
136                      keyed_net_group,

hebasto commented at 10:46 am on July 18, 2022: member

Concept ACK.

DrahtBot added the label Needs rebase on Jul 19, 2022

ajtowns force-pushed on Aug 5, 2022

DrahtBot removed the label Needs rebase on Aug 5, 2022

DrahtBot added the label Needs rebase on Aug 26, 2022

ajtowns force-pushed on Aug 29, 2022

DrahtBot removed the label Needs rebase on Aug 29, 2022

in src/net_processing.cpp:660 in 38ea8226ba outdated

656@@ -657,7 +657,7 @@ class PeerManagerImpl final : public PeerManager
657 
658     /** Whether we've completed initial sync yet, for determining when to turn
659       * on extra block-relay-only peers. */
660-    bool m_initial_sync_finished{false};
661+    bool m_initial_sync_finished GUARDED_BY(cs_main){false};

MarcoFalke commented at 7:52 am on August 29, 2022:

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.

ajtowns commented at 8:32 am on August 29, 2022:

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

in src/net.h:616 in 069fc1e4d8 outdated

612@@ -613,7 +613,7 @@ class CNode
613      * closed.
614      * Otherwise this unique_ptr is empty.
615      */
616-    std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex);
617+    std::unique_ptr<i2p::sam::Session> m_i2p_sam_session GUARDED_BY(m_sock_mutex) PT_GUARDED_BY(m_sock_mutex);

jonatack commented at 10:33 am on August 29, 2022:

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

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

ajtowns commented at 11:47 am on August 29, 2022:

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.

jonatack commented at 10:44 am on August 29, 2022: contributor

ACK first commit “net/net_processing: add missing thread safety annotations” modulo the GUARDED_BY(cs_main) annotations

net/net_processing: add missing thread safety annotations 06ebdc886f

net: mark TransportSerializer/m_serializer as const

The (V1)TransportSerializer instance CNode::m_serializer is used from
multiple threads via PushMessage without protection by a mutex. This
is only thread safe because the class does not have any mutable state,
so document that by marking the methods and the object as "const".

bbec32c9ad

net: mark CNode unique_ptr members as const

Dereferencing a unique_ptr is not necessarily thread safe. The reason
these are safe is because their values are set at construction and do
not change later; so mark them as const and set them via the initializer
list to guarantee that.

ef26f2f421

net: note CNode members that are treated as const

m_permissionFlags and m_prefer_evict are treated as const -- they're
only set immediately after construction before any other thread has
access to the object, and not changed again afterwards. As such they
don't need to be marked atomic or guarded by a mutex; though it would
probably be better to actually mark them as const...

9816dc96b7

ajtowns force-pushed on Aug 29, 2022

MarcoFalke commented at 1:04 pm on August 29, 2022: member

review ACK 9816dc96b77fd9780bb97891cd45a1b9798db8f5 📍

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 9816dc96b77fd9780bb97891cd45a1b9798db8f5 📍
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiGPQwAk8l83FaAio2oGaavZm/DW+AtDfrRktDHvmfZC78JmukdBl+ZFGYStrY+
 8b7WdSH1n98Xf1Czr1oHrMc1uEHxj/ajAw0lR3ELurht0jRv6o12YMiltI6E21olh
 9YFhrHVDPwPOgfcS8W1quNAv21MFX1tQTP7vFyyHiZbkP51dHlLKOJRah6SL9Ag9l
10CyFXjrDtIE6bgOF06+/T/3KFPDxbxy8vrhsMTGeg33oj6FcCvP+xhkLrwyrmdHnT
11mmcHCjfmoWRHcY5alEKOUQAFw+olpEzlLKq8mwsZoCt4prbk8tfihKu3If7LxscP
12uF4dDGD3nWiOL76mVgF27MMurdIY7yCI5IHNPOzBxQX8dfjuKY0OnPXLWFJjKOXf
13i2MOBQyxJ4fAkZ+KVyBo5qEh2PuwTA+oMFbIyNhaj/IDpOv9W9dHb7jqEkgVEynb
14BtmQztp7SkutBiv1rfarRRSWKRkrussJzMY43U7lFe+ClZFXMEw3NJh/AqtThtdV
15mG/YSC0D
16=34Gj
17-----END PGP SIGNATURE-----

ajtowns commented at 1:16 pm on August 29, 2022: contributor

Here’s one way of documenting “This member is only accessed by the scheduler thread” with (I believe) no runtime impact:

0class LOCKABLE CodeInspectionRequired { };
1class SCOPED_LOCKABLE CodeInspectionSaysSafeToUse
2{
3public:
4    CodeInspectionSaysSafeToUse(const CodeInspectionRequired& mut) EXCLUSIVE_LOCK_FUNCTION(mut) { }
5    ~CodeInspectionSaysSafeToUse() UNLOCK_FUNCTION() { }
6};
7#define SAFE_TO_USE(mut) CodeInspectionSaysSafeToUse UNIQUE_NAME(criticalblock)(mut)

You’d then use it to annotate variables where you have to manually inspect the logic used to access them:

0    static constexpr CodeInspectionRequired FROM_SCHEDULER_THREAD;
1    std::chrono::seconds m_stale_tip_check_time GUARDED_BY(FROM_SCHEDULER_THREAD){0s};

but could pass that logic up through functions:

0    void CheckForStaleTipAndEvictPeers() EXCLUSIVE_LOCKS_REQUIRED(FROM_SCHEDULER_THREAD) override;

to the caller:

0    scheduler.scheduleEvery([this] { SAFE_TO_USE(FROM_SCHEDULER_THREAD); this->CheckForStaleTipAndEvictPeers(); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});

In this particular case, you’d need to put the FROM_SCHEDULER_THREAD declaration in the PeerManager virtual class where CheckForStaleTipAndEvictPeers is first declared there, and also add SAFE_TO_USE annotations in the unit tests. Without the SAFE_TO_USE annotation, clang errors with:

0net_processing.cpp:1695:81: error: calling function 'CheckForStaleTipAndEvictPeers' requires holding mutex 'FROM_SCHEDULER_THREAD' exclusively [-Werror,-Wthread-safety-analysis]
1    scheduler.scheduleEvery([this] { this->CheckForStaleTipAndEvictPeers(); }, std::chrono::seconds{EXTRA_PEER_CHECK_INTERVAL});

Is something like the above a good idea? It seems better than what we have now, where we don’t document why a variable is safe at all, and could introduce a race by having one PR introduce a new access from another thread with cs_main held, while another introduces a new access from the scheduler thread without cs_main held, with a bug introduced because neither reviewers nor the compiler hasn’t been told what to expect.

But compared to just saying GUARDED_BY(cs_main) when it already is guarded by cs_main, the above seems needlessly complicated and like it puts too much work on developers rather than tooling.

jonatack commented at 3:38 pm on August 29, 2022: contributor

utACK 9816dc96b77fd9780bb97891cd45a1b9798db8f5

hebasto approved

hebasto commented at 9:27 am on August 30, 2022: member

ACK 9816dc96b77fd9780bb97891cd45a1b9798db8f5, I have reviewed the code and it looks OK. In particular, I verified the usage of variables which got GUARDED_BY annotations.

Still curious about PR author’s opinion about #25174 (review).

MarcoFalke commented at 9:34 am on August 30, 2022: member

Still curious about PR author’s opinion about #25174 (review).

See #25174 (review)

MarcoFalke merged this on Aug 30, 2022

MarcoFalke closed this on Aug 30, 2022

sidhujag referenced this in commit 212684be09 on Aug 30, 2022

bitcoin locked this on Aug 31, 2023

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

Conflicts