net: guard vRecvGetData with cs_vRecv and orphan_work_set with g_cs_orphans #19911

Consider using the WITH_LOCK macro for simple one-line statements:

            WITH_LOCK(pfrom.cs_vRecv, pfrom.vRecvGetData.push_back(inv));

done!

style: consider adding braces to if blocks when touching code, to conform with the current style guide.

done! did not do so to adjacent code i wasn't touching.

static void ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnman& connman, CTxMemPool& mempool, const std::atomic<bool>& interruptMsgProc)
    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 requires cs_main.

It is not about "shorter" :) See:

• https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#excludes
• https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative-capabilities

Could you share why you suggest switching from void static to static void? Is it for general cleanup?

E.g., fa0572d0f3b083b4c8e2e883a66e2b198c6779f1 by @MarcoFalke

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?

... vRecvGetData is a data structure used in the application layer (and should eventually move to the Peer struct)...

Maybe start from moving, and then introduce a mutex?

Maybe start from moving, and then introduce a mutex?

I think that's probably the cleanest order, but I don't want to scope-creep @narula's PR if she thinks it's better to add the mutex first. I can do the move later.

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?

I agree on a new mutex; I was wondering about that but assumed @sipa had some reason for suggesting using cs_vRecv to guard it in the first place.

I don't think cs_main is required here.

right!

This would be a slight sequencing change, but perhaps we should either call 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.

Would this be a behavior change?

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).

thanks for the pointer! fixed.

I think it's probably safer to just return if you can't find the Peer object here (as all other instances of GetPeerRef() except the FinalizeNode() call do) rather than assert.

addressed.

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().

done. I am fine with calling GetPeerRef again, so will leave as is and not change the signature of ProcessMessage().

Since you're touching every line that 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.

done

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:

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2872,8 +2872,6 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
             return;
         }
 
-        CInv inv;
-
         {
             LOCK(cs_main);
 
@@ -2892,17 +2890,18 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
                 return;
             }
 
-            // If an older block is requested (should never happen in practice,
-            // but can happen in tests) send a block response instead of a
-            // blocktxn response. Sending a full block response instead of a
-            // small blocktxn response is preferable in the case where a peer
-            // might maliciously send lots of getblocktxn requests to trigger
-            // expensive disk reads, because it will require the peer to
-            // actually receive all the data read from disk over the network.
-            inv.type = State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK;
         }
 
+        // If an older block is requested (should never happen in practice,
+        // but can happen in tests) send a block response instead of a
+        // blocktxn response. Sending a full block response instead of a
+        // small blocktxn response is preferable in the case where a peer
+        // might maliciously send lots of getblocktxn requests to trigger
+        // expensive disk reads, because it will require the peer to
+        // actually receive all the data read from disk over the network.
         LogPrint(BCLog::NET, "Peer %d sent us a getblocktxn for a block > %i deep\n", pfrom.GetId(), MAX_BLOCKTXN_DEPTH);
+        CInv inv;
+        inv.type = WITH_LOCK(cs_main, return State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK);
         inv.hash = req.blockhash;

It's slightly wasteful that cs_main is released and then taken again, but I think it's ok because:

1. As the comment says, we don't expect this to happen except in tests
2. 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:

./sync.h:257:45: note: expanded from macro 'WITH_LOCK'
#define WITH_LOCK(cs, code) [&] { LOCK(cs); code; }()
                                            ^~~~
net_processing.cpp:2934:20: error: assigning to 'uint32_t' (aka 'unsigned int') from incompatible type 'void'
        inv.type = WITH_LOCK(cs_main, State(pfrom.GetId())->fWantsCmpctWitness ? MSG_WITNESS_BLOCK : MSG_BLOCK);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./sync.h:257:29: note: expanded from macro 'WITH_LOCK'
#define WITH_LOCK(cs, code) [&] { LOCK(cs); code; }()
                            ^~~~~~~~~~~~~~~~~~~~~~~~~

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!

Just realised you add this as 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!

8803aee66813d27ddbdfce937ab9c35f8f7c35bc

To describe a member //! is recommended:

    //! Set of txids to reconsider once their parent transactions have been accepted.

I see lots of examples of /** 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.

Agree.

Resolution: I'll leave this as is.

8803aee66813d27ddbdfce937ab9c35f8f7c35bc, nit:

    if (!peer) return;

I'm trying to be consistent with other ways this is checked in the file, for example https://github.com/bitcoin/bitcoin/blob/8803aee66813d27ddbdfce937ab9c35f8f7c35bc/src/net_processing.cpp#L999 and https://github.com/bitcoin/bitcoin/blob/8803aee66813d27ddbdfce937ab9c35f8f7c35bc/src/net_processing.cpp#L1146

I don't think it is required to be consistent with a bad style of the surrounding code. Are we going to check every raw/smart pointer against nullptr explicitly everywhere in the code?

I think both are fine. We don't express a preference in our style guide. I think @hebasto's suggestion is more idiomatic, and if I were writing those lines again, I'd use the !ptr form.

8803aee66813d27ddbdfce937ab9c35f8f7c35bc, nit:

    if (!peer) return false;

673247b58cd1252ab7e99f7d63ead05cc100cef2, nit:

    std::set<uint256> m_orphan_work_set GUARDED_BY(::g_cs_orphans);

Thank you for the suggestion! Could you point me towards anything which indicates why this might be preferred? Based on my understanding, it seems like a style preference. 1) Globals are already prefixed with 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.

673247b58cd1252ab7e99f7d63ead05cc100cef2, nit:

        LOCK(::g_cs_orphans);

2d9f2fca43aadcdda4d644cddab36dca88b40b97

To describe a member //! is recommended:

    //! Work queue of items requested by this peer.

As noted above, leaving this as is.

2d9f2fca43aadcdda4d644cddab36dca88b40b97, nit:

    if (!peer) return;

2d9f2fca43aadcdda4d644cddab36dca88b40b97, nit:

    auto it = peer->vRecvGetData.begin();

We don't have guidance about 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 auto usage in our style guide.

Yes. And I think we should have it.

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.

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.

ok.

That is a very interesting post! I'm not sure I entirely agree with his reasoning against the readability argument, but perhaps we could debate that somewhere else :) I think this could go either way, and I prefer having the type explicit.

2d9f2fca43aadcdda4d644cddab36dca88b40b97

nit: add braces or make one line?

ba951812ec0cc8ebee5911a582f188525b76ff0a

To describe a member //! is recommended:

    //! Protects vRecvGetData.

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?

Whether a peer prefers witness data is independent from whether the block can be fetched from disk. The fact that they both happen to be guarded by cs_main is more of a historic accident than anything else. I think it's fine to release and retake cs_main here.

@jnewbery Thanks!