addrman: Add Clang thread safety annotations for variables guarded by CAddrMan.cs #13115

• Add Clang thread safety annotations for variables guarded by CAddrMan.cs
• Add missing CAddrMan.cs locks

<!--5d09a71f8925f3f132321140b44b946d-->The last travis run for this pull request was 84 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@MarcoFalke Would you mind reviewing this one? Should hopefully be trivial :-)

utACK 4f04bda90e113cd596b019c05d6a5cb3ac4be540

@Empact @promag @ajtowns Would you mind reviewing? Should hopefully be trivial :-)

Should squash or invert commit order otherwise 1st commit is broken? Also why rename CAddrMan::cs to CAddrMan::cs_addrMan?

@promag Fixed. Please re-review :-)

Unrelated feature_notifications.py failure in appveyor:

test  2018-10-08T14:19:56.724000Z TestFramework (ERROR): Unexpected exception caught during testing 
  Traceback (most recent call last):
    File "C:\projects\bitcoin\test\functional\test_framework\test_framework.py", line 172, in main
      self.run_test()
    File "C:\projects\bitcoin/test/functional/feature_notifications.py", line 55, in run_test
      os.remove(os.path.join(self.walletnotify_dir, tx_file))
  PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_runner_20181008_141018\\feature_notifications_38\\walletnotify\\0b3efb8afc557368538b69c082c9f343b4d9c3f52145405438b98a1384fe13ad'

utACK 116e77b.

nits:

• commit messages could mention CAddrMan.
• could squash.

@promag Thanks for the quick review. Feedback addressed. Please re-review :-)

@practicalswift could drop these EXCLUSIVE_LOCKS_REQUIRED? GUARDED_BY should be enough considering the public interface locks cs, and therefore there aren't atomicity concerns?

So:

diff --git a/src/addrman.h b/src/addrman.h
index a36f7ea10..0d9081252 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -192,31 +192,31 @@ private:
     mutable CCriticalSection cs;

     //! last used nId
-    int nIdCount;
+    int nIdCount GUARDED_BY(cs);

     //! table with information about all nIds
-    std::map<int, CAddrInfo> mapInfo;
+    std::map<int, CAddrInfo> mapInfo GUARDED_BY(cs);

     //! find an nId based on its network address
-    std::map<CNetAddr, int> mapAddr;
+    std::map<CNetAddr, int> mapAddr GUARDED_BY(cs);

     //! randomly-ordered vector of all nIds
-    std::vector<int> vRandom;
+    std::vector<int> vRandom GUARDED_BY(cs);

     // number of "tried" entries
-    int nTried;
+    int nTried GUARDED_BY(cs);

     //! list of "tried" buckets
-    int vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE];
+    int vvTried[ADDRMAN_TRIED_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);

     //! number of (unique) "new" entries
-    int nNew;
+    int nNew GUARDED_BY(cs);

     //! list of "new" buckets
-    int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE];
+    int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);

     //! last time Good was called (memory only)
-    int64_t nLastGood;
+    int64_t nLastGood GUARDED_BY(cs);

     //! Holds addrs inserted into tried table that collide with existing entries. Test-before-evict discipline used to resolve these collisions.
     std::set<int> m_tried_collisions;

@promag I'm not sure I follow here. Is you suggestion to drop all the EXCLUSIVE_LOCKS_REQUIRED:s in this PR? Then it won't compile due to warnings such as:

addrman.cpp:70:44: error: reading variable 'mapAddr' requires holding mutex 'cs' [-Werror,-Wthread-safety-analysis]
addrman.cpp:71:15: error: reading variable 'mapAddr' requires holding mutex 'cs' [-Werror,-Wthread-safety-analysis]
addrman.cpp:75:46: error: reading variable 'mapInfo' requires holding mutex 'cs' [-Werror,-Wthread-safety-analysis]
[…]

What am I missing? :-)

@practicalswift right, sorry about last comment.

ACK 3e9f6c8.

Please update PR title and description.

@MarcoFalke Would you mind re-reviewing your previous utACK? :-)

utACK 3e9f6c821b