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.

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

Jump to bottom

mark potential peer as invalid after a few failed connection attempts #35

Open
jan-ferdinand opened this issue Aug 10, 2023 · 6 comments · May be fixed by #105
Open

mark potential peer as invalid after a few failed connection attempts #35

jan-ferdinand opened this issue Aug 10, 2023 · 6 comments · May be fixed by #105
Assignees
@dan-da
Labels
enhancement New feature or request good first issue Good for newcomers networking

Comments

@jan-ferdinand
Copy link
Member

The client keeps track of potential peers by asking the peers it is currently connected to for their peers. Potential peers are kept in memory indefinitely. This leads to repeated failing connection attempts to stale peers.

Track the number of failed connection attempts per peer. If it crosses some threshold, don't attempt to connect to that peer. If the same peer gets announced again and some timeout has passed, reset the number of failed connection attempts and start connection attempts again.
@jan-ferdinand jan-ferdinand added enhancement New feature or request good first issue Good for newcomers networking labels Aug 10, 2023
@Sword-Smith
Copy link
Member

I'm thinking that this info can just be stored in RAM. I think it's OK that it is forgotten if the client is reset.

@jan-ferdinand
Copy link
Member Author

Are potential peers currently persisted?

@Sword-Smith
Copy link
Member

Sword-Smith commented Jan 23, 2024 via email

@jan-ferdinand
Copy link
Member Author

Then I think it makes perfect sense to also not persist the information described in the OP.

@dan-da
Copy link
Collaborator

dan-da commented Jan 24, 2024 •
edited
Loading

I was looking into this a little bit.

My initial thought is to use the existing sanction/bad_standing mechanism for this.

So what if we just add ConnectFailed variant to enum PeerSanctionReason? Then we call punish() for each failed connect, which eventually gets the peer banned. Banned peers are already checked for in ::check_if_connection_is_allowed(), so further connection attempts would not be allowed unless/until the peer's standing improves.

I haven't check deeply into how/when a peer's standing may be improved or reset. Maybe that is not yet implemented?

Anyway, I think instead the check_if_connection_is_allowed() could be modified to check if the Peer's latest_sanction is a ConnectFailed, and if so check the timestamp_of_latest_sanction. If the timestamp is too old, then we allow the connect attempt, and perhaps revert the previous ConnectFailed including bad_standing score.

This ignores whether the peer gets announced again or not. (Do we need to care?) It just uses a simple timeout approach, so basically it looks like:

  1. we attempt to connect to peer.
  2. attempt to connect fails.
  3. peer gets sanctioned
  4. further connection attempts are not allowed, until time X has passed.
  5. peer gets unsanctioned
  6. go-to 1.

It seems this would require very little new code.

@Sword-Smith
Copy link
Member

I was looking into this a little bit.

My initial thought is to use the existing sanction/bad_standing mechanism for this.

So what if we just add ConnectFailed variant to enum PeerSanctionReason? Then we call punish() for each failed connect, which eventually gets the peer banned. Banned peers are already checked for in ::check_if_connection_is_allowed(), so further connection attempts would not be allowed unless/until the peer's standing improves.

It seems this would require very little new code.

This makes a lot of sense to me. You can store a ConnectFailed variant with an associated timestamp as a peer sanction. Should a connection failure affect standing though? Maybe if the sanction can be removed after some timeout, then it's fine.

dan-da added a commit to dan-da/neptune-core that referenced this issue Feb 11, 2024
@dan-da
feat: sanction/reward peer for connect
b200010 
closes Neptune-Crypto#35

High Level Changes:
* add doc-comments describing how sanctioning system works
* Identify PeerStanding by SocketAddr instead of IP
   because peer's can share IP (not a unique ID)
* Add Unsanction (reward) capability
* Sanction peer for connect failure
* Unsanction peer for connect success
* Establish min peer_standing score equal to 0 - cli.peer_tolerance
  A peer is banned when the min score is reached.
* Establish max peer_standing score equal to cli.peer_tolerance
* A peer can become banned for ConnectFail but after a 5
  minute period is eligible to attempt connect again and
  becomes unbanned if successful.
  see: networking_state::CONNECT_FAILED_TIMEOUT_SECS

Cleanups/Tweaks:
* rename punish_peer to sanction_peer
* rename PeerStanding::standing to score
* impl strum::Display for PeerSanctionReason
* move some existing methods into GlobalState, NetworkingState
* wrap db iterator with block_in_place() to make it async-friendly

Dependencies:
* adds direct dep on thiserror; it already existed in our dep-tree

New Tests:
* can_track_peer_standing_by_port
* ban_peer_connect_fail_and_unban_connect_success
@dan-da dan-da linked a pull request Feb 11, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dan-da dan-da

Labels
enhancement New feature or request good first issue Good for newcomers networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: sanction/reward peer for connect dan-da/neptune-core
3 participants
@jan-ferdinand @dan-da @Sword-Smith