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

Implement connection limits #17

Closed
2 tasks done
Tracked by #140
altonen opened this issue Sep 25, 2023 · 2 comments
Closed
2 tasks done
Tracked by #140

Implement connection limits #17

altonen opened this issue Sep 25, 2023 · 2 comments
Assignees
Labels
easy Requires little knowledge of the codebase enhancement New feature or request

Comments

@altonen
Copy link
Contributor

altonen commented Sep 25, 2023

Tasks

Preview Give feedback
  1. enhancement
    lexnv
  2. enhancement
    lexnv
@altonen altonen added enhancement New feature or request medium Requires some knowledge of the codebase labels Sep 25, 2023
@altonen altonen added hard Requires good knowledge of the codebase and removed medium Requires some knowledge of the codebase labels Oct 29, 2023
@altonen altonen added easy Requires little knowledge of the codebase and removed hard Requires good knowledge of the codebase labels Feb 15, 2024
@lexnv
Copy link
Collaborator

lexnv commented Jul 9, 2024

Monitoring a litep2p node with a libp2p node, it is clear that litep2p can sustain many more connections.

This data point supports the connection limits, as I believe it will free up some more resources (ie not having to deal with that many connections):

Screenshot 2024-07-09 at 14 18 46

@altonen
Copy link
Contributor Author

altonen commented Jul 9, 2024

The connections also allocate quite a bit of memory, like ~600KB of memory per connection. Those numbers are configurable but it'd be better to have proper connection limits which could be as simple as just checking the total number of connections in TransportManager before accepting an inbound connection/starting to dial remote peer.

The more advanced option would be to not poll TCP/WebSocket listeners at all if the maximum number of connections has been reached but last time I looked into it, it required some non-trivial API rework in the transport interface.

If resource consumption is an issue, you can decrease the sizes of Noise read/write buffers in the transport configs.

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 self-assigned this Aug 7, 2024
@lexnv lexnv closed this as completed Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy Requires little knowledge of the codebase enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants