p2p: Replace RecursiveMutex `m_tx_inventory_mutex` with Mutex and rename it #24125

pull w0xlt wants to merge 6 commits into bitcoin:master from w0xlt:cs_tx_inventory_mutex changing 1 files +214 −158

w0xlt commented at 1:28 AM on January 22, 2022: contributor

This PR is related to #19303 and gets rid of the RecursiveMutex m_tx_inventory_mutex and also adds AssertLockNotHeld macros combined with LOCKS_EXCLUDED thread safety annotations to avoid recursive locking.
w0xlt marked this as a draft on Jan 22, 2022
DrahtBot added the label P2P on Jan 22, 2022
w0xlt force-pushed on Jan 22, 2022
w0xlt force-pushed on Jan 22, 2022
DrahtBot commented at 8:17 AM on January 22, 2022: 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

Approach ACK shaavan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26593 (tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C)
- #26569 (p2p: Ensure transaction announcements are only queued for fully connected peers by dergoegge)
- #26283 (p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs)
- #26140 (refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer by dergoegge)
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.
w0xlt force-pushed on Jan 22, 2022
w0xlt marked this as ready for review on Jan 22, 2022

Type	Reviewers
Concept ACK	hebasto
Approach ACK	shaavan

in src/net_processing.cpp:3848 in d097e5dc33 outdated

