Skip to content

Remove Banned connection state from peer database#4493

Closed
jmcph4 wants to merge 8 commits intosigp:unstablefrom
jmcph4:2772-remove-banned-conn-state
Closed

Remove Banned connection state from peer database#4493
jmcph4 wants to merge 8 commits intosigp:unstablefrom
jmcph4:2772-remove-banned-conn-state

Conversation

@jmcph4
Copy link
Contributor

@jmcph4 jmcph4 commented Jul 12, 2023

Issue Addressed

#2772

Proposed Changes

  • Add banned_since field to PeerInfo type, capturing the time at which a peer was banned
  • Add accessors for this field
    • banned_since
    • banned_since_mut
  • Remove references to the old PeerConnectionStatus::Banned state
  • Correct the definition of whether a peer is banned or not to rely solely on the ScoreState::Banned variant

Additional Info

N/A

@jmcph4 jmcph4 self-assigned this Jul 12, 2023
@jmcph4 jmcph4 added low-hanging-fruit Easy to resolve, get it before someone else does! Networking backlog PR is not currently prioritized code-quality labels Jul 12, 2023
divagant-martian
divagant-martian suggested changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

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

while this obviously compiles and indeed removes the PeerConnectionState::Banned, it's not fully untangling score and connection state, since NewConnectionState::Banned and NewConnectionState::Unbanned still exist.

Tests are failing because of this in part. NewConnectionState::Banned is adding banned peers and their ips, and later checks are removing them

divagant-martian
divagant-martian reviewed Jul 17, 2023
View reviewed changes
beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs
score: Score,
/// Time at which peer was banned
#[serde(skip)]
banned_since: Option<Instant>,
Copy link
Contributor

@divagant-martian divagant-martian Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this is the best way to keep track of this. Banned peers exist outside the PeerInfo, this keeps the possibility of inconsistency between that tracking and the inner one

@jmcph4
Copy link
Contributor Author

jmcph4 commented Jul 17, 2023

while this obviously compiles and indeed removes the PeerConnectionState::Banned, it's not fully untangling score and connection state, since NewConnectionState::Banned and NewConnectionState::Unbanned still exist.

Tests are failing because of this in part. NewConnectionState::Banned is adding banned peers and their ips, and later checks are removing them

I was thinking about this over the weekend and I don't get why NewConnectionState is strictly necessary? If you model the current connection for any given peer as a finite state machine, it doesn't really make sense to make a distinction between current state and new state -- they should be the same. When you compare the two different types it seems to reinforce this belief too:

lighthouse/beacon_node/lighthouse_network/src/peer_manager/peerdb/peer_info.rs

Lines 483 to 511 in 0d6530a

/// Connection Status of the peer.
#[derive(Debug, Clone, Default)]
pub enum PeerConnectionStatus {
/// The peer is connected.
Connected {
/// number of ingoing connections.
n_in: u8,
/// number of outgoing connections.
n_out: u8,
},
/// The peer is being disconnected.
Disconnecting {
// After the disconnection the peer will be considered banned.
to_ban: bool,
},
/// The peer has disconnected.
Disconnected {
/// last time the peer was connected or discovered.
since: Instant,
},
/// We are currently dialing this peer.
Dialing {
/// time since we last communicated with the peer.
since: Instant,
},
/// The connection status has not been specified.
#[default]
Unknown,
}

lighthouse/beacon_node/lighthouse_network/src/peer_manager/peerdb.rs

Lines 1110 to 1139 in 0d6530a

/// Internal enum for managing connection state transitions.
#[derive(Debug)]
enum NewConnectionState {
/// A peer has connected to us.
Connected {
/// An optional known ENR if the peer was dialed.
enr: Option<Enr>,
/// The seen socket address associated with the connection.
seen_address: Multiaddr,
/// The direction, incoming/outgoing.
direction: ConnectionDirection,
},
/// The peer is in the process of being disconnected.
Disconnecting {
/// Whether the peer should be banned after the disconnect occurs.
to_ban: bool,
},
/// We are dialing this peer.
Dialing {
/// An optional known ENR for the peer we are dialing.
enr: Option<Enr>,
},
/// The peer has been disconnected from our local node.
Disconnected,
/// The peer has been banned and actions to shift the peer to the banned state should be
/// undertaken
Banned,
/// The peer has been unbanned and the connection state should be updated to reflect this.
Unbanned,
}

Eager for your insight -- perhaps I'm oversimplifying it!

@divagant-martian
Copy link
Contributor

divagant-martian commented Jul 18, 2023

So this was added by age back in #2660.

It took me a bit to understand why it made sense again. The main difference is that the ConnectionState is pub while the NewConnectionState is private. This was made to isolate the connection state from outside modification since its container (the PeerInfo) needs to be mutated outside the PeerManager as well. I would be on board with creating perhaps a newtype to encapulate this without duplicating the state, achieves the same and maintains the isolation. That is a bit outside the scope of this tho, as the focus is first to stop the entanglement between score and connection state

@jmcph4 jmcph4 closed this by deleting the head repository Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@divagant-martian divagant-martian divagant-martian requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmcph4 jmcph4

Labels

backlog PR is not currently prioritized code-quality low-hanging-fruit Easy to resolve, get it before someone else does! Networking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jmcph4 @divagant-martian