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

Refuse connections from recent disconnects #429

Conversation

jan-ferdinand
Copy link
Member

@jan-ferdinand jan-ferdinand commented Feb 13, 2025

In order to reduce the potential connection flood, rapid re-connection attempts after a disconnect are ignored.

If node A decides to (gracefully) terminate the connection to some node B, then node A blocks any connection attempts from node B for some time. The cool-down time can be configured through the command-line interface's argument “reconnect_cooldown”. The argument is given in seconds. The default is 30 seconds.

This introduces a breaking change because the public enum ConnectionRefusedReason gets a new variant RecentlyDisconnected. This enum is now marked #[non_exhaustive] to prevent future variant additions from being breaking again.

This introduces a breaking change because the public structure NetworkingState gains a new, private field.

@jan-ferdinand
Copy link
Member Author

@aszepieniec, @Sword-Smith, I'm struggling to create a test for the new feature. The various state variables and channels wielded by other tests pertaining to peer connection seem to require an intimate understanding of the inner workings of the various tasks (which I lack), and seem to come with varying amounts of implicit behavior (for which documentation is lacking).

I have pushed my best attempt so far in commit 847d1b1. Clearly, there are some fundamentals I don't understand. I'd be grateful for some assistance.

@aszepieniec
Copy link
Contributor

This introduces a breaking change because the public enum ConnectionRefusedReason gets a new variant RecentlyDisconnected. This enum is now marked #[non_exhaustive] to prevent future variant additions from being breaking again.

I did not realize this change would be breaking, but it is. Let's avoid modifying existing messages and add new ones instead. (Actually, I'm assuming that new messages are simply ignored by old clients, but this assumption is worth verifying.)

@aszepieniec
Copy link
Contributor

Actually, I'm assuming that new messages are simply ignored by old clients, but this assumption is worth verifying.

It sounds like any change to PeerMessage would be backwards-incompatible 🤦 So for the time being, we are not allowed to change this struct at all. We can speculate about how to get around this in issue #440 but that's probably out of scope for this PR, which about getting the internals to work.

@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch from 847d1b1 to d625cb7 Compare February 18, 2025 11:06
@jan-ferdinand
Copy link
Member Author

Clearly, there are some fundamentals I don't understand.

As far as I can tell, the problem is that the channel from_peer_to_main_tx gets closed after calling answer_peer_inner. This only seems to happen the second time answer_peer_inner is called. I think I need to keep this channel open somehow, or create an equivalent channel. I'd be grateful for some assistance.

@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch from d625cb7 to 45c2b21 Compare February 18, 2025 15:00
@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch from 45c2b21 to c808194 Compare February 25, 2025 15:40
@jan-ferdinand jan-ferdinand marked this pull request as ready for review February 26, 2025 08:09
@jan-ferdinand jan-ferdinand marked this pull request as draft February 26, 2025 08:35
@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch 2 times, most recently from d325aca to dac5642 Compare February 26, 2025 09:06
@jan-ferdinand jan-ferdinand marked this pull request as ready for review February 26, 2025 09:07
@jan-ferdinand jan-ferdinand requested review from aszepieniec and Sword-Smith and removed request for aszepieniec February 26, 2025 09:07
@jan-ferdinand jan-ferdinand marked this pull request as draft February 26, 2025 09:55
@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch 2 times, most recently from df60eec to 4960d34 Compare February 26, 2025 10:34
@jan-ferdinand jan-ferdinand marked this pull request as ready for review February 26, 2025 10:37
@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch 2 times, most recently from fc89e17 to 4555e0b Compare February 26, 2025 13:45
@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch 3 times, most recently from 9b4be3c to c59cf5b Compare February 27, 2025 15:33
@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch from c59cf5b to cac18b9 Compare March 3, 2025 13:26
Copy link
Member

@Sword-Smith Sword-Smith left a comment

Choose a reason for hiding this comment

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

I think you mention the wrong default value in the commit message. Otherwise LGTM.

In order to reduce the potential connection flood, rapid re-connection
attempts after a graceful disconnect are ignored.

If node A decides to (gracefully) terminate the connection to some node
B, then node A blocks any connection attempts from node B for some
time. The cool-down time can be configured through the command-line
interface's argument “reconnect_cooldown”. The argument is given in
seconds. The default is 30 minutes.

BREAKING CHANGE: The public structure `NetworkingState` gains a new,
private field.
@jan-ferdinand jan-ferdinand force-pushed the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch from cac18b9 to 7fd7844 Compare March 3, 2025 14:44
@jan-ferdinand jan-ferdinand merged commit 7fd7844 into master Mar 3, 2025
5 of 8 checks passed
@jan-ferdinand jan-ferdinand deleted the jfs/411_temporarily_ignore_peer_with_recently_closed_connection_pr branch March 3, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bootstrap: Ignore (For a While) Peers whose Connections were Closed
3 participants