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: Add accept_pending/reject_pending for inbound connections and introduce inbound limits #194

Merged
merged 14 commits into from
Aug 7, 2024
Merged

transport: Add accept_pending/reject_pending for inbound connections and introduce inbound limits #194

merged 14 commits into from
Aug 7, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Aug 6, 2024 •
edited
Loading

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

@lexnv
transport: Accept / reject pending connection methods
7ee5a65 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
manager/limits: Check inbound connection limits
b7dc82f 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
manager: Accept/reject pending incoming
dbb3fac 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transports: Implement accept/reject pending for supported transports
ef0a816 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
quic/tests: Adjust testing to accept connection
5d5eb21 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tcp/tests: Check accept / reject connection
f2a7a5a 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv self-assigned this Aug 6, 2024
@lexnv lexnv changed the title transport: Add accept_pending/reject_pending for inbound connections and introduce limits transport: Add accept_pending/reject_pending for inbound connections and introduce inbound limits Aug 6, 2024
@lexnv lexnv added the enhancement New feature or request label Aug 6, 2024
@lexnv
Copy link
Collaborator Author

lexnv commented Aug 6, 2024

Something must have changed in the latest clippy version, will look into fixing that in a separate PR, then come back to this and rebase on origin/master

lexnv and others added 4 commits August 6, 2024 19:28
@lexnv
Merge branch 'master' into lexnv/transport-state-refactor
d97d8b6
@lexnv
Fix clippy
45110f1 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tranpsort: Easy path for connection limits
13ef0ba 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
quic: Fix clippy
644613e 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Copy link
Collaborator Author

lexnv commented Aug 6, 2024

Some of the clippy were introduced in this PR, so I decided to fix them here.
Should be ready for review 🙏

@dmitry-markin dmitry-markin self-requested a review August 6, 2024 17:01
@lexnv
Copy link
Collaborator Author

lexnv commented Aug 7, 2024

Screenshot 2024-08-07 at 11 14 15

In the left side of the gap, we have a litep2p node running without this PR, on the right side the litep2p is running with this PR. The CPU performance looks similar between nodes. (magenta is libp2p)

@lexnv lexnv mentioned this pull request Aug 7, 2024
Open
dmitry-markin
dmitry-markin approved these changes Aug 7, 2024
View reviewed changes
Copy link
Collaborator

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! 👍

src/transport/webrtc/mod.rs Show resolved Hide resolved
src/transport/webrtc/mod.rs Show resolved Hide resolved
src/transport/websocket/mod.rs Outdated Show resolved Hide resolved
src/transport/websocket/mod.rs Outdated Show resolved Hide resolved
lexnv and others added 4 commits August 7, 2024 20:36
@lexnv
webrtc: Add debug asserts for unreachable code paths
c06220a 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv @dmitry-markin
Update src/transport/websocket/mod.rs
366b0a8 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv @dmitry-markin
Update src/transport/websocket/mod.rs
8522876 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv
Merge remote-tracking branch 'origin/lexnv/transport-state-refactor'…
2b2a1c6 
… into lexnv/transport-state-refactor
@lexnv lexnv merged commit 6ffcc85 into master Aug 7, 2024
8 checks passed
@lexnv lexnv deleted the lexnv/transport-state-refactor branch August 7, 2024 17:57
@lexnv lexnv mentioned this pull request Aug 7, 2024
Closed
@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

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

Assignees

@lexnv lexnv

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

transport/manager: Add ability to accept/reject incoming connections before negotiation
2 participants
@lexnv @dmitry-markin