search

Neptune-Crypto / neptune-core

anonymous peer-to-peer cash
Apache License 2.0
24 stars 7 forks source link

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

Open jan-ferdinand opened 1 year ago

jan-ferdinand commented 1 year ago

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.

Sword-Smith commented 7 months ago

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 commented 7 months ago

Are potential peers currently persisted?

Sword-Smith commented 7 months ago

They are not.

On Tue, Jan 23, 2024, 23:22 Jan Ferdinand Sauer @.***> wrote:

Are potential peers currently persisted?

— Reply to this email directly, view it on GitHub https://github.com/Neptune-Crypto/neptune-core/issues/35#issuecomment-1907019860, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACAHF2AP5GREDNNHPOS4FO3YQAZZPAVCNFSM6AAAAAA3LGAHC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBXGAYTSOBWGA . You are receiving this because you commented.Message ID: @.***>

jan-ferdinand commented 7 months ago

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

dan-da commented 7 months ago

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 commented 7 months ago

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.