Skip to content
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.

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 by Bors] - Consistent tracking of disconnected peers #2650

Closed
wants to merge 3 commits into from

Conversation

AgeManning
Copy link
Member

Issue Addressed

N/A

Proposed Changes

When peers switching to a disconnecting state, decrement the disconnected peers counter. This also downgrades some crit logs to errors.

I've also added a re-sync point when peers get unbanned the disconnected peer count will match back to the number of disconnected peers if it has gone out of sync previously.

@AgeManning AgeManning added the ready-for-review The code is ready for review label Sep 30, 2021
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +520 to +525
if matches!(
peer_info.connection_status(),
PeerConnectionStatus::Disconnected { .. }
) {
self.disconnected_peers = self.disconnected_peers.saturating_sub(1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Checking that I understand the logic here correctly:

We're about to change the peer's status from Disconnected -> Disconnecting, so we decrement the number of disconnected peers. Usually a short time later we will re-increment it when the peer transitions from Disconnecting -> Disconnected, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, see, confuuuuusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

We shouldn't really be disconnecting, peers that are already disconnected, but it can happen that an old peer in our db tries to connect to us, and we have too many peers, so we dont register it as connected and just move it straight to disconnecting.

Copy link
Member

Choose a reason for hiding this comment

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

Damn, yeah, that's complicated.

@michaelsproul michaelsproul added v2.0.0 Altair on mainnet release (v2.0.0) ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Sep 30, 2021
@michaelsproul
Copy link
Member

Going to batch this with #2592 and #2546 (once the latter is ready)

@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 30, 2021
## Issue Addressed

N/A

## Proposed Changes

When peers switching to a disconnecting state, decrement the disconnected peers counter. This also downgrades some crit logs to errors. 

I've also added a re-sync point when peers get unbanned the disconnected peer count will match back to the number of disconnected peers if it has gone out of sync previously.
@bors
Copy link

bors bot commented Sep 30, 2021

This PR was included in a batch that was canceled, it will be automatically retried

bors bot pushed a commit that referenced this pull request Sep 30, 2021
## Issue Addressed

N/A

## Proposed Changes

When peers switching to a disconnecting state, decrement the disconnected peers counter. This also downgrades some crit logs to errors. 

I've also added a re-sync point when peers get unbanned the disconnected peer count will match back to the number of disconnected peers if it has gone out of sync previously.
@bors bors bot changed the title Consistent tracking of disconnected peers [Merged by Bors] - Consistent tracking of disconnected peers Sep 30, 2021
@bors bors bot closed this Sep 30, 2021
@michaelsproul michaelsproul deleted the quick-patch branch September 30, 2021 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v2.0.0 Altair on mainnet release (v2.0.0)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants