p2p: Replace RecursiveMutex `cs_totalBytesSent` with Mutex and rename it #24157

pull w0xlt wants to merge 3 commits into bitcoin:master from w0xlt:cs_totalBytesSent_mutex changing 2 files +55 −27
  1. w0xlt commented at 10:21 PM on January 25, 2022: contributor

    Related to #19303, this PR gets rid of the RecursiveMutex cs_totalBytesSent 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 25, 2022
  3. DrahtBot added the label P2P on Jan 25, 2022
  4. w0xlt force-pushed on Jan 26, 2022
  5. w0xlt marked this as ready for review on Jan 26, 2022
  6. in src/net.h:914 in 6cb0fab330 outdated 
     910 | @@ -910,24 +911,22 @@ class CConnman
 911 |      //! that peer during `net_processing.cpp:PushNodeVersion()`.
 912 |      ServiceFlags GetLocalServices() const;
 913 |  
 914 | -    uint64_t GetMaxOutboundTarget() const;
 915 | +    uint64_t GetMaxOutboundTarget() const LOCKS_EXCLUDED(m_total_bytes_sent_mutex);
    hebasto commented at 1:48 PM on February 2, 2022:

    Is it feasible

        uint64_t GetMaxOutboundTarget() const EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);

    here and after?

    For context and example see #24235?

    vasild commented at 2:12 PM on February 2, 2022:

    I think yes.

    hebasto commented at 3:09 PM on February 10, 2022:

    And in other places?

  7. w0xlt force-pushed on Feb 10, 2022
  8. w0xlt marked this as a draft on Feb 10, 2022
  9. w0xlt force-pushed on Feb 10, 2022
  10. w0xlt marked this as ready for review on Feb 10, 2022
  11. w0xlt commented at 6:57 PM on February 10, 2022: contributor

    Addressed @hebasto suggestion.

    All functions changed in the second commit use EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex) instead of LOCKS_EXCLUDED(m_total_bytes_sent_mutex).

    The only exception is void RecordBytesSent(uint64_t bytes) which displayed the error below when using EXCLUSIVE_LOCKS_REQUIRED.

    Not sure about the reason for this error.

    net.cpp:1655:29: error: calling function 'RecordBytesSent' requires negative capability '!m_total_bytes_sent_mutex' [-Werror,-Wthread-safety-analysis]
            if (bytes_sent) RecordBytesSent(bytes_sent);
                            ^
net.cpp:3077:21: error: calling function 'RecordBytesSent' requires negative capability '!m_total_bytes_sent_mutex' [-Werror,-Wthread-safety-analysis]
    if (nBytesSent) RecordBytesSent(nBytesSent);
  12. hebasto commented at 7:51 PM on February 10, 2022: member

    @w0xlt

    Not sure about the reason for this error.

    Maybe:

    --- a/src/net.h
+++ b/src/net.h
@@ -829,7 +829,8 @@ public:
 
     bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
 
-    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);
+    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     using NodeFn = std::function<void(CNode*)>;
     void ForEachNode(const NodeFn& func)
@@ -1024,7 +1025,8 @@ private:
     /**
      * Check connected and listening sockets for IO readiness and process them accordingly.
      */
-    void SocketHandler();
+    void SocketHandler()
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     /**
      * Do the read/write for connected sockets that are ready for IO.
@@ -1037,7 +1039,8 @@ private:
     void SocketHandlerConnected(const std::vector<CNode*>& nodes,
                                 const std::set<SOCKET>& recv_set,
                                 const std::set<SOCKET>& send_set,
-                                const std::set<SOCKET>& error_set);
+                                const std::set<SOCKET>& error_set)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     /**
      * Accept incoming connections, one from each read-ready listening socket.
@@ -1045,7 +1048,8 @@ private:
      */
     void SocketHandlerListening(const std::set<SOCKET>& recv_set);
 
-    void ThreadSocketHandler();
+    void ThreadSocketHandler()
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
     void ThreadDNSAddressSeed();
 
     uint64_t CalculateKeyedNetGroup(const CAddress& ad) const;
@@ -1074,7 +1078,8 @@ private:
 
     // Network stats
     void RecordBytesRecv(uint64_t bytes);
-    void RecordBytesSent(uint64_t bytes) LOCKS_EXCLUDED(m_total_bytes_sent_mutex);
+    void RecordBytesSent(uint64_t bytes)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     /**
      * Return vector of current BLOCK_RELAY peers.

    ?

  13. w0xlt force-pushed on Feb 10, 2022
  14. w0xlt commented at 10:45 PM on February 10, 2022: contributor

    @hebasto Thanks. This worked.

    Very interesting. So, using EXCLUSIVE_LOCKS_REQUIRED(!mutex) requires that all functions that call the annotated function also have the same annotation. It is a cascading effect.

  15. hebasto commented at 12:30 PM on February 11, 2022: member

    @hebasto Thanks. This worked.

    Very interesting. So, using EXCLUSIVE_LOCKS_REQUIRED(!mutex) requires that all functions that call the annotated function also have the same annotation. It is a cascading effect.

    That is why negative capabilities provide a stronger safety guarantee.

    I don't see any reason of splitting changes between second and third commits. Maybe squash them?

  16. w0xlt force-pushed on Feb 11, 2022
  17. w0xlt commented at 7:37 PM on February 11, 2022: contributor

    Maybe squash them?

    Sure. Done.

  18. in src/net.h:1042 in 0b6c5da930 outdated 
    1036 | @@ -1034,15 +1037,17 @@ class CConnman
1037 |      void SocketHandlerConnected(const std::vector<CNode*>& nodes,
1038 |                                  const std::set<SOCKET>& recv_set,
1039 |                                  const std::set<SOCKET>& send_set,
1040 | -                                const std::set<SOCKET>& error_set);
1041 | +                                const std::set<SOCKET>& error_set)
1042 | +        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
1043 | +;
    hebasto commented at 8:02 PM on February 12, 2022:

    A spare semicolon? :smile:

    w0xlt commented at 7:53 PM on February 13, 2022:

    Oops. Fixed.

  19. hebasto approved
  20. hebasto commented at 8:05 PM on February 12, 2022: member

    ACK 0b6c5da9304af71dedb262bca9a0493c8616985b

  21. DrahtBot commented at 2:19 PM on February 13, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24931 (Strengthen thread safety assertions by ajtowns)
    • #24356 (refactor: replace CConnman::SocketEvents() with mockable Sock::WaitMany() by vasild)

    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.

  22. w0xlt force-pushed on Feb 13, 2022
  23. hebasto approved
  24. hebasto commented at 9:51 PM on February 13, 2022: member

    re-ACK f04ffdd2a79a77ea70d892be972de7821c1b876c

  25. DrahtBot cross-referenced this on Feb 14, 2022 from issue Make all networking code mockable by vasild
  26. in src/net.cpp:2937 in f04ffdd2a7 outdated 
    2933 | +    AssertLockNotHeld(m_total_bytes_sent_mutex);
2934 | +    LOCK(m_total_bytes_sent_mutex);
2935 | +    return _GetMaxOutboundTimeLeftInCycle();
2936 | +}
2937 | +
2938 | +std::chrono::seconds CConnman::_GetMaxOutboundTimeLeftInCycle() const
    vasild commented at 1:29 PM on February 14, 2022:

    nit: I think that the unwritten convention is to put the underscore at the end, like e.g. AddrManImpl::Good_(). Or maybe I am mistaken, is there any other place in the code that uses prefix _?

    w0xlt commented at 7:13 PM on February 14, 2022:

    Changed the underscore to the end. I think only in AddrManImpl this is used.

  27. in src/net.h:782 in f04ffdd2a7 outdated 
     795 | @@ -796,7 +796,8 @@ class CConnman
 796 |          nReceiveFloodSize = connOptions.nReceiveFloodSize;
 797 |          m_peer_connect_timeout = std::chrono::seconds{connOptions.m_peer_connect_timeout};
 798 |          {
 799 | -            LOCK(cs_totalBytesSent);
 800 | +            AssertLockNotHeld(m_total_bytes_sent_mutex);
 801 | +            LOCK(m_total_bytes_sent_mutex);
    vasild commented at 1:33 PM on February 14, 2022:

    This is not the beginning of a function. AssertLockNotHeld() does not belong here - it should be put in the beginning of functions.

    w0xlt commented at 7:13 PM on February 14, 2022:

    Fixed. Thanks.

  28. in src/net.h:814 in f04ffdd2a7 outdated 
     828 | @@ -828,7 +829,7 @@ class CConnman
 829 |  
 830 |      bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
 831 |  
 832 | -    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);
 833 | +    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
    vasild commented at 1:38 PM on February 14, 2022:

    Why not add AssertLockNotHeld(m_total_bytes_sent_mutex) in the beginning of PushMessage(), SocketHandler(), SocketHandlerConnected() and ThreadSocketHandler()?

    w0xlt commented at 7:13 PM on February 14, 2022:

    Good catch. Done. Thanks.

  29. w0xlt force-pushed on Feb 14, 2022
  30. w0xlt commented at 7:15 PM on February 14, 2022: contributor

    d370938 addresses @vasild suggestions.

  31. DrahtBot cross-referenced this on Feb 17, 2022 from issue refactor: replace CConnman::SocketEvents() with mockable Sock::WaitMany() by vasild
  32. vasild approved
  33. vasild commented at 1:11 PM on February 18, 2022: contributor

    ACK eef2c7b8c2d3c35da027429bf89ebb792e55f739

  34. hebasto approved
  35. hebasto commented at 8:30 PM on February 20, 2022: member

    ~re-ACK eef2c7b8c2d3c35da027429bf89ebb792e55f739~, only suggested changes since my previous review.

    UPDATE: ACK retracted. See #24157 (review)

  36. DrahtBot cross-referenced this on Mar 12, 2022 from issue BIP324: Enable v2 P2P encrypted transport by dhruv
  37. DrahtBot cross-referenced this on Apr 5, 2022 from issue test/BIP324: functional tests for v2 P2P encryption by stratospher
  38. w0xlt commented at 5:59 PM on April 12, 2022: contributor

    Is any further action needed in this PR?

  39. in src/net.h:783 in eef2c7b8c2 outdated 
     780 | @@ -781,6 +781,8 @@ class CConnman
 781 |      };
 782 |  
 783 |      void Init(const Options& connOptions) {
    ajtowns commented at 4:52 PM on April 13, 2022:

    Missing EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex)

    hebasto commented at 6:25 PM on April 13, 2022:

    Agree. I've verified this PR rebased on top of the current master (f60a63cc5f16b738d9d2ada3f10b27cf999df323):

    $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_total_bytes_sent_mutex

    ends with warnings

    ./net.h:781:13: warning: acquiring mutex 'm_total_bytes_sent_mutex' requires negative capability '!m_total_bytes_sent_mutex' [-Wthread-safety-negative]
            LOCK(m_total_bytes_sent_mutex);

    To silent all of them, the following patch works:

    --- a/src/net.h
+++ b/src/net.h
@@ -760,7 +760,8 @@ public:
         bool m_i2p_accept_incoming;
     };
 
-    void Init(const Options& connOptions) {
+    void Init(const Options& connOptions) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex)
+    {
         AssertLockNotHeld(m_total_bytes_sent_mutex);
 
         nLocalServices = connOptions.nLocalServices;
@@ -791,7 +792,7 @@ public:
 
     CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, bool network_active = true);
     ~CConnman();
-    bool Start(CScheduler& scheduler, const Options& options);
+    bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     void StopThreads();
     void StopNodes();
    jonatack commented at 11:29 AM on April 15, 2022:

    Reproduced the issue and confirm that this patch works (nice catch!)

    w0xlt commented at 4:25 PM on April 18, 2022:

    Done. Thanks for the reviews and suggestions.

    ajtowns commented at 9:31 AM on April 20, 2022:

    #24931 may be of interest

  40. ajtowns commented at 4:57 PM on April 13, 2022: contributor

    Think there's a missing annotation, but otherwise looks good.

  41. jonatack commented at 11:47 AM on April 15, 2022: contributor

    ACK modulo the missing annotation

    Verified the methods updated here have both an annotation in the declaration and a corresponding assertion in the definition:

    GetMaxOutboundTarget
GetMaxOutboundTimeLeftInCycle
GetMaxOutboundTimeLeftInCycle_
GetOutboundTargetBytesLeft
GetTotalBytesSent
OutboundTargetReached
PushMessage
RecordBytesSent
SocketHandler
SocketHandlerConnected
ThreadSocketHandler
  42. scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex
    -BEGIN VERIFY SCRIPT-
sed -i 's/cs_totalBytesSent/m_total_bytes_sent_mutex/g' -- $(git grep --files-with-matches 'cs_totalBytesSent')
-END VERIFY SCRIPT-
    a237a065cc
  43. w0xlt force-pushed on Apr 18, 2022
  44. in src/net.h:795 in eff791863d outdated 
     791 | @@ -789,7 +792,7 @@ class CConnman
 792 |  
 793 |      CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, bool network_active = true);
 794 |      ~CConnman();
 795 | -    bool Start(CScheduler& scheduler, const Options& options);
 796 | +    bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
    jonatack commented at 5:16 PM on April 18, 2022:

    e59472f8af0a1962fa5c331e41fa22a4f8602db8 missing assertion in the corresponding definition in src/net.cpp

     bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
 {
+    AssertLockNotHeld(m_total_bytes_sent_mutex);
     Init(connOptions);
    w0xlt commented at 8:44 AM on April 22, 2022:

    Done. Thanks.

  45. ajtowns cross-referenced this on Apr 20, 2022 from issue Strengthen thread safety assertions by ajtowns
  46. vasild approved
  47. vasild commented at 6:52 AM on April 22, 2022: contributor

    ACK eff791863da5dd1fcda7bddc7d729a63d772659d

    Compiles with clang 15.

  48. p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex`
    Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
    8be75fd0f0
  49. p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex 709af67add
  50. w0xlt force-pushed on Apr 22, 2022
  51. w0xlt commented at 8:46 AM on April 22, 2022: contributor

    Added the suggestion #24157 (review)

  52. vasild approved
  53. vasild commented at 9:24 AM on April 22, 2022: contributor

    ACK 709af67add93f6674fb80e3ae8e3f175653a62f0

  54. hebasto approved
  55. hebasto commented at 9:46 AM on April 22, 2022: member

    ACK 709af67add93f6674fb80e3ae8e3f175653a62f0, tested on Ubuntu 22.04.

  56. fanquake requested review from ajtowns on Apr 22, 2022
  57. jonatack commented at 10:11 AM on April 22, 2022: contributor

    ACK 709af67add93f6674fb80e3ae8e3f175653a62f0 per git range-diff 7a4ac71 eff7918 709af67, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative

  58. hebasto cross-referenced this on Apr 25, 2022 from issue Replace all of the RecursiveMutex instances with the Mutex ones by hebasto
  59. laanwj merged this on Apr 26, 2022
  60. laanwj closed this on Apr 26, 2022
  61. sidhujag referenced this in commit 751ffb710d on Apr 26, 2022
  62. w0xlt deleted the branch on Apr 27, 2022
  63. bitcoin locked this on Apr 27, 2023
Contributors
w0xlthebastovasildDrahtBotajtownsjonatack


ajtowns

Labels
P2P
Linked (view graph)
#19303 Replace all of the RecursiveMutex instances with the Mutex ones#24235 validation: use stronger EXCLUSIVE_LOCKS_REQUIRED()#24931 Strengthen thread safety assertions
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