feat(multistream-select): Use Version::V1Lazy by default

[tcoratger]

Description

To fulfill #3563, Version::V1Lazy is set as default.

Notes & open questions

Change checklist

• I have performed a self-review of my own code
• A changelog entry has been made in the appropriate crates

[tcoratger]

There are still several places in the code where V1 is still used (e.g. as upgrade(Version::V1)), but I don't think that should be changed, which would totally depreciate V1 while we are just looking here to place V1Lazy as the default version for this PR.

[tcoratger]

As far as I understand, in this situation, with Version::V1Lazy as default, the test unsupported_doesnt_fail is not valid anymore since after the first swarm2.next_swarm_event().await, it directly throws a SwarmEvent::ConnectionClosed and not a Err(ping::Failure::Unsupported) as it was the case with Version::V1. So, for me, there are two options here:

1. We modify the test so that it confirms that the expected return is a ConnectionClosed event.
2. We remove this test as it is deprecated after the update from Version::V1 to Version::V1Lazy.
   What do you think?

[thomaseizinger]

As far as I understand, in this situation, with Version::V1Lazy as default, the test unsupported_doesnt_fail is not valid anymore since after the first swarm2.next_swarm_event().await, it directly throws a SwarmEvent::ConnectionClosed and not a Err(ping::Failure::Unsupported) as it was the case with Version::V1. So, for me, there are two options here:

1. We modify the test so that it confirms that the expected return is a `ConnectionClosed` event.

2. We remove this test as it is deprecated after the update from `Version::V1` to `Version::V1Lazy`.
   What do you think?

This is interesting. The point of the test unsupported_doesnt_fail test is to ensure we don't close a connection just because the other peer doesn't support the ping protocol. If we are closing the connection now, then that is a bug!

[tcoratger]

This is interesting. The point of the test unsupported_doesnt_fail test is to ensure we don't close a connection just because the other peer doesn't support the ping protocol. If we are closing the connection now, then that is a bug!

I don't have enough experience with the lib to confirm that there is a bug here. But here is my opinion:

• The main difference between Version::V1 and Version::V1Lazy is in the poll function of the DialerSelectFuture module:

match this.version {
  Version::V1 => *this.state = State::FlushProtocol { io, protocol },
  // This is the only effect that `V1Lazy` has compared to `V1`:
  // Optimistically settling on the only protocol that
  // the dialer supports for this negotiation. Notably,
  // the dialer expects a regular `V1` response.
  Version::V1Lazy => {
      log::debug!("Dialer: Expecting proposed protocol: {}", p);
      let hl = HeaderLine::from(Version::V1Lazy);
      let io = Negotiated::expecting(io.into_reader(), p, Some(hl));
      return Poll::Ready(Ok((protocol, io)));
  }
}              

• Do you think that there is something to modify here? Or maybe in the rest of the function to take into account the Version::V1Lazy (maybe part of the poll function is created according to Version::V1 rules with a problem when switching to Version::V1Lazy)

What do you think?

[thomaseizinger]

As @mxinden wrote in #3563 (comment), I think the ideal way of implementing this is to remove the Upgrade enum entirely and make V1Lazy the default.

The "footguns" documented in the docs of the variant should be able to be worked around with as an implementation of multistream-select.

It is a bit more involved than just changing the enum around but it'd be much appreciated. multistream-select in itself is fairly contained so you don't need to learn all of rust-libp2p to make this change.

I'd probably start by writing some more tests for multistream-select!

[tcoratger]

As @mxinden wrote in #3563 (comment), I think the ideal way of implementing this is to remove the Upgrade enum entirely and make V1Lazy the default.

Got it, I can work on this topic. So if I understand correctly, the interest here is to always use the V1Lazy to speed up the protocol and be able to send a first data packet before the receiver has accepted the protocol, optimistically so. For that we need to:

• Remove the Version enum in the multistream-select protocol.
• Remove the implementation of Version because there is no Version anymore.
• Remove everything related to upgrade::Version.
• More generally, change all the places where Version appears in the codebase to remove this variable as we always use V1Lazy now.

Regarding the approach, you indicate to start by adding more tests in multistream-select, I imagine that it is thus a question of securing the implementation of V1Lazy before completely removing V1 from the code.

• To clarify my ideas, are you thinking of any tests in particular (maybe a short list here could be useful to start)?
• I think we must first correct what is happening on the test that fails, what do you think?

[thomaseizinger]

Yep, you are fully on point.

The main issue is described here: multiformats/go-multistream#20

I think it would be good to construct a test that reproduces this edge-case.

To fix it, we'd have to internally listen for the na and if it is sent on a stream that we thought we had negotiated, we reset the stream.

It'd be cool if you could tackle this! It is quite involved so I think the best thing would be in a first iteration, to not make a breaking API change but instead offer a new API on multistream-select. Then, in a follow-up, we can remove the enum and make breaking changes. Those are gonna scatter through the codebase so it'd be good to separate the actual functional change from that.

Let me know if you are still up for that!

[mergify]

This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏

[mxinden]

Agreed to all above.

Meta level: Thanks @tcoratger and @thomaseizinger for the work here. I am sure a lot of future newcomers will appreciate it, even though I guess they will lively never know as the option will then be gone 🙂.

[thomaseizinger]

Closing this as stale. The conversation in here is valuable though so I'll reference it in the issue.

[mxinden]

To fix it, we'd have to internally listen for the na and if it is sent on a stream that we thought we had negotiated, we reset the stream.

That is the case today. In Negotiated when reading a Message::NotAvailable we return an Error.

rust-libp2p/misc/multistream-select/src/negotiated.rs

Line 182 in e545805

return Poll::Ready(Err(NegotiationError::Failed));

Does that address your request @thomaseizinger?

The main issue is described here: multiformats/go-multistream#20

If I recall correctly, the unsoundness can only occur on directly nested protocol negotiation (see also multiformats/go-multistream#20 (comment)). In other words, this is only unsound when one first negotiates protocol A and then directly thereafter without any other protocol-A specific message negotiates protocol A1.

I am not aware of a scenario where this can occur today. I am not aware of any directly-nested multistream-select usage. Are you @thomaseizinger?

With the above in mind, I don't think we need to address the (theoretical) unsoundness documented in multiformats/go-multistream#20.