transport: Reject secondary connections with different connection IDs #176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
skunert
approved these changes
Jul 9, 2024
|
How we can end up with a secondary connection with a different connection id? 🤔 |
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv
added a commit
that referenced
this pull request
Jul 17, 2024
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
This was referenced Jul 17, 2024
|
Have added a repro case in this PR as well. More details in the high-level fix: #179, which builds on top of this one :D |
dmitry-markin
approved these changes
Jul 29, 2024
…nager-error Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv
force-pushed
the
lexnv/fix-transp-manager-error
branch
from
38b0d1d
to
3c5a535
Compare
|
Forced push the merge origin/master commit to fix up testing |
lexnv
added a commit
that referenced
this pull request
Aug 2, 2024
…179) This PR fixes an issue where the secondary connection is rejected because it has a different connection ID. The issue comes from `fn dial_address` which can overwrite the state of a disconnected peer with in-flight dial requests. ### High level repro case - T0: dial_address 1 - T1: dial_address 2 which overwrites address 1 - T2: dialed address 1 responds with a connection - T3: connection is rejected While at it, this PR also: - removes the need to double lock the peer state (once to read and once to write) - enforces connection ID checks on dial records in `on_dial_failure` - adjust the repro case from #176 for the fix ### Detailed repro case - T0: Dial address A connection ID 1 -> State : Dialing { in-flight-1 } - T1: Connection is established by remote address C connection ID 3 -> State : Connected { in-flight-dial 1 } - T2: Closed connection established in T2 -> State : Disconnected { in-flight-dial 1} // This is where the state was wrongfully overwritten - T3: Dial address B connection ID 2 -> State: Dialing { in-flight-2 } - T4: Connection is established by remote address C connection ID 3 -> State : Connected { in-flight-dial 2 } // The issue was detected here - T5: Dial from T0 receives a response however it expects in-flight-dial 1 and the state has in-flight-dial 2 Closes: #172 --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv
added a commit
that referenced
this pull request
Sep 5, 2024
## [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 <alexandru.vasile@parity.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ensures that the transport manager rejects secondary connections.
When the secondary connection is rejected, the internal
dial_recordis preserved.Added a test to validate this behavior, with multiple wrong endpoints.
Part of: #172
Will keep the issue opened for to further into it.
cc @paritytech/networking