Muxer early negotiation integration test

[julian88110]

Integration test for early muxer negotiation in security handshake.

[marten-seemann]

I must have missed this in my first review, sorry for that.
Why is the TransportWithMuxer needed? It seems like defining a new SecureTransport here defeats the purpose of an integration test: We should just be testing a host constructed with standard components using libp2p.New.

[julian88110]

I must have missed this in my first review, sorry for that. Why is the TransportWithMuxer needed? It seems like defining a new SecureTransport here defeats the purpose of an integration test: We should just be testing a host constructed with standard components using libp2p.New.

From host point of view, we get exactly the same host by either supplying the security transport constructor or by supplying a security transport directly. The reason for TransportWithMuxer is to provide a pass-through wrapper to the security transport to enable some visibility of security transport state such as NextProto selected. Otherwise we don't have direct access to the internal state of selected muxer from a host, and that makes the test less direct. If we don't have the internal state of the security transport, it is not possible to know if the early muxer negotiation succeeded or not. Yes, we provided a pass-through muxer wrapper, but the system behavior is not changed, so it does not compromise any test fidelity.

[marten-seemann]

The TransportWithMuxer has some significant complexity, and doesn't reflect very well what a host constructed with libp2p.New would do. For example, this is not how the security protocol is selected. As such, this is not a proper integration test. What we should do is have a test that constructs two hosts via:

h, err := libp2p.New(
    libp2p.Transport(tcp.New),
    libp2p.Security(...), // TLS, Noise, or Plaintext
    libp2p.Muxer(...), // mplex and yamux, ordered by priority, in different combinations for client and server
)

We then connect the two hosts and observe the result of the negotiation.

In order to satisfy the requirements listed in #1836 (comment), we need to make sure that the correct muxer is selected, specifically, that the same muxer is selected no matter if we run on top of TLS, Noise or Plaintext (which would exercise the non-optimized code path and do the negotiation via multistream).
We can find out which muxer was selected by exposing it somewhere, of by using reflections on the connection that was returned.

[BigLep]

The TransportWithMuxer has some significant complexity, and doesn't reflect very well what a host constructed with libp2p.New would do. For example, this is not how the security protocol is selected.

I'm doing a drive-by and I obviously don't have experience on the codebase (or with go), but I'm surprised by this. In my reading of the PR as it stands today:

1. The test setup is using public libp2p API (libp2p.new with a passed in libp2p.Muxer to "inject" a TransportWithMuxer so we can capture what is happening when the two nodes do their security handshake.).
2. TransportWithMuxer doesn't seem to have complex logic to me. It seems to be a simple wrapper around a SecureTransport for capturing the selected Muxer. (maybe some top-level comments would help with clarity?)

I'm only commenting here because it seems like this integration test is snowballing and dragging on. If we're asserting the wrong thing in the test, then that is one thing and needs to be fixed. But if we're 1) not doing gross hacks like reflection in our test setup or as part of asserts and 2) we're asserting the right muxer is being selected, then I'm hoping we can get this merged without a lot of extra refactoring.

[BigLep]

This is the critical part of the test. I think readability could be increased if we had something like:

mplexEntity = {"/mplex/6.7.0", mplex.DefaultTransport}
yamuxEntity = {"/yamux/1.0.0", yamux.DefaultTransport}

(I don't know proper go syntax)

The goal is to get to something like the following that reduces the clutter/boilerplate so the inputs and expected outputs are really clear:

testCases := []muxerTest{
		{svrMuxers: mplexEntity,
			cliMuxers:     yamuxEntity,
                         expectedMuxer: ""},
                  // etc

[marten-seemann]

2. TransportWithMuxer doesn't seem to have complex logic to me. It seems to be a simple wrapper around a SecureTransport for capturing the selected Muxer. (maybe some top-level comments would help with clarity?)

It's the constructor of TransportWithMuxer (the New method) that's problematic, as it constructs the security implementation (TLS / Noise)l: https://github.com/libp2p/go-libp2p/pull/1840/files#diff-6d7556f17e0bef65cc1474e0c2bd6bd5030146d5c89c7a119928393557c13eccR40-R44. This is problematic since the security implementations needs to be constructed with the slice of the muxers, and the order of the items in the slice expresses the preference. For example here:

go-libp2p/p2p/security/tls/transport.go

Line 35 in c334288

func New(key ci.PrivKey, muxers []protocol.ID) (*Transport, error) {

This test bypasses the normal code path used for the construction of the security implementation.

---

An integration test for muxer negotiation at the bare minimum needs to cover (using the exact same code path that libp2p.New uses):

1. initialization of the security implementation (TLS, Noise) and
2. exercise the negotiation

As it stands, this test doesn't do (1).

If we're asserting the wrong thing in the test, then that is one thing and needs to be fixed. But if we're 1) not doing gross hacks like reflection in our test setup or as part of asserts and 2) we're asserting the right muxer is being selected, then I'm hoping we can get this merged without a lot of extra refactoring.

I'm open to different ways of asserting that the negotiation did the right thing. I don't have any particularly strong dislike of reflections in test code. If that's the easiest way to do this, so be it.
On the other hand, code that doesn't use reflections is always preferred to code that does :) And we'll need to expose the muxer anyway as part of our swarm metrics effort, which is on our roadmap for this quarter.