threads: qa: Add lock order annotation for TxMempool::cs #34813

1. 
   davidgumberg commented at 2:02 am on March 12, 2026: contributor

   I added this to see if static analysis would catch any lock order inversions while investigating #34731, it did not catch any but it is a worthwhile defense against statically-analyzable inversions of cs_main->TxMempool::cs.
2. qa: Add lock order annotation for TxMempool::cs 9085dee476
3. DrahtBot added the label Tests on Mar 12, 2026
4. 
   DrahtBot commented at 2:03 am on March 12, 2026: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sedited, hebasto, w0xlt

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

5. davidgumberg renamed this:
   qa: Add lock order annotation for `TxMempool::cs`
   thread: qa: Add lock order annotation for `TxMempool::cs`
   on Mar 12, 2026
6. davidgumberg renamed this:
   thread: qa: Add lock order annotation for `TxMempool::cs`
   threads: qa: Add lock order annotation for `TxMempool::cs`
   on Mar 12, 2026
7. 
   sedited commented at 7:47 am on March 12, 2026: contributor

   Concept ACK

   I was surprised initially that triggering this warning seems to still require -Wthread-safety-beta. Should we have been setting that flag at least in the CI somewhere? Looks like it is moved out of beta in the upcoming 23 release: https://github.com/llvm/llvm-project/pull/152853.

8. 
   fanquake commented at 9:54 am on March 12, 2026: member

   Looks like it is moved out of beta in the upcoming 23 release: https://github.com/llvm/llvm-project/pull/152853.

   That change was part of LLVM 22, which is currently being used in the CI (#34660).

9. 
   hebasto commented at 10:00 am on March 12, 2026: member

   Concept ACK.
10. 
    sedited commented at 11:11 am on March 12, 2026: contributor

    That change was part of LLVM 22, which is currently being used in the CI (https://github.com/bitcoin/bitcoin/pull/34660).

    Oh, should have checked the release notes properly, it is right there: https://releases.llvm.org/22.1.0/tools/clang/docs/ReleaseNotes.html#improvements-to-clang-s-diagnostics . Checked it manually in the meantime.

    tested ACK 9085dee476d21daab786e88848597c755beecee4

11. DrahtBot requested review from hebasto on Mar 12, 2026
12. hebasto approved
13. 
    hebasto commented at 11:29 am on March 12, 2026: member

    ACK 9085dee476d21daab786e88848597c755beecee4.

    Tested on Ubuntu 25.10 using Clang 22.1.1 by applying the following patch:

     0--- a/src/txmempool.h
     1+++ b/src/txmempool.h
     2@@ -257,7 +257,7 @@ public:
     3      * changing the chain tip. It's necessary to keep both mutexes locked until
     4      * the mempool is consistent with the new chain tip and fully populated.
     5      */
     6-    mutable RecursiveMutex cs ACQUIRED_AFTER(::cs_main);
     7+    mutable RecursiveMutex cs ACQUIRED_BEFORE(::cs_main);
     8     std::unique_ptr<TxGraph> m_txgraph GUARDED_BY(cs);
     9     mutable std::unique_ptr<TxGraph::BlockBuilder> m_builder GUARDED_BY(cs);
    10     indexed_transaction_set mapTx GUARDED_BY(cs);
    

    When using Clang 17.0.6, the annotation is ignored, as expected.

14. 
    w0xlt commented at 6:34 pm on March 12, 2026: contributor

    ACK 9085dee476d21daab786e88848597c755beecee4

    Built with cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DAPPEND_CXXFLAGS="-Wthread-safety-beta" (Clang 18.1.3).

    Clean build, and verified the annotation catches wrong lock order when introducing a deliberate violation.

15. fanquake merged this on Mar 13, 2026
16. fanquake closed this on Mar 13, 2026

    ---

Contributors
davidgumberg DrahtBot sedited fanquake hebasto w0xlt

Labels
Tests