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

transport: Allow manual setting of keep-alive timeout #155

Merged
merged 3 commits into from
Aug 13, 2024

Conversation

Ma233
Copy link
Contributor

@Ma233 Ma233 commented Jun 15, 2024

Allow configure time duration of keep_alive_timeouts in TransportService.
For RequestResponseProtocol, use its timeout configuration.
For other protocols, use the original default 5 seconds.

Related issue: #153

@lexnv
Copy link
Collaborator

lexnv commented Jun 25, 2024

Thanks for contributing! Generally, the approach looks good to me, I'll have another look over the PR soon!

Its always good to have the ability to configure the parameters to make litep2p suitable for more usecases.
I'm wondering, does this approach solve your issue from: #153? (ie being able to configure the keep-alive)

@Ma233
Copy link
Contributor Author

Ma233 commented Jun 26, 2024

@lexnv Hi, yes it solved the issue perfectly. Thank you for your support.

@dmitry-markin
Copy link
Collaborator

I would add one global keep_alive_timeout for all protocols to LiteP2pConfig and a corresponding with_keep_alive_timeout() to ConfigBuilder, and pass this directly to TransportManager upon construction. This way it would allow configuring timeouts for all protocols, and eliminate the need for hardcoding Duration::from_secs(5) in many places.

Otherwise looks good, I think we should add this to litep2p.

@jasl
Copy link

jasl commented Jul 26, 2024

Kindly ping.
Do you think this PR is merge-able?

@dmitry-markin
Copy link
Collaborator

Kindly ping. Do you think this PR is merge-able?

Yes, but it should be done as described in my previous comment.

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.

@lexnv
Copy link
Collaborator

lexnv commented Aug 6, 2024

Thanks a lot for contributing again!

I do like the suggestion from Dmitry, I think we can go that way here.

Please let us know if you have some capacity to implement it, otherwise we can tackle it and include it in our next release (hopefully in the upcoming week) 🙏

// cc @Ma233 @jasl @dmitry-markin let me know if you'd prefer to jump in on this and help with the implementation. We are moving as fast as we safely can and there are a few pending things in flight at the moment, any help is greatly appreciated here 🙏

Allow configure time duration of keep-alive-timeouts in `TransportService`.
For `RequestResponseProtocol`, use its timeout configuration.
For other protocols, use the original default 5 seconds.
@Ma233
Copy link
Contributor Author

Ma233 commented Aug 13, 2024

@dmitry-markin @lexnv Sorry for taking so long to reply to you. I have added a keep_alive_timeout option in Litep2pConfig.

@Ma233
Copy link
Contributor Author

Ma233 commented Aug 13, 2024

And the RequestResponseProtocol does not use its timeout configuration for connection timeout now. It use the keep_alive_timeout option as other protocols.

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/protocol/transport_service.rs Outdated Show resolved Hide resolved
src/transport/manager/mod.rs Outdated Show resolved Hide resolved
src/protocol/transport_service.rs Outdated Show resolved Hide resolved
@dmitry-markin dmitry-markin changed the title transport: Allow manual setting of keep-alive time transport: Allow manual setting of keep-alive timeout Aug 13, 2024
@dmitry-markin
Copy link
Collaborator

@Ma233 could you not force push, as it complicates the review, please?

@Ma233
Copy link
Contributor Author

Ma233 commented Aug 13, 2024

@dmitry-markin Hello, I have separated the changes into a single commit:

  1. Add a KEEP_ALIVE_TIMEOUT const and replace all hard coded time duration with it.
  2. Change the existed keep_alive_timeouts field to pending_keep_alive_timeouts to avoid confusion.

@dmitry-markin
Copy link
Collaborator

Thanks, I will have a look. Please don't force-push to public branches, as I now have to go through all the changes again, instead of just checking the latest commit.

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again for contributing here 🙏

We'll do a release shortly, probably next week or so which would include this PR as well! 👍

@lexnv lexnv merged commit 8d18cb5 into paritytech:master Aug 13, 2024
8 checks passed
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants