Skip to content

doc: Clarify scope of eviction protection of outbound block-relay peers #19871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 2, 2020
Merged

doc: Clarify scope of eviction protection of outbound block-relay peers #19871

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac71fe9
[doc] Clarify scope of eviction protection of outbound block-relay peers
Sep 4, 2020
d769254
[doc] Clarify semantic of peer's m_protect w.r.t to outbound eviction…
Sep 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,10 +321,17 @@ struct CNodeState {
*/
bool fSupportsDesiredCmpctVersion;

/** State used to enforce CHAIN_SYNC_TIMEOUT
* Only in effect for outbound, non-manual, full-relay connections, with
* m_protect == false
* Algorithm: if a peer's best known block has less work than our tip,
/** 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* marked as protected if all of these are true:
* - its connection type is IsBlockOnlyConn() == false
* - it gave us a valid connecting header
* - we haven't reached MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT yet
* - it has a better chain than we have
Copy link
Contributor

@amitiuttarwar amitiuttarwar Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

*
* CHAIN_SYNC_TIMEOUT: if a peer's best known block has less work than our tip,
* set a timeout CHAIN_SYNC_TIMEOUT seconds in the future:
* - If at timeout their best known block now has more work than our tip
* when the timeout was set, then either reset the timeout or clear it
Expand All @@ -334,6 +341,9 @@ struct CNodeState {
* and set a shorter timeout, HEADERS_RESPONSE_TIME seconds in future.
* If their best known block is still behind when that new timeout is
* reached, disconnect.
*
* EXTRA_PEER_CHECK_INTERVAL: after each interval, if we have too many outbound peers,
* drop the outbound one that least recently announced us a new block.
Copy link
Member

@jonatack jonatack Sep 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b55f979 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

Copy link
Author

@ariard ariard Sep 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jonatack jonatack Sep 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

*/
struct ChainSyncTimeoutState {
//! A timeout used for checking whether our peer has sufficiently synced
Expand Down Expand Up @@ -2018,11 +2028,12 @@ static void ProcessHeadersMessage(CNode& pfrom, CConnman& connman, ChainstateMan
}
}

// If this is an outbound full-relay peer, check to see if we should protect
// it from the bad/lagging chain logic.
// Note that outbound block-relay peers are excluded from this protection, and
// thus always subject to eviction under the bad/lagging chain logic.
// See ChainSyncTimeoutState.
if (!pfrom.fDisconnect && pfrom.IsFullOutboundConn() && nodestate->pindexBestKnownBlock != nullptr) {
// If this is an outbound full-relay peer, check to see if we should protect
// it from the bad/lagging chain logic.
// Note that block-relay-only peers are already implicitly protected, so we
// only consider setting m_protect for the full-relay peers.
if (g_outbound_peers_with_protect_from_disconnect < MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT && nodestate->pindexBestKnownBlock->nChainWork >= ::ChainActive().Tip()->nChainWork && !nodestate->m_chain_sync.m_protect) {
LogPrint(BCLog::NET, "Protecting outbound peer=%d from eviction\n", pfrom.GetId());
nodestate->m_chain_sync.m_protect = true;
Expand Down