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

practicalswift · 2018-04-29T19:06:59Z

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

DrahtBot · 2018-07-22T23:49:43Z

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.

practicalswift · 2018-08-13T12:11:47Z

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

maflcko · 2018-08-13T13:21:14Z

utACK 4f04bda90e113cd596b019c05d6a5cb3ac4be540

practicalswift · 2018-10-08T12:36:41Z

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

promag · 2018-10-08T13:45:11Z

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

practicalswift · 2018-10-08T13:51:24Z

@promag Fixed. Please re-review :-)

promag · 2018-10-08T14:51:07Z

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'

promag · 2018-10-08T14:53:50Z

~~utACK 116e77b~~.

nits:

commit messages could mention CAddrMan.
could squash.

practicalswift · 2018-10-08T15:20:09Z

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

promag · 2018-10-08T15:26:57Z

@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;

practicalswift · 2018-10-08T15:39:47Z

@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? :-)

promag · 2018-10-08T17:06:28Z

@practicalswift right, sorry about last comment.

ACK 3e9f6c8.

Please update PR title and description.

practicalswift · 2018-10-08T21:26:08Z

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

maflcko · 2018-10-09T03:54:05Z

utACK 3e9f6c8

…les guarded by CAddrMan.cs 3e9f6c8 Add missing locks and locking annotations for CAddrMan (practicalswift) Pull request description: * Add Clang thread safety annotations for variables guarded by `CAddrMan.cs ` * Add missing `CAddrMan.cs ` locks Tree-SHA512: c78d56d56eb63a4469333c04c95317545a8f97d5e3a36ff2699ee4a91a6433d416221eed6c5ff168e1e31f6936c2ae101a4c60b635f2b2309f40e3d66a727322

…eler connections to existing outbound netgroups ccb6a4a [addrman] Improve collision logging and address nits (Suhas Daftuar) aa3c805 [addrman] Ensure collisions eventually get resolved (Suhas Daftuar) efd1d6a [net] feeler connections can be made to outbound peers in same netgroup (Suhas Daftuar) afcffc4 [addrman] Improve tried table collision logging (Suhas Daftuar) Pull request description: Two pretty direct back ports for the address manager. Coming from bitcoin#13115 and bitcoin#15486. ACKs for top commit: random-zebra: rebase utACK ccb6a4a, and merging... Tree-SHA512: 89d42b3164f92fcee69bc981c7e0887cab09820088732297dbcadbf4ccfc3257e9564ef2b394205f3153c8a03e4c0a0e8145b1c291e8cd980dc06b0ffa65a28c

zcash: cherry picked from commit 3e9f6c8 zcash: bitcoin/bitcoin#13115

… variables guarded by CAddrMan.cs 3e9f6c8 Add missing locks and locking annotations for CAddrMan (practicalswift) Pull request description: * Add Clang thread safety annotations for variables guarded by `CAddrMan.cs ` * Add missing `CAddrMan.cs ` locks Tree-SHA512: c78d56d56eb63a4469333c04c95317545a8f97d5e3a36ff2699ee4a91a6433d416221eed6c5ff168e1e31f6936c2ae101a4c60b635f2b2309f40e3d66a727322

laanwj added the Refactoring label Apr 30, 2018

practicalswift changed the title ~~Add Clang thread safety annotations for variables guarded by cs_addrMan~~ addrman: Add Clang thread safety annotations for variables guarded by cs_addrMan Apr 30, 2018

practicalswift mentioned this pull request Apr 30, 2018

Meta-issue: Add Clang thread safety analysis annotations #13129

Closed

30 tasks

DrahtBot closed this Jul 22, 2018

DrahtBot reopened this Jul 22, 2018

practicalswift mentioned this pull request Aug 31, 2018

wallet: Remove unused local variable old_label #14117

Closed

DrahtBot mentioned this pull request Sep 12, 2018

p2p: Return a max of 1000 addrs from CAddrMan::GetAddr #14154

Closed

practicalswift force-pushed the guarded-by-cs_addrMan branch from 4f04bda to 0179bc0 Compare October 8, 2018 13:50

practicalswift force-pushed the guarded-by-cs_addrMan branch 2 times, most recently from e06b3fc to d1785a5 Compare October 8, 2018 13:56

Add missing locks and locking annotations for CAddrMan

3e9f6c8

practicalswift force-pushed the guarded-by-cs_addrMan branch from d1785a5 to 3e9f6c8 Compare October 8, 2018 15:19

practicalswift changed the title ~~addrman: Add Clang thread safety annotations for variables guarded by cs_addrMan~~ addrman: Add Clang thread safety annotations for variables guarded by CAddrMan.cs Oct 8, 2018

maflcko merged commit 3e9f6c8 into bitcoin:master Oct 9, 2018

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

UdjinM6 mentioned this pull request Mar 18, 2021

merge #17812, #16702, #16730, #18023: supplying and using asmap to improve IP bucketing in addrman dashpay/dash#4028

Merged

furszy mentioned this pull request Mar 22, 2021

[Backport] Ensure tried collisions resolve, and allow feeler connections to existing outbound netgroups PIVX-Project/PIVX#2257

Merged

LarryRuane mentioned this pull request Apr 5, 2021

Bitcoin 0.18 locking PRs zcash/zcash#5050

Open

practicalswift deleted the guarded-by-cs_addrMan branch April 10, 2021 19:36

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 29, 2021

Add missing locks and locking annotations for CAddrMan

e96d8fd

zcash: cherry picked from commit 3e9f6c8 zcash: bitcoin/bitcoin#13115

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Jun 1, 2021

Add missing locks and locking annotations for CAddrMan

7df9aef

zcash: cherry picked from commit 3e9f6c8 zcash: bitcoin/bitcoin#13115

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

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

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

Uh oh!

Conversation

practicalswift commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 22, 2018

Uh oh!

practicalswift commented Aug 13, 2018

Uh oh!

maflcko commented Aug 13, 2018

Uh oh!

practicalswift commented Oct 8, 2018

Uh oh!

promag commented Oct 8, 2018

Uh oh!

practicalswift commented Oct 8, 2018

Uh oh!

promag commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 8, 2018

Uh oh!

promag commented Oct 8, 2018

Uh oh!

practicalswift commented Oct 8, 2018

Uh oh!

promag commented Oct 8, 2018

Uh oh!

practicalswift commented Oct 8, 2018

Uh oh!

maflcko commented Oct 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

practicalswift commented Apr 29, 2018 •

edited

Loading

promag commented Oct 8, 2018 •

edited

Loading

promag commented Oct 8, 2018 •

edited

Loading