net_processing: lock clean up #21527

What do you think about removing this call? The comment above is incorrect (we only process one tx at most, not recursively), and we'll process the orphans in subsequent calls to ProcessMessages(). It seems strange that in this one case we can process up to two transactions in ProcessMessage() (the one that we just received, and up to one orphan child of it).

We could process 3 txs I think -- the last orphan already in the workset (in ProcessMessages), one from a just received TX message, and an additional orphan whose parent was the contents of that TX message. I don't think it's problematic that way, but 1-non-trivial-ATMP-call per ProcessMessages invocation could be reasonable.

Very good point. I hadn't considered processing the orphan before processing the net message.

How about moving this check up to immediately after the call to ProcessOrphanTx()? It's not possible to have both more_orphans be true and have m_getdata_requests be non-empty:

• ProcessOrphanTx(*peer) can only return true if there are elements in peer's orphan work set, which can only be true if the last network message processed was a tx (m_peer_work_set only gets added to when processing a tx or when processing an element of m_peer_work_set, and we won't process another network message until it's empty)
• m_getdata_requests can only be non-empty if the last network message processed was a getdata (m_getdata_requests only gets added to when processing a getdata, and we won't process another message until it's empty)

That'd eliminate the need for a more_orphans variable, and make it immediately obvious that we're not going to process an orphan and a getdata request on the same pass through ProcessMessage().

Can be more concisely written as:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 02e2785c24..c90d73cb95 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3854,11 +3854,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
         }
     }
 
-    bool more_orphans = false;
-    {
-        LOCK(cs_main);
-        more_orphans = ProcessOrphanTx(*peer);
-    }
+    const bool more_orphans{WITH_LOCK(cs_main, return ProcessOrphanTx(*peer);)};
 
     if (pfrom->fDisconnect)
         return false;

It'd be good to remove the EXCLUSIVE_LOCKS_REQUIRED(cs_main) and only take the cs_main lock inside the while look (once we know that we have orphans to reprocess). In vast majority of cases we're taking and releasing cs_main for no reason because we have no orphans to process.

I think:

• just call AddChildrenToWorkSet when a new tx arrives, don't also immediately process them (ie don't also call ProcessOrphanTx)
• when ProcessOrphanTx does work, don't continue on to also do a ProcessMessage, give another peer a chance to get some work done first
• have ProcessOrphanTx take cs_main only when necessary
• make the GetTxToReconsider signature simpler

would all fit together nicely and be a good idea, but it also feels like it's adding a bit much to this PR?

This PR changes orphan processing from being done for the peer that provided the parent to the peer that provided the orphan. I think that's fine, but perhaps we should split the PR into one that does refactoring only and one that has behaviour changes.

Hmm, might make sense to put the changes above together with the last commit in a separate PR, so this is just refactoring and the behaviour changes (who processes orphans, and how many orphans are processed in a cycle) are collected together.

Yes, I agree that it's better to split the PR into two: one that refactors and one that changes behaviour.

This is drafted at https://github.com/ajtowns/bitcoin/commits/202104-whohandlesorphans

Well remembered to do this. Perhaps add a TxOrphange::Empty() function that returns whether there are any elements in any of the containers in the orphanage, and then assert m_orphange.Empty() in the FinalizeNode() logic when the last peer is deleted.

It crossed my mind that this might be another case where a multi index could be a win.

Not sure that it's very interesting to log work_erased. It'll only ever be 0 (nothing in the work set) or 1 (items in the work set).

I don't think this is a good idea. Anything that you guard with this mutex will never be accessible on other threads, which would prevent us from eg exposing it to RPC methods, or using it in validation interface callbacks. This seems like a way of reinventing thread local storage, which I don't think we want.

In addition, this mutex is exposed and used in both the net and net_processing layer, which adds to the coupling between those components. That's the opposite direction that I think we want to go in.

The reasoning isn't that this is particularly "good", it's that it's documenting the safety mechanisms we already have. It would have caught #21528 (review) for instance.

