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

ajtowns · 2022-05-19T22:35:26Z

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 · 2022-05-19T22:36:20Z

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

jnewbery

concept ACK

jnewbery · 2022-05-20T07:24:00Z

src/net_processing.cpp

@@ -599,14 +599,14 @@ class PeerManagerImpl final : public PeerManager
    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};


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 (comment) and the whole point of breaking this PR out was to try to make some incremental progress in the meantime.

jnewbery · 2022-05-20T07:28:20Z

src/net_processing.cpp


    /** Whether this node is running in blocks only mode */
    const bool m_ignore_incoming_txs;

    /** 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};


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.

jnewbery · 2022-05-20T07:29:57Z

src/net.h

@@ -466,7 +466,7 @@ class CNode
     * 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)


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

jnewbery · 2022-05-20T07:30:48Z

src/net.h

@@ -413,10 +413,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


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.

DrahtBot · 2022-05-20T09:25:29Z

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 · 2022-05-20T16:36:05Z

Approach ACK.

vasild · 2022-05-24T09:51:58Z

src/net.h

-    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;


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:

diff

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; }

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.

vasild · 2022-05-24T10:14:18Z

src/net.h

-    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None};
+    NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester


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

diff

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,

hebasto · 2022-07-18T10:46:26Z

Concept ACK.

maflcko · 2022-08-29T07:52:53Z

src/net_processing.cpp

@@ -657,7 +657,7 @@ class PeerManagerImpl final : public PeerManager

    /** 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};


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

jonatack

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

src/net.h

jonatack · 2022-08-29T10:36:20Z

src/net.h

@@ -466,7 +466,7 @@ class CNode
     * 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)


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); }

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

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.

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

maflcko · 2022-08-29T13:04:41Z

review ACK 9816dc9 📍

Show signature

Signature:

ajtowns · 2022-08-29T13:16:32Z

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

class LOCKABLE CodeInspectionRequired { };
class SCOPED_LOCKABLE CodeInspectionSaysSafeToUse
{
public:
    CodeInspectionSaysSafeToUse(const CodeInspectionRequired& mut) EXCLUSIVE_LOCK_FUNCTION(mut) { }
    ~CodeInspectionSaysSafeToUse() UNLOCK_FUNCTION() { }
};
#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:

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

but could pass that logic up through functions:

    void CheckForStaleTipAndEvictPeers() EXCLUSIVE_LOCKS_REQUIRED(FROM_SCHEDULER_THREAD) override;

to the caller:

    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:

net_processing.cpp:1695:81: error: calling function 'CheckForStaleTipAndEvictPeers' requires holding mutex 'FROM_SCHEDULER_THREAD' exclusively [-Werror,-Wthread-safety-analysis]
    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 · 2022-08-29T15:38:57Z

utACK 9816dc9

hebasto

ACK 9816dc9, 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 (comment).

maflcko · 2022-08-30T09:34:26Z

Still curious about PR author's opinion about #25174 (comment).

See #25174 (comment)

…notatio…

ajtowns added the Refactoring label May 19, 2022

ajtowns requested review from hebasto, jnewbery and vasild May 19, 2022 22:39

ajtowns mentioned this pull request May 20, 2022

Additional thread safety annotations for CNode/Peer members accessed via the message processing thread #24474

Closed

jnewbery reviewed May 20, 2022

View reviewed changes

DrahtBot mentioned this pull request May 20, 2022

net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer #24970

Closed

vasild reviewed May 24, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 8, 2022

refactor: Introduce EvictionManager #25268

Closed

DrahtBot mentioned this pull request Jun 16, 2022

I2P: add support for transient addresses for outbound connections #25355

Merged

This was referenced Jun 30, 2022

net, test: Virtualise CConnman and add initial PeerManager unit tests #25515

Closed

net processing: Move CNode::nServices and CNode::nLocalServices to Peer #25514

Merged

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

DrahtBot mentioned this pull request Jul 8, 2022

refactor: Introduce EvictionManager and use it for the inbound eviction logic #25572

Closed

DrahtBot added the Needs rebase label Jul 19, 2022

ajtowns force-pushed the 202205-net-lock-annotations branch from 5df0bd1 to 5cdc128 Compare August 5, 2022 06:08

DrahtBot removed the Needs rebase label Aug 5, 2022

DrahtBot added the Needs rebase label Aug 26, 2022

ajtowns force-pushed the 202205-net-lock-annotations branch from 5cdc128 to 069fc1e Compare August 29, 2022 06:42

DrahtBot removed the Needs rebase label Aug 29, 2022

This was referenced Aug 29, 2022

net_processing: lock clean up #21527

Closed

net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758

Closed

maflcko reviewed Aug 29, 2022

View reviewed changes

jonatack reviewed Aug 29, 2022

View reviewed changes

ajtowns added 2 commits August 29, 2022 22:50

net/net_processing: add missing thread safety annotations

06ebdc8

ajtowns added 2 commits August 29, 2022 22:50

net: mark CNode unique_ptr members as const

ef26f2f

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.

ajtowns force-pushed the 202205-net-lock-annotations branch from 069fc1e to 9816dc9 Compare August 29, 2022 12:51

hebasto approved these changes Aug 30, 2022

View reviewed changes

maflcko merged commit cfda740 into bitcoin:master Aug 30, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 30, 2022

Merge bitcoin#25174: net/net_processing: Add thread safety related an…

212684b

…notatio…

ajtowns mentioned this pull request Sep 7, 2022

net: add NetEventsInterface::g_msgproc_mutex #26036

Merged

bitcoin locked and limited conversation to collaborators Aug 31, 2023

		NetPermissionFlags m_permissionFlags{NetPermissionFlags::None};
		NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester

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

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

Uh oh!

Conversation

ajtowns commented May 19, 2022

Uh oh!

ajtowns commented May 19, 2022

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

w0xlt commented May 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

jonatack Aug 29, 2022 •

edited

Loading

jonatack Aug 29, 2022 •

edited

Loading

DrahtBot commented May 20, 2022 •

edited

Loading

jonatack Aug 29, 2022 •

edited

Loading