net: refactor: replace Peer::TxRelay RecursiveMutex instances with Mutex #34824

1. 
   w0xlt commented at 8:07 am on March 14, 2026: contributor

   Part of #19303.

   Replace m_bloom_filter_mutex and m_tx_inventory_mutex in Peer::TxRelay from RecursiveMutex to Mutex. Neither mutex is acquired recursively anywhere in the codebase - all 18 lock sites are first-time acquisitions within their call contexts.

   To support the Mutex conversion with explicit thread-safety contracts, this PR adds annotated TxRelay helper methods and LOCK_RETURNED accessors for the remaining net_processing lock sites, routes SendMessages() txrelay mutations through those helpers, makes the mutex members private, and records the stable m_tx_inventory_mutex -> m_bloom_filter_mutex lock order with ACQUIRED_BEFORE / ACQUIRED_AFTER.

   In SendMessages(), both mutexes are still used together: m_tx_inventory_mutex is held as the outer lock while m_bloom_filter_mutex is locked separately for three short sections. These are two different mutexes, not recursive acquisition of the same one.

2. DrahtBot added the label P2P on Mar 14, 2026
3. 
   DrahtBot commented at 8:08 am on March 14, 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
   Concept ACK	hebasto, theuni
   Approach ACK	sedited

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34628 (p2p: Replace per-peer transaction rate-limiting with global rate limits by ajtowns)

   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.

4. 
   sedited commented at 9:09 am on March 14, 2026: contributor

   Concept ACK

   I looked this over and also didn’t find any cases where locking recursion would be required, but the change seems brittle to me. Not sure if just changing these is the best way to handle this.

5. w0xlt force-pushed on Mar 15, 2026
6. 
   w0xlt commented at 7:39 pm on March 15, 2026: contributor

   Thanks @sedited for the feedback.

   The original PR intentionally included only the first commit to avoid non-trivial refactoring and focus on the RecursiveMutex conversion. After your concerns that this minimal approach might be brittle, I took a second pass.

   A broader refactor seems justified: both TxRelay and net_processing benefit from clearer separation of concerns and improved modularization. The main changes are:

   • TxRelay is now a dedicated component in src/node/txrelay.h, instead of being embedded in src/net_processing.cpp.

   • SendMessages() and related call sites now use intent-named operations (ConsumeSendMempool, SetNextInvSendTime, TxInventoryKnownInsert, etc.) rather than mutating fields directly, which reduces local complexity.

   • Locking contracts are now explicit at API boundaries (EXCLUSIVE_LOCKS_REQUIRED, LOCK_RETURNED), improving static thread-safety checking and reviewability.

   • State transitions/invariants are centralized in TxRelay methods (PushInventory, VerifyInventoryPristine, ClearTxInventoryToSendIfNoRelayTxs), reducing the risk of missed field updates.

   • Lock-order reasoning is clearer because lock acquisition sites are now structured and annotated rather than scattered raw member-lock uses.

   • Future changes to TxRelay internals (containers/fields) should require fewer net-processing edits, since call sites depend more on behavior methods than storage details.

7. 
   hebasto commented at 4:12 pm on March 16, 2026: member

   Replace m_bloom_filter_mutex and m_tx_inventory_mutex in Peer::TxRelay from RecursiveMutex to Mutex.

   Concept ACK.

8. 
   theuni commented at 10:58 pm on March 18, 2026: member

   Concept ACK.

   Lock-order reasoning is clearer because lock acquisition sites are now structured and annotated rather than scattered raw member-lock uses.

   See also the ACQUIRED_BEFORE/ACQUIRED_AFTER annotations which may be useful when there’s an established ordering that should be followed.

   (Note that unless -Wthread-safety-beta is used, clang 22 is required to actually see the warnings: https://github.com/llvm/llvm-project/pull/152853)

9. 
   fanquake commented at 6:26 am on March 19, 2026: member

   (Note that unless -Wthread-safety-beta is used, clang 22 is required to actually see the warnings: https://github.com/llvm/llvm-project/pull/152853)

   (also note that we currently use Clang 22.1.2 in the CI)

10. in src/node/txrelay.h:27 in 8ed9eb0b24

    22+/**
    23+ * Per-peer state for transaction relay. Manages bloom filter and
    24+ * transaction inventory data, each protected by its own mutex.
    25+ */
    26+struct TxRelay {
    27+    mutable Mutex m_bloom_filter_mutex;
    

    ---

    ---

    

    sedited commented at 12:11 pm on March 20, 2026:

    Can we make these private now? m_tx_inventory_mutex is still annotated outside this class, but I think that annotation can be dropped now?

    ---

    

    w0xlt commented at 9:41 pm on March 24, 2026:

    Done. Thanks.
11. 
    sedited commented at 12:58 pm on March 20, 2026: contributor

    Approach ACK

    I think I prefer this over a bare bones mutex type change. The changes in the first four commits are pretty straight forward and make sense to me. The last commit is a bit more involved, but git color coding the moved bits does help a bit. I’d be curious to see what others think.

12. net: replace Peer::TxRelay m_bloom_filter_mutex and m_tx_inventory_mutex RecursiveMutex with Mutex

    Neither mutex is acquired recursively anywhere in the codebase. All lock
    acquisition sites are first-time acquisitions within their respective
    call contexts.
    
    m_bloom_filter_mutex sites (11 total):
    - GetNodeStateStats(): WITH_LOCK, standalone
    - ProcessGetBlockData(): LOCK, standalone
    - ProcessMessage() handlers (VERSION, WTXIDRELAY, SENDTXRCNCL,
      FILTERLOAD, FILTERADD, FILTERCLEAR): LOCK/WITH_LOCK, standalone
    - SendMessages(): LOCK, nested inside m_tx_inventory_mutex (cross-mutex
      ordering, not recursive)
    
    m_tx_inventory_mutex sites (7 total):
    - AddKnownTx(): LOCK, standalone (callers verified: ProcessInvalidTx,
      ProcessMessage INV/TX handlers — none hold m_tx_inventory_mutex)
    - GetNodeStateStats(): LOCK, standalone
    - InitiateTxBroadcastToAll(): LOCK, standalone
    - FindTxForGetData(): WITH_LOCK, standalone (already annotated with
      EXCLUSIVE_LOCKS_REQUIRED(!tx_relay.m_tx_inventory_mutex))
    - ProcessMessage() handlers (VERACK, MEMPOOL): WITH_LOCK/LOCK,
      standalone
    - SendMessages(): LOCK, outermost of the tx relay section
    
    Since neither mutex is locked recursively, the RecursiveMutex can be
    safely replaced with Mutex.

    fb9d02ccfa
13. refactor: extract Peer::TxRelay to node::TxRelay

    Move the TxRelay struct from a nested definition inside Peer
    (net_processing.cpp) to its own header (src/node/txrelay.h) as
    node::TxRelay.
    
    This is a pure move with no behavioral changes. Peer::TxRelay becomes
    a using alias for node::TxRelay. All data members, mutexes, and
    GUARDED_BY annotations are preserved exactly as they were.
    
    Extracting TxRelay into its own class enables adding annotated
    methods in a follow-up, which was not appropriate when it was a nested
    data struct inside Peer.

    7ccd50834a
14. net: add thread-safe accessors to node::TxRelay

    Add annotated methods to node::TxRelay for lock-acquire-access-release
    patterns, providing compile-time thread safety guarantees via
    EXCLUSIVE_LOCKS_REQUIRED(!mutex) annotations.
    
    m_bloom_filter_mutex:
    - GetRelayTxs(): replaces 3 WITH_LOCK read sites
    - SetRelayTxs(): replaces VERSION handler lock site
    - SetBloomFilter(): replaces FILTERLOAD handler lock site
    - AddToBloomFilter(): replaces FILTERADD handler lock site
    - ClearBloomFilter(): replaces FILTERCLEAR handler lock site
    
    m_tx_inventory_mutex:
    - AddKnownTx(): replaces standalone AddKnownTx() lock site
    - GetLastInvSequence(): replaces FindTxForGetData() WITH_LOCK site
    - SetSendMempool(): replaces MEMPOOL handler lock site
    - GetInventoryStats(): replaces GetNodeStateStats() lock site
    - PushInventory(): replaces InitiateTxBroadcastToAll() lock site
    - VerifyInventoryPristine(): replaces VERACK handler Assume check
    
    The remaining direct lock sites are in SendMessages() (complex
    cross-mutex block) and ProcessGetBlockData() (would require pulling
    block-level dependencies into txrelay.h).

    65a3226a50
15. net: annotate remaining TxRelay lock acquisitions

    Replace the remaining direct TxRelay mutex lock sites in net_processing with
    annotated interfaces, without moving policy or protocol logic out of
    PeerManagerImpl.
    
    - Add TxRelay::GetBloomFilterMutex() and GetTxInventoryMutex() with
      LOCK_RETURNED annotations for SendMessages lock sites.
    
    - Add TxRelay::WithBloomFilterIfSet() for the ProcessGetBlockData
      merkleblock path, so bloom filter access is encapsulated and annotated.
    
    This removes raw LOCK(tx_relay->m_*_mutex) usage from net_processing while
    preserving existing lock ordering and behavior.

    7f0e079745
16. net: route SendMessages txrelay mutations through TxRelay helpers

    This refactor removes direct member access to TxRelay internals from SendMessages() and uses lock-annotated helper methods instead.
    
    Why:
    
    - Keeps net_processing focused on relay policy/flow, while TxRelay owns its guarded state transitions.
    
    - Makes lock requirements explicit at the helper API boundary, so call sites are easier to audit during the recursive-mutex removal work.
    
    - Avoids introducing new layering concerns: behavior and lock scope stay in the same places, but state mutation/checks are centralized in TxRelay.
    
    What changed:
    
    - Added TxRelay helpers for inv timing, mempool-request consume, inventory erase/contains/insert, bloom relevance checks, sequence updates, and feefilter get/set.
    
    - Updated SendMessages() to use those helpers under the existing mutex lock order.
    
    - Updated remaining feefilter reads/writes in net_processing.cpp to use TxRelay accessors.
    
    No functional behavior change intended.

    475366b55a
17. w0xlt force-pushed on Mar 24, 2026
18. 
    w0xlt commented at 9:42 pm on March 24, 2026: contributor

    ACQUIRED_BEFORE/ACQUIRED_AFTER annotations added. Tested with clang 18 and -Wthread-safety-beta.

Contributors
w0xlt DrahtBot sedited hebasto theuni fanquake

Labels
P2P