net_processing is fundamentally coupled with net -- it implements the NetEventsInterface defined and called from net. I mean I guess if you preferred, it could be NetEventsInterface::m_mutex_message_handler? (EDIT: actually, whether or not you prefer that, I think I do...)

And if you're doing that then perhaps you can make it a PeerManagerImpl::m_mutex_message_handler and take it in ProcessMessages() and SendMessages(). Taking that lock once at the top of those functions would be very low cost (since there would never be any contention).

I've done this now -- the first three commits introduce m_mutex_messages, remove cs_sendProcessing and move extra txns to be protected by m_mutex_messages instead of g_cs_orphans. I think it would be good to cherry-pick those commits into #21186 so that the moved addr variables can actually be marked as guarded by something.

Also see #21563

Is this possible? I think the logic in GetTxToReconsider() ensures that if true is returned, then porphanTx will not be null.

assert(porphanTx != nullptr) seemed overkill and not having anything seemed like it might be annoying for static analysers

How about: if (!Assume(porphanTx)) break ?

This is a very strange interface. How about returning a std::optional<std::pair<CTransactionRef, NodeId>>, and then adding another public method HasTxToReconsider(). It's a bit less efficient, but we'll only ever call it after processing a transaction, so it seems negligible.

I guess this means you already tried using an optional in the return value of GetTxToReconsider()?

I don't think you need this lock. It was previously here to enforce that cs_main was taken before g_cs_orphans.

It can also be removed from the process_message and process_messages fuzz tests.

I updated the comments which should still indicate why cs_main is needed there -- it needs to be locked before cs_vNodes, but cs_vNodes is locked first if you call StopThreads directly?

Ah, very good point. And we can't take cs_main inside StopNodes because net isn't aware of cs_main.

It's outside the scope of this PR, but it'd be good to untangle this lock ordering dependency by eg making sure that cs_vNodes is not held when calling any PeerManager functions.

See #21563

21563 is merged

This comment is now incorrect. The multimap stores the node that provided the orphan tx.

Also, this data is redundant data with the fromPeer in the OrphanTx struct (this could easily be a set of txids to reconsider, and the originating peer is then looked up in the OrphanTx struct). We store it in the multimap for efficiency - the comment could be updated to indicate that.

I think it'd be cleaner for the m_mutex_messages lock to be taken by ProcessMessages, rather than ProcessMessage. That would remove the need to define an inner _ProcessMessage to be called when the mutex is already locked. It's also consistent with the comment on the m_mutex_messages member, which says "is acquired by ProcessMessages and SendMessages"

It is taken by ProcessMessages() (and also by SendMessages()), however ProcessMessage() is also called from the test suite, so cannot assume the lock has been taken. That's why the public interface, ProcessMessage(), is implemented as a wrapper that takes the lock, then calls the internal function.

