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

errors: Add DialError error and ListDialFailures event for better error reporting #206

Merged
merged 34 commits into from
Aug 21, 2024
Merged

errors: Add DialError error and ListDialFailures event for better error reporting #206

merged 34 commits into from
Aug 21, 2024

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Aug 14, 2024

The purpose of this PR is to pave the way for making the Identify protocol more robust, which is currently linked with the low number of peers and connective issues over a long period of time

This PR adds a coherent DialError that exposes the minimal information users need to know about dial failures.

A new litep2p event is added for reporting multiple dial errors that occur on different protocols back to the user:

    /// A list of multiple dial failures.
    ListDialFailures {
        /// List of errors.
        ///
        /// Depending on the transport, the address might be different for each error.
        errors: Vec<(Multiaddr, DialError)>,
    },

This event eases the debugging of substrate connectivity issues. At the same time, it can be used in a future PR to inform back to the Identify protocol which self-reported addresses of some peers are unreachable:

Next Steps

  • Add more tests
  • Warp sync + sync full nodes since this is touching individual transports

Future Work

@lexnv
error: Group error messages by functionality
b290160 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
crypto: Use NegotiationError instead of overarching Error
d258898 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tcp: Use SubstreamError instead of Error
75fe484 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
proto: Use SubstreamError in the TransportEvents
86dcc6d 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tcp/common: Use AddressError instead of Error
030889f 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tcp/common: Add specific error for DNS lookup
88da063 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tcp: Adopt new Dialerror
d1cca42 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
crypto/ed25519: Return a less descriptive error to avoid leakage
288cb58 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport: Introduce ListDialFailures event for multi dial failures
8271d71 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport: Untrack dial errors on the first connection success
2295001 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket: Adjust to the new error intraface
b0d1cd9 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
quic: Adjust to the new error intraface
412d270 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
crypto/noise: Adjust webrtc path for errors
0a3089e 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
webrtc: Adjust to the new error intraface
ed0826b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport: Fix clippy
dd45588 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
tests: Adjust testing
c9e7aff 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
error: Add documentation and cleanup variants
409b2d8 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport/tests: Check open failure propagates multiple errors
aed15ba 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
error: Fix typo
5ca073e 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
dmitry-markin
dmitry-markin approved these changes Aug 20, 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.

Good job and well done here! 🎉

src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/transport/manager/mod.rs
"open failure, but not the last transport",
);

self.opening_errors.entry(connection_id).or_default().extend(errors);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just thinking out loud: if the connection fails, we report all the errors for all the addresses, but if the connection succeeds, we don't report the errors for other addresses tried and failed. Feels like a little bit of asymmetry to me. Do we really need to report individual errors for all the addresses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds reasonable, we can probably look into extending a DialSuccess with a list of failed to dial addresses. Don't quite remember if libp2p is doing a similar thing, but that sounds familiar. Yep, wanted to catch a bit more details into why we have seen so many failures from our metrics, the UnknownError was a bit too cryptic :D

src/transport/websocket/connection.rs Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/transport/websocket/mod.rs Show resolved Hide resolved
lexnv and others added 10 commits August 21, 2024 11:14
@lexnv @dmitry-markin
Update src/error.rs
2dc3d5c 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv @dmitry-markin
Update src/error.rs
b858c4a 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv @dmitry-markin
Update src/error.rs
dcfe041 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv
protocol: Use thiserr
de1044d 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
websocket: Trace after stream conversion
57d271f 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport: Trace when the pending dial is not found
0e2c5a8 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
error: Invalid address for protocol message
e6a4b98 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
dns: Ip version mismatch
3f31972 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
error: Cleanup from implementations
6dec057 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
error: Use websocket wrapper error instead of from impl
bb3ca5c 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Fix clippy
adfc766 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
error: Remove invalid hash from parseError and use AddressError instead
c1d99c7 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
error: Address invalid multihash rename to invalid peerid
9b42793 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
transport: Pending inbounds connections don't have a dial associated
3696ab9 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
Merge remote-tracking branch 'origin/master' into lexnv/impr-errors
47c10d8
@lexnv lexnv merged commit 677e151 into master Aug 21, 2024
8 checks passed
@lexnv lexnv deleted the lexnv/impr-errors branch August 21, 2024 15:47
@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>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/litep2p-network-backend-updates/9973/1

@lexnv lexnv mentioned this pull request Oct 16, 2024
Open
@lexnv lexnv self-assigned this Oct 16, 2024
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
None yet
Projects
None yet
Milestone
No milestone
Development

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