3844 | @@ -3844,6 +3845,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3845 |      }
3846 |  
3847 |      if (msg_type == NetMsgType::MEMPOOL) {
3848 | +

shaavan commented at 10:57 AM on January 22, 2022:

nit:

w0xlt commented at 1:33 PM on January 22, 2022:

Done. Thanks.

shaavan commented at 10:58 AM on January 22, 2022: contributor
Approach ACK d097e5dc3356c1e2d9d4030af14ccc0ac058d6e0
- I agree with the name change of cs_tx_inventory -> m_tx_inventory_mutex.
- Checked that all the locking instances of m_tx_inventory_mutex are protected by a preceding AssertLockNotHeld() statement.
- LOCKS_EXCLUDED macro is appropriately used with all the function definitions where AssertLockNotHeld() is used.
There is a minor nit that you should address before merging this PR. :clinking_glasses:
w0xlt force-pushed on Jan 22, 2022
hebasto commented at 5:12 PM on January 24, 2022: member

Concept ACK.

But reviewing changes in PeerManagerImpl::SendMessages() is not trivial because of a huge critical section size. Also there are concerns about external synchronizing (similar to #24122#pullrequestreview-861076744).
w0xlt commented at 1:49 PM on January 25, 2022: contributor

To address the external synchronizing, the code snippets below should be encapsulated as CNode::TxRelay methods?

https://github.com/bitcoin/bitcoin/blob/bd482b3ffebc68130f8a18dabf08ed1aff7ea159/src/net_processing.cpp#L2038-L2041

https://github.com/bitcoin/bitcoin/blob/bd482b3ffebc68130f8a18dabf08ed1aff7ea159/src/net_processing.cpp#L1904-L1910

https://github.com/bitcoin/bitcoin/blob/bd482b3ffebc68130f8a18dabf08ed1aff7ea159/src/net_processing.cpp#L4807-L4935
hebasto commented at 1:53 PM on January 25, 2022: member

To address the external synchronizing, the code snippets below should be encapsulated as CNode::TxRelay methods?

That is my guess. Of course, it would be nice to hear other devs opinion.
w0xlt force-pushed on Feb 12, 2022
w0xlt force-pushed on Feb 12, 2022
w0xlt commented at 2:32 AM on February 12, 2022: contributor

The new commit addresses most of #24125 (comment).

All code snippets protected by cs_tx_inventory\m_tx_inventory_mutex (except for the one in PeerManagerImpl::SendMessages(CNode* pto)) were encapsulated in struct CNode::TxRelay.

Annotations that use negative capabilities were also added.

It doesn't seem trivial to bring the code in PeerManagerImpl::SendMessages(CNode* pto) into net.cpp, as it uses a significant amount of net_processing objects and references. I will try to find a way to do this.
DrahtBot added the label Needs rebase on Mar 25, 2022
hebasto commented at 8:28 PM on May 16, 2022: member

@w0xlt

Still working on this?
w0xlt commented at 1:33 PM on June 24, 2022: contributor

It doesn't seem trivial to bring the code in PeerManagerImpl::SendMessages(CNode* pto) into net.cpp, as it uses a significant amount of net_processing objects and references. I will try to find a way to do this. @hebasto I hadn't seen the message before. I will try to solve the issue described in the comment above.
net: add `AddKnownTx()` method to `struct TxRelay` 12e7276353
net: add `RelayTransaction()` method to `struct TxRelay` 6b2b9fc4dc
net: add `VerifyUnconfirmedParent()` method to `struct TxRelay` da7b59a599
net: add `SetSendMempool()` method to `struct TxRelay` 7a44768cf2
w0xlt force-pushed on Jun 24, 2022
DrahtBot removed the label Needs rebase on Jun 24, 2022
net: add `Relay()` method to `struct TxRelay` 88777c4748
net: replace `RecursiveMutex m_tx_inventory_mutex` with `Mutex` 60519a8797
w0xlt commented at 8:10 PM on June 24, 2022: contributor

It doesn't seem trivial to bring the code in PeerManagerImpl::SendMessages(CNode* pto) into net.cpp, as it uses a significant amount of net_processing objects and references. I will try to find a way to do this. @hebasto 88777c4 does the trick, apparently with no change in behavior.. However, I'm not sure it's an elegant solution due to the amount of parameters. I'm pushing it anyway so that reviewers can eventually suggest some improvement.

This commit also makes it easier to change RecursiveMutex m_bloom_filter_mutex to Mutex
achow101 commented at 6:57 PM on October 12, 2022: member

Are you still working on this?
hebasto commented at 7:20 PM on October 17, 2022: member

After 1066d10f71e6800c78012d789ff6ae19df0243fe, the PR title and description should be updated, no?
w0xlt renamed this:
~~p2p: Replace RecursiveMutex `cs_tx_inventory` with Mutex and rename it~~
p2p: Replace RecursiveMutex `m_tx_inventory_mutex` with Mutex and rename it
on Oct 17, 2022
w0xlt commented at 8:14 PM on October 17, 2022: contributor

Are you still working on this?

The PR is ready for reviews.

PR title and description should be updated

Done.
in src/net_processing.cpp:337 in 60519a8797
```
 332 | +            AssertLockNotHeld(m_tx_inventory_mutex);
 333 | +            LOCK(m_tx_inventory_mutex);
 334 | +            m_send_mempool = value;
 335 | +        }
 336 | +
 337 | +        void Relay(
```
maflcko commented at 6:35 AM on October 18, 2022:

I haven't reviewed this pull request, but this would be the first time that a net processing logic is put in the peers struct, instead of the net processing implementation.

w0xlt commented at 2:20 PM on October 18, 2022:

The idea is to prevent external synchronizing (acquiring m_tx_inventory_mutex from outside of the CNode::TxRelay instance). Perhaps this requires a more complex refactoring.

fanquake commented at 3:59 PM on February 6, 2023:

@dergoegge you might have thoughts here?
DrahtBot commented at 4:29 PM on December 2, 2022: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot added the label Needs rebase on Dec 2, 2022
achow101 added the label Up for grabs on Apr 25, 2023
achow101 closed this on Apr 25, 2023
bitcoin locked this on Apr 24, 2024

Contributors

Labels

P2P Up for grabs Needs rebase

Linked (view graph)

#19303 Replace all of the RecursiveMutex instances with the Mutex ones

p2p: Replace RecursiveMutex `m_tx_inventory_mutex` with Mutex and rename it #24125

Reviews

Conflicts