Ah, sorry - misread.

    /** Get an orphan transaction for a peer to work on and erase it from the peer's workset.

Done-ish

thank you!

nit fec0ff16627c1cbf5545c9de8db19b749c90beee:

The lock order is m_mutex_message_handling -> cs_main -> g_cs_orphans, so it would be nice to also order the Asserts here in the same way.

57900348db566105351b525ae18cc2830e9665b5:

This undocumented one-line-if might be a bit too minimal. Previously there was a nice comment as to why return early is needed here. Now the comment is gone, or at least can't be trivially attached to the return here.

Not sure what you mean by "the comment is gone" ? The "maintains the order of responses and prevents ~vRecvGetData~ m_getdata_requests to grow unbounded" is still there, but at least I thought that sensibly described the ~!pfrom->vRecvGetData.empty()~ !peer->m_getdata_requests.empty() check it remains attached to, and didn't really fit the "more orphans to work on" check. (EDIT: oops, was looking at 0.19 to see if there was any other comment)

Added a comment.

67618f024690bc9b926aa48ead23a0f687f03fe4: I know there is a check to avoid duplicate insertion of the same txid, but is there a reason to pick this datastructure, which doesn't disallow duplicate entries like set? Also insertion is trivially more expensive (log(all peers' work sets) vs log(this peers' work set)). Finally handling is done in insertion order, not in sorted order.

... is there a reason to pick this datastructure, which doesn't disallow duplicate entries ...

I think the reason could be to allow (nodeX, tx1), (nodeX, tx2)?

Note that if you've got N peers each with K items in their work set, then a log search over everything is log(N*K), and a log search for the peer followed by a log search or the work item is log(N) + log(K) which is the same. If there's one peer with K items and every other peer has 0, you instead get log(K) vs log(N) + 1/N log(K); so a map of sets would be better than a set of pairs provided N < K, but probably isn't very interesting.

I picked the data structure because I thought it was simpler than a set of pairs, but I don't think there's an adequate check to avoid duplicate insertion of the same txid (and to do that, you'd have to search through all the entries for a peer, which would be annoying).

Changed to map<NodeId,set<uint256>>

If it is single-threaded, then we don't need to guard anything with a mutex?

It looks to me that PeerManagerImpl::m_mutex_message_handling is useless because it is only being acquired from a single thread. No two threads are ever going to race to acquire it at the same time:

ThreadMessageHandler() // there is just one thread executing this
   ProcessMessages()
       ...
       lock m_mutex_message_handling
       ...
   ...
   SendMessages()
       ...
       lock m_mutex_message_handling
       ...

I may be missing something but to me it looks like m_mutex_message_handling can be removed safely, which will reduce the complexity of the code.

You don't need the mutex at runtime, but having the GUARDED_BY(m_mutex_message_handling) at compile time makes it easy to verify nobody's accidentally accessing the object from some other thread (either currently, or as a result of some future change, eg adding an RPC call that wants to expose some of the information stored in those objects). The cost of also having the mutex at runtime is trivial since there's never any contention on it.

NACK the addition of m_mutex_message_handling.

Following the logic of

GUARDED_BY() makes it easy to verify nobody's accidentally accessing...

should we add a dummy mutex for every variable?

IMO mutexes should be used only where concurrent access is possible.

@vasild can you take a look at the commits that build on top of this branch in https://github.com/jnewbery/bitcoin/commits/2021-05-use-mutex-message-handling. Currently, a lot of the data in net_processing is guarded by cs_main, which is also used in validation. One way to loosen that coupling between validation and net_processing is to introduce an internal mutex inside net_processing (m_mutex_message_handling) that guards its own members.

I think that https://github.com/jnewbery/bitcoin/commits/2021-05-use-mutex-message-handling can be simplified and the common m_mutex_message_handling avoided by changing some of the variables to atomics and using a dedicated mutex for e.g. m_txrequest.

Btw, it does not compile.

<details> <summary>minor fixups</summary>

diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index c56863e5ef..cc04679683 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -401,13 +401,13 @@ private:
      * Set mapBlockSource[hash].second to false if the node should not be
      * punished if the block is invalid.
      */
     std::map<uint256, std::pair<NodeId, bool>> mapBlockSource GUARDED_BY(cs_main);
 
     /** Number of peers with wtxid relay. */
