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!
Imo could squash everything, since most of the commits only touch 2 lines.
@MarcoFalke Rebased and squashed! Please re-review :-)
Rebased and added missing lock:
diff --git a/src/net.cpp b/src/net.cpp
index 529a2f82f..7fcf6a3ef 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -715,7 +715,10 @@ void CNode::copyStats(CNodeStats &stats)
X(nRecvBytes);
}
X(fWhitelisted);
- X(minFeeFilter);
+ {
+ LOCK(cs_feeFilter);
+ X(minFeeFilter);
+ }
// It is common for nodes with good ping times to suddenly become lagged,
// 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;
nit: move this down?
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);
nit: separate lines?
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?
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
@MarcoFalke @Empact Updated. Please re-review :-)
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;
Is moving those lines required? It makes automated review harder, so best to keep them where they were?
Good point! Fixed. Please re-review :-)
utACK 4894133dc5095ed971bf98d695384e7cbb16e54c
[1]
WARNING: ThreadSanitizer: data race (pid=30947)
Read of size 8 at 0x7d6c0005e688 by thread T12 (mutexes: write M131981):
[#0](/github-metadata-backup-bitcoin-bitcoin/0/) CNode::copyStats(CNodeStats&) /bitcoin/src/net.cpp:718 (bitcoind+0x0000005224e5)
[#1](/github-metadata-backup-bitcoin-bitcoin/1/) CConnman::GetNodeStats(std::vector<CNodeStats, std::allocator<CNodeStats> >&) /bitcoin/src/net.cpp:2567 (bitcoind+0x0000005346b7)
[#2](/github-metadata-backup-bitcoin-bitcoin/2/) getpeerinfo(JSONRPCRequest const&) /bitcoin/src/rpc/net.cpp:131 (bitcoind+0x000000617137)
...
Previous write of size 8 at 0x7d6c0005e688 by thread T19 (mutexes: write M134272):
[#0](/github-metadata-backup-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)
@practicalswift why are nNextAddrSend and nNextLocalAddrSend now guarded by cs_sendProcessing? I don't think those things should be connected.
@jnewbery I agree that sure looks like an error! Good catch!
Thanks for the quick reply. I have a PR moving some of those fields around, so I'll get rid of the lock annotations there.
@jnewbery Thanks a lot for fixing (and finding)! Much appreciated!
@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.
@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.