feat(ping): making the ignoring of the first error configurable

[stormshield-frb]

Description

Fix #5004

Notes & open questions

Instead of using a silent_first_error: bool, we could also use a ignored_errors_nb: usize. However, since there was previously a self.config.max_failures and it was removed, I did not re-do it. Of course, if it is preferred I can do it.

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

[thomaseizinger]

I am overall okay with this but I think we should name this something like a "compatibility"-mode with implementations that use one stream per ping.

Maybe something like:

pub fn with_allow_new_stream_per_ping() { }

In the rust docs, we can then explain that this is for compatibility with other implementations.

I don't actually know what the recommended way is. In general, we prefer short-lived streams. I think we could actually remove a lot of this code if we'd move away from a long-lived ping stream. Old rust-libp2p clients are already compatible with this.

@mxinden What do you think? Do you have any historical knowledge on this?

[mxinden]

@mxinden What do you think? Do you have any historical knowledge on this?

No historical knowledge beyond what is posted above. I am fine with either direction. Preference for no additional config flag if possible.

[thomaseizinger]

@mxinden What do you think? Do you have any historical knowledge on this?

No historical knowledge beyond what is posted above. I am fine with either direction. Preference for no additional config flag if possible.

Okay thank you! Re-reading the spec, I think it is safe to move away from a long-lived stream and instead only ever send a single ping and close the stream afterwards. @stormshield-frb Would you mind re-writing this PR to do that? I think that should also solve #5004.

[stormshield-frb]

About moving away from long-lived stream, will it not be a costly to open and close streams with TCP or Yamux for example ? I understand that it is almost free with QUIC but unfortunately it is not the case with every transports. What do you think about it ?

[thomaseizinger]

About moving away from long-lived stream, will it not be a costly to open and close streams with TCP or Yamux for example ? I understand that it is almost free with QUIC but unfortunately it is not the case with every transports. What do you think about it ?

Yamux streams are similarly cheap to QUIC streams! :)

[mergify]

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