-    int m_wtxid_relay_peers GUARDED_BY(m_mutex_message_handing) {0};
+    int m_wtxid_relay_peers GUARDED_BY(m_mutex_message_handling) {0};
 
     /** Number of outbound peers with m_chain_sync.m_protect. */
     int m_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0;
 
     bool AlreadyHaveTx(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
@@ -1000,20 +1000,15 @@ void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds)
     CNodeState *state = State(node);
     if (state) state->m_last_block_announcement = time_in_seconds;
 }
 
 void PeerManagerImpl::InitializeNode(CNode *pnode)
 {
-    LOCK(m_mutex_message_handling);
-
     NodeId nodeid = pnode->GetId();
-    {
-        LOCK(cs_main);
-        mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(pnode->IsInboundConn()));
-        assert(m_txrequest.Count(nodeid) == 0);
-    }
+    WITH_LOCK(cs_main, mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(pnode->IsInboundConn())));
+    WITH_LOCK(m_mutex_message_handling, assert(m_txrequest.Count(nodeid) == 0));
     {
         PeerRef peer = std::make_shared<Peer>(nodeid, m_mutex_message_handling);
         LOCK(m_peer_mutex);
         m_peer_map.emplace_hint(m_peer_map.end(), nodeid, std::move(peer));
     }
     if (!pnode->IsInboundConn()) {

</details>

<details> <summary>compilation errors</summary>

(clang 13)

net_processing.cpp:986:33: error: reading variable 'fPreferredDownload' requires holding mutex 'peer.m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
    const bool preferred = peer.fPreferredDownload;
                                ^
net_processing.cpp:1033:13: error: calling function '_RelayTransaction' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
            _RelayTransaction(txid, tx->GetWitnessHash());
            ^
net_processing.cpp:1062:41: error: reading variable 'fPreferredDownload' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
            nPreferredDownload -= peer->fPreferredDownload;
                                        ^
net_processing.cpp:1063:42: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
            m_wtxid_relay_peers -= peer->m_wtxid_relay;
                                         ^
net_processing.cpp:1537:41: error: calling function '_RelayTransaction' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
    WITH_LOCK(m_mutex_message_handling, _RelayTransaction(txid, wtxid););
                                        ^
net_processing.cpp:1550:43: error: reading variable 'm_wtxid_relay' requires holding mutex 'it->second->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
        node->PushTxInventory(it->second->m_wtxid_relay ? wtxid : txid);
                                          ^
net_processing.cpp:2485:15: error: writing variable 'fPreferredDownload' requires holding mutex 'peer->m_mutex_message_handling' exclusively [-Werror,-Wthread-safety-analysis]
        peer->fPreferredDownload = (!pfrom.IsInboundConn() || pfrom.HasPermission(PF_NOBAN)) && !pfrom.IsAddrFetchConn() && !pfrom.fClient;
              ^
net_processing.cpp:2486:37: error: reading variable 'fPreferredDownload' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
        nPreferredDownload -= peer->fPreferredDownload;
                                    ^
net_processing.cpp:2653:24: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
            if (!peer->m_wtxid_relay) {
                       ^
net_processing.cpp:2654:23: error: writing variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' exclusively [-Werror,-Wthread-safety-analysis]
                peer->m_wtxid_relay = true;
                      ^
net_processing.cpp:2777:23: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
            if (peer->m_wtxid_relay) {
                      ^
net_processing.cpp:3049:37: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
        const uint256& hash = peer->m_wtxid_relay ? wtxid : txid;
                                    ^
net_processing.cpp:3051:19: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
        if (peer->m_wtxid_relay && txid != wtxid) {
                  ^
net_processing.cpp:3075:13: error: calling function 'AlreadyHaveTx' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        if (AlreadyHaveTx(GenTxid(/* is_wtxid=*/true, wtxid))) {
            ^
net_processing.cpp:3084:21: error: calling function '_RelayTransaction' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
                    _RelayTransaction(tx.GetHash(), tx.GetWitnessHash());
                    ^
net_processing.cpp:3090:44: error: calling function 'AcceptToMemoryPool' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        const MempoolAcceptResult result = AcceptToMemoryPool(m_chainman.ActiveChainstate(), m_mempool, ptx, false /* bypass_limits */);
                                           ^
net_processing.cpp:3094:23: error: calling function 'check' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
            m_mempool.check(m_chainman.ActiveChainstate());
                      ^
net_processing.cpp:3099:13: error: calling function '_RelayTransaction' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
            _RelayTransaction(tx.GetHash(), tx.GetWitnessHash());
            ^
net_processing.cpp:3114:13: error: calling function 'ProcessOrphanTx' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
            ProcessOrphanTx(*peer);
            ^
net_processing.cpp:3131:21: error: reading variable 'recentRejects' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
                if (recentRejects->contains(parent_txid)) {
                    ^
net_processing.cpp:3147:26: error: calling function 'AlreadyHaveTx' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
                    if (!AlreadyHaveTx(gtxid)) AddTxAnnouncement(pfrom, *peer, gtxid, current_time);
                         ^
net_processing.cpp:3172:17: error: reading variable 'recentRejects' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
                recentRejects->insert(tx.GetHash());
                ^
net_processing.cpp:3173:17: error: reading variable 'recentRejects' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
                recentRejects->insert(tx.GetWitnessHash());
                ^
net_processing.cpp:3192:24: error: reading variable 'recentRejects' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
                assert(recentRejects);
                       ^
net_processing.cpp:3193:17: error: reading variable 'recentRejects' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
                recentRejects->insert(tx.GetWitnessHash());
                ^
net_processing.cpp:3204:21: error: reading variable 'recentRejects' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
                    recentRejects->insert(tx.GetHash());
                    ^
net_processing.cpp:4291:29: error: reading variable 'fPreferredDownload' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
        bool fFetch = peer->fPreferredDownload || (nPreferredDownload == 0 && !pto->fClient && !pto->IsAddrFetchConn()); // Download if this is a nice peer, or we have no nice peers and this one might do.
                            ^
net_processing.cpp:4504:53: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
                        const uint256& hash = peer->m_wtxid_relay ? txinfo.tx->GetWitnessHash() : txinfo.tx->GetHash();
                                                    ^
net_processing.cpp:4505:40: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
                        CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash);
                                       ^
net_processing.cpp:4536:85: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
                    CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool, peer->m_wtxid_relay);
                                                                                    ^
net_processing.cpp:4548:40: error: reading variable 'm_wtxid_relay' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
                        CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash);
                                       ^
net_processing.cpp:4636:117: error: reading variable 'fPreferredDownload' requires holding mutex 'peer->m_mutex_message_handling' [-Werror,-Wthread-safety-analysis]
                if (current_time > state.m_headers_sync_timeout && nSyncStarted == 1 && (nPreferredDownload - peer->fPreferredDownload >= 1)) {
                                                                                                                    ^
32 errors generated.

</details>

"Should we add a dummy mutex for every variable" -- we should have a GUARDED_BY for every object that's reachable by multiple threads, yes. The mutex is not a dummy, it's a real mutex.

For this specific case, here's an example of a patch that adds an access from another thread: https://github.com/ajtowns/bitcoin/commits/202105-whyhaveaguard . With the GUARDED_BY(m_mutex_message_handling) this is caught at compile time; without the guard (removed in the second commit), it compiles fine but introduces a race condition.

we should have a GUARDED_BY for every object that's reachable by multiple threads

I assume here you mean -- even if the current code accesses it from a single thread.

If yes, then I think that is an overkill - it means having a mutex attached to every non-const global or class member variable, including private ones (coz in the future somebody may access it from another thread).

For this specific case, here's an example...

Yeah, but one can do such an example for any other variable. IMO annotating everything with GUARDED_BY(), just in case, would have a net-adverse effect on the code base.

<details> <summary>Similar example for two other, randomly picked variables</summary>

diff --git i/src/net.h w/src/net.h
index 6b9a7ef136..18d57a7d8e 100644
--- i/src/net.h
+++ w/src/net.h
@@ -1256,12 +1256,18 @@ private:
      * an address and port that are designated for incoming Tor connections.
      */
     std::vector<CService> m_onion_binds;
 
     friend struct CConnmanTest;
     friend struct ConnmanTestMsg;
+
+public:
+    size_t ResponseCachesSize() const
+    {
+        return m_addr_response_caches.size();
+    }
 };
 
 /** Return a timestamp in the future (in microseconds) for exponentially distributed events. */
 std::chrono::microseconds PoissonNextSend(std::chrono::microseconds now, std::chrono::seconds average_interval);
 
 /** Dump binary message to file, with timestamp */
diff --git i/src/rpc/net.cpp w/src/rpc/net.cpp
index 1f6b6e8d7e..cfab1ebafc 100644
--- i/src/rpc/net.cpp
+++ w/src/rpc/net.cpp
@@ -631,12 +631,14 @@ static RPCHelpMan getnetworkinfo()
     NodeContext& node = EnsureAnyNodeContext(request.context);
     if (node.connman) {
         ServiceFlags services = node.connman->GetLocalServices();
         obj.pushKV("localservices", strprintf("%016x", services));
         obj.pushKV("localservicesnames", GetServicesNames(services));
     }
+    obj.pushKV("bug1", node.connman->ResponseCachesSize());
+    obj.pushKV("bug2", node.addrman->m_asmap.size());
     if (node.peerman) {
         obj.pushKV("localrelay", !node.peerman->IgnoresIncomingTxs());
     }
     obj.pushKV("timeoffset",    GetTimeOffset());
     if (node.connman) {
         obj.pushKV("networkactive", node.connman->GetNetworkActive());

</details>

diff --git i/src/net.h w/src/net.h

Yes, net.cpp/net.h is terrible for having globally accessible variables that aren't annotated for thread safety. Being able to introduce races that the compiler doesn't catch is a bad thing... txmempool's a better example, though it's also missing some guards (cf #22003).

IMO annotating everything with GUARDED_BY(), just in case, would have a net-adverse effect on the code base.

GUARDED_BY is always a "just in case" thing -- any time the code compiles correctly with GUARDED_BY it will also compile correctly without it -- the benefit is "just in case" someone forgets while modifying the code later. With a guard, the compiler will catch the mistake; without it, you have to hope someone reviewing the code remembers, or wait for some obscure bug to be noticed and reported.

Not consistently guarding variables has an easily observable adverse affect on the code bas as a whole: it makes it hard to change code, because you're not sure when things might be safe to reuse in a different context. eg see #21061 (review)

it means having a mutex attached to every non-const global or class member variable, including private ones

Yes and no -- no, in that most classes don't need to manually manage locks, but should rather rely on whatever instantiates them to ensure the entire object is accessed safely; but for the ones that do, yes: every non-const, non-atomic member should be guarded (as should every class's non-const, non-atomic static members, since they're effectively globals themselves).

@ajtowns, thanks for the elaboration.

I still think that mutexes should be introduced when needed, not beforehand (with a reason that in the future somebody may access a variable from a different thread).

Looks like neither one of us is convinced by the other's arguments. It's ok, it would have been too boring if everybody agreed on everything all the time.

I think it is fine to not have thread safety stuff on things that aren't meant to be called by more than one thread. For example, most code in init.cpp doesn't need them because they are only called once per process. Though, other stuff should communicate to the developer whether it is thread safe. Either via code comments (https://github.com/bitcoin/bitcoin/blob/e3de7cb9039770e0fd5b8bb8a5cba35c87ae8f00/src/random.h#L67, https://github.com/bitcoin/bitcoin/blob/e3de7cb9039770e0fd5b8bb8a5cba35c87ae8f00/src/random.h#L129) or via annotations.

    // Look up this peer's work set, ensuring it exists.

In commit bdd227493602437d4b6ee2d366ca0a83189f071c:

The new value for it is unused. No need to set it here:

        orphan_work_set.erase(it);

I left a comment about this interface further up that seems to have been lost without being resolved. I think the interface is needlessly complex and doing too much. From my comment above:

How about returning a std::optional<std::pair<CTransactionRef, NodeId>>, and then adding another public method HasTxToReconsider(). It's a bit less efficient, but we'll only ever call it after processing a transaction, so it seems negligible.

See #21527 (review)

(if you're keeping more), the following is equivalent, and maybe more direct/clear:

                more = !(work_set.empty());

Previously this would have allowed concurrent executions of SendMessages() for different peers, whereas now the concurrency is reduced by serializing all SendMessages() under the newly introduced single mutex m_mutex_message_handling.

Currently this code is executed by a single thread, so that is irrelevant, but in a possible future where we might want to do concurrent SendMessages() for different peers, the deleted cs_sendProcessing will have to be re-introduced.

Actually, sharing block headers/compact blocks messages in parallel has been discussed a while back (iirc https://bitcoincore.org/en/meetings/2018/05/03/, see "Call ProcessNewBlock asynchronously") though we could insert some queued interface between a net_processing thread and multiple net threads ?

That said, if it's direction we agree on, I think the discussion is worthy to have.

Before this PR we would have returned false (no more work) if fDisconnect was set regardless of the outcome of ProcessOrphanTx() and regardless of whether peer->m_orphan_work_set was empty.

Now fDisconnect may be set, but we may return true if ProcessOrphanTx() returns true.

If this change is not intentional, then maybe swap the order:

    if (pfrom->fDisconnect) {
        return false;
    }

    {
        LOCK(cs_main);
        if (ProcessOrphanTx(*peer)) return true;
    }

Changed to return !pfrom->fDisconnect to preserve the same behaviour.

Can be simplified?

    std::set<uint256>& orphan_work_set = m_peer_work_set[peer];

Changed to C++17's try_emplace which simplifies it slightly. Not really a fan of map::operator[]

s/map/unordered_map/ to make lookup O(1) (we don't rely on this being ordered by NodeId). Same for the set of transaction ids.

    std::unordered_map<NodeId, std::unordered_set<uint256>> m_peer_work_set GUARDED_BY(m_mutex);

Left as map; I don't think performance matters enough to justify using extra space for the hashmap; and longer term, using a multiindex rather than a bunch of different containers referencing each other is probably better.

This Assume() seems unnecessary because GetTxToReconsider() is documented to populate porphanTx if it returns true.

All Assume's should be unnecessary by definition? See #21527 (review) for prior discussion.

Assume() makes sense if the value is derived in an obscure way. But not for checking whether a function has set its "out" parameters as documented.

For example, when calling CSHA256::Finalize(result) we don't set result to a dummy value before the call and check that it is not that dummy value after the call with Assume(). Same for any other function that has "out" parameters.

If you insist to check that porphanTx was set by the function, then I guess you should do the same with from_peer: Assume(from_peer != -1).

Is it possible for TxOrphanage::m_orphans to contain keys (transaction ids) which are not in TxOrphanage::m_peer_work_set or the other way around?

m_orphans will normally contain txids that aren't in m_peer_work_set -- they only get added to the work set when a parent appears and are (hopefully) very quickly removed from the work set after retrying ATMP.

It's also possible for something to expire from m_orphans and still be present in m_peer_work_set: if a tx that's in the work set is removed via LimitOrphans or EraseForBlock you'll get that case. It will eventually be removed from the work set when GetTxToReconsider is called.

I think second part of this comment "This peer's set may be added..." is confusing and annotating the parameter as an output doesn't make sense anymore.

After this PR, parameter is no more a std::map<NodeId, std::set<uint256>> and new one can be actually a const.

Dropped the out which doesn't make sense. Knowing that this may increase the size of the set, not simply reduce it is valuable I think, so have left that text alone.

What do you think about EXCLUSIVE_LOCKS_EXCLUDED(!(...)) ? The logical negation operator is easy to miss for a reviewer.

Seems like something more for clang upstream if anything?

At commit f86a525, I don't think this lock is useful to protect vExtraTxnForCompact, there is already one in AddToCompactExtraTransactions ? Though it might be useful at branch tip ?

I was just adding AssertLockHeld to correspond with the EXCLUSIVE_LOCKS_REQUIRED annotation as a matter of course (see the recommendation in developer-notes.md).

I think this introduce a lightweight behavior change.

Previously, if our peer has fDisconnect=true set up from a previous message processing and the orphan work set is still non-empty after the ProcessOrphanTx above, we would have return false.

From now on, if the orphan work set is still non-empty after ProcessOrphanTx, we're going to return true.

Though i don't think it has any impact on allowing our peer to abuse more our CPU time, when we return from ProcessMessages to ThreadMessageHandler, we go to SendMessages which calls MaybeDiscourageAndDisconnect as first thing.

(Actually why the check on fDisconnect isn't first in ProcessMessages, should be before any kind of processing work attempted with ProcessGetData/ProcessOrphanTx ?)

fDisconnect is already checked in net.cpp prior to ProcessMessages being called, but there's always a chance that there's a race and fDisconnect is set while in the middle of ProcessMessages for the peer.

/** Message handling mutex.

• Message processing is single-threaded, so anything only accessed
• by ProcessMessage(s) or SendMessages can be guarded by this mutex,
• which guarantees it's only accessed by a single thread.

I'm confused. This mutex ends up protecting everything accessed in those functions. For example, in SendMessages, it also protects MaybeDiscourageAndDisconnect that accesses things like peer.m_misbehavior_mutex, which can be accessed by the validation thread. So this mutex is guarding more things than what is only accessed by the ProcessMessage(s) and SendMessages thread.

I suppose it's safe that this mutex also ends up guarding things that are accessed by multiple threads, but I find the comment confusing.

If you have:

Mutex mutex;
int x GUARDED_BY(mutex);
int y;
static void A() { LOCK(mutex); ++x; ++y; }
static void B() { LOCK(mutex); --x; }
void ThreadA() { for (int i = 0; i < 100; ++i) { A(); } }
void ThreadB() { for (int i = 0; i < 100; ++i) { B(); } }

then I'd say that only x is guarded by the mutex, even though y is only actually accessed while the mutex is held. If you later introduce

static void C() { y *= 2; }

then that would still be safe/correct if ThreadA were to start calling C(), but y would no longer always be protected by the mutex; conversely if ThreadB were to start calling C(), then y would no longer be thread safe, but because it's not guarded by anything, the compiler would not catch that bug.

I guess I'd look at it more as mutexes protect code from being run simultaneously; it's the GUARDED_BY annotations that protect the variables. If the variables aren't annotated, they don't have any automatic protections, only manual protection by the programmer constantly being careful.

I guess I'd look at it more as mutexes protect code from being run simultaneously

IMO mutexes protect variables, not code.

From https://en.wikipedia.org/wiki/Lock_(computer_science)

... a mechanism that enforces limits on access to a resource ... ... each thread cooperates by acquiring the lock before accessing the corresponding data ...

Read your second quote carefully -- it's not the mutex/lock that protects the data, it's the threads that protect it by cooperatively acquiring the lock before accessing the data.

It's 99% true that the point of using a mutex is to protect data, but it's not the mutex itself that ensures the data is protected, it's how you use the mutex. It's like looking both ways before crossing the road -- if you don't do that, you'll probably walk into traffic; but looking alone isn't enough, you have to make sure you do it before crossing the road, and depending on what you see, delay crossing the road. The point of annotations here is to ensure at compile time that you're not metaphorically stepping into the road before looking, or literally not accessing variables that are only meant to be accessed by a particular thread from another thread.

(I think I've repeated this point enough now; if there's something not clear, feel free to ask, but I'm not seeing any point to going around in circles another time)

do I understand correctly that after these changes PeerManagerImpl::ProcessMessage is a test-only function? If so, I think the comment in net_processing.h should be updated. Or even better, the function renamed to something like ProcessMessageTest :)

PeerManager::ProcessMessage has always only been exposed for tests, and is already documented that way in the header file:

    /** Process a single message from a peer. Public for fuzz testing */
    virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,

PeerManagerImpl::ProcessMessage is thus needed to complete the PeerManager interface.