Always accept incoming connections from trusted peers #7140
Conversation
AbnerZheng
requested review from
emhane,
mattsse,
Rjected and
gakonst
as code owners
| /// Maximum occupied slots for outbound connections. | ||
| pub fn with_max_pending_outbound(mut self, num_outbound: usize) -> Self { | ||
| self.connection_info.num_outbound = num_outbound; | ||
| self | ||
| } | ||
|
|
||
| /// Maximum occupied slots for inbound connections. | ||
| pub fn with_max_pending_inbound(mut self, num_inbound: usize) -> Self { | ||
| self.connection_info.num_inbound = num_inbound; | ||
| self | ||
| } |
| // if a peer is not trusted and we don't have capacity for more inbound connections, | ||
| // disconnecting the peer | ||
| if !value.is_trusted() && !self.connection_info.has_in_capacity() { | ||
| self.queued_actions.push_back(PeerAction::Disconnect { | ||
| peer_id, | ||
| reason: Some(DisconnectReason::TooManyPeers), |
There was a problem hiding this comment.
I'm wondering if we have actually have a bug here where we allow multiple incoming sessions from the same peerid.
could you please add a test case for this scenario, because I think we should check the peer's state here
similar setup as https://github.com/paradigmxyz/reth/blob/main/crates/net/network/src/peers/manager.rs#L1913-L1913
There was a problem hiding this comment.
Here is the test I can imagine:
- setup a peer with
max_in_bound= 1. - receive a incoming session from the another peer X and established the session.
- receive another incoming session from peer X again and should be rejected by
AlreadyConnected, the connected peers should not be connected and removed.
The point is that a connected peer shouldn't be disconnected by another incoming session. Am I in the right direction?
There was a problem hiding this comment.
receive another incoming session from peer X again and should be rejected by AlreadyConnected
exactly
There was a problem hiding this comment.
Yes, there seems be a bug that num_inbound should have be decreased when on_already_connected, and I add a unit test and an integration test, PTAL.
Also, I think num_inbound should only be increased when session established which may simplify the code to maintain it.
There was a problem hiding this comment.
Also, I think num_inbound should only be increased when session established which may simplify the code to maintain it.
I believe this makes sense, I'd like to introduce a new state PendingIn, similar to PendingOut
we can do this separately
mattsse
added
C-enhancement
close #7001