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.

Sign up for GitHub

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

Jump to bottom

transport: Reject secondary connections with different connection IDs #176

Merged
merged 9 commits into from
Jul 31, 2024
Merged

transport: Reject secondary connections with different connection IDs #176

merged 9 commits into from
Jul 31, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Jul 9, 2024

This PR ensures that the transport manager rejects secondary connections.

When the secondary connection is rejected, the internal dial_record is 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

lexnv added 5 commits July 9, 2024 19:21
@lexnv
transport: Reject secondary connections with different connection IDs
a0d00e8 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport: Fix comment typos
a93f930 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport/tests: Double check connection etablished status
edf07eb 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport/tests: Check secondary connections with invalid connection IDs
9db59dd 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
trasport: Preserve the state on wrong connections
8b14f96 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Jul 9, 2024
@dmitry-markin
Copy link
Collaborator

How we can end up with a secondary connection with a different connection id? 🤔

@lexnv
transport/types: Fix typo
900da6e 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport/test: Add repro case for edge-case
6d158bf 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
lexnv added a commit that referenced this pull request Jul 17, 2024
@lexnv
transport/tests: Backport test from PR #176
daa147b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
This was referenced Jul 17, 2024
Merged
Closed
@lexnv
Copy link
Collaborator Author

lexnv commented 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

@lexnv lexnv mentioned this pull request Jul 29, 2024
Merged
@lexnv
Merge remote-tracking branch 'origin/master' into lexnv/fix-transp-ma…
3c5a535 
…nager-error

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv force-pushed the lexnv/fix-transp-manager-error branch from 38b0d1d to 3c5a535 Compare July 31, 2024 10:34
@lexnv
Copy link
Collaborator Author

lexnv commented Jul 31, 2024

Forced push the merge origin/master commit to fix up testing

@lexnv lexnv merged commit 171e36a into master Jul 31, 2024
8 checks passed
@lexnv lexnv deleted the lexnv/fix-transp-manager-error branch July 31, 2024 10:52
lexnv added a commit that referenced this pull request Aug 2, 2024
@lexnv
transport: Fix double lock and state overwrite on disconnected peers (#…
7befb3a 
…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 lexnv mentioned this pull request Sep 5, 2024
Merged
lexnv added a commit that referenced this pull request Sep 5, 2024
@lexnv
chore: Release litep2p 0.7.0 (#240)
a68b713 
## [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
Reviewers

@skunert skunert skunert approved these changes

@dmitry-markin dmitry-markin dmitry-markin approved these changes

Assignees

@lexnv lexnv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lexnv @dmitry-markin @skunert