net: add NetEventsInterface::g_msgproc_mutex #26036
Conversation
|
Resurrected from #24474. The main purpose of this PR is to document the variables we assume are only accessed via one thread, and hence which we don't bother to grab a mutex before accessing. Since these annotations are checked automatically by clang in CI, this should help avoid possible bugs in future. To do this, this PR replaces the per-peer It would also be possible to have a "dummy" lockable type and use that to document these variables, rather than an actual mutex. I've done an example of that sort of approach at #25174 (comment) but for this case where the lock is only ever acquired once at startup and has no possible contention, the overhead seems so minimal as to not be worth worrying about. In the long-term, simplifying the locking scheme, and perhaps allowing more things to happen in parallel are obviously still worthwhile; this PR doesn't mean to imply that these things should only be used from a single thread in future, rather it's just documenting that they are right now. See also #25454 (review) #25557 (comment) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
f55747f to
22e7c46
Compare
Original point of all this was just to be able to make I don't think multiple |
|
Any reason #26036 (comment) wasn't addressed in the last push? It would be helpful to know if the thread safety guidelines in the developer notes should be updated. |
There was a problem hiding this comment.
review ACK 22e7c46 🍡
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK 22e7c464db9719ebc8eba0143f5a6024aac658c3 🍡
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhiGQwAkEL9Sna2ocIewx1Haykhnz0jJc7HCkseZAGV6fG6B7XaIvRJzv72iDgu
cxRU8pnGwa0GNai6aL6oGVJN61ic6UmAtHjn13T+hLTngOVJKq5G9QfuIAaafyRZ
Mv9zQ1n+qbydjD4uBrkkelQc65lX1wASK1q1h4S2iZR1q4oree5rQr9QpOQ7APSX
hfpqRrkYXkLKdlJ89aeNYHaJ6KFtT0b2oJsCCQpkU/Ou3ywluqnN+7QnkNbIDQmu
DQl0gTrImpldkLsgqmSE4CDtcy7Oopx0AbbDyn3ClKj2wOnRXAaa0QUmtcYdACF0
3Wv88lr3mEr+SReN7E5CaijpQELQG2wAbHykl1FQ8kPQ8oQkCaA0gA6D4VL5Ol85
MOAU3gr2mazuo1KKF+tT9HAIb5pFJBzFcFIHpQpKijVWVp6wAqQubQKQOd/qf2Pr
p7r3kqlbXurQIaEn4KpxbYJ7hg2F5X/q+RdS2vgtPr3dC2MLsEEB4ZqjpPe7MiyH
YQJlcdGU
=OIUy
-----END PGP SIGNATURE-----
src/net_processing.cpp
Outdated
| @@ -5099,7 +5099,7 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros | |||
|
|
|||
| // Remove addr records that the peer already knows about, and add new | |||
| // addrs to the m_addr_known filter on the same pass. | |||
| auto addr_already_known = [&peer](const CAddress& addr) { | |||
| auto addr_already_known = [&peer](const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { | |||
There was a problem hiding this comment.
db5c65b: Any reason to add the namespace bloat here, but not elsewhere? It seems unlikely that there is another symbol with the same name in the global or another namespace. For that reason I'd also prefer to just make it a global in the global namespace to also de-bloat the other places.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 594141ca84..d9a1124b64 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -537,7 +537,7 @@ public:
private:
/** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
- void ConsiderEviction(CNode& pto, Peer& peer, std::chrono::seconds time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main, NetEventsInterface::g_msgproc_mutex);
+ void ConsiderEviction(CNode& pto, Peer& peer, std::chrono::seconds time_in_seconds) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_msgproc_mutex);
/** If we have extra outbound peers, try to disconnect the one with the oldest block announcement */
void EvictExtraOutboundPeers(std::chrono::seconds now) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
@@ -601,7 +601,7 @@ private:
void ProcessHeadersMessage(CNode& pfrom, Peer& peer,
std::vector<CBlockHeader>&& headers,
bool via_compact_block)
- EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex, NetEventsInterface::g_msgproc_mutex);
+ EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
/** Various helpers for headers processing, invoked by ProcessHeadersMessage() */
/** Return true if headers are continuous and have valid proof-of-work (DoS points assigned on failure) */
bool CheckHeadersPoW(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams, Peer& peer);
@@ -610,7 +610,7 @@ private:
/** Deal with state tracking and headers sync for peers that send the
* occasional non-connecting header (this can happen due to BIP 130 headers
* announcements for blocks interacting with the 2hr (MAX_FUTURE_BLOCK_TIME) rule). */
- void HandleFewUnconnectingHeaders(CNode& pfrom, Peer& peer, const std::vector<CBlockHeader>& headers) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+ void HandleFewUnconnectingHeaders(CNode& pfrom, Peer& peer, const std::vector<CBlockHeader>& headers) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
/** Return true if the headers connect to each other, false otherwise */
bool CheckHeadersAreContinuous(const std::vector<CBlockHeader>& headers) const;
/** Try to continue a low-work headers sync that has already begun.
@@ -633,7 +633,7 @@ private:
*/
bool IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfrom,
std::vector<CBlockHeader>& headers)
- EXCLUSIVE_LOCKS_REQUIRED(peer.m_headers_sync_mutex, !m_headers_presync_mutex, NetEventsInterface::g_msgproc_mutex);
+ EXCLUSIVE_LOCKS_REQUIRED(peer.m_headers_sync_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
/** Check work on a headers chain to be processed, and if insufficient,
* initiate our anti-DoS headers sync mechanism.
*
@@ -649,7 +649,7 @@ private:
bool TryLowWorkHeadersSync(Peer& peer, CNode& pfrom,
const CBlockIndex* chain_start_header,
std::vector<CBlockHeader>& headers)
- EXCLUSIVE_LOCKS_REQUIRED(!peer.m_headers_sync_mutex, !m_peer_mutex, !m_headers_presync_mutex, NetEventsInterface::g_msgproc_mutex);
+ EXCLUSIVE_LOCKS_REQUIRED(!peer.m_headers_sync_mutex, !m_peer_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
/** Return true if the given header is an ancestor of
* m_chainman.m_best_header or our current tip */
@@ -659,7 +659,7 @@ private:
* We don't issue a getheaders message if we have a recent one outstanding.
* This returns true if a getheaders is actually sent, and false otherwise.
*/
- bool MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& locator, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+ bool MaybeSendGetHeaders(CNode& pfrom, const CBlockLocator& locator, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
/** Potentially fetch blocks from this peer upon receipt of a new headers tip */
void HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, const CBlockIndex* pindexLast);
/** Update peer state based on received headers message */
@@ -683,10 +683,10 @@ private:
void MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now);
/** Send `addr` messages on a regular schedule. */
- void MaybeSendAddr(CNode& node, Peer& peer, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+ void MaybeSendAddr(CNode& node, Peer& peer, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
/** Send a single `sendheaders` message, after we have completed headers sync with a peer. */
- void MaybeSendSendHeaders(CNode& node, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+ void MaybeSendSendHeaders(CNode& node, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
/** Relay (gossip) an address to a few randomly chosen nodes.
*
@@ -695,10 +695,10 @@ private:
* @param[in] fReachable Whether the address' network is reachable. We relay unreachable
* addresses less.
*/
- void RelayAddress(NodeId originator, const CAddress& addr, bool fReachable) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, NetEventsInterface::g_msgproc_mutex);
+ void RelayAddress(NodeId originator, const CAddress& addr, bool fReachable) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex);
/** Send `feefilter` message. */
- void MaybeSendFeefilter(CNode& node, Peer& peer, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+ void MaybeSendFeefilter(CNode& node, Peer& peer, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
const CChainParams& m_chainparams;
CConnman& m_connman;
@@ -1010,7 +1010,7 @@ private:
* @return True if address relay is enabled with peer
* False if address relay is disallowed
*/
- bool SetupAddressRelay(const CNode& node, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
+ bool SetupAddressRelay(const CNode& node, Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
};
const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
@@ -5099,7 +5099,7 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
// Remove addr records that the peer already knows about, and add new
// addrs to the m_addr_known filter on the same pass.
- auto addr_already_known = [&peer](const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
+ auto addr_already_known = [&peer](const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) {
bool ret = peer.m_addr_known->contains(addr.GetKey());
if (!ret) peer.m_addr_known->insert(addr.GetKey());
return ret;There was a problem hiding this comment.
Ah, didn't cross my mind that it wasn't necessary here. m_last_block_inv_triggering_headers_sync also doesn't need the class name. I've left it in the class since that class essentially defines "message processing" so it seems more appropriate than not.
There was a problem hiding this comment.
g_msgproc_mutex already clarifies that this is for message processing, so still think the namespace/class can be dropped
There was a problem hiding this comment.
Then, should it be GlobalMutex? I had some plans to eradicate that one.
There was a problem hiding this comment.
If it were a normal global rather than a static member of the class it would need to be GlobalMutex (otherwise you get an error that CConnman::ThreadMessageHandler() needs to be annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_msgproc_mutex), and probably similar in the tests). With the lock being a class member, clang assumes functions outside of the class aren't called with the lock held.
There was a problem hiding this comment.
Yes, I mean that if "the namespace/class is dropped" then it becomes normal global and thus its type must be changed.
There are many cases where we assume message processing is single-threaded in order for how we access node-related memory to be safe. Add an explicit mutex that we can use to document this, which allows the compiler to catch any cases where we try to access that memory from other threads and break that assumption.
SendMessages() is now protected g_msgproc_mutex; so this additional per-node mutex is redundant.
22e7c46 to
60682dc
Compare
|
review ACK 60682dc 🤞 Show signatureSignature: |
…ed only via the msgproc thread
…bers accessed only via the msgproc thread
60682dc to
d575a67
Compare
There was a problem hiding this comment.
ACK d575a67 modulo the missing runtime checks
It would be really nice not to violate the developer notes, or at least if we occasionally do, to be for a better reason than given in #26036 (comment).
|
review ACK d575a67 📽 Show signatureSignature: |
|
Code review ACK d575a67 There are a couple |
There are many cases where we assume message processing is single-threaded in order for how we access node-related memory to be safe. Add an explicit mutex that we can use to document this, which allows the compiler to catch any cases where we try to access that memory from other threads and break that assumption.