refactor: Make CAddrMan::cs non-recursive #19238

pull hebasto wants to merge 10 commits into bitcoin:master from hebasto:200610-addrman-mx changing 4 files +75 −38
  1. hebasto commented at 3:39 pm on June 10, 2020: member

    This PR replaces RecursiveMutex CAddrMan::cs with Mutex CAddrMan::cs.

    All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident.

    Related to #19303.

    Based on #22025, and first three commits belong to it.

  2. hebasto commented at 3:41 pm on June 10, 2020: member
    cc @vasild :)
  3. DrahtBot added the label Refactoring on Jun 10, 2020
  4. DrahtBot added the label Tests on Jun 10, 2020
  5. DrahtBot commented at 4:50 pm on June 10, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21940 (refactor: Mark CAddrMan::Select const by MarcoFalke)
    • #20233 (addrman: Make consistency checks a runtime option by jnewbery)

    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.

  6. MarcoFalke commented at 4:51 pm on June 10, 2020: member
    Concept ACK. I think the reason that LOCKS_EXCLUDED isn’t widely used in the code base is that it doesn’t propagate up the call stack, so has a rather limited use case. Though, the new syntax claims to do that.
  7. MarcoFalke removed the label Tests on Jun 10, 2020
  8. MarcoFalke commented at 4:53 pm on June 10, 2020: member
    oh wait you are not adding the annotations. Any reason for that?
  9. hebasto commented at 5:01 pm on June 10, 2020: member

    @MarcoFalke

    oh wait you are not adding the annotations. Any reason for that?

    Added :)

  10. hebasto commented at 5:04 pm on June 10, 2020: member
    @MarcoFalke probably a1071f8b9ae12bbb6f0fddae40655b3860c785f0 deserves its own PR as it blocks the similar changes in other parts of the code, right?
  11. hebasto closed this on Jun 11, 2020

  12. hebasto reopened this on Jun 11, 2020

  13. in src/addrman.h:566 in d9572933f1 outdated
    557+    size_t size() const EXCLUSIVE_LOCKS_REQUIRED(!m_addrman_mutex)
    558     {
    559-        LOCK(cs); // TODO: Cache this in an atomic to avoid this overhead
    560-        return vRandom.size();
    561+        LOCK(m_addrman_mutex); // TODO: Cache this in an atomic to avoid this overhead
    562+        return sizeNonLockerHelper();
    


    vasild commented at 11:27 am on June 11, 2020:

    It would be nice to implement that TODO comment because it would make it possible to avoid any mutex requirements from size().

    I did so in https://github.com/vasild/bitcoin/commit/f25d8a7c6de0e45b037f91138e1b4a72e7a1653f. If you like it you can replace the commit refactor: Prevent double lock in CAddrMan::size() with that one. In this branch https://github.com/vasild/bitcoin/tree/200610-addrman-mx that is done and also the subsequent commits are adjusted.

    Or, if you don’t like the caching, then maybe just delete the TODO comment.


    hebasto commented at 1:46 pm on June 11, 2020:
    The commit you suggested does change CAddrMan::size() behavior. So I’d prefer to keep this PR as clean refactoring with no behavior changes.

    vasild commented at 2:50 pm on June 11, 2020:
    ok
  14. in src/sync.h:117 in d9572933f1 outdated
    102@@ -103,6 +103,9 @@ class LOCKABLE AnnotatedMixin : public PARENT
    103     }
    104 
    105     using UniqueLock = std::unique_lock<PARENT>;
    106+#ifdef __clang__
    107+    const AnnotatedMixin& operator!() const { return *this; }
    108+#endif // __clang__
    


    vasild commented at 11:39 am on June 11, 2020:

    Here and in StdMutex - I find this very confusing because it would end up with m being the same as !m. It is a huge hack imposed by clang. I would suggest to add some comment to explain what is that. Something like Required by the clang thread safety instrumentation so that we can declare that a mutex should not be held when a function is called - EXCLUSIVE_LOCKS_REQUIRED(!m) (aka "negative capabilities").

    The example in https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h also has some comment.

    PS I wonder what would EXCLUSIVE_LOCKS_REQUIRED(!!m) do. :eyes:


    hebasto commented at 2:21 pm on June 11, 2020:
    Done in #19249.
  15. vasild commented at 11:44 am on June 11, 2020: member

    Looks good, some suggestions below.

    The last two commits:

    0Add means to handle negative capabilities in thread safety annotations
    1refactor: Add negative thread safety annotations to CAddrMan
    

    are somewhat unrelated and maybe deserve a separate PR if they cause some controversy or if another reviewer asks to move them away to ease review. Since I already reviewed everything I am fine with them as they are.

  16. hebasto force-pushed on Jun 11, 2020
  17. hebasto force-pushed on Jun 11, 2020
  18. hebasto commented at 2:20 pm on June 11, 2020: member

    Updated d9572933f1fb0201b259d9cb7b40873bdbad7b6f -> b6712ece8ada3b1c9230f1264da81cbf11fe4595 (pr19238.02 -> pr19238.04):

    • the commit “Add means to handle negative capabilities in thread safety annotations” has been separated into #19249, and this PR has been rebased on the latter one
    • negative mutex requirements added to each commit opportunistically
  19. vasild approved
  20. vasild commented at 2:49 pm on June 11, 2020: member
    ACK b6712ece8
  21. MarcoFalke referenced this in commit 9a482d3604 on Jun 17, 2020
  22. hebasto force-pushed on Jun 17, 2020
  23. hebasto commented at 11:29 am on June 17, 2020: member

    Updated b6712ece8ada3b1c9230f1264da81cbf11fe4595 -> 9cd0ed858c96b1052580e2044069c5cb9434628e (pr19238.04 -> pr19238.05):

    • rebased due to the merging of #19249
    • all of the related code branches are covered by appropriate lock assertions to insure that the locking policy has not been changed by accident
  24. hebasto commented at 11:33 am on June 17, 2020: member
    The OP has been updated.
  25. in src/addrman.h:634 in 9cd0ed858c outdated
    659+    void ResolveCollisions() EXCLUSIVE_LOCKS_REQUIRED(!m_addrman_mutex)
    660     {
    661-        LOCK(cs);
    662-        Check();
    663+        AssertLockNotHeld(m_addrman_mutex);
    664+        LOCK(m_addrman_mutex);
    


    vasild commented at 6:55 pm on June 17, 2020:

    The pattern AssertLockNotHeld(m); LOCK(m); seems too verbose. Following that, should we add AssertLockNotHeld() before every LOCK() in the source code?

    What about adding AssertLockNotHeld() inside LOCK()?

    What happens if we try to lock a mutex which we already own? https://en.cppreference.com/w/cpp/thread/mutex/lock says:

    If lock is called by a thread that already owns the mutex, the behavior is undefined

    The following test case

     0void f(Mutex& m)
     1{
     2    LOCK(m);
     3}
     4
     5BOOST_AUTO_TEST_CASE(double_lock)
     6{
     7    Mutex m;
     8    LOCK(m);
     9    f(m);
    10}
    

    throws an exception std::__1::system_error: mutex lock failed: Resource deadlock avoided by the OS itself (from inside std::mutex::lock()). So, we don’t have a protection in our code and we need one because want to avoid the undefined behavior (can’t rely on the OS to always throw an exception).

    The following succeeds (why not also exception?):

    0void f(std::mutex& m) {
    1    std::unique_lock<std::mutex> lock(m);
    2}
    3
    4int main(int, char**) {
    5    std::mutex m;
    6    std::unique_lock<std::mutex> lock(m);
    7    f(m);
    8    return 0;
    9}
    

    hebasto commented at 7:48 pm on June 17, 2020:

    Following that, should we add AssertLockNotHeld() before every LOCK() in the source code?

    Only for Mutex instances, not for RecursiveMutex ones.

    What about adding AssertLockNotHeld() inside LOCK()?

    Not now because the RecursiveMutex instance could be an argument of LOCK().


    vasild commented at 5:26 pm on June 18, 2020:

    What about checking, inside LOCK(), whether the mutex being locked is instance of Mutex (and not RecursiveMutex) and only asserting that we don’t own it in that case:

     0diff --git i/src/sync.h w/src/sync.h
     1index 60e5a87ae..36c348898 100644
     2--- i/src/sync.h
     3+++ w/src/sync.h
     4@@ -10,12 +10,13 @@
     5 #include <util/macros.h>
     6 
     7 #include <condition_variable>
     8 #include <mutex>
     9 #include <string>
    10 #include <thread>
    11+#include <type_traits>
    12 
    13 ////////////////////////////////////////////////
    14 //                                            //
    15 // THE SIMPLE DEFINITION, EXCLUDING DEBUG CODE //
    16 //                                            //
    17 ////////////////////////////////////////////////
    18@@ -145,12 +146,15 @@ private:
    19         return Base::owns_lock();
    20     }
    21 
    22 public:
    23     UniqueLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock)
    24     {
    25+        if (std::is_base_of<::Mutex, Mutex>::value) {
    26+            AssertLockNotHeldInternal(pszName, pszFile, nLine, &mutexIn);
    27+        }
    28         if (fTry)
    29             TryEnter(pszName, pszFile, nLine);
    30         else
    31             Enter(pszName, pszFile, nLine);
    32     }
    33 
    34diff --git i/src/test/sync_tests.cpp w/src/test/sync_tests.cpp
    35index 5c6c2ee38..a66d55519 100644
    36--- i/src/test/sync_tests.cpp
    37+++ w/src/test/sync_tests.cpp
    38@@ -46,7 +46,27 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
    39 
    40     #ifdef DEBUG_LOCKORDER
    41     g_debug_lockorder_abort = prev;
    42     #endif
    43 }
    44 
    45+template <typename M>
    46+void lock(M& m)
    47+{
    48+    LOCK(m);
    49+}
    50+
    51+BOOST_AUTO_TEST_CASE(double_lock_mutex)
    52+{
    53+    Mutex m;
    54+    LOCK(m);
    55+    lock(m);
    56+}
    57+
    58+BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex)
    59+{
    60+    RecursiveMutex m;
    61+    LOCK(m);
    62+    lock(m);
    63+}
    64+
    65 BOOST_AUTO_TEST_SUITE_END()
    

    It works - the test double_lock_mutex fails because of the newly added check while the double_lock_recursive_mutex succeeds.

    The double colon in ::Mutex is required because the class template type template <typename Mutex, ...> UniqueLock ... shadows the global typedef ... Mutex;.


    hebasto commented at 5:38 pm on June 18, 2020:
    @vasild Mind submitting a PR that it could be reviewed before this one?

    hebasto commented at 6:07 pm on June 18, 2020:

    Currently, it is assumed the following patterns:

    • for public interface methods:
     0# foo.h
     1class Foo
     2{
     3public: void foo() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
     4}
     5# foo.cpp
     6void Foo::foo()
     7{
     8    AssertLockNotHeld(m_mutex);
     9    LOCK(m_mutex);
    10    // do magic
    11}
    
    • for internal methods that are called from critical sections:
     0# foo.h
     1class Foo
     2{
     3pivate: void foo_cs() EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
     4}
     5# foo.cpp
     6void Foo::foo_cs()
     7{
     8    AssertLockHeld(m_mutex);
     9    // do magic
    10}
    

    The common places are thread safety annotations and lock assertions. It seems placing AssertLockNotHeld() into internals of the LOCK() breaks uniformity, no?


    vasild commented at 7:22 pm on June 18, 2020:

    Mind submitting a PR that it could be reviewed before this one?

    Yes, I think that deserves a separate PR. I have to consider this a little bit before opening a PR. Maybe it would be possible to optimize it - the patch above would lock lockdata.dd_mutex and dig into the stack of locks for the current thread 2 times - once from the newly added AssertLockNotHeld() and a second time from push_lock(). It should be possible to do everything in push_lock() and avoid adding the call to AssertLockNotHeld().

    …breaks uniformity, no?

    I don’t see it that way - LOCK() already implies that we don’t own the mutex.


    vasild commented at 1:52 pm on June 20, 2020:
    Here is a PR that would detect a double lock. With the check from inside LOCK() it makes unnecessary to add AssertLockNotHeld() calls before each LOCK(): https://github.com/bitcoin/bitcoin/pull/19337
  26. hebasto commented at 9:09 am on June 18, 2020: member
    @sipa Mind reviewing this PR?
  27. hebasto commented at 6:14 pm on June 18, 2020: member
    btw, how about convention to add the _cs suffix to the name of the function that is called from within the critical section already guarded by the locked instance of the Mutex?
  28. vasild commented at 7:28 pm on June 18, 2020: member

    btw, how about convention to add the _cs suffix to the name of the function that is called from within the critical section already guarded by the locked instance of the Mutex?

    I like it, and it is shorter than ...NonLockHelper(), but what about the CamelCaseConvention? Also, it would get fuzzy if more than one mutex is involved and a method requires one to be held and the other not…

  29. hebasto commented at 1:56 pm on June 20, 2020: member
    Note for reviewers: please review #19337 at fist.
  30. sidhujag referenced this in commit 09ab074c00 on Jul 7, 2020
  31. DrahtBot added the label Needs rebase on Aug 12, 2020
  32. hebasto force-pushed on Aug 15, 2020
  33. hebasto commented at 6:38 pm on August 15, 2020: member

    Updated 9cd0ed858c96b1052580e2044069c5cb9434628e -> d985aeeac48114f1078980888303f34ffdb78e74 (pr19238.05 -> pr19238.06):

    • rebased due to the conflict with #19658
    • implemented naming convention with :+1: from @ajtowns
    • separated commit with potentially controversial mutex renaming
  34. DrahtBot removed the label Needs rebase on Aug 15, 2020
  35. DrahtBot added the label Needs rebase on Oct 11, 2020
  36. hebasto marked this as a draft on Nov 8, 2020
  37. laanwj referenced this in commit 50091592dd on Nov 25, 2020
  38. jnewbery commented at 1:12 pm on December 31, 2020: member
    Concept ACK. Needs rebase.
  39. hebasto force-pushed on Jan 1, 2021
  40. hebasto marked this as ready for review on Jan 1, 2021
  41. hebasto commented at 10:01 pm on January 1, 2021: member

    Updated d985aeeac48114f1078980888303f34ffdb78e74 -> f1246cb12f573c4a8b32d75d6ea377eeec3b43e7 (pr19238.06 -> pr19238.07):

    • rebased
    • dropped the mutex renaming commit
  42. DrahtBot removed the label Needs rebase on Jan 1, 2021
  43. hebasto force-pushed on Jan 1, 2021
  44. in src/addrman.h:273 in 0c0fb742ad outdated
    268     uint256 nKey;
    269 
    270     //! Source of random numbers for randomization in inner loops
    271     FastRandomContext insecure_rand;
    272 
    273+private:
    


    jnewbery commented at 9:55 am on January 2, 2021:
    Can you consolidate all the private members and protected members to be next to each other? Multiple private and protected access specifiers make this harder to read than is necessary.

    vasild commented at 9:54 am on January 14, 2021:
    Yeah, class declaration is easier to read if there is just one instance of public:, protected: and private: (in that order).

    hebasto commented at 12:16 pm on May 23, 2021:
  45. in src/addrman.h:300 in 0c0fb742ad outdated
    296@@ -262,7 +297,7 @@ friend class CAddrManTest;
    297     bool Add_(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
    298 
    299     //! Mark an entry as attempted to connect.
    300-    void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
    301+    void AttemptWithLockHeld(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
    


    jnewbery commented at 1:42 pm on January 2, 2021:

    I don’t think we need to expose this as a public method just for the addrman tests. Just release the lock in that test before calling Attempt():

     0--- a/src/test/addrman_tests.cpp
     1+++ b/src/test/addrman_tests.cpp
     2@@ -73,13 +73,12 @@ public:
     3     // Simulates connection failure so that we can test eviction of offline nodes
     4     void SimConnFail(const CService& addr)
     5     {
     6-         LOCK(cs);
     7-         int64_t nLastSuccess = 1;
     8-         Good_(addr, true, nLastSuccess); // Set last good connection in the deep past.
     9+         // Set last good connection in the deep past.
    10+         WITH_LOCK(cs, {Good_(addr, true, /* nLastSuccess= */ 1);});
    11 
    12          bool count_failure = false;
    13          int64_t nLastTry = GetAdjustedTime()-61;
    14-         AttemptWithLockHeld(addr, count_failure, nLastTry);
    15+         Attempt(addr, count_failure, nLastTry);
    

    The unit test is single threaded, so there’s no need to hold the mutex between Good_() and Attempt().


    hebasto commented at 6:35 pm on May 23, 2021:
    Thanks! Done in bc3694a49a93b9ae6f8cacfd0d5b1def5f09b004.
  46. in src/addrman.h:232 in 0c0fb742ad outdated
    228@@ -229,13 +229,48 @@ friend class CAddrManTest;
    229     //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
    230     std::set<int> m_tried_collisions;
    231 
    232+    void CheckWithLockHeld() EXCLUSIVE_LOCKS_REQUIRED(cs)
    


    jnewbery commented at 1:43 pm on January 2, 2021:
    Make this (and other Check() functions) const?

    hebasto commented at 4:59 pm on May 23, 2021:

    This change is not trivial due to the non-const std::map::operator[]:

    0addrman.cpp:433:20: error: no viable overloaded operator[] for type 'const std::map<CNetAddr, int>'
    1        if (mapAddr[info] != n)
    2            ~~~~~~~^~~~~
    
  47. in src/addrman.h:242 in 0c0fb742ad outdated
    237+            LogPrintf("ADDRMAN CONSISTENCY CHECK FAILED!!! err=%i\n", err);
    238+        }
    239+#endif // DEBUG_ADDRMAN
    240+    }
    241+
    242+    void ClearWithLockHeld() EXCLUSIVE_LOCKS_REQUIRED(cs)
    


    jnewbery commented at 1:45 pm on January 2, 2021:

    It’d be good to get rid of the Clear() functions entirely:

    • the call to Clear() in deserialize could be replaced with an assert that size() is 0 (we should never deserialize into a populated CAddrman)
    • the calls in the tests can be replaced by instantiating new CAddrmans. Clearing then reusing an existing CAddrman is not a realistic usage pattern.

    If you don’t want to do that in this PR, it feels like this function should be called Clear_() and be defined in the cpp file, for consistency with the other functions.


    hebasto commented at 5:56 pm on May 23, 2021:

    @jnewbery

    It’d be good to get rid of the Clear() functions entirely:

    I totally agree with you.

    • the call to Clear() in deserialize could be replaced with an assert that size() is 0 (we should never deserialize into a populated CAddrman)

    Going to implement this suggestion.

    • the calls in the tests can be replaced by instantiating new CAddrmans. Clearing then reusing an existing CAddrman is not a realistic usage pattern.

    This part is not trivial, and probably deserves its own pr.


    hebasto commented at 6:34 pm on May 23, 2021:
    Thanks! Updated.
  48. in src/addrman.h:692 in 0c0fb742ad outdated
    691@@ -679,9 +692,9 @@ friend class CAddrManTest;
    692         CAddrInfo ret;
    


    jnewbery commented at 1:49 pm on January 2, 2021:

    It feels like this is a good opportunity to make the access patterns consistent here:

     0@@ -689,13 +689,10 @@ public:
     1     //! Randomly select an address in tried that another address is attempting to evict.
     2     CAddrInfo SelectTriedCollision()
     3     {
     4-        CAddrInfo ret;
     5-        {
     6-            LOCK(cs);
     7-            CheckWithLockHeld();
     8-            ret = SelectTriedCollision_();
     9-            CheckWithLockHeld();
    10-        }
    11+        LOCK(cs);
    12+        CheckWithLockHeld();
    13+        CAddrInfo ret = SelectTriedCollision_();
    14+        CheckWithLockHeld();
    15         return ret;
    16     }
    17 
    18@@ -704,26 +701,21 @@ public:
    19      */
    20     CAddrInfo Select(bool newOnly = false)
    21     {
    22-        CAddrInfo addrRet;
    23-        {
    24-            LOCK(cs);
    25-            CheckWithLockHeld();
    26-            addrRet = Select_(newOnly);
    27-            CheckWithLockHeld();
    28-        }
    29+        LOCK(cs);
    30+        CheckWithLockHeld();
    31+        CAddrInfo addrRet = Select_(newOnly);
    32+        CheckWithLockHeld();
    33         return addrRet;
    34     }
    35 
    36     //! Return a bunch of addresses, selected at random.
    37     std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct)
    38     {
    39-        Check();
    40+        LOCK(cs);
    41+        CheckWithLockHeld();
    42         std::vector<CAddress> vAddr;
    43-        {
    44-            LOCK(cs);
    45-            GetAddr_(vAddr, max_addresses, max_pct);
    46-        }
    47-        Check();
    48+        GetAddr_(vAddr, max_addresses, max_pct);
    49+        CheckWithLockHeld();
    50         return vAddr;
    51     }
    

    (i.e. for all of these functions take the lock, run Check(), run the inner function, run Check() again, release lock/return)


    vasild commented at 9:44 am on January 14, 2021:
    :+1: This would make the Check() function unused, so it can be removed.

    hebasto commented at 6:33 pm on May 23, 2021:
    Thanks! Done in e21c0dd57009197dfeedbc4ed4d30454b7a05cf6.
  49. hebasto commented at 1:57 pm on January 2, 2021: member
    @jnewbery Thank you for your review. While working on this pull I had similar thoughts about points you mentioned. So I’m happy to make some improvements.
  50. jnewbery commented at 2:23 pm on January 2, 2021: member
    @hebasto Whilst you’re looking at this code, do you mind taking a look at #20233. Without that PR, CAddrMan::Check() is basically useless, since it’s enabled by a compile-time option which I expect nobody uses. That PR would make it a runtime check, so that we could enable it for the unit, functional and fuzz tests. It’s obviously much more likely to catch bugs if it’s being run by those tests!
  51. hebasto commented at 2:34 pm on January 2, 2021: member

    @hebasto Whilst you’re looking at this code, do you mind taking a look at #20233. Without that PR, CAddrMan::Check() is basically useless, since it’s enabled by a compile-time option which I expect nobody uses. That PR would make it a runtime check, so that we could enable it for the unit, functional and fuzz tests. It’s obviously much more likely to catch bugs if it’s being run by those tests!

    Sure!

  52. in src/addrman.h:625 in 0c0fb742ad outdated
    624@@ -608,23 +625,19 @@ friend class CAddrManTest;
    625     void Check()
    


    ajtowns commented at 3:33 am on January 6, 2021:
    I was expecting to see EXCLUSIVE_LOCKS_REQUIRED(!cs) added to these functions that lock cs to get compile time detection of double-locking. Any reason not to have them?

    hebasto commented at 6:32 pm on May 23, 2021:
    Of course! Done in 7f01942d2d3d9d66a8932d6a1fe08281d9b52273.
  53. vasild commented at 9:53 am on January 14, 2021: member
    0c0fb742ad7b686209efb627a8ec43d7f96c3754 looks good, I agree with the above suggestions from @jnewbery and @ajtowns.
  54. hebasto force-pushed on May 23, 2021
  55. hebasto commented at 6:30 pm on May 23, 2021: member

    @jnewbery @ajtowns @vasild

    Thank you for your reviews!

    Updated 0c0fb742ad7b686209efb627a8ec43d7f96c3754 -> 148a7c9454adc3e311867f3566e01d261b362f71 (pr19238.08 -> pr19238.09):

    • addressed comments
    • rebased on top of the recent CI changes
  56. hebasto renamed this:
    refactor: Replace RecursiveMutex with Mutex in CAddrMan
    refactor: Make CAddrMan::cs non-recursive
    on May 23, 2021
  57. in src/addrman.h:727 in 148a7c9454 outdated
    722+    void Check()
    723+        EXCLUSIVE_LOCKS_REQUIRED(cs)
    724+    {
    725+#ifdef DEBUG_ADDRMAN
    726+        AssertLockHeld(cs);
    727+        if (const int err = Check_(); err != 0) {
    


    vasild commented at 9:29 am on May 24, 2021:

    It is a matter of taste. Personally, I find the “classic” easier to read:

    0const int err = Check_();
    1if (err != 0) {
    

    It is also more gdb-friendly because one can set breakpoint on the first or on the second line.


    hebasto commented at 10:14 am on May 24, 2021:
    Thanks! Updated.
  58. vasild approved
  59. vasild commented at 10:05 am on May 24, 2021: member

    ACK 148a7c9454adc3e311867f3566e01d261b362f71

    This PR includes #22025.

    Compiles without warnings with Clang 13.0.0.

  60. hebasto force-pushed on May 24, 2021
  61. vasild approved
  62. vasild commented at 12:25 pm on May 24, 2021: member
    ACK ee79105070778bd1f3f9693536e3c92801ae63a8
  63. laanwj referenced this in commit ea1e5c2c71 on May 27, 2021
  64. hebasto force-pushed on May 27, 2021
  65. hebasto commented at 2:16 pm on May 27, 2021: member
    Rebased ee79105070778bd1f3f9693536e3c92801ae63a8 -> e2787fb24a675e62a82bcb43400ad8e1a68761d2 (pr19238.10 -> pr19238.11) due to the merging of #22025.
  66. vasild approved
  67. vasild commented at 10:54 am on May 28, 2021: member
    ACK e2787fb24a675e62a82bcb43400ad8e1a68761d2
  68. DrahtBot added the label Needs rebase on Jun 12, 2021
  69. hebasto force-pushed on Jun 12, 2021
  70. hebasto commented at 12:33 pm on June 12, 2021: member
    Rebased e2787fb24a675e62a82bcb43400ad8e1a68761d2 -> af9550561d7fa8cfa8da910db3e46993f848a0f5 (pr19238.11 -> pr19238.12) due to the conflict with #18722.
  71. DrahtBot removed the label Needs rebase on Jun 12, 2021
  72. vasild approved
  73. vasild commented at 1:06 pm on June 14, 2021: member
    ACK af9550561d7fa8cfa8da910db3e46993f848a0f5
  74. jnewbery commented at 1:30 pm on June 14, 2021: member
    Code review ACK af9550561d7fa8cfa8da910db3e46993f848a0f5
  75. MarcoFalke commented at 2:17 pm on June 14, 2021: member
    There might be a silent merge conflict
  76. MarcoFalke commented at 2:19 pm on June 14, 2021: member
    0test/fuzz/addrman.cpp:110:30: error: 'Check' is a private member of 'CAddrMan'
    1    (void)/*const_*/addr_man.Check();
    2                             ^
    3./addrman.h:722:10: note: declared private here
    4    void Check()
    5         ^
    61 error generated.
    
  77. test: Drop excessive locking in CAddrManTest::SimConnFail
    The unit test is single threaded, so there's no need to hold the mutex
    between Good() and Attempt().
    
    This change avoids recursive locking in the CAddrMan::Attempt function.
    
    Co-authored-by: John Newbery <john@johnnewbery.com>
    2da95545ea
  78. refactor: Avoid recursive locking in CAddrMan::size efc6fac951
  79. hebasto commented at 2:24 pm on June 14, 2021: member

    There might be a silent merge conflict

    With #21941. Rebasing.

  80. Make CAddrMan::Check private
    Change in the addrman.h header is move-only.
    06703973c7
  81. refactor: Fix CAddrMan::Check style
    This change improves readability, and follows Developer Notes.
    f77d9c79aa
  82. refactor: Avoid recursive locking in CAddrMan::Check 187b7d2bb3
  83. refactor: Apply consistent pattern for CAddrMan::Check usage
    Co-authored-by: John Newbery <john@johnnewbery.com>
    f79a664314
  84. refactor: Avoid recursive locking in CAddrMan::Clear
    Co-authored-by: John Newbery <john@johnnewbery.com>
    b138973a8b
  85. Add thread safety annotations to CAddrMan public functions 5ef1d0b698
  86. Add AssertLockHeld to CAddrMan private functions f5d1c7fac7
  87. refactor: Make CAddrMan::cs non-recursive ae98aec9c0
  88. hebasto force-pushed on Jun 14, 2021
  89. hebasto commented at 2:30 pm on June 14, 2021: member

    @MarcoFalke @jnewbery @vasild

    Rebased af9550561d7fa8cfa8da910db3e46993f848a0f5 -> ae98aec9c0521cdcec76459c8200bd45ff6a1485 (pr19238.12 -> pr19238.13) due to the silent merge conflict with #21941.

  90. vasild approved
  91. vasild commented at 2:37 pm on June 14, 2021: member
    ACK ae98aec9c0521cdcec76459c8200bd45ff6a1485
  92. MarcoFalke merged this on Jun 14, 2021
  93. MarcoFalke closed this on Jun 14, 2021

  94. hebasto deleted the branch on Jun 14, 2021
  95. sidhujag referenced this in commit 12d86d6ab8 on Jun 14, 2021
  96. gwillen referenced this in commit 6b9494054a on Jun 1, 2022
  97. DrahtBot locked this on Aug 16, 2022

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-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me