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

Disable "connected peers" and "peer info" protocols. #2295

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

shamil-gadelshin
Copy link
Contributor

This PR temporarily disables connected-peers and related peer-info protocols for debugging purposes. It also disables the NotificationReason::WentOnlineSubspace for subspace-service.

Code contributor checklist:

@shamil-gadelshin shamil-gadelshin added the networking Subspace networking (DSN) label Dec 6, 2023
@shamil-gadelshin shamil-gadelshin self-assigned this Dec 6, 2023
Copy link
Member

@nazar-pc nazar-pc left a 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.

@shamil-gadelshin
Copy link
Contributor Author

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.

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.

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.

It seems that peer-info protocol maintains an expected connection sometimes. It wasn't the case to the best of my knowledge before the last libp2p upgrade. I didn't have time for debugging the issue yet which we should do next. I disabled it to remove the possible influence because we don't use peer-info protocol without connected-peers protocol.

@nazar-pc
Copy link
Member

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.

You wrote the code, you can trace what is being used and what isn't when value is set to 0. I do not understand what you're worrying about.

It seems that peer-info protocol maintains an expected connection sometimes. It wasn't the case to the best of my knowledge before the last libp2p upgrade. I didn't have time for debugging the issue yet which we should do next. I disabled it to remove the possible influence because we don't use peer-info protocol without connected-peers protocol.

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.

@nazar-pc nazar-pc added this pull request to the merge queue Dec 11, 2023
Merged via the queue into main with commit 8e3e13b Dec 11, 2023
10 checks passed
@nazar-pc nazar-pc deleted the disable-connected-peers branch December 11, 2023 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking Subspace networking (DSN)
Projects
Development

Successfully merging this pull request may close these issues.

2 participants