feat(ping): don't close connections upon failures

[thomaseizinger]

Description

Previously, the libp2p-ping module came with a policy to close a connection after X failed pings. This is only one of many possible policies on how users would want to do connection management.

We remove this policy without a replacement. If users wish to restore this functionality, they can easily implement such policy themselves: The default value of max_failures was 1. To restore the previous functionality users can simply close the connection upon the first received ping error.

In this same patch, we also simplify the API of ping::Event by removing the layer of ping::Success and instead reporting the RTT to the peer directly.

Related: #3591.

Notes & open questions

Patch-by-patch review is recommended.

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[mxinden]

Do I understand correctly, that we expect all Transport implementations, e.g. TCP and QUIC, to close a malfunctioning connection?

If yes, fine with me to proceed with this pull request.

If not, I would expect all users to want some mechanism along the lines of what libp2p-ping provides today, namely to close malfunctioning connections, and thus I would advocate for keeping the mechanism in-place here.

[thomaseizinger]

If not, I would expect all users to want some mechanism along the lines of what libp2p-ping provides today, namely to close malfunctioning connections, and thus I would advocate for keeping the mechanism in-place here.

Define malfunctioning?

I don't think libp2p-ping is a suitable way of identifying a malfunctioning connection:

1. A remote peer is not guaranteed to support libp2p-ping, hence we cannot rely on it actually operating.
2. A remote peer may deprioritize ping messages, i.e. not treat them with the highest priority and thus run into a timeout. That doesn't mean that the underlying connection is faulty.
3. A remote peer may have a bug in the ping implementation but implement other protocols correctly.

I tried to already make a point that equating a working ping with a working connection is a policy and I believe that users should be in charge of policy. Do you not agree with that?

For example, another policy could be to disconnect all peers with a latency higher than 500ms or all that don't have a latency in the 95th percentile of all currently active connections.

Do I understand correctly, that we expect all Transport implementations, e.g. TCP and QUIC, to close a malfunctioning connection?

If they can reliably detect a malfunctioning connection, then yes absolutely.

[thomaseizinger]

For example, another policy could be to disconnect all peers with a latency higher than 500ms or all that don't have a latency in the 95th percentile of all currently active connections.

I am happy to add some more docs to libp2p-ping to explain that.

[mxinden]

Fine with proceeding here. Thanks for the details. Just one thing on how the user should close a specific connection.

[mxinden]

Good to merge from my end.

[mxinden]

Is this backwards compatibility still relevant? This implementation should be compatible with any recent other implementation adhering to the specification, right?

[thomaseizinger]

We could reword it but the functionality still needs to be there. JS for example uses a new stream per ping and we should not report that as an error.