backport: bitcoin#21202, bitcoin#24931 and multiple thread safety asserion fixes #6880
Conversation
|
WalkthroughThe patch introduces a new GlobalMutex type and updates Assert/MaybeCheckNotHeld overloads and locking macros to recognise it. It adds or tightens EXCLUSIVE_LOCKS_REQUIRED and related thread-safety annotations across many modules (LLMQ, networking, CoinJoin, InstantSend, evo, signing, spork, masternode, stats, rpc, wallet, validation, tests, Qt, etc.). Several file-scope Mutex globals are changed to GlobalMutex. Spork cache locking is consolidated under cs_cache. CheckInputScripts signature and pvChecks default were changed to require cs_main. A WITH_LOCK wrapper was removed in governance.cpp. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
WalkthroughThis change introduces GlobalMutex and updates lock macros to include MaybeCheckNotHeld checks. Numerous methods across networking, LLMQ, evo, CoinJoin, InstantSend, stats, RPC, and GUI gain EXCLUSIVE_LOCKS_REQUIRED annotations. Multiple global/static Mutex instances are changed to GlobalMutex (e.g., best block, RPC warmup, net localhost, proxy info, time offset, wallet load). Validation API shifts from ChainstateManager to CChainState with new locking on ActivateBestChain and PreciousBlock. Spork cache locks are unified. Several functions add or tighten lock requirements (e.g., CheckInputScripts, net_processing paths). One governance tip retrieval drops a WITH_LOCK(::cs_main) wrapper. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/spork.h (1)
260-264: Tighten ProcessMessage lock contract
ProcessMessage()callsProcessSpork()/ProcessGetSporks(), both of which acquirecs. If callers ever invokeProcessMessage()while already holdingcs, they will deadlock. Please extend the annotation to forbid holdingcsas well:- [[nodiscard]] MessageProcessingResult ProcessMessage(... ) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); + [[nodiscard]] MessageProcessingResult ProcessMessage(... ) EXCLUSIVE_LOCKS_REQUIRED(!cs, !cs_cache);src/coinjoin/client.h (1)
98-105: Potential UAF race from Get/Flush raw pointer usage outside lockAnnotations match the implementation, but returning a raw pointer from Get() and using it in Flush() after releasing cs_wallet_manager_map can race with concurrent Remove(name), leading to use-after-free.
Recommended:
- Prefer a safer access pattern (e.g., execute operations via ForAnyCJClientMan/ForEachCJClientMan lambdas under controlled lifetime, or migrate map values to shared_ptr), or document/thread-confine Add/Remove/Flush to a single thread.
Also applies to: 107-120
src/stats/client.cpp (1)
51-74: API-level not-held contracts are correct; duplicate annotation on template definition
- EXCLUSIVE_LOCKS_REQUIRED(!cs) on dec/inc/count/gauge/gaugeDouble/timing/send overloads matches _send()’s internal WITH_LOCK(cs). Good.
- Consider also adding the attribute to the out-of-class template definition to keep Clang TSA happy for template instantiations.
Example (outside selected lines, for reference):
// Also add on the definition: template <typename T1> inline bool StatsdClientImpl::_send(std::string_view key, T1 value, std::string_view type, float sample_rate) EXCLUSIVE_LOCKS_REQUIRED(!cs) { // ... }Optionally, mirror these annotations on the StatsdClient interface to propagate checks to overrides.
Based on learnings
Also applies to: 76-100, 106-108
src/net.h (1)
1630-1630: Remove duplicate lock annotation.The
EXCLUSIVE_LOCKS_REQUIREDannotation lists!cs_sendable_receivable_nodestwice. While harmless (the compiler ignores duplicates), it should be cleaned up for clarity.Apply this diff:
- void SocketHandlerConnected(const Sock::EventsPerSock& events_per_sock) - EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); + void SocketHandlerConnected(const Sock::EventsPerSock& events_per_sock) + EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode);src/llmq/quorums.h (2)
271-276: CQuorumManager annotations look right; consider also excluding cs_quorumBaseBlockIndexCache.
- Methods now lock cs_db/cs_map_quorums/cs_scan_quororums internally; requiring them “not held” is appropriate and prevents re-entrant locking.
- GetQuorum(…, quorumHash) also locks cs_quorumBaseBlockIndexCache inside; to avoid accidental self-deadlocks, consider adding EXCLUSIVE_LOCKS_REQUIRED(!cs_quorumBaseBlockIndexCache) too, unless there are valid call sites that already hold it.
Can you confirm that no call path holds cs_quorumBaseBlockIndexCache when calling GetQuorum(llmqType, quorumHash)? If confirmed, adding the exclusion would strengthen analysis.
Also applies to: 277-280, 287-291, 293-296, 299-301, 302-305, 307-309, 319-320
8-22: Header should include sync.h for Mutex and annotations.This header uses Mutex and thread-safety macros but doesn’t include <sync.h>. Add it here to avoid relying on transitive includes.
Apply:
#include <unordered_lru_cache.h> +#include <sync.h> #include <protocol.h>src/llmq/signing.h (1)
239-240: Remove stray semicolon after WorkThreadMain declaration.Minor nit: the extra semicolon is harmless but unnecessary.
Apply:
- void WorkThreadMain(PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners); - ; + void WorkThreadMain(PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners);src/net_processing.cpp (2)
327-349: TxRelay accessors: annotations and locking look correct; optional API polishNegative lock requirements avoid re-entrant locking; bodies acquire the mutex internally. Optional:
- Return TxRelay& from GetInvRelay() to reflect non-null invariant, or document non-null explicitly.
- Mark both GetTxRelay() overloads [[nodiscard]].
620-623: ProcessMessages/SendMessages: broadened lock requirements are consistent; consider adding cs_main exclusionBoth assert g_msgproc_mutex and avoid holding m_peer_mutex/m_most_recent_block_mutex. Consider adding LOCKS_EXCLUDED(::cs_main) to both signatures to encode that they acquire cs_main internally and callers must not hold it.
Also applies to: 641-643
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
doc/developer-notes.md(2 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/util.h(1 hunks)src/evo/creditpool.h(1 hunks)src/evo/deterministicmns.h(7 hunks)src/evo/mnhftx.h(3 hunks)src/governance/governance.cpp(1 hunks)src/httpserver.cpp(1 hunks)src/init.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/blockprocessor.h(2 hunks)src/llmq/debug.h(1 hunks)src/llmq/dkgsession.h(1 hunks)src/llmq/dkgsessionhandler.h(4 hunks)src/llmq/dkgsessionmgr.h(3 hunks)src/llmq/quorums.h(4 hunks)src/llmq/signing.h(5 hunks)src/llmq/signing_shares.h(2 hunks)src/net.cpp(1 hunks)src/net.h(10 hunks)src/net_processing.cpp(5 hunks)src/netbase.cpp(1 hunks)src/qt/clientmodel.h(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/server.cpp(1 hunks)src/spork.cpp(6 hunks)src/spork.h(5 hunks)src/stats/client.cpp(1 hunks)src/sync.h(4 hunks)src/test/txvalidationcache_tests.cpp(1 hunks)src/timedata.cpp(1 hunks)src/util/system.cpp(1 hunks)src/validation.cpp(3 hunks)src/validation.h(1 hunks)src/versionbits.h(1 hunks)src/wallet/wallet.cpp(1 hunks)src/warnings.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/clientmodel.hsrc/versionbits.hsrc/rpc/blockchain.cppsrc/netbase.cppsrc/wallet/wallet.cppsrc/warnings.cppsrc/spork.cppsrc/llmq/dkgsessionmgr.hsrc/test/txvalidationcache_tests.cppsrc/httpserver.cppsrc/llmq/debug.hsrc/timedata.cppsrc/llmq/signing_shares.hsrc/evo/creditpool.hsrc/governance/governance.cppsrc/init.cppsrc/sync.hsrc/spork.hsrc/net.cppsrc/evo/mnhftx.hsrc/coinjoin/client.hsrc/net.hsrc/validation.cppsrc/validation.hsrc/llmq/dkgsession.hsrc/llmq/blockprocessor.hsrc/util/system.cppsrc/coinjoin/util.hsrc/llmq/dkgsessionhandler.hsrc/rpc/server.cppsrc/evo/deterministicmns.hsrc/net_processing.cppsrc/instantsend/instantsend.hsrc/stats/client.cppsrc/llmq/signing.hsrc/llmq/quorums.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/txvalidationcache_tests.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/creditpool.hsrc/evo/mnhftx.hsrc/evo/deterministicmns.h
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/developer-notes.md
🧠 Learnings (6)
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/net.hsrc/stats/client.cpp
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks.
Applied to files:
src/net.h
🧬 Code graph analysis (22)
src/qt/clientmodel.h (2)
src/qt/clientmodel.cpp (2)
TipChanged(258-279)TipChanged(258-258)src/net_processing.cpp (16)
EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(661-661)EXCLUSIVE_LOCKS_REQUIRED(664-664)EXCLUSIVE_LOCKS_REQUIRED(670-670)EXCLUSIVE_LOCKS_REQUIRED(673-673)EXCLUSIVE_LOCKS_REQUIRED(755-755)EXCLUSIVE_LOCKS_REQUIRED(775-775)EXCLUSIVE_LOCKS_REQUIRED(835-835)EXCLUSIVE_LOCKS_REQUIRED(837-837)EXCLUSIVE_LOCKS_REQUIRED(842-842)EXCLUSIVE_LOCKS_REQUIRED(844-844)EXCLUSIVE_LOCKS_REQUIRED(845-845)EXCLUSIVE_LOCKS_REQUIRED(853-853)EXCLUSIVE_LOCKS_REQUIRED(854-854)
src/versionbits.h (1)
src/net_processing.cpp (16)
EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(661-661)EXCLUSIVE_LOCKS_REQUIRED(664-664)EXCLUSIVE_LOCKS_REQUIRED(670-670)EXCLUSIVE_LOCKS_REQUIRED(673-673)EXCLUSIVE_LOCKS_REQUIRED(755-755)EXCLUSIVE_LOCKS_REQUIRED(775-775)EXCLUSIVE_LOCKS_REQUIRED(835-835)EXCLUSIVE_LOCKS_REQUIRED(837-837)EXCLUSIVE_LOCKS_REQUIRED(842-842)EXCLUSIVE_LOCKS_REQUIRED(844-844)EXCLUSIVE_LOCKS_REQUIRED(845-845)EXCLUSIVE_LOCKS_REQUIRED(853-853)EXCLUSIVE_LOCKS_REQUIRED(854-854)
src/llmq/dkgsessionmgr.h (1)
src/llmq/dkgsessionmgr.cpp (6)
UpdatedBlockTip(79-93)UpdatedBlockTip(79-79)GetVerifiedContributions(298-345)GetVerifiedContributions(298-298)CleanupCache(380-391)CleanupCache(380-380)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (6)
AsyncSign(1510-1514)AsyncSign(1510-1510)SignPendingSigShares(1516-1538)SignPendingSigShares(1516-1516)WorkThreadMain(1486-1508)WorkThreadMain(1486-1486)
src/evo/creditpool.h (1)
src/evo/creditpool.cpp (8)
GetCreditPool(224-239)GetCreditPool(224-224)GetFromCache(118-138)GetFromCache(118-118)AddToCache(140-149)AddToCache(140-140)ConstructCreditPool(151-222)ConstructCreditPool(151-152)
src/governance/governance.cpp (1)
src/node/interfaces.cpp (3)
tip(96-104)tip(546-550)tip(567-575)
src/sync.h (5)
src/validation.cpp (17)
LOCKS_EXCLUDED(3349-3370)LOCKS_EXCLUDED(3372-3378)EXCLUSIVE_LOCKS_REQUIRED(370-381)EXCLUSIVE_LOCKS_REQUIRED(620-620)EXCLUSIVE_LOCKS_REQUIRED(627-627)EXCLUSIVE_LOCKS_REQUIRED(633-633)EXCLUSIVE_LOCKS_REQUIRED(667-667)EXCLUSIVE_LOCKS_REQUIRED(672-672)EXCLUSIVE_LOCKS_REQUIRED(676-676)EXCLUSIVE_LOCKS_REQUIRED(682-682)EXCLUSIVE_LOCKS_REQUIRED(687-687)EXCLUSIVE_LOCKS_REQUIRED(695-695)EXCLUSIVE_LOCKS_REQUIRED(698-711)EXCLUSIVE_LOCKS_REQUIRED(1332-1358)EXCLUSIVE_LOCKS_REQUIRED(2887-2901)EXCLUSIVE_LOCKS_REQUIRED(3991-4043)EXCLUSIVE_LOCKS_REQUIRED(4051-4117)src/net_processing.cpp (9)
LOCKS_EXCLUDED(1035-1035)LOCKS_EXCLUDED(1037-1037)EXCLUSIVE_LOCKS_REQUIRED(644-644)LOCK(327-333)LOCK(340-344)LOCK(345-349)WITH_LOCK(335-338)WITH_LOCK(3172-3172)WITH_LOCK(3194-3194)src/sync.cpp (5)
AssertLockNotHeldInternal(291-296)AssertLockNotHeldInternal(291-291)AssertLockNotHeldInternal(297-297)AssertLockNotHeldInternal(298-298)AssertLockNotHeldInternal(299-299)src/httpserver.cpp (2)
LOCK(90-105)LOCK(130-135)src/validationinterface.cpp (1)
WAIT_LOCK(80-80)
src/spork.h (3)
src/net_processing.cpp (9)
ProcessMessage(3617-5411)ProcessMessage(3617-3622)peer(681-681)peer(814-814)peer(919-919)peer(920-920)peer(1027-1027)peer(1035-1035)from(1040-1040)src/spork.cpp (12)
SporkValueIfActive(27-56)SporkValueIfActive(27-27)ProcessMessage(129-137)ProcessMessage(129-129)ProcessSpork(139-196)ProcessSpork(139-139)UpdateSpork(209-240)UpdateSpork(209-209)IsSporkActive(242-261)IsSporkActive(242-242)GetSporkValue(263-289)GetSporkValue(263-263)src/llmq/signing_shares.cpp (2)
ProcessMessage(220-299)ProcessMessage(220-221)
src/evo/mnhftx.h (1)
src/evo/mnhftx.cpp (14)
ProcessBlock(203-253)ProcessBlock(203-203)UndoBlock(255-278)UndoBlock(255-255)GetSignalsStage(60-91)GetSignalsStage(60-60)ForceSignalDBUpdate(390-409)ForceSignalDBUpdate(390-390)AddToCache(355-366)AddToCache(355-355)GetForBlock(280-312)GetForBlock(280-280)GetFromCache(314-353)GetFromCache(314-314)
src/coinjoin/client.h (1)
src/coinjoin/client.cpp (20)
Add(1902-1908)Add(1902-1902)DoMaintenance(1841-1848)DoMaintenance(1841-1841)DoMaintenance(1850-1867)DoMaintenance(1850-1850)DoMaintenance(1910-1916)DoMaintenance(1910-1910)Remove(1918-1921)Remove(1918-1918)Flush(1923-1928)Flush(1923-1923)Get(1930-1935)Get(1930-1930)ProcessMessage(42-143)ProcessMessage(42-43)ProcessMessage(145-167)ProcessMessage(145-145)ProcessMessage(184-231)ProcessMessage(184-184)
src/net.h (1)
src/net.cpp (31)
CopyStats(640-690)CopyStats(640-640)StopNodes(4129-4180)StopNodes(4129-4129)OpenMasternodeConnection(3630-3633)OpenMasternodeConnection(3630-3630)use_v2transport(1990-1990)AddAddrFetch(143-147)AddAddrFetch(143-143)ProcessAddrFetch(2783-2802)ProcessAddrFetch(2783-2783)ThreadOpenConnections(2894-3270)ThreadOpenConnections(2894-2894)ThreadMessageHandler(3637-3683)ThreadMessageHandler(3637-3637)ThreadI2PAcceptIncoming(3685-3727)ThreadI2PAcceptIncoming(3685-3685)AcceptConnection(1857-1883)AcceptConnection(1857-1857)SocketHandler(2368-2417)SocketHandler(2368-2368)SocketHandlerConnected(2419-2554)SocketHandlerConnected(2419-2419)SocketHandlerListening(2556-2567)SocketHandlerListening(2556-2556)ThreadSocketHandler(2626-2646)ThreadSocketHandler(2626-2626)ThreadOpenMasternodeConnections(3374-3484)ThreadOpenMasternodeConnections(3374-3375)PerformReconnections(4840-4867)PerformReconnections(4840-4840)
src/validation.cpp (1)
src/net_processing.cpp (16)
EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(661-661)EXCLUSIVE_LOCKS_REQUIRED(664-664)EXCLUSIVE_LOCKS_REQUIRED(670-670)EXCLUSIVE_LOCKS_REQUIRED(673-673)EXCLUSIVE_LOCKS_REQUIRED(755-755)EXCLUSIVE_LOCKS_REQUIRED(775-775)EXCLUSIVE_LOCKS_REQUIRED(835-835)EXCLUSIVE_LOCKS_REQUIRED(837-837)EXCLUSIVE_LOCKS_REQUIRED(842-842)EXCLUSIVE_LOCKS_REQUIRED(844-844)EXCLUSIVE_LOCKS_REQUIRED(845-845)EXCLUSIVE_LOCKS_REQUIRED(853-853)EXCLUSIVE_LOCKS_REQUIRED(854-854)
src/llmq/dkgsession.h (1)
src/llmq/dkgsession.cpp (28)
ReceiveMessage(280-360)ReceiveMessage(280-280)ReceiveMessage(592-651)ReceiveMessage(592-592)ReceiveMessage(802-892)ReceiveMessage(802-802)ReceiveMessage(1113-1188)ReceiveMessage(1113-1113)qc(513-513)qc(947-947)VerifyPendingContributions(367-419)VerifyPendingContributions(367-367)VerifyAndComplain(421-460)VerifyAndComplain(421-421)VerifyConnectionAndMinProtoVersions(462-505)VerifyConnectionAndMinProtoVersions(462-462)SendComplaint(507-547)SendComplaint(507-507)VerifyAndJustify(653-692)VerifyAndJustify(653-653)SendJustification(694-741)SendJustification(694-695)VerifyAndCommit(894-937)VerifyAndCommit(894-894)SendCommitment(939-1049)SendCommitment(939-939)FinalizeCommitments(1190-1297)FinalizeCommitments(1190-1190)
src/llmq/blockprocessor.h (1)
src/llmq/blockprocessor.cpp (23)
ProcessMessage(75-172)ProcessMessage(75-76)EXCLUSIVE_LOCKS_REQUIRED(262-274)ProcessBlock(174-245)ProcessBlock(174-174)UndoBlock(392-429)UndoBlock(392-392)AddMineableCommitment(691-718)AddMineableCommitment(691-691)HasMineableCommitment(685-689)HasMineableCommitment(685-685)GetMineableCommitmentByHash(720-729)GetMineableCommitmentByHash(720-720)GetMineableCommitments(733-786)GetMineableCommitments(733-733)GetMineableCommitmentsTx(788-808)GetMineableCommitmentsTx(788-788)HasMinedCommitment(511-524)HasMinedCommitment(511-511)ProcessCommitment(276-390)ProcessCommitment(276-277)GetNumCommitmentsRequired(473-493)GetNumCommitmentsRequired(473-473)
src/coinjoin/util.h (2)
src/net_processing.cpp (19)
EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(661-661)EXCLUSIVE_LOCKS_REQUIRED(664-664)EXCLUSIVE_LOCKS_REQUIRED(670-670)EXCLUSIVE_LOCKS_REQUIRED(673-673)EXCLUSIVE_LOCKS_REQUIRED(755-755)EXCLUSIVE_LOCKS_REQUIRED(775-775)EXCLUSIVE_LOCKS_REQUIRED(835-835)EXCLUSIVE_LOCKS_REQUIRED(837-837)EXCLUSIVE_LOCKS_REQUIRED(842-842)EXCLUSIVE_LOCKS_REQUIRED(844-844)EXCLUSIVE_LOCKS_REQUIRED(845-845)EXCLUSIVE_LOCKS_REQUIRED(853-853)EXCLUSIVE_LOCKS_REQUIRED(854-854)LOCK(327-333)LOCK(340-344)LOCK(345-349)src/wallet/interfaces.cpp (6)
LOCK(250-259)LOCK(260-263)LOCK(286-290)LOCK(291-295)LOCK(348-356)LOCK(474-486)
src/llmq/dkgsessionhandler.h (1)
src/llmq/dkgsessionhandler.cpp (24)
PushPendingMessage(61-91)PushPendingMessage(61-61)PopPendingMessages(93-104)PopPendingMessages(93-93)HasSeen(106-110)HasSeen(106-106)Clear(118-124)Clear(118-118)UpdatedBlockTip(128-153)UpdatedBlockTip(128-128)GetPhaseAndQuorumHash(208-212)GetPhaseAndQuorumHash(208-208)WaitForNextPhase(217-257)WaitForNextPhase(217-220)WaitForNewQuorum(259-276)WaitForNewQuorum(259-259)SleepBeforePhase(279-338)SleepBeforePhase(279-282)HandlePhase(340-354)HandlePhase(340-345)HandleDKGRound(530-612)HandleDKGRound(530-530)PhaseHandlerThread(614-628)PhaseHandlerThread(614-614)
src/evo/deterministicmns.h (1)
src/evo/deterministicmns.cpp (25)
s(1461-1461)to_sml(272-286)to_sml(272-272)PoSePunish(302-328)PoSePunish(302-302)DecreaseScores(330-346)DecreaseScores(330-330)PoSeDecrease(348-355)PoSeDecrease(348-348)BuildDiff(357-390)BuildDiff(357-357)ApplyDiff(392-414)ApplyDiff(392-392)AddMN(416-481)AddMN(416-416)UpdateMN(483-558)UpdateMN(483-483)UpdateMN(560-567)UpdateMN(560-560)UpdateMN(569-575)UpdateMN(569-569)RemoveMN(577-635)RemoveMN(577-577)DoMaintenance(1393-1400)DoMaintenance(1393-1393)
src/net_processing.cpp (3)
src/httpserver.cpp (2)
LOCK(90-105)LOCK(130-135)src/validationinterface.cpp (3)
LOCK(47-53)LOCK(55-63)LOCK(69-76)src/random.cpp (2)
LOCK(450-459)LOCK(464-477)
src/instantsend/instantsend.h (2)
src/instantsend/instantsend.cpp (6)
ProcessMessage(104-167)ProcessMessage(104-104)TransactionAddedToMempool(428-461)TransactionAddedToMempool(428-428)BlockConnected(480-508)BlockConnected(480-480)src/dsnotificationinterface.cpp (4)
TransactionAddedToMempool(101-109)TransactionAddedToMempool(101-102)BlockConnected(119-126)BlockConnected(119-119)
src/stats/client.cpp (2)
src/net_processing.cpp (16)
EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(661-661)EXCLUSIVE_LOCKS_REQUIRED(664-664)EXCLUSIVE_LOCKS_REQUIRED(670-670)EXCLUSIVE_LOCKS_REQUIRED(673-673)EXCLUSIVE_LOCKS_REQUIRED(755-755)EXCLUSIVE_LOCKS_REQUIRED(775-775)EXCLUSIVE_LOCKS_REQUIRED(835-835)EXCLUSIVE_LOCKS_REQUIRED(837-837)EXCLUSIVE_LOCKS_REQUIRED(842-842)EXCLUSIVE_LOCKS_REQUIRED(844-844)EXCLUSIVE_LOCKS_REQUIRED(845-845)EXCLUSIVE_LOCKS_REQUIRED(853-853)EXCLUSIVE_LOCKS_REQUIRED(854-854)src/spork.h (1)
cs(230-342)
src/llmq/signing.h (1)
src/llmq/signing.cpp (42)
HasRecoveredSigForId(43-61)HasRecoveredSigForId(43-43)HasRecoveredSigForId(748-751)HasRecoveredSigForId(748-748)HasRecoveredSigForSession(63-79)HasRecoveredSigForSession(63-63)HasRecoveredSigForSession(753-756)HasRecoveredSigForSession(753-753)HasRecoveredSigForHash(81-97)HasRecoveredSigForHash(81-81)GetRecoveredSigByHash(116-125)GetRecoveredSigByHash(116-116)GetRecoveredSigById(127-130)GetRecoveredSigById(127-127)WriteRecoveredSig(132-167)WriteRecoveredSig(132-132)CleanupOldRecoveredSigs(217-266)CleanupOldRecoveredSigs(217-217)RemoveRecoveredSig(169-206)RemoveRecoveredSig(169-169)AlreadyHave(344-357)AlreadyHave(344-344)ProcessMessage(395-435)ProcessMessage(395-395)PushReconstructedRecoveredSig(630-634)PushReconstructedRecoveredSig(630-630)CollectPendingRecoveredSigsToVerify(437-502)CollectPendingRecoveredSigsToVerify(437-440)ProcessPendingReconstructedRecoveredSigs(504-512)ProcessPendingReconstructedRecoveredSigs(504-504)ProcessPendingRecoveredSigs(514-577)ProcessPendingRecoveredSigs(514-514)ProcessRecoveredSig(580-628)ProcessRecoveredSig(580-580)Cleanup(641-654)Cleanup(641-641)RegisterRecoveredSigsListener(656-660)RegisterRecoveredSigsListener(656-656)UnregisterRecoveredSigsListener(662-667)UnregisterRecoveredSigsListener(662-662)WorkThreadMain(814-826)WorkThreadMain(814-814)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (45)
SetVerificationVector(89-99)SetVerificationVector(89-89)SetSecretKeyShare(101-109)SetSecretKeyShare(101-101)HasVerificationVector(139-142)HasVerificationVector(139-139)IsValidMember(118-127)IsValidMember(118-118)GetMemberIndex(155-164)GetMemberIndex(155-155)GetPubKeyShare(129-137)GetPubKeyShare(129-129)GetSkShare(149-153)GetSkShare(149-149)HasVerificationVectorInternal(144-147)HasVerificationVectorInternal(144-144)WriteContributions(166-182)WriteContributions(166-166)ReadContributions(184-208)ReadContributions(184-184)TriggerQuorumDataRecoveryThreads(249-299)TriggerQuorumDataRecoveryThreads(249-249)UpdatedBlockTip(301-324)UpdatedBlockTip(301-301)ProcessMessage(705-888)ProcessMessage(705-705)HasQuorum(473-476)HasQuorum(473-473)GetQuorum(639-660)GetQuorum(639-639)GetQuorum(662-678)GetQuorum(662-662)ScanQuorums(525-529)ScanQuorums(525-525)ScanQuorums(531-637)ScanQuorums(531-531)CheckQuorumConnections(326-394)CheckQuorumConnections(326-327)BuildQuorumFromCommitment(396-442)BuildQuorumFromCommitment(396-396)BuildQuorumContributions(444-471)BuildQuorumContributions(444-444)CQuorum(77-79)MigrateOldQuorumDB(1146-1206)MigrateOldQuorumDB(1146-1146)
🔇 Additional comments (43)
src/governance/governance.cpp (1)
721-721: LGTM! Redundant WITH_LOCK correctly removed.The removal of the
WITH_LOCK(::cs_main, ...)wrapper is correct because::cs_mainis already held viaLOCK2(::cs_main, cs)at line 711. Thetippointer is used only at lines 722-723 while the lock remains held, making this access thread-safe.This change aligns with the PR's objective of strengthening thread-safety assertions. The refactored code likely reflects new
EXCLUSIVE_LOCKS_REQUIRED(::cs_main)annotations onActiveChain().Tip()that make redundant locking wrappers unnecessary and potentially problematic.src/llmq/debug.h (1)
106-117: Approve thread-safety annotations and implementations. Verified each method acquirescs_lockStatusinternally and no call sites invoke them while holding the lock.src/timedata.cpp (1)
19-32: Global mutex migration aligns with the new locking model.The upgrade to
GlobalMutexkeeps the existing locking semantics intact and is consistent with the rest of the PR’s thread-safety hardening.src/util/system.cpp (1)
100-143: Directory lock guard updated correctly.Switching
cs_dir_lockstoGlobalMutexpreserves the existing LOCK usage while conforming to the broader mutex refactor.src/rpc/server.cpp (1)
26-598: RPC globals now use the hardened mutex type.Both warmup and deadline timer guards move to
GlobalMutex, matching the updated locking guidance without altering control flow.src/warnings.cpp (1)
16-61: Warnings cache guard upgraded cleanly.Using
GlobalMutexkeeps the warning state protections intact while aligning with the project-wide locking changes.src/llmq/dkgsessionmgr.h (1)
90-118: Clearer cache locking contracts.Adding
EXCLUSIVE_LOCKS_REQUIRED(!contributionsCacheCs)documents the expectation that callers arrive without holding the cache mutex, preventing accidental self-deadlocks while the methods lock internally.src/versionbits.h (1)
88-103: Statistics now advertises its own locking.The new annotation mirrors the implementation (which locks
m_mutexinternally) and safeguards against recursive locking mistakes.src/coinjoin/util.h (1)
120-125: CountOutputs now self-synchronizes.Acquiring
cs_outputsinside the accessor plus the!cs_outputsrequirement keeps the method safe to call while avoiding re-entrant locking.src/evo/mnhftx.h (1)
113-115: Lock annotations look correct; please confirm AddSignal implementation and call-site compliance
- EXCLUSIVE_LOCKS_REQUIRED(!cs_cache) on ProcessBlock/UndoBlock/GetSignalsStage/AddToCache/GetForBlock/GetFromCache matches .cpp where cs_cache is locked internally. Good.
- ForceSignalDBUpdate annotated with EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !cs_cache) aligns with chain/tip access and internal cache usage.
Please verify:
- AddSignal(const CBlockIndex*, int) is implemented in .cpp with the same not-held cs_cache contract.
- No call sites hold cs_cache when invoking these methods.
Also applies to: 124-124, 127-128, 149-149, 152-152, 160-160, 166-166
src/coinjoin/client.h (1)
254-256: ProcessMessage lock contract tightened appropriatelyRequiring both !cs_vecqueue and !cs_ProcessDSQueue matches the implementation that locks them internally. Good change.
src/evo/deterministicmns.h (2)
165-169: Thread-safety annotations align with implementation and Immer usage
- Adding EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex) across SML cache read/modify paths is consistent with the .cpp locking (LOCK/LOCK2) and avoids double-locking.
- Use of immer::map satisfies the immutable data structure requirement for evo modules.
LGTM.
Also applies to: 194-210, 233-234, 244-245, 376-376, 397-405, 412-423
680-681: Verify lock acquisition order
DoMaintenance() acquires cs_cleanup then cs; confirm there are no code paths acquiring cs before cs_cleanup to prevent deadlocks.src/netbase.cpp (1)
32-32: GlobalMutex migration for proxyinfo is consistentChanging g_proxyinfo_mutex to GlobalMutex aligns with the global-locking policy and existing LOCK() usage.
src/init.cpp (1)
803-803: Global mutex upgrade looks solid.Switching the genesis wait lock to
GlobalMutexpreserves the existing wait/notify pattern and lines up with the new annotation scheme. Looks good.src/rpc/blockchain.cpp (1)
80-80: Annotation-friendly lock swap is correct.Updating
cs_blockchangetoGlobalMutexkeeps the condition-variable waits working while enabling the stronger thread-safety tooling. Approved.src/wallet/wallet.cpp (1)
200-201: LGTM! GlobalMutex transition for wallet synchronization.The replacement of
MutexwithGlobalMutexfor these global wallet synchronization primitives is correct and consistent with the broader PR objectives. This change strengthens thread-safety assertions as part of the upstream backports (bitcoin#21202, bitcoin#24931). All existing usages of these mutexes remain compatible.src/validation.h (1)
114-114: LGTM! Consistent GlobalMutex adoption.The change from
MutextoGlobalMutexforg_best_block_mutexis correct and consistent with the repository-wide adoption ofGlobalMutexfor global synchronization primitives. This strengthens thread-safety analysis as part of the upstream backports.src/net.cpp (1)
131-133: Global mutex conversion looks good.Switching
g_maplocalhost_mutextoGlobalMutexlines up with the rest of the GlobalMutex rollout and leaves the call sites unchanged. Looks good to me.src/httpserver.cpp (1)
162-164: HTTP path handler lock update LGTM.The
g_httppathhandlers_mutexswitch toGlobalMutexkeeps the existing locking intact and is consistent with the broader synchronization refactor.src/qt/clientmodel.h (1)
126-126: Nice negative lock annotation.The
EXCLUSIVE_LOCKS_REQUIRED(!m_cached_tip_mutex)addition makes the contract explicit and mirrors the internalWITH_LOCKusage—good improvement.src/net.h (4)
202-202: LGTM: GlobalMutex type correctly applied.The change from
MutextoGlobalMutexforg_maplocalhost_mutexis appropriate for a global-scope mutex and aligns with the backported thread-safety improvements from Bitcoin Core.
1030-1030: LGTM: CopyStats lock requirements correctly specified.The annotation properly reflects that
CopyStatsmust not be called while holding any of the locks it acquires internally (m_subver_mutex,m_addr_local_mutex,cs_vSend,cs_vRecv,cs_mnauth).
1044-1082: LGTM: Masternode authentication method annotations are correct.All the getter and setter methods for masternode authentication (
GetSentMNAuthChallenge,GetReceivedMNAuthChallenge,GetVerifiedProRegTxHash,GetVerifiedPubKeyHash,SetSentMNAuthChallenge,SetReceivedMNAuthChallenge,SetVerifiedProRegTxHash,SetVerifiedPubKeyHash) correctly specifyEXCLUSIVE_LOCKS_REQUIRED(!cs_mnauth)since they acquirecs_mnauthinternally.
1247-1248: LGTM: cs_mapSocketToNode annotations correctly added.The addition of
cs_mapSocketToNodetoEXCLUSIVE_LOCKS_REQUIREDannotations across thread/connection management methods is correct. These functions acquirecs_mapSocketToNodeinternally (confirmed byrelevant_code_snippetsshowingLOCK(cs_mapSocketToNode)orWITH_LOCK(cs_mapSocketToNode, ...)in implementations), so they must not be called while holding this lock to prevent deadlocks.Also applies to: 1277-1279, 1480-1480, 1582-1591, 1605-1605, 1623-1623, 1637-1640, 1644-1644, 1937-1937
src/validation.cpp (3)
128-128: Global mutex annotation looks goodUpgrading
g_best_block_mutextoGlobalMutextightens thread-safety analysis without changing behavior—nice improvement.
164-168: Lock contract reinforcement approvedAdding
EXCLUSIVE_LOCKS_REQUIRED(cs_main)(with the defaultedpvChecks) matches how we already guard the script cache and will keep clang TSA happy.
1801-1804: Definition stays consistent with the new declarationThe implementation mirrors the locked declaration and keeps the optional
pvCheckshandling unchanged. 👍src/llmq/dkgsession.h (1)
360-384: LGTM! Thread-safety annotations correctly enforce lock contracts.The added
EXCLUSIVE_LOCKS_REQUIREDannotations with negative capabilities properly document that callers must not holdinvCsorcs_pendingwhen calling these methods, preventing recursive locking. This aligns with the implementations which acquire these locks internally.src/sync.h (3)
147-156: LGTM! GlobalMutex properly distinguishes global-scope mutexes.The new
GlobalMutexclass derived fromMutexcorrectly marks mutexes with global scope, allowing thread-safety analysis to apply different negative-capability checks (LOCKS_EXCLUDED vs EXCLUSIVE_LOCKS_REQUIRED).
323-340: LGTM! MaybeCheckNotHeld overloads correctly enforce lock semantics.The overloads distinguish between:
Mutex: UsesEXCLUSIVE_LOCKS_REQUIRED(!cs)to prevent holding the same non-recursive lockGlobalMutex,RecursiveMutex,SharedMutex: UseLOCKS_EXCLUDED(cs)for weaker surrounding-scope checksThis properly supports both strict and relaxed lock checking based on mutex type.
342-349: LGTM! Locking macros correctly integrate MaybeCheckNotHeld.All locking macros now pass mutexes through
MaybeCheckNotHeld(cs)before lock acquisition, ensuring compile-time enforcement of the negative lock requirements based on mutex type.Also applies to: 388-389
doc/developer-notes.md (2)
950-963: LGTM! Thread-safety guidance properly expanded.The updated guidance correctly emphasizes:
- Declaration-only annotations to avoid false positives
- Preference for private/internal locks
- Combining annotations with runtime asserts
965-1020: LGTM! Code examples accurately reflect the new API patterns.The examples correctly demonstrate:
CChainStatewithm_chainstate_mutex(replacingChainstateManager)- Proper use of
EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex)andLOCKS_EXCLUDED(::cs_main)- Runtime asserts (
AssertLockNotHeld) in implementationssrc/llmq/blockprocessor.h (1)
61-80: LGTM! Lock annotations correctly enforce thread-safety contracts.The added
EXCLUSIVE_LOCKS_REQUIREDannotations properly document:
- Methods requiring
::cs_mainfor chain state access- Negative requirement
!minableCommitmentsCsto prevent recursive lockingThis matches the implementation patterns where methods acquire
minableCommitmentsCsinternally and some assert::cs_mainis held.Also applies to: 93-96
src/llmq/dkgsessionhandler.h (1)
79-97: LGTM! Thread-safety annotations correctly enforce lock contracts.The added
EXCLUSIVE_LOCKS_REQUIREDannotations with negative capabilities properly document that:
- CDKGPendingMessages methods must not be called while holding
cs_messages- CDKGSessionHandler phase methods must not be called while holding
cs_phase_qhashThis prevents recursive locking as implementations acquire these locks internally.
Also applies to: 170-204
src/llmq/quorums.h (1)
200-208: CQuorum lock annotations match implementation (locks inside; require “not held”).SetVerificationVector/SetSecretKeyShare/HasVerificationVector/GetPubKeyShare/GetSkShare/Read|WriteContributions all lock cs_vvec_shShare internally; EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare) is correct. Matches quorums.cpp.
Also applies to: 209-216, 219-221
src/llmq/signing.h (2)
129-133: CRecoveredSigsDb: annotations align with internal locking.HasRecoveredSigFor{Id,Session,Hash}, WriteRecoveredSig, TruncateRecoveredSig, CleanupOldRecoveredSigs, and RemoveRecoveredSig all lock cs_cache inside; requiring “not held” is correct and matches signing.cpp.
Also applies to: 135-139, 149-151
186-191: CSigningManager: good use of “not held” contracts.AlreadyHave, ProcessMessage, PushReconstructedRecoveredSig, Collect/Process pending methods, ProcessRecoveredSig, Register/Unregister acquire locks inside; the EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners) contracts are appropriate.
Also applies to: 194-196, 204-212, 214-216, 222-224
src/net_processing.cpp (4)
615-615: NewPoWValidBlock lock annotation is correctRequiring !m_most_recent_block_mutex aligns with implementation (cs_main then m_most_recent_block_mutex). LGTM.
647-647: _RelayTransaction annotation matches call sites and lock orderRequires cs_main and !m_peer_mutex; function locks m_peer_mutex inside; callers wrap with WITH_LOCK(cs_main). LGTM.
855-856: ProcessGetBlockData: !m_most_recent_block_mutex is appropriateFunction locks m_most_recent_block_mutex internally; annotation prevents inversion. LGTM.
1037-1037: ProcessGetData: tight lock contract is correctRequires peer.m_getdata_requests_mutex and excludes cs_main; also requires !m_most_recent_block_mutex. Matches both call sites and internal usage. LGTM.
| bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, | ||
| const CCoinsViewCache& inputs, unsigned int flags, bool cacheSigStore, | ||
| bool cacheFullScriptStore, PrecomputedTransactionData& txdata, | ||
| std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); |
There was a problem hiding this comment.
Hold cs_main everywhere we call the helper.
By adding EXCLUSIVE_LOCKS_REQUIRED(cs_main) here (and on ValidateCheckInputsForAllFlags), every caller must already have cs_main locked. The invocations in the P2SH / CHECKLOCKTIMEVERIFY / CHECKSEQUENCEVERIFY test blocks further down still call ValidateCheckInputsForAllFlags without wrapping them in LOCK(cs_main), so clang’s thread-safety analysis will complain (and fail the build under -Werror). Please guard those call sites with LOCK(cs_main) (or otherwise ensure the mutex is held) before this lands.
🤖 Prompt for AI Agents
In src/test/txvalidationcache_tests.cpp around lines 20-23, the declaration of
CheckInputScripts (and ValidateCheckInputsForAllFlags) now requires
EXCLUSIVE_LOCKS_REQUIRED(cs_main), so any call sites must hold cs_main; locate
the P2SH / CHECKLOCKTIMEVERIFY / CHECKSEQUENCEVERIFY test blocks further down
that call ValidateCheckInputsForAllFlags (and any other helper calls) and wrap
each call in a LOCK(cs_main) { ... } scope (or, if the test already holds the
lock logically, insert AssertLockHeld(cs_main) before the call) so that cs_main
is actually held at the call sites and clang thread-safety warnings are
resolved.
|
needs rebase and probably 586ebde |
…edValues to cs_cache
Motivation: they are captured almost always together. Get one mutex is faster than get 2,
there are no performance benefits
Example of usage:
WITH_LOCK(cs_mapSporksCachedActive, mapSporksCachedActive.erase(spork.nSporkID));
WITH_LOCK(cs_mapSporksCachedValues, mapSporksCachedValues.erase(spork.nSporkID));
…rovements 25c57d6 [doc] Add a note about where lock annotations should go. (Amiti Uttarwar) ad5f01b [validation] Move the lock annotation from function definition to declaration (Amiti Uttarwar) Pull request description: Based on reviewing bitcoin#21188 the first commit switches the lock annotations on `CheckInputScripts` to be on the function declaration instead of on the function definition. this ensures that all call sites are checked, not just ones that come after the definition. the second commit adds a note to the developer-notes section to clarify where the annotations should be applied. ACKs for top commit: MarcoFalke: ACK 25c57d6 🥘 promag: Code review ACK 25c57d6. Tree-SHA512: 61b6ef856bf6c6016d535fbdd19daf57b9e59fe54a1f30d47282a071b9b9d60b2466b044ee57929e0320cb1bdef52e7a1687cacaa27031bbc43d058ffffe22ba
ce893c0 doc: Update developer notes (Anthony Towns) d285291 sync.h: Imply negative assertions when calling LOCK (Anthony Towns) bba87c0 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns) a559509 sync.h: Add GlobalMutex type (Anthony Towns) be6aa72 qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns) f24bd45 net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns) Pull request description: This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions. This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`). ACKs for top commit: MarcoFalke: review ACK ce893c0 🐦 hebasto: ACK ce893c0 Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
This PR is going to have a logic conflict and require a rebase every 2nd merged PR...
done |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/net_processing.cpp (1)
605-643: Interface: stronger lock annotations align with usage; minor typo
- Annotations for BlockConnected/Disconnected/UpdatedBlockTip/BlockChecked/NewPoWValidBlock/Initialize/Finalize/ProcessMessages/SendMessages/ProcessMessage match asserts and internal locking. Good tightening.
- Line 631 has a double semicolon.
Apply to fix the typo:
- void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);; + void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);src/test/txvalidationcache_tests.cpp (2)
236-251: GuardValidateCheckInputsForAllFlagswithcs_main.
ValidateCheckInputsForAllFlagsnow requiresEXCLUSIVE_LOCKS_REQUIRED(cs_main), but in this scope we never acquirecs_mainbefore calling it. Clang’s thread-safety analysis (and runtime expectations) will flag this. Please wrap the call (and any associated setup usingCoinsTip()) in aLOCK(cs_main)block socs_mainis held for the duration.
273-308: Holdcs_mainbefore calling tSAN-annotated helpers.
ValidateCheckInputsForAllFlagsandCheckInputScriptsnow both demandEXCLUSIVE_LOCKS_REQUIRED(cs_main). In these CHECKLOCKTIMEVERIFY / CHECKSEQUENCEVERIFY sections we invoke them without first takingcs_main, so the build will fail under clang’s thread-safety analysis. Wrap each block’s logic (including the view access) in aLOCK(cs_main)scope—same fix is needed for the P2SH block above.src/spork.h (1)
229-233: Include<limits>forstd::numeric_limits.
std::numeric_limitsis used to initializenMinSporkKeys, but this header doesn’t include<limits>. The new code won’t compile on a clean translation unit. Please add#include <limits>(or move the initializer to a.cppthat already includes it).
🧹 Nitpick comments (10)
src/llmq/quorums.h (1)
247-249: New cs_scan_quorums: define lock orderAdding a separate cs_scan_quorums is fine, but many methods now exclude cs_db, cs_map_quorums, and cs_scan_quorums. Please document a canonical lock order among these three (and cs_cleanup) to prevent future LOR issues. The TODO to merge mutexes is good; until then, a brief comment on order helps.
src/instantsend/instantsend.h (1)
121-123: Expanded lock preconditions; document lock orderAdding cs_timingsTxSeen to AddNonLockedTx/TransactionAddedToMempool/BlockConnected is correct given timingsTxSeen access. With multiple locks involved (!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_timingsTxSeen), please document a canonical acquisition order for these four to prevent lock-order inversions across methods.
Also applies to: 145-147, 148-153
src/evo/mnhftx.h (1)
113-115: cs_cache contract and cs_main ordering made explicit
- Not-held cs_cache preconditions for ProcessBlock/UndoBlock/GetSignalsStage/AddSignal/AddToCache/GetForBlock/GetFromCache look correct for internal locking.
- ForceSignalDBUpdate requiring ::cs_main (and not cs_cache) clarifies order; please document expected lock order (cs_main before cs_cache) near cs_cache to guide future changes.
Also applies to: 124-124, 127-128, 132-132, 149-150, 152-153, 160-161, 166-167
src/llmq/signing.h (2)
186-186: cs_pending/cs_listeners contracts: encode lock order to prevent ABBA deadlocksThe “not-held” annotations are appropriate. To harden ordering:
- Consider declaring cs_pending ACQUIRED_BEFORE(cs_listeners) to encode acquisition order used inside these methods.
Example (outside this hunk):
// within CSigningManager: mutable Mutex cs_pending ACQUIRED_BEFORE(cs_listeners); mutable Mutex cs_listeners;Please confirm implementations lock in the same order (cs_pending first, then cs_listeners) where both are acquired.
Also applies to: 189-191, 194-196, 204-212, 214-216, 221-224
239-241: Remove stray semicolonThere is an extra standalone “;” after the method declaration.
- void WorkThreadMain(PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners); - ; + void WorkThreadMain(PeerManager& peerman) EXCLUSIVE_LOCKS_REQUIRED(!cs_pending, !cs_listeners);src/net.h (2)
1031-1031: CopyStats lock acquisition: keep a consistent orderGood “not-held” contract. Since this acquires multiple locks internally, ensure a single, consistent lock order is used inside (e.g., m_subver_mutex → m_addr_local_mutex → cs_vSend → cs_vRecv → cs_mnauth) to avoid deadlocks.
Optionally document the intended order in the implementation or encode partial ordering with ACQUIRED_BEFORE/AFTER on members where feasible.
1630-1631: Remove duplicate lock in annotation listcs_sendable_receivable_nodes appears twice in EXCLUSIVE_LOCKS_REQUIRED; remove the duplicate for clarity.
- void SocketHandlerConnected(const Sock::EventsPerSock& events_per_sock) - EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode, !cs_sendable_receivable_nodes); + void SocketHandlerConnected(const Sock::EventsPerSock& events_per_sock) + EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc, !cs_sendable_receivable_nodes, !cs_mapSocketToNode);src/validation.cpp (1)
164-169: Unify CheckInputScripts signature and annotation; avoid default arg in .cpp forward decl
- Keep the default argument only in the public header, not in a local forward declaration. Having it here can diverge from the header and confuse callers in other TUs.
- Also add the EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation to the definition to help the analyzer on this TU as well.
Suggested changes:
- Remove "= nullptr" from the forward declaration at Lines 164-169.
- Add the EXCLUSIVE_LOCKS_REQUIRED(cs_main) annotation on the definition at Lines 1800-1804.
Also applies to: 1800-1804
src/masternode/meta.h (1)
73-80: Serialize/Unserialize should not stream std::atomic directly; use temporaries and load/storeDirectly serializing std::atomic relies on non‑portable overloads. Safer to use temporaries and atomic load/store under the class lock.
Apply this diff in Serialize/Unserialize bodies:
- s << proTxHash << nLastDsq << nMixingTxCount << mapGovernanceObjectsVotedOn << outboundAttemptCount - << lastOutboundAttempt << lastOutboundSuccess << m_platform_ban << m_platform_ban_updated; + auto last_dsq = nLastDsq.load(); + auto mixing_cnt = nMixingTxCount.load(); + auto out_attempts = outboundAttemptCount.load(); + auto last_out_attempt = lastOutboundAttempt.load(); + auto last_out_success = lastOutboundSuccess.load(); + s << proTxHash << last_dsq << mixing_cnt << mapGovernanceObjectsVotedOn << out_attempts + << last_out_attempt << last_out_success << m_platform_ban << m_platform_ban_updated;- s >> proTxHash >> nLastDsq >> nMixingTxCount >> mapGovernanceObjectsVotedOn >> outboundAttemptCount >> - lastOutboundAttempt >> lastOutboundSuccess >> m_platform_ban >> m_platform_ban_updated; + int64_t last_dsq{}; + int mixing_cnt{}; + int out_attempts{}; + int64_t last_out_attempt{}; + int64_t last_out_success{}; + s >> proTxHash >> last_dsq >> mixing_cnt >> mapGovernanceObjectsVotedOn >> out_attempts + >> last_out_attempt >> last_out_success >> m_platform_ban >> m_platform_ban_updated; + nLastDsq.store(last_dsq); + nMixingTxCount.store(mixing_cnt); + outboundAttemptCount.store(out_attempts); + lastOutboundAttempt.store(last_out_attempt); + lastOutboundSuccess.store(last_out_success);Also applies to: 81-87
src/spork.cpp (1)
233-235: Minor: scope the cs_cache lock for clarityLock is correct; consider bracing to mirror ProcessSpork style and keep scope explicit.
- LOCK(cs_cache); - mapSporksCachedActive.erase(spork.nSporkID); - mapSporksCachedValues.erase(spork.nSporkID); + { + LOCK(cs_cache); + mapSporksCachedActive.erase(spork.nSporkID); + mapSporksCachedValues.erase(spork.nSporkID); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
doc/developer-notes.md(2 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/util.h(1 hunks)src/evo/creditpool.h(1 hunks)src/evo/deterministicmns.h(7 hunks)src/evo/mnhftx.h(3 hunks)src/governance/governance.cpp(1 hunks)src/httpserver.cpp(1 hunks)src/init.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/blockprocessor.h(2 hunks)src/llmq/debug.h(1 hunks)src/llmq/dkgsession.h(1 hunks)src/llmq/dkgsessionhandler.h(4 hunks)src/llmq/dkgsessionmgr.h(3 hunks)src/llmq/quorums.h(4 hunks)src/llmq/signing.h(5 hunks)src/llmq/signing_shares.h(2 hunks)src/masternode/meta.cpp(3 hunks)src/masternode/meta.h(3 hunks)src/net.cpp(1 hunks)src/net.h(10 hunks)src/net_processing.cpp(5 hunks)src/netbase.cpp(1 hunks)src/qt/clientmodel.h(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/server.cpp(1 hunks)src/spork.cpp(6 hunks)src/spork.h(5 hunks)src/stats/client.cpp(1 hunks)src/sync.h(4 hunks)src/test/txvalidationcache_tests.cpp(1 hunks)src/timedata.cpp(1 hunks)src/util/system.cpp(1 hunks)src/validation.cpp(3 hunks)src/validation.h(1 hunks)src/versionbits.h(1 hunks)src/wallet/wallet.cpp(1 hunks)src/warnings.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/net.hsrc/spork.cppsrc/coinjoin/util.hsrc/netbase.cppsrc/llmq/signing_shares.hsrc/util/system.cppsrc/instantsend/instantsend.hsrc/net_processing.cppsrc/timedata.cppsrc/versionbits.hsrc/llmq/quorums.hsrc/validation.hsrc/warnings.cppsrc/llmq/dkgsessionmgr.hsrc/httpserver.cppsrc/rpc/server.cppsrc/evo/deterministicmns.hsrc/llmq/dkgsessionhandler.hsrc/governance/governance.cppsrc/evo/creditpool.hsrc/llmq/dkgsession.hsrc/rpc/blockchain.cppsrc/spork.hsrc/evo/mnhftx.hsrc/net.cppsrc/stats/client.cppsrc/sync.hsrc/masternode/meta.hsrc/qt/clientmodel.hsrc/llmq/blockprocessor.hsrc/wallet/wallet.cppsrc/llmq/signing.hsrc/masternode/meta.cppsrc/init.cppsrc/coinjoin/client.hsrc/validation.cppsrc/test/txvalidationcache_tests.cppsrc/llmq/debug.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.hsrc/evo/creditpool.hsrc/evo/mnhftx.hsrc/masternode/meta.hsrc/masternode/meta.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/txvalidationcache_tests.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/developer-notes.md
🧠 Learnings (6)
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/net.hsrc/stats/client.cpp
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/net.h
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
PR: dashpay/dash#6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks.
Applied to files:
src/net.h
🔇 Additional comments (53)
src/versionbits.h (1)
88-88: LGTM! Thread-safety annotation completed.The addition of
EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)correctly documents that callers must not holdm_mutexwhen invokingStatistics, consistent with the other public methods in this class (lines 93, 96, 100, 102). This completes the thread-safety annotations for all public non-static methods.src/llmq/signing_shares.h (2)
436-437: LGTM! Thread safety annotation correctly applied.The
EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns)annotation properly documents thatcs_pendingSignsmust not be held when callingAsyncSign, preventing recursive locking issues and enabling compile-time deadlock detection.
489-490: LGTM! Thread safety annotations correctly applied.Both
SignPendingSigSharesandWorkThreadMainare correctly annotated withEXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns), ensuring these methods can safely acquirecs_pendingSignsinternally without risking deadlock from caller-held locks. This is consistent with the annotation onAsyncSign(line 437) and aligns with the PR's objective to strengthen thread-safety assertions across the codebase.src/coinjoin/util.h (1)
120-124: LGTM! Thread-safety annotation correctly implemented.The inline definition properly acquires
cs_outputsbefore accessing the guardedvecOutputsmember. TheEXCLUSIVE_LOCKS_REQUIRED(!cs_outputs)annotation correctly documents that callers must not hold the lock, allowing the method to acquire it internally. This change aligns with the PR's goal of strengthening thread-safety assertions across the codebase.src/llmq/debug.h (1)
106-117: Annotation update looks goodThe new
EXCLUSIVE_LOCKS_REQUIRED(!cs_lockStatus)preconditions align with the internal locking done in these helpers and give the analyzer the right signal that callers must arrive lock-free.src/coinjoin/client.h (4)
98-104: LGTM! Thread safety annotations correctly document locking contracts.The
EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map)annotations correctly indicate these methods must be called without holding the lock (they acquire it internally). This prevents deadlock and makes lock ordering explicit at compile-time.
107-113: LGTM! Implementation correctly matches annotation.The method acquires
cs_wallet_manager_mapon line 109, which correctly matches theEXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map)annotation requiring the caller not hold the lock.
116-120: LGTM! Implementation correctly matches annotation.The method acquires
cs_wallet_manager_mapon line 118, correctly matching the annotation.
254-256: Verifycs_vecqueueis defined in base class.The annotation references
cs_vecqueue, which is not defined in this class. SinceCCoinJoinClientQueueManagerinherits fromCCoinJoinBaseManager(line 233), verify thatcs_vecqueueis defined in the base class.src/evo/deterministicmns.h (1)
165-165: LGTM! Thread-safety annotations are correctly implemented.The negative lock assertions (
EXCLUSIVE_LOCKS_REQUIRED(!m_cached_sml_mutex)) correctly indicate that the mutex must NOT be held when entering these methods, as they acquire it internally. This prevents double-locking issues.The implementation follows good thread-safety practices:
- Uses immutable data structures (immer::map) for concurrent access, as required by coding guidelines
- Protects the mutable cache (
m_cached_sml) with a dedicated mutex- Consistently applies annotations to all methods that invalidate the cache
The assignment operator at line 196 correctly requires both mutexes to be unheld, then acquires them with LOCK2 after copying the immutable maps, which is safe since immer::map reads don't require locking.
Based on coding guidelines.
Also applies to: 196-196, 233-233, 244-244, 376-376, 397-397, 399-399, 404-404, 412-413, 415-422, 680-680
src/llmq/quorums.h (7)
200-208: Good: not-held preconditions for vvec/skshare settersThe EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare) matches the internal LOCK pattern. Please ensure the out-of-line implementations also acquire cs_vvec_shShare internally and are only called from unlocked contexts.
209-216: Accessors annotated with not-held preconditionsLooks correct for functions that lock internally. Confirm all call sites do not pre-hold cs_vvec_shShare to avoid analyzer complaints.
219-221: I/O helpers: consistent not-held contractWriteContributions/ReadContributions using EXCLUSIVE_LOCKS_REQUIRED(!cs_vvec_shShare) is consistent. Keep the internal LOCK in their definitions.
271-276: Updated tip/data recovery: stricter preconditionsThe “not-held” preconditions for cs_db/cs_scan_quorums/cs_map_quorums are appropriate for functions that lock internally. Verify call sites comply (Clang TSA should catch mismatches).
277-280: ProcessMessage lock exclusionsExcluding cs_map_quorums and cs_db here is sensible. Ensure the implementation never assumes either is already held.
286-296: GetQuorum/ScanQuorums: clear lock contractsThe preconditions align with “short cs_main” comments. Confirm implementations acquire the respective mutexes in a consistent order.
299-305: Private helpers: internal locking contractThe “not-held” requirements look correct for helpers that manage their own locking. All good.
Also applies to: 307-309, 319-320
src/wallet/wallet.cpp (1)
200-201: Switch to GlobalMutex for wallet globalsGood migration. Please double-check that WAIT_LOCK(g_wallet_release_mutex, lock) works with std::condition_variable::wait(lock) across GCC/Clang (GlobalMutex should alias std::mutex so the type matches).
src/rpc/server.cpp (1)
26-26: GlobalMutex adoption for RPC globalsLooks correct and consistent with WITH_LOCK/LOCK usage below (e.g., StopRPC, RPCRunLater). No issues spotted.
Also applies to: 33-34
src/evo/creditpool.h (1)
135-144: Credit pool cache: not-held locking contractRequiring !cache_mutex on GetCreditPool/GetFromCache/AddToCache/ConstructCreditPool is appropriate if each function locks internally. Ensure all definitions use LOCK(cache_mutex) and that callers never pre-hold cache_mutex.
src/llmq/dkgsessionmgr.h (1)
90-92: Contributions cache: explicit not-held preconditionsThe additions align with guarding contributionsCache via contributionsCacheCs. Verify implementations acquire/release the lock internally and that any callers do not hold it to avoid TSA violations.
Also applies to: 104-109, 117-118
src/llmq/signing.h (1)
130-133: cs_cache “not-held” lock contracts look correct; please verify implementations lock internallyGood additions. Since caches are GUARDED_BY(cs_cache), the “not-held” contracts make sense if implementations acquire cs_cache themselves.
- Ensure each annotated method locks cs_cache internally (LOCK/WITH_LOCK) and does not assume it is held by callers.
- This avoids nested/recursive locking and makes call sites simpler.
Run to spot-check implementations and locking:
Also applies to: 135-138, 149-151
src/llmq/blockprocessor.h (1)
61-63: Lock contracts are consistent; encode and verify cs_main → minableCommitmentsCs orderingThe added requirements look right: hold ::cs_main, do not hold minableCommitmentsCs, then lock minableCommitmentsCs inside. To prevent ordering bugs:
- Consider annotating minableCommitmentsCs with ACQUIRED_AFTER(::cs_main) or adding a brief comment documenting the intended order.
Example (outside this hunk):
mutable Mutex minableCommitmentsCs /* ACQUIRED_AFTER(::cs_main) */;Check for call sites that might pre-lock minableCommitmentsCs before calling these:
Also applies to: 64-68, 70-81, 93-97
src/net.h (3)
202-204: GlobalMutex for localhost map: ensure definition matches declarationThe switch to GlobalMutex is aligned with thread-safety analysis. Please verify the net.cpp definition also uses GlobalMutex to avoid ODR/type mismatches.
1045-1082: cs_mnauth accessors: contract looks right; check call-sitesThe “not-held” requirement matches the internal LOCK(cs_mnauth) usage. Please ensure no call site invokes these while holding cs_mnauth; thread-safety analysis should flag any offenders.
1248-1252: Broader “not-held” annotations look appropriate; avoid holding these locks at call sitesThese functions now declare “not-held” requirements for various mutexes. This clarifies ownership and helps static analysis. Ensure all callers comply; prefer WITH_LOCK/LOCK inside these methods.
Also applies to: 1278-1281, 1480-1481, 1606-1606, 1624-1624, 1637-1638, 1640-1641, 1938-1938
src/net_processing.cpp (6)
327-349: TxRelay accessors: negative lock requirement is correctUsing EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) and locking internally prevents accidental re-entrancy. Callers must not pre-hold m_tx_relay_mutex. LGTM.
647-648: _RelayTransaction(): lock contract matches call sitesRequiring cs_main and forbidding holding m_peer_mutex is right; function acquires m_peer_mutex internally. Calls (RelayTransaction, ReattemptInitialBroadcast, ProcessOrphanTx, PostProcessMessage) already hold cs_main. LGTM.
855-856: ProcessGetBlockData: avoid holding m_most_recent_block_mutexThe EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex) aligns with internal LOCK(m_most_recent_block_mutex) usage. Call paths via ProcessGetData respect this. LGTM.
945-947: AlreadyHave: cs_main required, m_recent_confirmed_transactions_mutex excludedThis reflects internal locking (briefly locks m_recent_confirmed_transactions_mutex while cs_main is held). Callers already hold cs_main. LGTM.
1516-1536: Object timing helpers: lock annotations are appropriateGet/UpdateObjectRequestTime and CalculateObjectGetDataTime correctly require cs_main, matching their use in RequestObject. LGTM.
Also applies to: 1566-1584
1037-1038: ProcessGetData: requires getdata queue lock and excludes cs_mainContract is correct: caller must hold peer.m_getdata_requests_mutex; function asserts cs_main is not held and later acquires it where needed. Verified two call sites hold the lock. LGTM.
To ensure there are no other call sites violating the contract, run:
src/llmq/dkgsessionhandler.h (2)
79-85: Pending-messages queue: negative lock requirements are correctRequiring !cs_messages for Push/Pop/HasSeen/Clear and templated variants matches internal locking and avoids re-entrancy. LGTM.
Also applies to: 86-93, 95-118
170-170: DKG phase/state methods: lock contracts clarifiedEXCLUSIVE_LOCKS_REQUIRED(!cs_phase_qhash) on UpdatedBlockTip, GetPhaseAndQuorumHash, and phase wait/handle routines is consistent with internal locking and intended call patterns. LGTM.
Also applies to: 184-205
src/net.cpp (1)
131-132: LGTM! Type change enhances thread-safety analysis.The change from
MutextoGlobalMutexforg_maplocalhost_mutexis correct. All usages throughout the file (lines 184, 238, 299, 318, 326, 337, 4454) use the standardLOCK()macro, which is compatible with both types. TheGUARDED_BYannotation correctly references the mutex.Assuming the
GlobalMutextype is properly defined insrc/sync.h(as indicated in the PR summary), this change aligns with the broader thread-safety annotation improvements being backported.src/init.cpp (1)
803-812: Missing INVOKE LOCK to match GlobalMutex semantics.
GlobalMutexenforces process-wide semantics and must be paired withAssertLockHeld(g_genesis_wait_mutex);orLOCK/WAIT_LOCKguard that accepts the new type. All existing call sites already useLOCK/WAIT_LOCK, so this replacement is safe. ✅src/httpserver.cpp (1)
150-164: Confirm GlobalMutex compatibility with WAIT_LOCK usage.
g_httppathhandlers_mutexis locked viaLOCK/WAIT_LOCK, so the upgrade toGlobalMutexkeeps annotations accurate. ✅src/validation.h (1)
113-118: Good update aligning mutex type.Replacing
MutexwithGlobalMutexkeeps the external declaration in sync with implementation changes to enforce stronger thread-safety analysis. ✅src/validation.cpp (1)
128-129: GlobalMutex with std::condition_variable: confirm compatibility of waitersg_best_block_mutex switched to GlobalMutex while g_best_block_cv remains std::condition_variable. Please verify that all wait sites use std::unique_lockstd::mutex (or condition_variable_any if not), and that GlobalMutex aliases std::mutex here; otherwise wait/notify could be ill-formed.
src/masternode/meta.h (1)
89-96: Lock annotations match internal locking; LGTMAnnotations with EXCLUSIVE_LOCKS_REQUIRED(!cs) and internal LOCK(cs) are consistent for ToJson/GetProTxHash/Governance/PlatformBan.
Also applies to: 102-106, 112-129
src/llmq/dkgsession.h (1)
360-362: Verify that implementations actually take the annotated locks and callers don’t hold themMethods are now annotated with EXCLUSIVE_LOCKS_REQUIRED(!invCs/!cs_pending), implying they acquire these locks internally and must not be called while held. Please confirm:
- Implementations do LOCK(invCs)/LOCK(cs_pending) accordingly.
- Call sites (e.g., in CDKGSessionHandler) don’t hold these locks on entry, to avoid deadlocks and satisfy the analyzer.
Also applies to: 364-366, 369-370, 372-373, 375-376, 381-382, 384-385
src/masternode/meta.cpp (7)
69-81: GetMetaInfo: LGTMInternal locking and conditional creation are correct.
118-125: RemoveGovernanceObject: LGTMManager-level lock then per-entry call is fine; watch lock order (manager.cs -> meta.cs) across codebase.
Please verify there are no call paths that lock meta.cs first and then manager.cs to avoid lock-order inversions.
126-131: GetAndClearDirtyGovernanceObjectHashes: LGTMWITH_LOCK swap is safe and efficient.
133-137: AlreadyHavePlatformBan: LGTMProper locking around LRU cache access.
139-148: GetPlatformBan: LGTMReturns optional safely under lock.
150-154: RememberPlatformBan: LGTMInsert under lock; OK.
156-161: MasternodeMetaStore::ToString: LGTMRead under lock; atomics are fine.
src/stats/client.cpp (1)
51-107: LGTM! Thread-safety annotations correctly applied.The addition of
EXCLUSIVE_LOCKS_REQUIRED(!cs)to all public methods and the_sendtemplate is correct. These annotations ensure that callers must not holdcswhen invoking these methods, which is necessary because the methods internally acquirecsviaWITH_LOCK(line 244) to access theinsecure_randPRNG. The reformatting from single-line to block-scoped implementations is purely stylistic to accommodate the annotations on separate lines.The pattern correctly prevents deadlocks by enforcing that the lock cannot already be held when these methods attempt to acquire it.
src/spork.cpp (3)
187-191: Good: clear caches atomically under cs_cache on new sporkErasing cached active/value entries under cs_cache avoids stale reads after ProcessSpork updates.
34-38: Correct: cache access under cs_cache with clear lock orderUsing cs (AssertLockHeld) then cs_cache is sane and consistent. This reduces races on mapSporksCachedValues.
Also applies to: 48-50
246-250: Good: unified cs_cache path in IsSporkActive; avoids holding it across GetSporkValueFast-path true cache read/write under cs_cache, then compute under cs via GetSporkValue. Lock ordering looks safe.
Also applies to: 257-259
src/qt/clientmodel.h (1)
126-126: Good: enforce “not-held” contract for m_cached_tip_mutexAnnotation matches existing getBestBlockHash pattern and prevents re-entrant locking. Please verify all call sites comply (no callers holding m_cached_tip_mutex).
| template <typename Stream> | ||
| void Serialize(Stream& s) const EXCLUSIVE_LOCKS_REQUIRED(!cs) | ||
| { | ||
| LOCK(obj.cs); | ||
| READWRITE(obj.proTxHash, obj.nLastDsq, obj.nMixingTxCount, obj.mapGovernanceObjectsVotedOn, | ||
| obj.outboundAttemptCount, obj.lastOutboundAttempt, obj.lastOutboundSuccess, obj.m_platform_ban, | ||
| obj.m_platform_ban_updated); | ||
| LOCK(cs); | ||
| s << proTxHash << nLastDsq << nMixingTxCount << mapGovernanceObjectsVotedOn << outboundAttemptCount | ||
| << lastOutboundAttempt << lastOutboundSuccess << m_platform_ban << m_platform_ban_updated; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Data race in copy constructor: lock cs when copying cs-guarded fields
CMasternodeMetaInfo’s copy ctor (Lines 61-70) copies fields guarded by cs (proTxHash, mapGovernanceObjectsVotedOn, platform flags) without locking. This can race with concurrent updates.
Minimal fix (outside this hunk, shown as code snippet):
CMasternodeMetaInfo::CMasternodeMetaInfo(const CMasternodeMetaInfo& ref) {
LOCK(ref.cs);
proTxHash = ref.proTxHash;
nLastDsq.store(ref.nLastDsq.load());
nMixingTxCount.store(ref.nMixingTxCount.load());
mapGovernanceObjectsVotedOn = ref.mapGovernanceObjectsVotedOn;
lastOutboundAttempt.store(ref.lastOutboundAttempt.load());
lastOutboundSuccess.store(ref.lastOutboundSuccess.load());
m_platform_ban = ref.m_platform_ban;
m_platform_ban_updated = ref.m_platform_ban_updated;
outboundAttemptCount.store(ref.outboundAttemptCount.load());
}🤖 Prompt for AI Agents
In src/masternode/meta.h around lines 73-80 (and the copy ctor at ~61-70), the
copy constructor copies cs-guarded members without locking which can cause
races; fix by acquiring LOCK(ref.cs) at the start of the CMasternodeMetaInfo
copy ctor, then copy proTxHash, mapGovernanceObjectsVotedOn, m_platform_ban, and
m_platform_ban_updated under that lock, and copy atomic/atomic-like members
using their .load() into local values (or using .store(...) on the destination)
while still under the lock to ensure a consistent snapshot; do this change in
the .cpp where the copy ctor is implemented, not in the Serialize() shown.
2 participants
What was done?
This PR includes 2 backports related to clang lock annotations and strenthen of thread safety assertions and several fixes for dash specific code to make code compileable with these improvements.
How Has This Been Tested?
Build locally with thread safety analysis:
CC=clang CXX=clang++ ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-werror --enable-debug --enable-stacktraces --enable-crash-hooks --enable-maintainer-mode --enable-sanitizer=fuzzer,address,undefined,integer
Breaking Changes
N/A
Checklist: