refactor: replace RecursiveMutex cs_SubVer with Mutex (and rename) #24079

1. 
   theStack commented at 1:44 am on January 16, 2022: member

   This PR is related to #19303 and gets rid of the RecursiveMutex cs_SubVer. Both of the critical sections only directly access the guarded variable, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex.
2. theStack renamed this:
   refactor: replace RecursiveMutex `cs SubVer` with Mutex (and rename)
   refactor: replace RecursiveMutex `cs_SubVer` with Mutex (and rename)
   on Jan 16, 2022
3. DrahtBot added the label P2P on Jan 16, 2022
4. DrahtBot added the label Refactoring on Jan 16, 2022
5. in src/net.h:437 in 2b92572d0e outdated

   433@@ -434,12 +434,12 @@ class CNode
   434     //! Whether this peer is an inbound onion, i.e. connected via our Tor onion service.
   435     const bool m_inbound_onion;
   436     std::atomic<int> nVersion{0};
   437-    RecursiveMutex cs_SubVer;
   438+    Mutex subver_mutex;
   

   ---

   ---

   

   hebasto commented at 8:33 am on January 16, 2022:

   0    Mutex m_subver_mutex;
   

   ---

   

   theStack commented at 3:54 pm on January 16, 2022:

   Thanks, done.
6. 
   hebasto commented at 8:59 am on January 16, 2022: member

   This mutex was added in 22b4966a29501c4f3f2e970ac5008fbd91e665a9 back in 2017.

   Maybe it is out of this PR scope, but in the current codebase I see no reasons for it because a write operation is executed once only during processing the received VERSION message.

7. scripted-diff: rename `cs_SubVer` -> `m_subver_mutex`

   -BEGIN VERIFY SCRIPT-
   sed -i 's/cs_SubVer/m_subver_mutex/g' ./src/net.h ./src/net.cpp ./src/net_processing.cpp
   -END VERIFY SCRIPT-

   0639aba42a
8. refactor: replace RecursiveMutex `m_subver_mutex` with Mutex

   In each of the critical sections, only the the guarded variable is
   accessed, without any chance that within one section another one is
   called.  Hence, we can use an ordinary Mutex instead of RecursiveMutex.

   30927cb530
9. theStack force-pushed on Jan 16, 2022
10. 
    theStack commented at 3:59 pm on January 16, 2022: member

    Maybe it is out of this PR scope, but in the current codebase I see no reasons for it because a write operation is executed once only during processing the received VERSION message.

    I agree that it’s unlikely, but isn’t there still a non-zero probability that access could happen concurrently? Processing of a VERSION message could in theory happen during a getpeerinfo RPC call – a peer is already in the nodes list before the version exchange is completed.

11. hebasto approved
12. 
    hebasto commented at 4:07 pm on January 16, 2022: member

    ACK 30927cb5306d01da2716786c2d9457c49ec49d0f, I have reviewed the code and it looks OK, I agree it can be merged.
13. MarcoFalke merged this on Jan 17, 2022
14. MarcoFalke closed this on Jan 17, 2022

    ---

15. theStack deleted the branch on Jan 17, 2022
16. sidhujag referenced this in commit 2ec7929b1c on Jan 18, 2022
17. DrahtBot locked this on Jan 17, 2023

Contributors
theStack hebasto

Labels
Refactoring P2P