Block-relay-only peers were introduced by #15759. According to its author, it was intented to make them only immune to outbound peer rotation-based eviction and not from all eviction as modified comment leans to think of.
Clearly indicate that outbound block-relay peers aren’t protected from eviction by the bad/lagging chain logic.
Fix #19863
2020@@ -2021,8 +2021,8 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan
2021 if (!pfrom.fDisconnect && pfrom.IsFullOutboundConn() && nodestate->pindexBestKnownBlock != nullptr) {
2022 // If this is an outbound full-relay peer, check to see if we should protect
2023 // it from the bad/lagging chain logic.
2024- // Note that block-relay-only peers are already implicitly protected, so we
2025- // only consider setting m_protect for the full-relay peers.
2026+ // Note that outbound block-relay peers are excluded from this protection and
2027+ // thus passive of eviction by bad/lagging chain logic.
How about:
0// Note that outbound block-relay peers are excluded from this protection, and thus always subject to eviction under the bad/lagging chain logic.
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.
ChainSyncTimeoutState in net_processing.cpp should also be updated.
m_protect or clarify ChainSyncTimeout name but I don’t want to interfere with actual refactoring of CNodeState (#19398). It would be easier to reorganize structures after completion of this project.
Block-relay-only peers were introduced by #15759. According to its
author, it was intented to make them only immune to outbound peer
rotation-based eviction and not from all eviction as modified comment
leans to think of.
Clearly indicate that outbound block-relay peers aren't protected
from eviction by the bad/lagging chain logic.
2020@@ -2021,8 +2021,8 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan
2021 if (!pfrom.fDisconnect && pfrom.IsFullOutboundConn() && nodestate->pindexBestKnownBlock != nullptr) {
2022 // If this is an outbound full-relay peer, check to see if we should protect
2023 // it from the bad/lagging chain logic.
2024- // Note that block-relay-only peers are already implicitly protected, so we
2025- // only consider setting m_protect for the full-relay peers.
2026+ // Note that outbound block-relay peers are excluded from this protection, and
This code is in the conditional && pfrom.IsFullOutboundConn().
Why comments about block-relay-only connections belong inside a block related to full conns?
I would probably prefer to have a good big comment before the following line:
0 if (!pfrom.fDisconnect && pfrom.IsFullOutboundConn() && nodestate->pindexBestKnownBlock != nullptr) {
338@@ -334,6 +339,9 @@ struct CNodeState {
339 * and set a shorter timeout, HEADERS_RESPONSE_TIME seconds in future.
340 * If their best known block is still behind when that new timeout is
341 * reached, disconnect.
342+ *
343+ * EXTRA_PEER_CHECK_INTERVAL: after each interval, if we have too many outbound peers,
344+ * drop to the outbound one that least recently announced us a new block.
* drop to the outbound one that least recently announced us a new block. ->
* drop the outbound one that least recently announced us a new block. (“to” does not belong here)
329+ *
330+ * Both are only in effect for outbound, non-manual, non-protected connections.
331+ * Any peer protected (m_protect = true) is not chosen for eviction. Block-relay
332+ * peers are always excluded from protection. A peer is marked as protected under
333+ * the condition of receiving at least a valid connecting header, having a better
334+ * chain than us and not having reached yet MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT.
and not having reached yet MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT makes it sound like the peer is not having reached, not us. I suggest this:
A peer is marked as protected if - 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
340@@ -334,6 +341,9 @@ struct CNodeState {
341 * and set a shorter timeout, HEADERS_RESPONSE_TIME seconds in future.
342 * If their best known block is still behind when that new timeout is
343 * reached, disconnect.
344+ *
345+ * EXTRA_PEER_CHECK_INTERVAL: after each interval, if we have too many outbound peers,
346+ * drop the outbound one that least recently announced us a new block.
b55f9790 suggestions
0- /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logics.
1+ /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logic.
2 *
3 * Both are only in effect for outbound, non-manual, non-protected connections.
4 * Any peer protected (m_protect = true) is not chosen for eviction. A peer is
5- * marked as protected if :
6+ * marked as protected if all of these are true:
7 * - its connection type is IsBlockOnlyConn() == false
8 * - it gave us a valid connecting header
9- * - it has a better chain than we have
10 * - we haven't reached MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT yet
11+ * - it has a better chain than we have
12+ * - 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.
The field m_protect is used to protect from eviction both by bad/lagging
chain and extra outbound peers logics. Outbound block-relay peers are
always excluded from this protection.
326- * m_protect == false
327- * Algorithm: if a peer's best known block has less work than our tip,
328+ /** State used to enforce CHAIN_SYNC_TIMEOUT and EXTRA_PEER_CHECK_INTERVAL logic.
329+ *
330+ * Both are only in effect for outbound, non-manual, non-protected connections.
331+ * Any peer protected (m_protect = true) is not chosen for eviction. A peer is
m_protect better, but I don’t care enough to ask for this… Consider this if you gonna update it.
331+ * Any peer protected (m_protect = true) is not chosen for eviction. A peer is
332+ * marked as protected if all of these are true:
333+ * - its connection type is IsBlockOnlyConn() == false
334+ * - it gave us a valid connecting header
335+ * - we haven't reached MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT yet
336+ * - it has a better chain than we have
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?