How about:

// Note that outbound block-relay peers are excluded from this protection, and thus always subject to eviction under the bad/lagging chain logic.

Also, you may want to consider adding a comment that explains how m_protect is used by stale-tip-based eviction as well. In particular, the stale-tip -> extra full outbound peer logic could cause an existing full outbound to be evicted, but peers with m_protect set will not be chosen for eviction.

Thanks for the suggestion. Also added a commit on scope of m_protect w.r.t to both eviction logics.

b55f9790 suggestions

-    /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logics.
+    /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logic.
       *
       * Both are only in effect for outbound, non-manual, non-protected connections.
       * Any peer protected (m_protect = true) is not chosen for eviction. A peer is
-      * marked as protected if :
+      * marked as protected if all of these are true:
       *   - its connection type is IsBlockOnlyConn() == false
       *   - it gave us a valid connecting header
-      *   - it has a better chain than we have
       *   - we haven't reached MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT yet
+      *   - it has a better chain than we have
+      *   - it is not already marked as protected

Thanks, I'm going to leave it as it is. It's a bit confusing saying that something is "marked as protected if it is not already marked as protected".

Note, actually CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL are two different logics, thus the plural.

I'm aware. Logic is most often used as an uncountable noun, but it's not an issue.

While rewriting this, I think it would be good to clarify that the list of conditions is "if all of these" and not "if any of these".

The reordering suggestion is to follow the order of the actual checks.

I verified that the proposed documentation corresponds to the code.

Thanks for the noun suggestion. Took it, also the re-ordering. But not the "it is not already marked as protected" it's a tautology IMO (it would false future triggering of protection if we don't due to g_outbound_peers_with_protect_from_disconnect is incremented in same conditional, but technically the state transition is defined without)

I think this explanation actually belongs to the declaration of m_protect better, but I don't care enough to ask for this... Consider this if you gonna update it.

I don't think this is quite right?

the relevant part of the check in ProcessHeadersMessage checks nodestate->pindexBestKnownBlock->nChainWork >= ::ChainActive().Tip()->nChainWork, aka would set m_protect to true if they have the same chain tip as we do.

I think saying the peer "has a better chain" is misleading, because then I'd expect a long-running node to usually not have protected peers. Based on the logic I see, I'd expect a long-running node to usually have MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT.

what do you think?