Add annotations to guard vRecvGetData and orphan_work_set and fix up places where they were accessed without a lock. There is no current data race because they happen to be accessed by only one thread, but this might not always be the case.
Original discussion: #18861 (review)
2882@@ -2880,7 +2883,10 @@ void PeerLogicValidation::ProcessMessage(CNode& pfrom, const std::string& msg_ty
2883 CInv inv;
2884 inv.type = State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK;
2885 inv.hash = req.blockhash;
2886- pfrom.vRecvGetData.push_back(inv);
2887+ {
2888+ LOCK(pfrom.cs_vRecv);
2889+ pfrom.vRecvGetData.push_back(inv);
2890+ }
Consider using the WITH_LOCK macro for simple one-line statements:
0 WITH_LOCK(pfrom.cs_vRecv, pfrom.vRecvGetData.push_back(inv));
3860@@ -3855,15 +3861,20 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
3861 //
3862 bool fMoreWork = false;
3863
3864- if (!pfrom->vRecvGetData.empty())
3865- ProcessGetData(*pfrom, chainparams, m_connman, m_mempool, interruptMsgProc);
3866+ {
3867+ LOCK(pfrom->cs_vRecv);
3868+ if (!pfrom->vRecvGetData.empty())
Concept ACK.
I’ve left a couple of style comments inline. Will review fully once this is rebased.
1721@@ -1722,7 +1722,7 @@ static CTransactionRef FindTxForGetData(const CTxMemPool& mempool, const CNode&
1722 return {};
1723 }
1724
1725-void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnman& connman, CTxMemPool& mempool, const std::atomic<bool>& interruptMsgProc) LOCKS_EXCLUDED(cs_main)
1726+void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnman& connman, CTxMemPool& mempool, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(pfrom.cs_vRecv) LOCKS_EXCLUDED(cs_main)
0static void ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnman& connman, CTxMemPool& mempool, const std::atomic<bool>& interruptMsgProc)
1 EXCLUSIVE_LOCKS_REQUIRED(!cs_main, pfrom.cs_vRecv)
Thanks @hebasto! TIL. It’s certainly shorter, but I’m worried people might miss the ! if reading quickly, and think this function requires cs_main.
Could you share why you suggest switching from void static to static void? Is it for general cleanup?
Thanks @hebasto! TIL. It’s certainly shorter, but I’m worried people might miss the
!if reading quickly, and think this function requirescs_main.
It is not about “shorter” :) See:
Could you share why you suggest switching from
void statictostatic void? Is it for general cleanup?
E.g., fa0572d0f3b083b4c8e2e883a66e2b198c6779f1 by @MarcoFalke
825@@ -825,7 +826,7 @@ class CNode
826
827 RecursiveMutex cs_sendProcessing;
828
829- std::deque<CInv> vRecvGetData;
830+ std::deque<CInv> vRecvGetData GUARDED_BY(cs_vRecv);
I don’t think it’s right to reuse this mutex to guard vRecvGetData (despite the name similarities). cs_vRecv was added in 321d0fc6b6624c65508f8b9059418cb936f0bbbe to guard nRecvBytes and mapRecvBytesPerMsgCmd, which are statistics used in the net layer. vRecvGetData is a data structure used in the application layer (and should eventually move to the Peer struct), so should be guarded by a different mutex.
I do have some commits that move both vRecvGetData and orphan_work_set to the Peer struct in https://github.com/jnewbery/bitcoin/commits/2020-06-cs-main-split-needs-rebase (everything from
END move tx data to net processing // START move work queue data to net processing to END move work queue data to net processing // START move subversion to net_processing). It’s slightly orthogonal to this PR, but perhaps we should just do that now while we’re touching these fields?
…
vRecvGetDatais a data structure used in the application layer (and should eventually move to thePeerstruct)…
Maybe start from moving, and then introduce a mutex?
I don’t think it’s right to reuse this mutex to guard
vRecvGetData(despite the name similarities).
I agree on not reusing of irrelevant mutex. To keep this PR focused, would it more correctly to introduce a new Mutex m_recvgetdata_mutex?
The thread sanitizer is complaining about locking order; I’m looking into it.
Could I suggest some code reorganization to keep locking order constant: https://github.com/hebasto/bitcoin/commits/pr19911-0908 ?
3866+ {
3867+ LOCK(pfrom->cs_vRecv);
3868+ if (!pfrom->vRecvGetData.empty()) return true;
3869+ }
3870+ {
3871+ LOCK2(cs_main, g_cs_orphans);
cs_main is required here.
2894-
2895- SendBlockTransactions(pfrom, block, req);
2896+ LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block > %i deep\n", pfrom.GetId(), MAX_BLOCKTXN_DEPTH);
2897+ inv.hash = req.blockhash;
2898+ WITH_LOCK(pfrom.cs_vRecv, pfrom.vRecvGetData.push_back(inv));
2899+ // The message processing loop will go around again (without pausing) and we'll respond then (without cs_main)
ProcessGetData() here, or not call it in the GETDATA processing for consistency. It seems from this comment that the only reason we weren’t calling ProcessGetData() here was that cs_main was being held, which is no longer the case.
I’ve pushed a new series of commits which move vRecvGetData and orphan_work_set to Peer and then guard them appropriately. It’s a bit more of a change, but happy to help move things over there if that’s the end goal.
I do have some commits that move both
vRecvGetDataandorphan_work_setto thePeerstruct in https://github.com/jnewbery/bitcoin/commits/2020-06-cs-main-split-needs-rebase (everything from END move tx data to net processing // START move work queue data to net processing to END move work queue data to net processing // START move subversion to net_processing). It’s slightly orthogonal to this PR, but perhaps we should just do that now while we’re touching these fields? @jnewbery I tried to cherry-pick some of your commits but they did not move cleanly given the scope of your branch’s change; I also didn’t do one of the renames you did to minimize the change. Let me know if there’s an appropriate way to credit you.
1729@@ -1722,11 +1730,11 @@ static CTransactionRef FindTxForGetData(const CTxMemPool& mempool, const CNode&
1730 return {};
1731 }
1732
1733-void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnman& connman, CTxMemPool& mempool, const std::atomic<bool>& interruptMsgProc) LOCKS_EXCLUDED(cs_main)
1734+void static ProcessGetData(CNode& pfrom, PeerRef peer, const CChainParams& chainparams, CConnman& connman, CTxMemPool& mempool, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(!cs_main, peer->cs_vRecvGetData)
PeerRef is a shared pointer, so by passing it by value here, you’re incrementing the ref count, and decrementing it again when you leave the function, which incurs a performance cost, since those operations need to be atomic.
This function doesn’t need to take shared ownership of the underlying Peer object, so you should pass by Peer& (see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r30-take-smart-pointers-as-parameters-only-to-explicitly-express-lifetime-semantics).
2341@@ -2334,6 +2342,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2342 return;
2343 }
2344
2345+ PeerRef peer = GetPeerRef(pfrom.GetId());
2346+ assert(peer);
Peer object here (as all other instances of GetPeerRef() except the FinalizeNode() call do) rather than assert.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
2986@@ -2984,6 +2987,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2987 }
2988
2989 std::list<CTransactionRef> lRemovedTxn;
2990+ PeerRef peer = GetPeerRef(pfrom.GetId());
In commit “Move m_orphan_work_set to net_processing”: I think you should put this call at the top of ProcessMessage() in this commit, rather than putting it here, and then moving it up in a later commit.
Alternatively, you could change the ProcessMessage() signature to take a Peer&. I don’t think that’s necessary, but you may prefer it to fetching the PeerRef in ProcessMessages() and then again in ProcessMessage().
GetPeerRef again, so will leave as is and not change the signature of ProcessMessage().
492+ std::set<uint256> m_orphan_work_set GUARDED_BY(g_cs_orphans);
493+
494+ /** Protects vRecvGetData **/
495+ Mutex cs_vRecvGetData;
496+ /** Work queue of items requested by this peer **/
497+ std::deque<CInv> vRecvGetData GUARDED_BY(cs_vRecvGetData);
vRecvGetData is on already, you may as well add a scripted-diff commit to this branch that updates the name to modern style guidelines, e.g. something like m_get_data_requests or similar. Same for the mutex name - it could be renamed to m_get_data_requests_mutex or similar.
cs_vRecvGetData be held before cs_main. The alternative would be to make cs_main be held before cs_vRecvGetData, which would mean you don’t need to change the logic in GETBLOCKTXN handling, but would need to change the internals of ProcessGetData(). I think both options are fine.
2897+ SendBlockTransactions(pfrom, block, req);
2898+ return;
2899+ }
2900
2901- if (pindex->nHeight < ::ChainActive().Height() - MAX_BLOCKTXN_DEPTH) {
2902 // If an older block is requested (should never happen in practice,
Rather than splitting this logic up (hoisting the CInv declaration above the code block, fetching the type inside the cs_main scope, and then putting the rest of the logic outside the cs_main scope), I recommend you keep all the logic together:
0--- a/src/net_processing.cpp
1+++ b/src/net_processing.cpp
2@@ -2872,8 +2872,6 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
3 return;
4 }
5
6- CInv inv;
7-
8 {
9 LOCK(cs_main);
10
11@@ -2892,17 +2890,18 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
12 return;
13 }
14
15- // If an older block is requested (should never happen in practice,
16- // but can happen in tests) send a block response instead of a
17- // blocktxn response. Sending a full block response instead of a
18- // small blocktxn response is preferable in the case where a peer
19- // might maliciously send lots of getblocktxn requests to trigger
20- // expensive disk reads, because it will require the peer to
21- // actually receive all the data read from disk over the network.
22- inv.type = State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK;
23 }
24
25+ // If an older block is requested (should never happen in practice,
26+ // but can happen in tests) send a block response instead of a
27+ // blocktxn response. Sending a full block response instead of a
28+ // small blocktxn response is preferable in the case where a peer
29+ // might maliciously send lots of getblocktxn requests to trigger
30+ // expensive disk reads, because it will require the peer to
31+ // actually receive all the data read from disk over the network.
32 LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block > %i deep\n", pfrom.GetId(), MAX_BLOCKTXN_DEPTH);
33+ CInv inv;
34+ inv.type = WITH_LOCK(cs_main, return State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK);
35 inv.hash = req.blockhash;
It’s slightly wasteful that cs_main is released and then taken again, but I think it’s ok because:
fWantsCmpctWitness should move to the Peer struct eventually, removing the need to take cs_main
Agreed, this is nicer! Changed.
Interestingly, your exact suggestion didn’t work: I had to move the assignment to inv.type inside the WITH_LOCK macro. I got the following error the other way:
0./sync.h:257:45: note: expanded from macro 'WITH_LOCK'
1#define WITH_LOCK(cs, code) [&] { LOCK(cs); code; }()
2 ^~~~
3net_processing.cpp:2934:20: error: assigning to 'uint32_t' (aka 'unsigned int') from incompatible type 'void'
4 inv.type = WITH_LOCK(cs_main, State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK);
5 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
6./sync.h:257:29: note: expanded from macro 'WITH_LOCK'
7#define WITH_LOCK(cs, code) [&] { LOCK(cs); code; }()
8 ^~~~~~~~~~~~~~~~~~~~~~~~~
It looks like you missed out the return from inside the WITH_LOCK macro’s second argument:
inv.type = WITH_LOCK(cs_main, return State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK);
But your way is also fine!
500@@ -501,6 +501,14 @@ struct Peer {
501 /** Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission). */
502 bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false};
503
504+ /** Set of txids to reconsider once their parent transactions have been accepted **/
505+ std::set<uint256> m_orphan_work_set GUARDED_BY(g_cs_orphans);
506+
507+ /** Protects m_getdata_requests **/
508+ Mutex m_getdata_requests_mutex;
cs_vRecvGetData in one commit and then change it with a scripted diff in the subsequent commit. You can just name it m_getdata_requests_mutex in the first commit!
This helps distinguish the member from any local variables.
In commit Move m_orphan_work_set to net_processing, you’re not removing CNode::m_orphan_work_set.
Other than that, looks good!
This requires slightly reorganizing the logic in GETBLOCKTXN to
maintain locking order.
-BEGIN VERIFY SCRIPT-
sed -i 's/vRecvGetData/m_getdata_requests/g' src/net_processing.cpp
-END VERIFY SCRIPT-
In commit Move m_orphan_work_set to net_processing, you’re not removing CNode::m_orphan_work_set.
Other than that, looks good!
Good catch, I lost that in the last rebase! Fixed.
511@@ -512,6 +512,9 @@ struct Peer {
512 /** Whether this peer should be disconnected and marked as discouraged (unless it has the noban permission). */
513 bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false};
514
515+ /** Set of txids to reconsider once their parent transactions have been accepted **/
8803aee66813d27ddbdfce937ab9c35f8f7c35bc
To describe a member //! is recommended:
0 //! Set of txids to reconsider once their parent transactions have been accepted.
/** being used for member comments throughout the codebase. We should update the developer docs to say both styles are allowed rather than try to change all comments to fit to one style.
2365@@ -2363,6 +2366,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2366 return;
2367 }
2368
2369+ PeerRef peer = GetPeerRef(pfrom.GetId());
2370+ if (peer == nullptr) return;
8803aee66813d27ddbdfce937ab9c35f8f7c35bc, nit:
0 if (!peer) return;
nullptr explicitly everywhere in the code?
3869@@ -3865,12 +3870,15 @@ bool PeerManager::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgP
3870 {
3871 bool fMoreWork = false;
3872
3873+ PeerRef peer = GetPeerRef(pfrom->GetId());
3874+ if (peer == nullptr) return false;
8803aee66813d27ddbdfce937ab9c35f8f7c35bc, nit:
0 if (!peer) return false;
512@@ -513,7 +513,7 @@ struct Peer {
513 bool m_should_discourage GUARDED_BY(m_misbehavior_mutex){false};
514
515 /** Set of txids to reconsider once their parent transactions have been accepted **/
516- std::set<uint256> m_orphan_work_set;
517+ std::set<uint256> m_orphan_work_set GUARDED_BY(g_cs_orphans);
673247b58cd1252ab7e99f7d63ead05cc100cef2, nit:
0 std::set<uint256> m_orphan_work_set GUARDED_BY(::g_cs_orphans);
g_ making it very unlikely to have a conflicting local variable 2) I don’t see anywhere else in the codebase using this operator for globals.
git grep ::cs or https://github.com/bitcoin/bitcoin/blob/f2e6d14430137a271d153348d207df6ab8086bc6/src/init.cpp#L216
Based on my understanding, it seems like a style preference.
Correct. Feel free to ignore this nit.
3888@@ -3887,7 +3889,10 @@ bool PeerManager::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgP
3889 // this maintains the order of responses
3890 // and prevents vRecvGetData to grow unbounded
3891 if (!pfrom->vRecvGetData.empty()) return true;
3892- if (!peer->m_orphan_work_set.empty()) return true;
3893+ {
3894+ LOCK(g_cs_orphans);
673247b58cd1252ab7e99f7d63ead05cc100cef2, nit:
0 LOCK(::g_cs_orphans);
514@@ -515,6 +515,9 @@ struct Peer {
515 /** Set of txids to reconsider once their parent transactions have been accepted **/
516 std::set<uint256> m_orphan_work_set GUARDED_BY(g_cs_orphans);
517
518+ /** Work queue of items requested by this peer **/
2d9f2fca43aadcdda4d644cddab36dca88b40b97
To describe a member //! is recommended:
0 //! Work queue of items requested by this peer.
1756@@ -1754,7 +1757,10 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
1757 {
1758 AssertLockNotHeld(cs_main);
1759
1760- std::deque<CInv>::iterator it = pfrom.vRecvGetData.begin();
1761+ PeerRef peer = GetPeerRef(pfrom.GetId());
1762+ if (peer == nullptr) return;
2d9f2fca43aadcdda4d644cddab36dca88b40b97, nit:
0 if (!peer) return;
1756@@ -1754,7 +1757,10 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
1757 {
1758 AssertLockNotHeld(cs_main);
1759
1760- std::deque<CInv>::iterator it = pfrom.vRecvGetData.begin();
1761+ PeerRef peer = GetPeerRef(pfrom.GetId());
1762+ if (peer == nullptr) return;
1763+
1764+ std::deque<CInv>::iterator it = peer->vRecvGetData.begin();
2d9f2fca43aadcdda4d644cddab36dca88b40b97, nit:
0 auto it = peer->vRecvGetData.begin();
auto usage in our style guide. I personally think auto should only be used if the type is immediately obvious from the surrounding code, and it’s saving redundant and verbose keystrokes.
We don’t have guidance about
autousage in our style guide.
Yes. And I think we should have it.
I personally think
autoshould only be used if the type is immediately obvious from the surrounding code, and it’s saving redundant and verbose keystrokes.
https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/
Thanks @hebasto. I hadn’t seen that particular blog post before, but I am familiar with the arguments for and against auto. Scott Meyers also dedicates a chapter to it in Exceptional Modern C++.
Here, I think it’s useful to explicitly name the type, since CInv is part of the interface. Sure, we could switch out std::deque for another container at some point, but to know what it->IsGenTxMsg() is doing a few lines further down, you need to know that it is an iterator over some container of CInvs. I’d much rather the code told me that explicitly than having to go find out what m_getdata_requests is from somewhere else.
3927@@ -3921,7 +3928,7 @@ bool PeerManager::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgP
3928 ProcessMessage(*pfrom, msg_type, msg.m_recv, msg.m_time, interruptMsgProc);
3929 if (interruptMsgProc)
3930 return false;
3931- if (!pfrom->vRecvGetData.empty())
3932+ if (!peer->vRecvGetData.empty())
3933 fMoreWork = true;
2d9f2fca43aadcdda4d644cddab36dca88b40b97
nit: add braces or make one line?
514@@ -515,8 +515,10 @@ struct Peer {
515 /** Set of txids to reconsider once their parent transactions have been accepted **/
516 std::set<uint256> m_orphan_work_set GUARDED_BY(g_cs_orphans);
517
518+ /** Protects vRecvGetData **/
ba951812ec0cc8ebee5911a582f188525b76ff0a
To describe a member //! is recommended:
0 //! Protects vRecvGetData.
2955+ // might maliciously send lots of getblocktxn requests to trigger
2956+ // expensive disk reads, because it will require the peer to
2957+ // actually receive all the data read from disk over the network.
2958+ LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block > %i deep\n", pfrom.GetId(), MAX_BLOCKTXN_DEPTH);
2959+ CInv inv;
2960+ WITH_LOCK(cs_main, inv.type = State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK);
I think this could change behavior as between two consequent ::cs_main holding the guarded state could be changed.
Maybe just place State call into the guarded block above?
Approach ACK da0988daf1d665a4644ad3f1ddf3f8a8bdd88cde.
Mind making the first rename commit a scripted-diff as well?
Mind making the first rename commit a scripted-diff as well?
It can’t be, since orphan_work_set is currently used as the name for both a member variable and a local variable.
Mind making the first rename commit a scripted-diff as well?
It can’t be, since
orphan_work_setis currently used as the name for both a member variable and a local variable.
Indeed. Sorry.
I think all comments have been addressed except the remaining review comments which I can summarize to the following nits:
if (peer == nullptr) to if (!peer) in the places where I add a new nullptr checkI don’t think these are important, and I already have one ACK on the code so I’d prefer not to change them. However, if others disagree, I can do so.
ACK da0988daf1d665a4644ad3f1ddf3f8a8bdd88cde, I have reviewed the code and it looks correct, I agree it can be merged.
My only concern was resolved by @jnewbery. My other comments are non-blocking nits obviously.
I don’t think these are important, and I already have one ACK on the code so I’d prefer not to change them. However, if others disagree, I can do so.
Now you’ve got two of them :smiley:
review ACK da0988daf1d665a4644ad3f1ddf3f8a8bdd88cde 🐬
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK da0988daf1d665a4644ad3f1ddf3f8a8bdd88cde 🐬
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUh3Kwv/Z92YkezB0vKDt8zCSK2kekyViu2esvi5EQIv7t59sPpc1lksTFvC75lX
8rmr8hVvOHb6l1pMTGEXLJHVgamn4B/xd5jnJCEGcI9GR1bamL6wRlSKLZEyhnVjV
9Siyp7qPFIQPTQxUsUhgJxeLqqYWV1TLmxRvekBnwtfksqpHTBR6icrBsQpg7Nu7Q
10f9uiggfUZq3UrnyzF9dJqYUG5CyGiymnRH9YUE89W+7cg/L7RllSQh6K5lrbeb9b
11WwAbJL64iDR3Npv4d9bwIQkZNgadyZA8yYw/kskriD9Gt+Qxg7V0VnBbExhve3mp
12vc2i6mpPqgDS0n4a+tv3pB7g4zvv9w8kLNt4wXiBIETVdbfH5acdDw3GvTOAhQBl
13Nwu0KhCxCkXtWDP56vJo2D0V8WsaHMQv53DUAjsAKHot7c0ihXd+V3zhwUHhJXEP
14d1BHU3W+II9DB0c2e9gXyiS/ZHWbEhzwvtdj2pVAtETJG6dt8gM9aEmdzh7Lnfga
15QWbax8bo
16=//W2
17-----END PGP SIGNATURE-----
Timestamp of file with hash 350e455a9cfde318c3ab04c709fac25fca0a0b8205b4cfd5c3a0765e95f4dff4 -