-
Notifications
You must be signed in to change notification settings - Fork 11
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 connection limits for inbound and outbound established connections #185
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I considered may be using plain outbound/inbound counters instead of hashmaps, but this would require introducing dialer/listener role to the peer context to correctly track disconnects, that would use memory anyway, and at the same time is error prone due to simultaneous connections. So, IMO, your approach is good.
let result = manager | ||
.on_connection_established( | ||
peer, | ||
&Endpoint::listener(first_addr.clone(), second_connection_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the logic behind using first_addr
with second_connection_id
. Aren't two connections per peer only possible when the peers dial each other simultaneously? I.e., one outbound plus one inbound connection per peer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current code logic in the transport manager, we can allow a maximum of 2 connections.
We can definitely have 2 inbound connections:
- I believe this is an artifact of each transport negotiating and accepting connections. We may end-up with two consecutive
Endpoint::Listener
. After we handle transport/manager: Add ability to accept/reject incoming connections before negotiation #186, we can then check if we have an established inbound connection already, since we are most probably going to change the Peerstate a bit - If we manage to dial the peer before a second connection happens, the connection is still accepted. These PRs should fix it;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the initial design about supporting a maximum of 2 connections, only if the inbound connection happens at the same time with the outbound connection? If so we can prob have a look into the peerstate and modify it :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have created #189 as followup from this, offhand it seems beneficial to reject a second inbound connection
Signed-off-by: Alexandru Vasile <[email protected]>
## [0.7.0] - 2024-09-05 This release introduces several new features, improvements, and fixes to the litep2p library. Key updates include enhanced error handling, configurable connection limits, and a new API for managing public addresses. ### [Exposing Public Addresses API](#212) A new `PublicAddresses` API has been added, enabling users to manage the node's public addresses. This API allows developers to add, remove, and retrieve public addresses, which are shared with peers through the Identify protocol. ```rust // Public addresses are accessible from the main litep2p instance. let public_addresses = litep2p.public_addresses(); // Add a new public address to the node. if let Err(err) = public_addresses.add_address(multiaddr) { eprintln!("Failed to add public address: {:?}", err); } // Remove a public address from the node. public_addresses.remove_address(&multiaddr); // Retrieve all public addresses of the node. for address in public_addresses.get_addresses() { println!("Public address: {}", address); } ``` **Breaking Change**: The Identify protocol no longer includes public addresses in its configuration. Instead, use the new `PublicAddresses` API. Migration Guide: ```rust // Before: let (identify_config, identify_event_stream) = IdentifyConfig::new( "/substrate/1.0".to_string(), Some(user_agent), config.public_addresses, ); // After: let (identify_config, identify_event_stream) = IdentifyConfig::new("/substrate/1.0".to_string(), Some(user_agent)); // Public addresses must now be added using the `PublicAddresses` API: for address in config.public_addresses { if let Err(err) = public_addresses.add_address(address) { eprintln!("Failed to add public address: {:?}", err); } } ``` ### [Dial Error and List Dial Failures Event](#206) The `DialFailure` event has been enhanced with a new `DialError` enum for more precise error reporting when a dial attempt fails. Additionally, a `ListDialFailures` event has been introduced, listing all dialed addresses and their corresponding errors when multiple addresses are involved. Other litep2p errors, such as `ParseError`, `AddressError`, and `NegotiationError`, have been refactored for improved error propagation. ### [Immediate Dial Error and Request-Response Rejection Reasons](#227) This new feature paves the way for better error handling in the `litep2p` library and moves away from the overarching `litep2p::error::Error` enum. The newly added `ImmediateDialError` enum captures errors occurring before a dial request is sent (e.g., missing peer IDs). It also enhances the `RejectReason` enum for request-response protocols, offering more detailed rejection reasons. ```rust match error { RequestResponseError::Rejected(reason) => { match reason { RejectReason::ConnectionClosed => "connection-closed", RejectReason::DialFailed(Some(ImmediateDialError::AlreadyConnected)) => "already-connected", _ => "other", } } _ => "other", } ``` ### [Connection Limits](#185) Developers can now set limits on the number of inbound and outbound connections to manage resources and optimize performance. ```rust // Configure connection limits for inbound and outbound established connections. let litep2p_config = Config::default() .with_connection_limits(ConnectionLimitsConfig::default() .max_incoming_connections(Some(3)) .max_outgoing_connections(Some(2)) ); ``` ### [Feature Flags for Optional Transports](#192) The library now supports feature flags to selectively enable or disable transport protocols. By default, only the `TCP` transport is enabled. Optional transports include: - `quic` - Enables QUIC transport. - `websocket` - Enables WebSocket transport. - `webrtc` - Enables WebRTC transport. ### [Configurable Keep-Alive Timeout](#155) The keep-alive timeout for connections is now configurable, providing more control over connection lifecycles. ```rust // Set keep alive timeout for connections. let litep2p_config = Config::default() .with_keep_alive_timeout(Duration::from_secs(30)); ``` Thanks for contributing to this @[Ma233](https://github.com/Ma233)! ### Added - errors: Introduce immediate dial error and request-response rejection reasons ([#227](#227)) - Expose API for `PublicAddresses` ([#212](#212)) - transport: Implement `TransportService::local_peer_id()` ([#224](#224)) - find_node: Optimize parallelism factor for slow to respond peers ([#220](#220)) - kad: Handle `ADD_PROVIDER` & `GET_PROVIDERS` network requests ([#213](#213)) - errors: Add `DialError` error and `ListDialFailures` event for better error reporting ([#206](#206)) - kad: Add support for provider records to `MemoryStore` ([#200](#200)) - transport: Add accept_pending/reject_pending for inbound connections and introduce inbound limits ([#194](#194)) - transport/manager: Add connection limits for inbound and outbound established connections ([#185](#185)) - kad: Add query id to log messages ([#174](#174)) ### Changed - transport: Replace trust_dns_resolver with hickory_resolver ([#223](#223)) - crypto/noise: Generate keypair only for Curve25519 ([#214](#214)) - transport: Allow manual setting of keep-alive timeout ([#155](#155)) - kad: Update connection status of an existing bucket entry ([#181](#181)) - Make transports optional ([#192](#192)) ### Fixed - kad: Fix substream opening and dialing race ([#222](#222)) - query-executor: Save the task waker on empty futures ([#219](#219)) - substream: Use write_all instead of manually writing bytes ([#217](#217)) - minor: fix tests without `websocket` feature ([#215](#215)) - Fix TCP, WebSocket, QUIC leaking connection IDs in `reject()` ([#198](#198)) - transport: Fix double lock and state overwrite on disconnected peers ([#179](#179)) - kad: Do not update memory store on incoming `GetRecordSuccess` ([#190](#190)) - transport: Reject secondary connections with different connection IDs ([#176](#176)) ### Testing Done - pulled the latest changes into Substrate - performed warp sync in kusama cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]>
This PR adds connection limits for:
The public API exposes for the connection limits a builder, both on the Litep2pConfig, as well for the limits:
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 tomax_incoming_connections
andnum_of_outbound
tomax_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:
Part of: