#34824 net: encapsulate TxRelay state and replace recursive mutexes
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #34824 net: encapsulate TxRelay state and replace recursive mutexes | Ready | 0 (+2) | Strong | 396 + 407 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
This PR extracts per-peer transaction relay state from `net_processing.cpp` into a dedicated `node::TxRelay` type, making all member state private. Callers in `SendMessages()` now operate on a move-only `TxInventoryBatch` snapshot instead of locking and mutating `TxRelay` internals directly. With lock boundaries strictly encapsulated, both `RecursiveMutex` members are replaced with plain `Mutex` and annotated with Clang thread-safety annotations, accompanied by new unit and fuzz tests.
Problem: Previously, `Peer::TxRelay` exposed its fields and `RecursiveMutex` instances publicly in `net_processing.cpp`, making lock ordering fragile and preventing safe replacement of recursive mutexes without risking deadlocks or subtle concurrency regressions.
Reviewability
Ready
Nothing known that would invalidate a review now.
Agreement
Strong
substantive support, no open objection (sedited, sedited, sedited, pablomartin4btc, pablomartin4btc, pablomartin4btc)
Thread notes: First record for this PR. Initial concerns by sedited regarding brittleness and by maflcko regarding TSA circumvention through exposed mutex getters and callbacks under lock were both resolved after the author completely encapsulated TxRelay state and introduced TxInventoryBatch.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited (member) | objection, safety | replacing recursive mutexes with non-recursive mutexes without encapsulating state or clarifying lock boundaries is brittle | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2026-03-14: 'the change seems brittle to me. Not sure if just changing these is the best way to handle this.' Status from: 2026-03-20: sedited: 'I think I prefer this over a bare bones mutex type change.' |
| sedited (member) | objection, correctness | clang-tidy compilation diagnostic failure due to missing std::chrono_literals | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2026-05-03: 'I'm getting a clang tidy warning here (and for the other one): ... no matching literal operator' Status from: 2026-05-17: sedited: 're-ACK 67527c70975e1c3bb4918ba0ebeb4313d8c741be' |
| maflcko (member) | objection, safety | exposing mutex getters while converting to non-recursive mutexes undermines Clang thread safety analysis and risks deadlocks or unsafe code | fixed · fix pushed · v1, not yet re-checked | changed | yes | 2026-06-12: 'Just changing a RecursiveMutex to a Mutex without proper TSA doesn't make sense, imo. This just risks unsafe code down the line in the future.' Status from: 2026-06-20: w0xlt: 'The current version no longer has the public mutex getter or the callback-under-lock pattern.' |
| maflcko (member) | objection, safety | executing an arbitrary callable while holding a mutex cannot be verified by Clang TSA and may cause recursion or undefined behavior | fixed · fix pushed · v1, not yet re-checked | changed | yes | 2026-06-12: 'Is this pattern even safe? Does clang TSA detect if callable were to lock the mutex and thus lead to UB?' Status from: 2026-06-13: w0xlt: 'Done. Removed this callback pattern. The filtered-block path now uses `TxRelay::MakeMerkleBlock()` instead.' |
Support:
- sedited (member) (Concept ACK, 2026-03-14): didn't find any cases where locking recursion would be required, but the change seems brittle to me.
- hebasto (member) (Concept ACK, 2026-03-16): Concept ACK. [not substantive]
- theuni (member) (Concept ACK, 2026-03-18): Concept ACK. [not substantive]
- sedited (member) (Approach ACK, 2026-03-20): 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.
- sedited (member) (ACK, 2026-05-03): I also like the additional assurances and encapsulation.
- sedited (member) (ACK, 2026-05-17): re-ACK 67527c70975e1c3bb4918ba0ebeb4313d8c741be [not substantive]
- pablomartin4btc (member) (Concept ACK, 2026-06-07): The first commits look straightforward (as said earlier), and commit 5 seems to preserve the existing lock order while moving the remaining TxRelay state access in SendMessages() behind annotated helpers.
- pablomartin4btc (member) (ACK, 2026-06-12): Checked both m_bloom_filter_mutex and m_tx_inventory_mutex in Peer::TxRelay aren't acquired recursively, makes sense to move them into non-recursive mutexes
- pablomartin4btc (member) (ACK, 2026-07-01): WithBloomFilterIfSet() and the public mutex getter were removed - addressing maflcko's concerns that exposing the mutex reference undoes the TSA safety guarantees.
Participants:
- sedited (member, objection, 9 statements 2026-03-14 to 2026-06-29): raised concern that switching mutexes without encapsulation was brittle, then supported and ACKed subsequent refactoring iterations
- hebasto (member, support, 1 statements 2026-03-16 to 2026-03-16): Concept ACK
- theuni (member, support, 1 statements 2026-03-18 to 2026-03-18): Concept ACK and recommended ACQUIRED_BEFORE/ACQUIRED_AFTER lock annotations
- fanquake (member, neutral, 1 statements 2026-03-19 to 2026-03-19): noted current Clang version used in CI
- pablomartin4btc (member, support, 3 statements 2026-06-07 to 2026-07-01): Concept ACK and subsequent ACKs across rebases and encapsulation updates
- maflcko (member, objection, 5 statements 2026-06-12 to 2026-06-19): objected that exposing mutex getters and callbacks under lock defeated Clang thread-safety analysis
Dependencies
Enables:
Files
407 lines under test/bench/ci.
- src/test/txrelay_tests.cpp +310/-0
- src/net_processing.cpp +114/-190
- src/node/txrelay.h +280/-0
- src/test/fuzz/txrelay.cpp +95/-0
- src/net_processing.h +2/-2
- src/test/CMakeLists.txt +1/-0
- src/test/fuzz/CMakeLists.txt +1/-0
Card
The short text the ranking pass compares across PRs.
This PR encapsulates per-peer transaction relay state into a dedicated node::TxRelay class and converts its RecursiveMutex members to plain Mutexes. It replaces direct internal field manipulation in net_processing with private state transitions and an inventory batch snapshotting mechanism. The change addresses issue #19303 by removing recursive locking while introducing Clang thread safety annotations, unit tests, and a fuzz harness. Reviewers strongly support the architecture, with earlier thread-safety concerns raised by maflcko addressed in subsequent revisions.
Data
thread read through 2026-09-15 (35 statements), head 4ac05f01c1
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: P2P
Categories
P2P
P3 · cleanup
Encapsulates peer transaction relay state and replaces recursive mutexes with standard mutexes and thread-safety annotations. This resolves fragile lock ordering in net_processing, though it is primarily an internal refactoring.
Membership: Matches the P2P label and touches src/net_processing.cpp and src/node/txrelay.h to manage transaction relay state.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 1