[net] Thread safety annotations in net_processing #13423

skeees · 2018-06-08T18:12:07Z

(note that this depends on #13417)
This commit fully annotates all globals in net_processing with clang thread safety annotations. Subsequent commits will begin transitioning some of this data away from cs_main into locks that are local to net_processing. Static thread safety analysis should it easier to verify correctness of that process.

promag · 2018-06-10T21:12:57Z

Concept ACK.

Well done in 4c0b978, improves thread safety analysis in a lot of places.

jnewbery · 2018-06-14T17:11:33Z

utACK 00c1bae9091447229cd09b5146ee25a357f592fe (although I'm not an expert on cs_main)

skeees · 2018-06-19T17:29:57Z

@DrahtBot - rebased

DrahtBot · 2018-06-19T22:46:45Z

Note to reviewers: This pull request conflicts with the following ones:

Use explicit captures in lambda expressions #13770 (Use explicit captures in lambda expressions by practicalswift)
Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection #11640 (Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection by ryanofsky)
scripted-diff: Small locking rename #11599 (scripted-diff: Small locking rename by ryanofsky)

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.

jnewbery · 2018-06-21T17:58:28Z

reACK af288e3f09f0724390334d9902742883a9db388e

…analysis

skeees · 2018-07-25T17:47:40Z

rebased now that #13417 has been merged

maflcko · 2018-07-25T18:09:51Z

src/net_processing.cpp


    /** Number of outbound peers with m_chain_sync.m_protect. */
-    int g_outbound_peers_with_protect_from_disconnect = 0;
+    int g_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0;

    /** When our tip was last updated. */
    std::atomic<int64_t> g_last_tip_update(0);

    /** Relay map, protected by cs_main. */


Can remove the now redundant mention of cs_main here?

maflcko · 2018-07-25T18:09:58Z

src/net_processing.cpp


    /** When our tip was last updated. */
    std::atomic<int64_t> g_last_tip_update(0);

    /** Relay map, protected by cs_main. */
    typedef std::map<uint256, CTransactionRef> MapRelay;
-    MapRelay mapRelay;
+    MapRelay mapRelay GUARDED_BY(cs_main);
    /** Expiration-time ordered list of (expire time, relay map entry) pairs, protected by cs_main). */


Same here (and everywhere else)

maflcko · 2018-07-26T17:35:34Z

src/net_processing.cpp

@@ -76,7 +76,7 @@ std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);
 void EraseOrphansFor(NodeId peer);

 /** Increase a node's misbehavior score. */
-void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="");
+void Misbehaving(NodeId nodeid, int howmuch, const std::string& message="") EXCLUSIVE_LOCKS_REQUIRED(cs_main);


nit: should prefix this with static to make sure the annotation is properly added where this function is declared.

sorry - don't quite understand - this is also used in src/test/denialofservice_tests.cpp
wouldn't declaring static break that?

Ah, I see. Thanks for the clarification. I only built bitcoind to check my patch.

