Clarify eviction protection scope of outbound block-relay-only peers

ariard commented at 9:47 pm on September 3, 2020: member

AFAICT, as of commit a0a422c, we have 2 different logics to evict outbound peers : lagging-chain (ConsiderEviction) and (CheckForStaleTipAndEvictPeers). The former to sanitize out lazy/buggy peers who have never sent us a valid header and are always staying behind our tip, the latter triggered in case of stale tip due to block variance to seek a better chain by rotating our outbound peers if we already reach an outbound limit (EvictExtraOutboundPeers).

Block-relay-only peers were introduced by #15719. Contrary to outbound full-relay peers who provided us at least a useful header, they’re not protected by lagging-chain eviction.

We actually have a comment inside ProcessHeadersMessage indicating the contrary behavior. We should either document cleanly that block-relay-only aren’t protected or effectively extend the scope of protection to them.

Hinted during #19724 review, see #19724 (review)

ariard added the label Bug on Sep 3, 2020

sdaftuar commented at 10:50 pm on September 3, 2020: member

Couple corrections:

the latter triggered in case of stale tip due to block variance to seek a better chain by rotating our outbound peers if we already reach an outbound limit (EvictExtraOutboundPeers).

The reason we have a stale tip check that triggers seeking extra outbound peers is in case we are partitioned off the honest network, not because we want to do something different in the case of normal block variance.

Block-relay only peers were introduced in #15759.

I mentioned here (https://github.com/bitcoin/bitcoin/pull/19724#discussion_r480327555) that the documentation was an oversight while working on #15759. See comment #15759#pullrequestreview-281092623, specifically this part:

Reworked the outbound eviction logic so that we’ll evict block-relay-only peers that fail the nMinimumChainWork test, but protect them otherwise.

So that was the intended behavior and what I think the reviewers of the PR understood as well; in my opinion the simplest fix is to correct the comments in the code to reflect that.

ariard commented at 1:57 pm on September 4, 2020: member

Clarified in #19871, let me know if modified comment captures your original intent.

The reason we have a stale tip check that triggers seeking extra outbound peers is in case we are partitioned off the honest network, not because we want to do something different in the case of normal block variance.

Thanks, I confused the conditions under which stale tip logic may trigger (block variance) with its design goal. To resume, if I understand well, there is 3 internal sources of eviction for outbound peers ?

bad/lagging chain logic (ConsiderEviction)
stale tip logic (EvictExtraOutboundPeers)
minimum chain-work (net_processing.cpp, L2002, in ProcessHeadersMessage)

I guess the difference between lagging and stale tip logics, is the former to prune out slow/buggy peers based on a relative comparison between their and our tip and the latter to actively seek a moving-forward chain in case of partition based on a required minimal frequency of block announcement ?

laanwj closed this on Oct 2, 2020

sidhujag referenced this in commit 2f44b26c5b on Oct 4, 2020

MarcoFalke locked this on Feb 15, 2022

Clarify eviction protection scope of outbound block-relay-only peers #19863