refactor: Improve thread safety analysis by propagating some negative capabilities #25175

1. 
   hebasto commented at 11:40 am on May 20, 2022: member

   This PR is a follow-up for bitcoin/bitcoin#22778 and bitcoin/bitcoin#24062, and it seems required for bitcoin/bitcoin#24931.

   See details in the commit messages.

2. refactor: Propagate negative `!m_most_recent_block_mutex` capability

   Could be verified with
   $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
   $ make clean
   $ make 2>&1 | grep m_most_recent_block_mutex

   5a6e3c1db3
3. refactor: Propagate negative `!m_tx_relay_mutex` capability

   Could be verified with
   $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
   $ make clean
   $ make 2>&1 | grep m_tx_relay_mutex

   2b3373c152
4. DrahtBot added the label P2P on May 20, 2022
5. DrahtBot added the label Refactoring on May 20, 2022
6. in src/net_processing.cpp:476 in 2b3373c152

   471@@ -472,15 +472,16 @@ class PeerManagerImpl final : public PeerManager
   472         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
   473     void BlockChecked(const CBlock& block, const BlockValidationState& state) override
   474         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
   475-    void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override;
   476+    void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override
   477+        EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex);
   

   ---

   ---

   

   MarcoFalke commented at 12:47 pm on May 20, 2022:

   I wonder how useful it is to annotate interface functions with member mutexes. The only allowed caller can’t see the mutex, as it is private.

   ---

   

   hebasto commented at 1:24 pm on May 20, 2022:

   An annotation is not for a caller, rather for Clang Thread Safety Analysis C++ extension.

   ---

   

   ajtowns commented at 3:33 pm on May 20, 2022:

   (An external caller (accessing via the virtual/override) can’t see the mutex, but an in-class caller could)

   ---

   

   MarcoFalke commented at 5:43 pm on May 20, 2022:

   Yeah, I was just thinking that it would be incredibly unlikely or even wrong to even have an in-class caller.
7. MarcoFalke approved
8. 
   MarcoFalke commented at 12:48 pm on May 20, 2022: member

   lgtm
9. 
   jonatack commented at 1:33 pm on May 20, 2022: member

   Concept ACK
10. 
    ajtowns commented at 2:17 pm on May 20, 2022: member

    ACK 2b3373c1520aa0b41277cd89956224e08cbd79dd
11. w0xlt approved
12. 
    w0xlt commented at 3:24 pm on May 20, 2022: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/25175/commits/2b3373c1520aa0b41277cd89956224e08cbd79dd
13. MarcoFalke merged this on May 20, 2022
14. MarcoFalke closed this on May 20, 2022

    ---

15. hebasto deleted the branch on May 20, 2022
16. sidhujag referenced this in commit e1cec3b50d on May 28, 2022
17. DrahtBot locked this on May 20, 2023

Contributors
hebasto MarcoFalke ajtowns jonatack w0xlt

Labels
Refactoring P2P