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

Make transports optional #192

Merged
merged 22 commits into from
Aug 6, 2024
Merged

Make transports optional #192

merged 22 commits into from
Aug 6, 2024

Conversation

dmitry-markin
Copy link
Collaborator

Put extra transports behind feature flags to make some dependencies optional.

While at it, also made sure no warnings are emitted on the recent rust version.

Resolves #161.

@dmitry-markin dmitry-markin added the enhancement New feature or request label Aug 2, 2024
@dmitry-markin dmitry-markin self-assigned this Aug 2, 2024
lexnv
lexnv reviewed Aug 6, 2024
View reviewed changes
tests/connection/mod.rs
Litep2p, Litep2pEvent, PeerId,
};

#[cfg(feature = "websocket")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It might be worth having this module structured as:

  • mod.rs to bridge the quic.rs websocket.rs and tcp.rs depending on feature flags; and here we can have the common logic as well
  • then have the tests moved to individual files so that the only cfg we state is in mod.rs
#mod.rs

#[cfg(feature = "quic")]
use quic.rs
...

We can also handle this later since I expect to run into some edge-cases along the way which may involve a closer refactor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about putting quick/websocket/webrtc tests into dedicated files, but it looks like the tests only differ in the transport used, and belong better to the connection/protocol test files. I.e., it would be easier to have these tests along each other in the same file. I will think if there is another alternative to having cfg spread over the files. Let me know if you have anything in mind

lexnv
lexnv reviewed Aug 6, 2024
View reviewed changes
tests/protocol/identify.rs Outdated
WebSocket(WebSocketConfig),
}

fn add_transport(config: ConfigBuilder, transport: Transport) -> ConfigBuilder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: By looking at this fn, I believe we can extract some common bits into tests/common.rs? Can be done as a followup :D

Copy link
Collaborator Author

@dmitry-markin dmitry-markin Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Done. While doing this I ran into issue that tests/common.rs is considered to be a test of it's own, and rust generates dead code warnings. So I ended up putting it in tests/common/mod.rs.

lexnv
lexnv approved these changes Aug 6, 2024
View reviewed changes
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! Nice work here 👍

I like that we guard dependencies by feature flags. This should be quite important for users that only desire individual transports. It adds a bit of complexity for us in managing feature-flags, and we can look into the future at structuring our testing in a friendlier way for us, although put in balance I believe this is good 🙏

@dmitry-markin dmitry-markin merged commit 3b664f7 into master Aug 6, 2024
8 checks passed
@dmitry-markin dmitry-markin deleted the dm-optional-transports branch August 6, 2024 16:18
@nazar-pc
Copy link

nazar-pc commented Aug 6, 2024

Thank you, very appreciated!

@nazar-pc nazar-pc mentioned this pull request Aug 19, 2024
Open
@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

@lexnv lexnv lexnv approved these changes

Assignees

@dmitry-markin dmitry-markin

Labels
enhancement New feature or request
Projects
Networking
Status: Blocked ⛔️
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make features optional
3 participants
@dmitry-markin @nazar-pc @lexnv