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.

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

Add identify protocol to test-plans #91

Open
p-shahi opened this issue Dec 14, 2022 · 5 comments
Open

Add identify protocol to test-plans #91

p-shahi opened this issue Dec 14, 2022 · 5 comments

Comments

@p-shahi
Copy link
Member

p-shahi commented Dec 14, 2022

Ideally our Testground tests would catch issues like this in the future. Given that our current Testground tests run the libp2p-ping protocol only, they have not caught it.

See: libp2p/rust-libp2p#3244

@marten-seemann
Copy link
Contributor

Unless this happens organically, this is probably difficult to write a reliable test case for. We’d need to have one implementation generate a multiaddr that another implementation doesn’t understand. An Identify implementation most likely doesn’t have an API to inject invalid addresses.

It sounds like this would be a good thing to test in a unit test though.

@thomaseizinger
Copy link
Contributor

This particular issue could be caught if our test binaries would listen all interfaces they support. A node supporting quic would then return that in its identify payload, even if the test says use TCP as the transport. Running this against a node that doesn't speak QUIC would have caught this problem.

Would it be a good idea to always enable support for everything protocol we have on the listener side and only use the env variable on the dialer side to limit support for a specific stack?

@marten-seemann
Copy link
Contributor

There’s two types of „unsupported addresses“:

  1. addresses of transports that you’re not listening on
  2. addresses containing multiaddress components you don’t know about. This happens when we define new transports in the future.

(1) is easy to test, (2) is more challenging. If I understand correctly, the nature of the failure in libp2p/rust-libp2p#3244 was of the second nature.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Feb 1, 2023

Correct. If a go node listens on QUIC and TCP and is paired with a Rust v0.49.0 node (doesn't support QUIC) to test the TCP + yamux setup, we would have noticed that an incoming identify payload with a QUIC address in it causes problems.

@marten-seemann
Copy link
Contributor

That’s more of a coincidence though. We have no way of testing this reliably, i.e. test that the most recent version of rust-libp2p (or go-libp2p, for that matter), doesn’t stumble over this (unless we add another transport with its own multiaddr).

To be clear, having Identify tests is useful anyway, and I think we should add a test plan. However, there are failure modes that are easy to test in interop testing and other that are not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants