net: Restrict period when cs_vNodes mutex is locked #21563

This PR restricts the period when the cs_vNodes mutex is locked, prevents the only case when cs_vNodes could be locked before the ::cs_main.

This change makes the explicit locking of recursive mutexes in the explicit order redundant.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #21527 (net_processing: lock clean up by ajtowns)

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.

EDIT: This PR has substantially changed. The comment below refers to the old branch.

This does indeed seem useless. The mutex was added by @TheBlueMatt in commit d7c58ad514ee00db00589216166808258bc16b60:

Split CNode::cs_vSend: message processing and message sending

cs_vSend is used for two purposes - to lock the datastructures used to queue messages to place on the wire and to only call SendMessages once at a time per-node. I believe SendMessages used to access some of the vSendMsg stuff, but it doesn't anymore, so these locks do not need to be on the same mutex, and also make deadlocking much more likely.

We're implicitly guaranteed to only call SendMessages() serially since it's only ever called by message handler thread. If we want to be explicit about it, it'd be better to add a global lock to PeerManagerImpl (or per-Peer object), so that net_processing is enforcing its own synchronization internally.

Rebased 8ca2ee6377f534b15b47440b819b1564c9c06b88 -> b58097facb4fec80df8fa140dfefd9c461c93832 (pr21563.02 -> pr21563.03) due to the conflict with #21571.

This seems safe to me. The only thing that CNode.cs_sendProcessing enforces is preventing SendMessages() from being called for the same CNode concurrently. However, SendMessages() can only be called by threadMessageHandler, so there's no possibility of it being called concurrently.

I wonder if instead of simply removing this, we should replace it with something that more closely documents our expectations. I think that calling any of the NetEventsInterface methods (InitializeNode, FinalizeNode, SendMessages, ProcessMessages) in PeerManagerImpl concurrently could lead to problems, so perhaps a global mutex in PeerManagerImpl should be added that's taken whenever any of those functions are called? If we ever want to add concurrency to PeerManagerImpl, we could look at loosening that restriction.

@jnewbery

I wonder if instead of simply removing this, we should replace it with something that more closely documents our expectations. I think that calling any of the NetEventsInterface methods (InitializeNode, FinalizeNode, SendMessages, ProcessMessages) in PeerManagerImpl concurrently could lead to problems, so perhaps a global mutex in PeerManagerImpl should be added that's taken whenever any of those functions are called? If we ever want to add concurrency to PeerManagerImpl, we could look at loosening that restriction.

Maybe postpone it until #19398 is fulfilled?

Maybe postpone it until #19398 is fulfilled?

I think they can be done independently. But I think I'd like to see the new internal PeerManagerImpl lock being introduced in the same PR that removes cs_sendProcessing.

Updated b58097facb4fec80df8fa140dfefd9c461c93832 -> 4de76058f8c3150de07d7a02faaceb261d732499 (pr21563.03 -> pr21563.04):

• rebased on top of the recent changes in CI
• addressed @jnewbery's comment

 node0 stderr libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument 

https://cirrus-ci.com/task/5038649289998336?logs=ci#L3615

 node0 stderr libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument 

https://cirrus-ci.com/task/5038649289998336?logs=ci#L3615

Looks like a mutex is prematurely destroyed. Weird.

Updated 4de76058f8c3150de07d7a02faaceb261d732499 -> b1e5ca217c3557ed89d985c7e8ea28c2b9ed303b (pr21563.04 -> pr21563.05, diff):

• fixed bug

Updated b1e5ca217c3557ed89d985c7e8ea28c2b9ed303b -> a3663329ad1899cc012437167a316040d77e00bc (pr21563.05 -> pr21563.06):

• rebased on top of the recent CI changes
• addressed @jnewbery's comments

This seems good to me. @MarcoFalke - you introduced the LOCK2(::cs_main, ::g_cs_orphans); in #18458. Do you see any problem with locking cs_vNodes, grabbing a copy of vNodes, releasing the lock and then deleting the nodes in CConnman::Stop()? This is similar to what happens in the socket handler and message handler threads?

Removing a mutex that guards nothing then adding a mutex that also doesn't actually guard anything seems a bit backwards...

#21527 has it guarding the extratxns data structures (as part of getting rid of g_cs_orphans). It would be good for it to be able to guard the address data structures in struct Peer, but without it being a global, that will probably be awkward at best, since Peer doesn't have a reference back to PeerManagerImpl.

The cs_vNodes changes don't seem to be mentioned in the title or PR description?

The fist commit seems separate from the other changes?

Updated a3663329ad1899cc012437167a316040d77e00bc -> 9766b7f43d4fedd1740f4444689b6ba2ff33d12d (pr21563.06 -> pr21563.07):

• dropped all commits but the first one in favor of #21527

The cs_vNodes changes don't seem to be mentioned in the title or PR description?

The PR description updated.

I think this change is fine. It steals the CNode*s from vNodes, releases the mutex and then cleans up the nodes. That's very similar to the pattern in SocketHandler():

https://github.com/bitcoin/bitcoin/blob/018045347110d76cfc1f89a79fb49a867e3b8449/src/net.cpp#L1481-L1487

and in ThreadMessageHandler():

https://github.com/bitcoin/bitcoin/blob/018045347110d76cfc1f89a79fb49a867e3b8449/src/net.cpp#L2185-L2192

The difference being that in those cases, an extra reference is taken and nRefCount is incremented. Here, the reference count is not incremented since the pointer is stolen from vNodes before clearing that vector.

Perhaps we could document our assumptions by calling Release() on the CNode object when stealing it from vNodes or immediately before calling DeleteNode(), and then asserting that nRefCount == 0 in the CNode destructor? Maybe in a follow up.

Seems sensible at first glance, and I think any bugs would be caught by CI. Need to have a more thorough look though.

ACK 9766b7f43d4fedd1740f4444689b6ba2ff33d12d

Something to consider for further improvement:

It is not even necessary to lock cs_vNodes inside StopNodes(). Why? Because by the time StopNodes() is called the other threads have been shut down.

If it was necessary to protect the code in StopNodes() with a mutex, then this PR would be broken:

lock()
tmp = std::move(shared_vector)
unlock()
destroy each element in tmp
// What prevents new entries from being added to shared_vector
// here, since we have unlock()ed? Nothing. So when this function
// completes, shared_vector is still alive and kicking (with new
// elements being added to it in the meantime).

The LOCK(cs_vNodes); is only needed to keep the thread safety analysis happy. This code actually belongs to the destructor ~CConnman() where we can access vNodes without locking and without upsetting the TSA. I think it can be moved to the destructor with some further changes (outside of this PR).

This code actually belongs to the destructor ~CConnman() where we can access vNodes without locking and without upsetting the TSA. I think it can be moved to the destructor with some further changes (outside of this PR).

Maybe, but that's a much more invasive change. CConnman::Stop() calls PeerManager::DeleteNode() for all the nodes. The destructor for CConnman is called after the destructor for PeerManager (in the reverse order that they're constructed). That can be changed, but we'd need be very careful.

utACK 9766b7f43d4fedd1740f4444689b6ba2ff33d12d

Agree with @MarcoFalke that the move & clear is more elegantly expressed as std::vector::swap().

<details> <summary>Diff</summary>

diff --git a/src/net.cpp b/src/net.cpp
index b7c1b8c6c4..94029491ab 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2637,11 +2637,7 @@ void CConnman::StopNodes()
 
     // Delete peer connections
     std::vector<CNode*> nodes;
-    {
-        LOCK(cs_vNodes);
-        nodes = std::move(vNodes);
-        vNodes.clear();
-    }
+    WITH_LOCK(cs_vNodes, nodes.swap(vNodes));
 
     for (CNode* pnode : nodes) {
         pnode->CloseSocketDisconnect();

</details>

Updated 9766b7f43d4fedd1740f4444689b6ba2ff33d12d -> 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f (pr21563.07 -> pr21563.08, diff).

Addressed @MarcoFalke's comments:

• #21563 (review)

• #21563 (review)

Could achieve the same in one less line of code with https://en.cppreference.com/w/cpp/container/vector/swap ?

minor nit: would be nice to do style-fixups in a separate commit

ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f

utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f

utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f - logic seems sound

Couple of comment improvements that would be good; longer term would also be better to add missing guards to things in net so that the compiler can catch more errors, and simplify the ownership/interaction between CConnman/PeerManager/NodeContext...

(consider out of scope of this PR)

Could we assert that all threads are stopped when StopNodes() starts executing? Something like

assert(threadMessageHandler.get_id() == std::thread::id());
assert(threadSocketHandler.get_id() == std::thread::id());
...

Or even call StopThreads() at the start of StopNodes()?

Or even call StopThreads() at the start of StopNodes()?

This used to be the case before commit fa369651c5523f393e0bfcfa328b8e27006711aa. Maybe that commit should be reverted now?

I think the changes are nice and can be merged

review ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f 👢

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f 👢
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiYLgwAy9ZbgTGcc2V3HKj7fNh6KWOQ2DgAVsBLAmRP1D+443eCC3twNxELvtOe
Mq2gmwseUZK1oNKU0RnCHvg5bZOqcId1N1Sf6BEbJOmkGFPktaVLTz2TmQpLLE42
ZTMrqWEJreQ9DXqEDDKV222Zu4ucLmUFuNqs5MYj+moUTBdedFcbeF96cJK2hBc1
OgTJXL6MLoMAFeL4PKS6j+E2yHXOFuKHWE9WHPJ0wFKujTqlBPtntZxoWb7fby2A
ZHrA8wxjdNQbgovYRjToVe/bif2SNuFbfNwE2XYalQ50U8bY5n3hC2SUB/poGTaJ
GoSOWiv1jAUimiMUVyicP+JDfprAjPkB9hXKC7k4kvbluTpqYz4UGxhnBmKnU/qe
+gU1fPldReMkVGg5YjG3pg7Kd4izdq2YzyNMXdyIRHJaWn+gvsl4vQjPLUe7+WFL
DvX4dkpJ9fHF/DoHLsYi5/g2sG3G2G1Z4jrV/TFqAInAa0tuEqdWwYBomHi5QBQX
GzrRIkFr
=lXKU
-----END PGP SIGNATURE-----

Timestamp of file with hash 2744498e52ef3957483818d251ab7a217f1deba6d21ad39bc26dd3c078c33d07 -

</details>

There is a related pr, which is waiting for review donations: #21750