1e3bcd2 [net_processing] Add thread safety annotations (Jesse Cohen) f393a53 Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis (Jesse Cohen) Pull request description: (note that this depends on bitcoin#13417) This commit fully annotates all globals in net_processing with clang thread safety annotations. Subsequent commits will begin transitioning some of this data away from cs_main into locks that are local to net_processing. Static thread safety analysis should it easier to verify correctness of that process. Tree-SHA512: b47aa410cb9ada21072370176aea9a74c575643fa1ee8cf1d43c8e28675eef17f33e5242ac422f840e8178e132ecb58412034c6334b68f1b57c686df80d4e8e2

…analysis zcash: cherry picked from commit f393a53 zcash: bitcoin/bitcoin#13423

zcash: cherry picked from commit 1e3bcd2 zcash: bitcoin/bitcoin#13423

…analysis zcash: cherry picked from commit f393a53 zcash: bitcoin/bitcoin#13423

zcash: cherry picked from commit 1e3bcd2 zcash: bitcoin/bitcoin#13423

skeees force-pushed the net_processing-thread-annotations branch from 8bea6e5 to 7bf1391 Compare June 8, 2018 19:35

fanquake added P2P Refactoring labels Jun 9, 2018

skeees force-pushed the net_processing-thread-annotations branch 5 times, most recently from 3002c61 to 00c1bae Compare June 10, 2018 13:03

DrahtBot added the Needs rebase label Jun 12, 2018

skeees force-pushed the net_processing-thread-annotations branch from 00c1bae to af288e3 Compare June 19, 2018 17:28

DrahtBot removed the Needs rebase label Jun 19, 2018

This was referenced Jun 19, 2018

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

Closed

DrahtBot mentioned this pull request Jun 26, 2018

[net] Tighten scope in net_processing #13417

Merged

DrahtBot added the Needs rebase label Jul 14, 2018

Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety …

f393a53

…analysis

skeees force-pushed the net_processing-thread-annotations branch from af288e3 to 4d3732d Compare July 25, 2018 14:46

fanquake removed the Needs rebase label Jul 25, 2018

skeees force-pushed the net_processing-thread-annotations branch from 4d3732d to 6a2e1ee Compare July 25, 2018 15:30

maflcko added the Needs rebase label Jul 25, 2018

maflcko reviewed Jul 25, 2018

View reviewed changes

DrahtBot removed the Needs rebase label Jul 25, 2018

[net_processing] Add thread safety annotations

1e3bcd2

maflcko reviewed Jul 26, 2018

View reviewed changes

ryanofsky mentioned this pull request Aug 7, 2018

Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection #11640

Merged

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

ryanofsky mentioned this pull request Aug 31, 2018

Add AssertLockHeld assertions in CWallet::ListCoins #10605

Merged

sickpig mentioned this pull request Mar 19, 2020

Add missing locks and fix false positive clang warnings - part 3 BitcoinUnlimited/BitcoinUnlimited#2126

Merged

hebasto mentioned this pull request Apr 14, 2020

Replace -Wthread-safety-analysis with broader -Wthread-safety #18635

Merged

ryanofsky mentioned this pull request Sep 14, 2020

scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865

Closed

LarryRuane mentioned this pull request Mar 26, 2021

Bitcoin 0.17 locking PRs zcash/zcash#5034

Open

str4d mentioned this pull request Apr 12, 2021

Sync backports zcash/zcash#5057

Merged

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

Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety …

1606c61

…analysis zcash: cherry picked from commit f393a53 zcash: bitcoin/bitcoin#13423

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

[net_processing] Add thread safety annotations

b03729d

zcash: cherry picked from commit 1e3bcd2 zcash: bitcoin/bitcoin#13423

volbil mentioned this pull request Apr 29, 2021

Fix edge case race condition RapidsOfficial/Rapids#41

Merged

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety …

61f63df

…analysis zcash: cherry picked from commit f393a53 zcash: bitcoin/bitcoin#13423

LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request May 27, 2021

[net_processing] Add thread safety annotations

b23bc94

zcash: cherry picked from commit 1e3bcd2 zcash: bitcoin/bitcoin#13423

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[net] Thread safety annotations in net_processing #13423

[net] Thread safety annotations in net_processing #13423

Uh oh!

skeees commented Jun 8, 2018

Uh oh!

promag commented Jun 10, 2018

Uh oh!

jnewbery commented Jun 14, 2018

Uh oh!

skeees commented Jun 19, 2018

Uh oh!

DrahtBot commented Jun 19, 2018 •

edited

Loading

Uh oh!

jnewbery commented Jun 21, 2018

Uh oh!

skeees commented Jul 25, 2018

Uh oh!

maflcko Jul 25, 2018

Uh oh!

maflcko Jul 25, 2018 •

edited

Loading

Uh oh!

maflcko Jul 26, 2018

Uh oh!

skeees Jul 26, 2018

Uh oh!

maflcko Jul 26, 2018

Uh oh!

Uh oh!

[net] Thread safety annotations in net_processing #13423

[net] Thread safety annotations in net_processing #13423

Uh oh!

Conversation

skeees commented Jun 8, 2018

Uh oh!

promag commented Jun 10, 2018

Uh oh!

jnewbery commented Jun 14, 2018

Uh oh!

skeees commented Jun 19, 2018

Uh oh!

DrahtBot commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jun 21, 2018

Uh oh!

skeees commented Jul 25, 2018

Uh oh!

maflcko Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

skeees Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jun 19, 2018 •

edited

Loading

maflcko Jul 25, 2018 •

edited

Loading