net: Add Clang thread safety annotations for guarded variables in the networking code #13123

practicalswift · 2018-04-30T08:47:36Z

Add Clang thread safety annotations for variables guarded by:

cs_addrLocal
cs_addrName
cs_feeFilter
cs_filter
cs_hSocket
cs_inventory
cs_mapLocalHost
cs_most_recent_block
cs_proxyInfos
cs_sendProcessing
cs_setBanned
cs_SubVer
cs_vOneShots
cs_vProcessMsg
cs_vRecv
cs_vSend

Changed files:

src/net.{cpp,h}
src/netbase.cpp

practicalswift · 2018-07-11T23:26:58Z

Rebased!

maflcko · 2018-08-12T22:05:05Z

Imo could squash everything, since most of the commits only touch 2 lines.

practicalswift · 2018-08-13T07:39:30Z

@MarcoFalke Rebased and squashed! Please re-review :-)

practicalswift · 2018-08-30T13:37:28Z

Rebased and added missing lock:

diff --git a/src/net.cpp b/src/net.cpp
index 529a2f82f..7fcf6a3ef 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -715,7 +715,10 @@ void CNode::copyStats(CNodeStats &stats)
         X(nRecvBytes);
     }
     X(fWhitelisted);
-    X(minFeeFilter);
+    {
+        LOCK(cs_feeFilter);
+        X(minFeeFilter);
+    }

     // It is common for nodes with good ping times to suddenly become lagged,
     // due to a new block arriving or other large transfer.

src/net.h

DrahtBot · 2018-09-20T22:37:57Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14605 (Return of the Banman by dongcarl)
#14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)

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.

practicalswift · 2018-10-08T13:30:57Z

@MarcoFalke @Empact Updated. Please re-review :-)

practicalswift · 2020-06-22T13:10:27Z

@jnewbery I agree that sure looks like an error! Good catch!

jnewbery · 2020-06-22T13:19:30Z

Thanks for the quick reply. I have a PR moving some of those fields around, so I'll get rid of the lock annotations there.

practicalswift · 2020-06-22T14:18:48Z

@jnewbery Thanks a lot for fixing (and finding)! Much appreciated!

jnewbery · 2020-06-22T14:44:45Z

@practicalswift sorry I meant to say that I have a branch that moves the fields around that I haven't PR'ed yet. I'll ping you when the PR is open.

See bitcoin#13123 (comment)

This locking was mistakenly introduced in PR bitcoin#13123. Related conversation: bitcoin#13123 (comment) Making these fields atomic would ensure safety if multiple RPC accesses them.

…_local_addr_send This locking was mistakenly introduced in PR bitcoin#13123. Related conversation: bitcoin#13123 (comment) Making these fields atomic would ensure safety if multiple RPC accesses them.

m_next_addr_send and m_next_local_addr_send do not need to be guarded by cs_sendProcessing. These fields are only read/writen by the message handling thread and the annotation was added unnecessarily in commit b312cd7. See discussion at bitcoin#13123 (comment). Therefore remove this unnecessary lock annotation.

jnewbery · 2021-02-28T11:53:47Z

@practicalswift sorry I meant to say that I have a branch that moves the fields around that I haven't PR'ed yet. I'll ping you when the PR is open.

PR is open here: #21236. It's a pure refactor and should be easy to review.

m_next_addr_send and m_next_local_addr_send are currently guarded by cs_sendProcessing. These fields are only read/writen by the message handling thread and the annotation was added unnecessarily in commit b312cd7. See discussion at bitcoin#13123 (comment). We add a new m_addr_mutex to guard these fields. Once all the addr relay fields have been moved to the Peer object in net_processing, they will be protected by the same mutex.

zcash: note: more work will be needed to the overall code base in order zcash: for annotations to compile successfully. zcash: cherry picked from commit b312cd7 zcash: bitcoin/bitcoin#13123

zcash: cherry picked from commit 4894133 zcash: bitcoin/bitcoin#13123

zcash: note: more work will be needed to the overall code base in order zcash: for annotations to compile successfully. zcash: cherry picked from commit b312cd7 zcash: bitcoin/bitcoin#13123

zcash: cherry picked from commit 4894133 zcash: bitcoin/bitcoin#13123

…rded variables in the networking code 4894133 Add missing lock in CNode::copyStats(...) (practicalswift) b312cd7 Add missing locking annotations (practicalswift) Pull request description: Add Clang thread safety annotations for variables guarded by: * `cs_addrLocal` * `cs_addrName` * `cs_feeFilter` * `cs_filter` * `cs_hSocket` * `cs_inventory` * `cs_mapLocalHost` * `cs_most_recent_block` * `cs_proxyInfos` * `cs_sendProcessing` * `cs_setBanned` * `cs_SubVer` * `cs_vOneShots` * `cs_vProcessMsg` * `cs_vRecv` * `cs_vSend` Changed files: * `src/net.{cpp,h}` * `src/netbase.cpp` Tree-SHA512: 319a1574a07d766e81fab19b9cfdcf8b5f0b175034ebef220cd406f1672b4ef2c57f5c456c623456ca7a1f96308de69c73535792e9e4c34b848b55fd4f35fc95

practicalswift changed the title ~~Add Clang thread safety annotations for guarded variables in the networking code~~ net: Add Clang thread safety annotations for guarded variables in the networking code Apr 30, 2018

fanquake added Refactoring P2P labels Apr 30, 2018

practicalswift mentioned this pull request Apr 30, 2018

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

Closed

30 tasks

DrahtBot mentioned this pull request Jun 7, 2018

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

Closed

DrahtBot added the Needs rebase label Jul 9, 2018

practicalswift force-pushed the net-guarded-by branch from ec9d07a to 0287117 Compare July 11, 2018 23:26

DrahtBot removed the Needs rebase label Jul 12, 2018

DrahtBot mentioned this pull request Aug 2, 2018

Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. #13766

Closed

DrahtBot mentioned this pull request Aug 12, 2018

Dandelion transaction relay (BIP 156) #13947

Closed

practicalswift force-pushed the net-guarded-by branch from 0287117 to 507867a Compare August 13, 2018 07:38

This was referenced Aug 23, 2018

Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032

Closed

net: Refactor message parsing (CNetMessage), adds flexibility #14046

Closed

practicalswift force-pushed the net-guarded-by branch from 507867a to 36ecc9e Compare August 30, 2018 13:37

practicalswift mentioned this pull request Aug 31, 2018

wallet: Remove unused local variable old_label #14117

Closed

Empact reviewed Sep 2, 2018

View reviewed changes

src/net.h Show resolved Hide resolved

Empact reviewed Sep 2, 2018

View reviewed changes

src/net.h Show resolved Hide resolved

Empact reviewed Sep 2, 2018

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

practicalswift force-pushed the net-guarded-by branch from 36ecc9e to 9d944ef Compare October 8, 2018 13:28

practicalswift force-pushed the net-guarded-by branch 5 times, most recently from 09d46b5 to d4dd3f9 Compare October 8, 2018 13:55

jnewbery mentioned this pull request Oct 6, 2020

Refactoring and minor improvement for self-advertisements #19843

Closed

naumenkogs added a commit to naumenkogs/bitcoin that referenced this pull request Oct 6, 2020

Drop unnecessary lock

ed5c6de

See bitcoin#13123 (comment)

naumenkogs added a commit to naumenkogs/bitcoin that referenced this pull request Oct 6, 2020

Drop unnecessary lock

e8da279

See bitcoin#13123 (comment)

naumenkogs added a commit to naumenkogs/bitcoin that referenced this pull request Oct 7, 2020

Drop unnecessary lock

c3ad35e

See bitcoin#13123 (comment)

jnewbery mentioned this pull request Feb 28, 2021

net processing: Extract addr send functionality into MaybeSendAddr() #21236

Merged

LarryRuane mentioned this pull request Apr 5, 2021

Bitcoin 0.18 locking PRs zcash/zcash#5050

Open

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

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

Add missing lock in CNode::copyStats(...)

98f7b77

zcash: cherry picked from commit 4894133 zcash: bitcoin/bitcoin#13123

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

Add missing lock in CNode::copyStats(...)

bcf71e9

zcash: cherry picked from commit 4894133 zcash: bitcoin/bitcoin#13123

jnewbery mentioned this pull request Aug 17, 2021

net/net processing: Move tx inventory into net_processing #21160

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: Add Clang thread safety annotations for guarded variables in the networking code #13123

net: Add Clang thread safety annotations for guarded variables in the networking code #13123

practicalswift commented Apr 30, 2018 •

edited

Loading

practicalswift commented Jul 11, 2018

maflcko commented Aug 12, 2018

practicalswift commented Aug 13, 2018

practicalswift commented Aug 30, 2018

DrahtBot commented Sep 20, 2018 •

edited

Loading

practicalswift commented Oct 8, 2018

practicalswift commented Jun 22, 2020

jnewbery commented Jun 22, 2020

practicalswift commented Jun 22, 2020

jnewbery commented Jun 22, 2020

jnewbery commented Feb 28, 2021

net: Add Clang thread safety annotations for guarded variables in the networking code #13123

net: Add Clang thread safety annotations for guarded variables in the networking code #13123

Conversation

practicalswift commented Apr 30, 2018 • edited Loading

practicalswift commented Jul 11, 2018

maflcko commented Aug 12, 2018

practicalswift commented Aug 13, 2018

practicalswift commented Aug 30, 2018

DrahtBot commented Sep 20, 2018 • edited Loading

Conflicts

practicalswift commented Oct 8, 2018

practicalswift commented Jun 22, 2020

jnewbery commented Jun 22, 2020

practicalswift commented Jun 22, 2020

jnewbery commented Jun 22, 2020

jnewbery commented Feb 28, 2021

practicalswift commented Apr 30, 2018 •

edited

Loading

DrahtBot commented Sep 20, 2018 •

edited

Loading