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

    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.

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

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

  15. w0xlt force-pushed on Feb 12, 2022
  16. w0xlt force-pushed on Feb 12, 2022
  17. 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.

  18. DrahtBot added the label Needs rebase on Mar 25, 2022
  19. hebasto commented at 8:28 pm on May 16, 2022: member

    @w0xlt

    Still working on this?

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

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

  30. achow101 commented at 6:57 pm on October 12, 2022: member
    Are you still working on this?
  31. hebasto commented at 7:20 pm on October 17, 2022: member
    After 1066d10f71e6800c78012d789ff6ae19df0243fe, the PR title and description should be updated, no?
  32. 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
  33. 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.

  34. 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?
  35. DrahtBot commented at 4:29 pm on December 2, 2022: contributor

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

  36. DrahtBot added the label Needs rebase on Dec 2, 2022
  37. achow101 added the label Up for grabs on Apr 25, 2023
  38. achow101 closed this on Apr 25, 2023
  39. bitcoin locked this on Apr 24, 2024


w0xlt DrahtBot shaavan hebasto achow101 maflcko fanquake

Labels
P2P Up for grabs Needs rebase

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: 2024-12-22 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me