-
Notifications
You must be signed in to change notification settings - Fork 247
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
Disable "connected peers" and "peer info" protocols. #2295
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand and agree with the end result here, but I think there is a cleaner way to achieve the same goal.
For example, instead of physically removing code and adding dead code suppressions, we could simply set target and limit of connected peers to 0. In this case it will not keep any connections alive, which has the same exact result as what this PR does with a much smaller and cleaner change.
For peer info protocol, I'm not exactly sure why we need to remove it either. If we identify that connected peers protocol wasn't an issue, it would be beneficial for peer info to still be supported by network participants such that upgraded peers can maintain connections with them again once we re-enable the protocol.
Setting those limits to zero would not exclude the "unknown influence" of the feature that can cause problems. Commenting and restoring the code doesn't cost much but makes the test much clearer.
It seems that |
You wrote the code, you can trace what is being used and what isn't when value is set to
If you don't understand how it happens, why changing in this PR then? Leave it alone the way it is until you understand what it does. The only reason it would keep connection alive is to make a request, after that it should not prevent connection from shutting down unless we keep the stream alive for some other reason. |
This PR temporarily disables
connected-peers
and relatedpeer-info
protocols for debugging purposes. It also disables theNotificationReason::WentOnlineSubspace
forsubspace-service
.Code contributor checklist: