Add Clang thread safety annotations for variables guarded by:
cs_addrLocalcs_addrNamecs_feeFiltercs_filtercs_hSocketcs_inventorycs_mapLocalHostcs_most_recent_blockcs_proxyInfoscs_sendProcessingcs_setBannedcs_SubVercs_vOneShotscs_vProcessMsgcs_vRecvcs_vSendChanged files:
src/net.{cpp,h}src/netbase.cpp
Rebased and added missing lock:
0diff --git a/src/net.cpp b/src/net.cpp
1index 529a2f82f..7fcf6a3ef 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -715,7 +715,10 @@ void CNode::copyStats(CNodeStats &stats)
5 X(nRecvBytes);
6 }
7 X(fWhitelisted);
8- X(minFeeFilter);
9+ {
10+ LOCK(cs_feeFilter);
11+ X(minFeeFilter);
12+ }
13
14 // It is common for nodes with good ping times to suddenly become lagged,
15 // due to a new block arriving or other large transfer.
630- std::deque<std::vector<unsigned char>> vSendMsg;
631 CCriticalSection cs_vSend;
632- CCriticalSection cs_hSocket;
633+ uint64_t nSendBytes GUARDED_BY(cs_vSend);
634+ std::deque<std::vector<unsigned char>> vSendMsg GUARDED_BY(cs_vSend);
635 CCriticalSection cs_vRecv;
652@@ -653,8 +653,8 @@ class CNode
653 // to be printed out, displayed to humans in various forms and so on. So we sanitize it and
654 // store the sanitized version in cleanSubVer. The original should be used when dealing with
655 // the network or wire types and the cleaned string used when displayed or logged.
656- std::string strSubVer, cleanSubVer;
657 CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer
658+ std::string strSubVer GUARDED_BY(cs_SubVer), cleanSubVer GUARDED_BY(cs_SubVer);
673+ bool fRelayTxes GUARDED_BY(cs_filter);
674 bool fSentAddr;
675 CSemaphoreGrant grantOutbound;
676- CCriticalSection cs_filter;
677- std::unique_ptr<CBloomFilter> pfilter;
678+ std::unique_ptr<CBloomFilter> pfilter GUARDED_BY(cs_filter);
PT_GUARDED_BY?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
673@@ -673,15 +674,15 @@ class CNode
674 const bool fInbound;
675 std::atomic_bool fSuccessfullyConnected;
676 std::atomic_bool fDisconnect;
677+ mutable CCriticalSection cs_filter;
utACK 4894133dc5095ed971bf98d695384e7cbb16e54c
[1]
0WARNING: ThreadSanitizer: data race (pid=30947)
1 Read of size 8 at 0x7d6c0005e688 by thread T12 (mutexes: write M131981):
2 [#0](/bitcoin-bitcoin/0/) CNode::copyStats(CNodeStats&) /bitcoin/src/net.cpp:718 (bitcoind+0x0000005224e5)
3 [#1](/bitcoin-bitcoin/1/) CConnman::GetNodeStats(std::vector<CNodeStats, std::allocator<CNodeStats> >&) /bitcoin/src/net.cpp:2567 (bitcoind+0x0000005346b7)
4 [#2](/bitcoin-bitcoin/2/) getpeerinfo(JSONRPCRequest const&) /bitcoin/src/rpc/net.cpp:131 (bitcoind+0x000000617137)
5
6...
7
8 Previous write of size 8 at 0x7d6c0005e688 by thread T19 (mutexes: write M134272):
9 [#0](/bitcoin-bitcoin/0/) ProcessMessage(CNode*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CDataStream&, long, CChainParams const&, CConnman*, std::atomic<bool> const&, bool) /bitcoin/src/net_processing.cpp:2927 (bitcoind+0x000000562dc8)
nNextAddrSend and nNextLocalAddrSend now guarded by cs_sendProcessing? I don’t think those things should be connected.
@practicalswift sorry I meant to say that I have a branch that moves the fields around that I haven’t PR’ed yet. I’ll ping you when the PR is open.
PR is open here: #21236. It’s a pure refactor and should be easy to review.