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

Always accept incoming connections from trusted peers #7001

Closed
mattsse opened this issue Mar 6, 2024 · 5 comments · Fixed by #7140
Closed

Always accept incoming connections from trusted peers #7001

mattsse opened this issue Mar 6, 2024 · 5 comments · Fixed by #7140
Assignees
Labels
A-networking Related to networking in general C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started

Comments

@mattsse
Copy link
Collaborator

mattsse commented Mar 6, 2024

Describe the feature

currently we're always rejecting incoming connections if we're at capacity

if !self.connection_info.has_in_capacity() {
return Err(InboundConnectionError::ExceedsLimit(self.connection_info.max_inbound))
}

this makes it impossible to accept connections from trusted peers

the problem is that we only get the peer id after negotiating the rlpx session

so we can't directly disconnect these connections as long as the ip is not banned:

// ensure we can handle an incoming connection from this address
if let Err(err) =
self.state_mut().peers_mut().on_incoming_pending_session(remote_addr.ip())
{

/// Sends a disconnect message to the peer with the given [DisconnectReason].
pub(crate) fn disconnect_incoming_connection(
&mut self,
stream: TcpStream,
reason: DisconnectReason,
) {

instead we need to establish them and then apply the capacity check here, if the peer is not trusted:

pub(crate) fn on_incoming_session_established(&mut self, peer_id: PeerId, addr: SocketAddr) {

the expected behaviour should mimic:

https://github.com/ethereum/go-ethereum/blob/b590cae89232299d54aac8aada88c66d00c5b34c/p2p/server.go#L817-L829

TODO

  • only immediately disconnect on incoming if IP is banned
  • apply capacity/trusted check on_incoming_session_established

Additional context

No response

@mattsse mattsse added C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started A-networking Related to networking in general labels Mar 6, 2024
@freezlite
Copy link

I think in this change port should not be checked because incoming port can be different every time.

For example geth allows to add trusted enodes without IP and Port. But reth requires both.

@mattsse
Copy link
Collaborator Author

mattsse commented Mar 6, 2024

For example geth allows to add trusted enodes without IP and Port. But reth requires both.

do you mean via rpc or config?

only checked briefly but unsure where we check the port

@freezlite
Copy link

do you mean via rpc or config?

Yes, I tried rpc. At least there empty or 0 port is rejected. I'm fine to put not real one, important just not check the port(and maybe ip) on incoming connection. Probably just public key.

@mattsse
Copy link
Collaborator Author

mattsse commented Mar 6, 2024

gotcha, this is a separate issue then, will track this separately:

#7003

@AbnerZheng
Copy link
Contributor

Will try it since it is related to #7003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-enhancement New feature or request D-good-first-issue Nice and easy! A great choice to get started
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants