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
  1. 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.

  2. w0xlt marked this as a draft on Jan 22, 2022
  3. DrahtBot added the label P2P on Jan 22, 2022
  4. w0xlt force-pushed on Jan 22, 2022
  5. w0xlt force-pushed on Jan 22, 2022
  6. DrahtBot commented at 8:17 AM on January 22, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  7. w0xlt force-pushed on Jan 22, 2022
  8. w0xlt marked this as ready for review on Jan 22, 2022
  9. 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.

  10. 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:

  11. w0xlt force-pushed on Jan 22, 2022
  12. DrahtBot cross-referenced this on Jan 22, 2022 from issue refactor: Release cs_main before MaybeSendFeefilter by maflcko
  13. DrahtBot cross-referenced this on Jan 22, 2022 from issue net_processing: lock clean up by ajtowns
  14. DrahtBot cross-referenced this on Jan 23, 2022 from issue refactor: replace RecursiveMutex `cs_vProcessMsg` with Mutex (and rename) by theStack
  15. hebasto cross-referenced this on Jan 24, 2022 from issue Replace all of the RecursiveMutex instances with the Mutex ones by hebasto
  16. 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).

  18. 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.

  19. DrahtBot cross-referenced this on Feb 4, 2022 from issue net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery
  20. DrahtBot cross-referenced this on Feb 4, 2022 from issue net/net processing: Move tx inventory into net_processing by jnewbery
  21. w0xlt force-pushed on Feb 12, 2022
  22. w0xlt force-pushed on Feb 12, 2022
  23. 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.

  24. DrahtBot cross-referenced this on Mar 4, 2022 from issue Additional thread safety annotations for CNode/Peer members accessed via the message processing thread by ajtowns
  25. DrahtBot added the label Needs rebase on Mar 25, 2022
  26. hebasto commented at 8:28 PM on May 16, 2022: member

    @w0xlt

    Still working on this?

  27. 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.

  28. net: add `AddKnownTx()` method to `struct TxRelay` 12e7276353
  29. net: add `RelayTransaction()` method to `struct TxRelay` 6b2b9fc4dc
  30. net: add `VerifyUnconfirmedParent()` method to `struct TxRelay` da7b59a599
  31. net: add `SetSendMempool()` method to `struct TxRelay` 7a44768cf2
  32. w0xlt force-pushed on Jun 24, 2022
  33. DrahtBot removed the label Needs rebase on Jun 24, 2022
  34. net: add `Relay()` method to `struct TxRelay` 88777c4748
  35. net: replace `RecursiveMutex m_tx_inventory_mutex` with `Mutex` 60519a8797
  36. 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

  37. DrahtBot cross-referenced this on Sep 21, 2022 from issue refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer by dergoegge
  38. DrahtBot cross-referenced this on Oct 4, 2022 from issue refactor: Make m_mempool optional in PeerManager by maflcko
  39. DrahtBot cross-referenced this on Oct 8, 2022 from issue p2p: Fill reconciliation sets and request reconciliation (Erlay) by naumenkogs
  40. achow101 commented at 6:57 PM on October 12, 2022: member

    Are you still working on this?

  41. hebasto commented at 7:20 PM on October 17, 2022: member

    After 1066d10f71e6800c78012d789ff6ae19df0243fe, the PR title and description should be updated, no?

  42. 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
  43. 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.

  44. 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?

  45. DrahtBot cross-referenced this on Nov 16, 2022 from issue Erlay: bandwidth-efficient transaction relay protocol by naumenkogs
  46. DrahtBot cross-referenced this on Nov 25, 2022 from issue p2p: Ensure transaction announcements are only queued for fully connected peers by dergoegge
  47. DrahtBot cross-referenced this on Nov 29, 2022 from issue tracing: Only prepare tracepoint arguments when actually tracing by 0xB10C
  48. DrahtBot commented at 4:29 PM on December 2, 2022: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

  49. DrahtBot added the label Needs rebase on Dec 2, 2022
  50. achow101 added the label Up for grabs on Apr 25, 2023
  51. achow101 closed this on Apr 25, 2023
  52. bitcoin locked this on Apr 24, 2024
Contributors
w0xltDrahtBotshaavanhebastoachow101maflckofanquake
Labels
P2PUp for grabsNeeds rebase
Linked (view graph)
#19303 Replace all of the RecursiveMutex instances with the Mutex ones
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC