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

transport/manager: Add ability to accept/reject incoming connections before negotiation #186

Closed
Tracked by #140
lexnv opened this issue Jul 26, 2024 · 0 comments · Fixed by #194
Closed
Tracked by #140
Assignees
Labels
enhancement New feature or request

Comments

@lexnv
Copy link
Collaborator

lexnv commented Jul 26, 2024

Refactor the transport manager and every transport to allow the transport manager to accept or reject incoming connections.

Currently, each transport is responsible for polling the underlying socket and all incoming connections are immediately accepted and negotiated:

fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
while let Poll::Ready(event) = self.listener.poll_next_unpin(cx) {
match event {
None | Some(Err(_)) => return Poll::Ready(None),
Some(Ok((connection, address))) => {
self.on_inbound_connection(connection, address);
}
}
}

self.pending_connections.push(Box::pin(async move {
TcpConnection::accept_connection(
connection,
connection_id,
keypair,
address,
yamux_config,
max_read_ahead_factor,
max_write_buffer_size,
connection_open_timeout,
substream_open_timeout,
)
.await
.map_err(|error| (connection_id, error))

It would be beneficial in terms of resources to move the accept/reject responsibility to the transport manager for inbound connections (similar how we handle outbound ones). We can reject the connection immediately rather than negotiating it to simply reject later.

One option would be to extend the TransportEvent::ConnectionOpened to support an Endpoint instead of a connection + address. Then we can emit this event from each transport to the transport manager. Then, the transport manager can check the limits and handle the negotiation. This may also require a refactor to the peer state logic.

@lexnv lexnv added the enhancement New feature or request label Jul 26, 2024
lexnv added a commit that referenced this issue Jul 30, 2024
…ablished connections (#185)

This PR adds connection limits for: 
- maximum number of inbound established (negotiated) connections
- maximum number of outbound established (negotiated) connections
- any dial attempts are immediately rejected if we reach capacity for
the maximum number of outbound established connections

The public API exposes for the connection limits a builder, both on the
Litep2pConfig, as well for the limits:

```rust
let litep2p_config = Config::default()
 .with_connection_limits(ConnectionLimitsConfig::default()
      .max_incoming_connections(Some(3))
      .max_outgoing_connections(Some(2)));
```

The connection limits increments and decrements connections identified
by their `ConnectionId(usize)`.
Regarding memory consumption, we use 2 hash-maps that require
`sizeof(usize) * (num_of_inbound + num_of_outbound)` for elements.
Where,`num_of_inbound` may be capped to `max_incoming_connections` and
`num_of_outbound` to `max_outgoing_connections`.

We do not need to introduce a limit on the total number of connections
established with a single peer. Litep2p already assumes we can have a
maximum of 2 established connections for each peer.

We could also limit the number of inbound non negotiated connections,
similar to how we handle outbound connections via dial methods. However,
each protocol eagerly accepts and negotiates all inbound connections.
This is not optimal, because we can save resources (CPU + mem) by
rejecting non negotiated connections when we are at inbound capacity.

To achieve this, a refactoring needs to move the accepting of inbound
connections back into the ownership of the TransportManger, instead of
the Transport itself. This PR introduces some changes, to also make it
easier for review will look into it as a follow-up:
- #186

Part of:
- #17

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv added a commit that referenced this issue Aug 7, 2024
…and introduce inbound limits (#194)

This PR refactors the transport trait to allow the transport manager to
decide if the incoming connection should be negotiated or rejected
immediately.

Before this PR, the transports (TCP, ws) eagerly accepted inbound
connections and started negotiating.
This consumed resources if the node was already at the limits or
connected.

To achieve this, each transport was refactored to pass the
responsibility of accepting connections to the transport manager.

After the refactor, we are able to look at the connection limits before
accepting a new inbound connection.

### Notes

This PR aims to introduce the methods and only looks at the connection
limits.
In the future (soonish), we need to tackle separately:
- state machine refactoring to accept only 1 inbound connection per peer
context
- accept/reject inbounds after looking at peer context (ie if already
connected there's no benefit)
- s2n-quic transport is out of date and outside the scope of this PR

### Next Steps
- Look at the performance impact of communicating between transport
manager <-> individual transports
- If the impact is significant (although under load I expect the channel
communication of 2 messages to be faster than eagerly negotiating), then
we can look into simplifying the events. However, this gives us a robust
view of each transport, and allows us not to negotiate a connection if
the peer is already connected.


Closes: #186
Part of: #17

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv lexnv closed this as completed in #194 Aug 7, 2024
@lexnv lexnv self-assigned this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
1 participant