Added kotlin-libp2p to test-plans

[erwin-kok]

Added kotlin-libp2p to test-plans for interoperability testing.

[erwin-kok]

@mxinden I just created a draft pull request, I just want to test first before its ready for review.
Testing locally works, in GitHub actions not sure yet.

Is it sufficient to add the new implementation in versions.ts?
{
id: "kotlin-v0.2.0",
transports: ["tcp"],
secureChannels: ["noise"],
muxers: ["mplex"],
},

And, of course, the impl/kotlin/** folders. Is anything more needed?

[mxinden]

🚀

You need to extend https://github.com/libp2p/test-plans/blob/master/multidim-interop/Makefile with your implementation.

[erwin-kok]

Thanks @mxinden !

I missed that one. Just added it and pushed.

[erwin-kok]

@mxinden The build failed because the versionFolder (line 21 versions.ts) does not expect the patch version.
I dropped the patch version in the latest commit.

[MarcoPolo]

Optional:

If you want to specify a branch instead of a commit sha, you can follow the pattern of creating a version.lock from here: https://github.com/libp2p/test-plans/pull/252/files#diff-ec9982cd1fe2ff166978a9f012d8c250aa7b2181d814eedd05dc2af191d02f89R20.

It creates a lock file that locks the expected sha256 of the contents of a branch.

[erwin-kok]

Thanks for this suggestion, I added this mechanism as well.

Btw @MarcoPolo , two remarks:

• If you put a '-' in front of a command in Makefile, it ignores when it fails. So, I do:
  -rm image.json
  -rm kotlin-libp2p-.zip
  -rm -rf kotlin-libp2p-
  If "image.json" is not present, it still continues removing the other files.
  This is different then in the Go Makefile. Just a suggestion (I can add it there as well if wanted)

• In generator.ts the "redis_addr" was missing. Many/all implementations defaults on the address being "redis:6379". So, that works. In my implementation, the default was "localhost:6379" (to test locally), therefore it was not working in the GitHub build. Or do I miss something?

[MarcoPolo]

1. Good to know. I didn’t know that. Thanks.
2. The default is defined here: https://github.com/libp2p/test-plans/blob/master/multidim-interop/README.md. By default it should be redis:6379. In retrospect using local host may have been nicer, although I’m not sure it’s worth the effort to change and back port.

[erwin-kok]

Thanks @MarcoPolo . Can you or @mxinden trigger a new build?
(https://github.com/libp2p/test-plans/actions/runs/5900727248)

[erwin-kok]

Thanks for triggering the build @MarcoPolo !
Almost... I see that rust-v0.52 --> kotlin-v.0.2 doesn't work yet
So, I have something to work on... :-)

[mxinden]

If you put a '-' in front of a command in Makefile, it ignores when it fails. So, I do:

Today I learned. Thanks!

[mxinden]

I like the approach you are taking here, namely to integrate your new implementation into the interop test-suite early on.

[erwin-kok]

I like the approach you are taking here, namely to integrate your new implementation into the interop test-suite early on.

To be honest, I wasn't aware of the test-plans before you mentioned it in your mail.
But indeed, I hope (after the remaining issue is solved) we can regularly test to be sure it stays stable and behaving as expected. This gives us some insights.

[mxinden]

I hope (after the remaining issue is solved) we can regularly test to be sure it stays stable and behaving as expected.

In most implementations we run these tests on every pull request @erwin-kok.

https://github.com/libp2p/rust-libp2p/blob/master/.github/workflows/interop-test.yml

[erwin-kok]

@mxinden There is still one issue left for me to solve the interoperability test. And unfortunately, I didn't have much time to work on it, until now...

I tried to reproduce it locally, and it seems that the problem is that the rust implementation requests (and waits) for an identify stream ("/ipfs/id/1.0.0") before requesting the ping stream. However, the kotlin-libp2p does not support identify streams yet.

I understand the identify stream is mandatory and the first requested stream, so going to implement it (before implementing Quic)... :-) So much to do... :-)

[mxinden]

I tried to reproduce it locally, and it seems that the problem is that the rust implementation requests (and waits) for an identify stream ("/ipfs/id/1.0.0") before requesting the ping stream. However, the kotlin-libp2p does not support identify streams yet.

That would surprise me. As far as I am aware the rust-libp2p interop binary does not require the remote to support libp2p-identify.

https://github.com/libp2p/rust-libp2p/blob/e974efb7558a88195a36647c3d6af4ca00c50bf3/interop-tests/src/lib.rs#L38-L39

What makes you think it does @erwin-kok?

[erwin-kok]

I tried to reproduce it locally, and it seems that the problem is that the rust implementation requests (and waits) for an identify stream ("/ipfs/id/1.0.0") before requesting the ping stream. However, the kotlin-libp2p does not support identify streams yet.

That would surprise me. As far as I am aware the rust-libp2p interop binary does not require the remote to support libp2p-identify.

https://github.com/libp2p/rust-libp2p/blob/e974efb7558a88195a36647c3d6af4ca00c50bf3/interop-tests/src/lib.rs#L38-L39

What makes you think it does @erwin-kok?

Mmm... perhaps I'm mistaken. I added some logging on my side to see which protocols are requested. And I thought "/ipfs/id/1.0.0" was one of them. Ok, let me dig a bit deeper to see what the problem is.

[thomaseizinger]

to see which protocols are requested.

Perhaps there is a misunderstanding here but libp2p does not have a notion of requesting protocols. Instead, nodes offer certain protocols. This basically means that if you open a stream and run multistream-select with one or more of the offered protocols, the negotiation will succeed to the first matching one.

The list of supported/offered protocols is only advisory though. Your implementation must gracefully handle the case where a node attempts to open a stream to you with a protocol you don't support.

The rust-libp2p interop test binary will attempt to open a stream with the identify protocol to you. We had to add that because there is / was(?) a race condition in nim-libp2p that required it. Your implementation can simply ignore that stream though.

The only thing that is required for the interop test is that you correctly handle the ping protocol.

[thomaseizinger]

I understand the identify stream is mandatory and the first requested stream

This is definitely not the case and I'd advise caution in making it a requirement in your implementation because it will add 1 RTT to every established connection.

[erwin-kok]

to see which protocols are requested.

Perhaps there is a misunderstanding here but libp2p does not have a notion of requesting protocols. Instead, nodes offer certain protocols. This basically means that if you open a stream and run multistream-select with one or more of the offered protocols, the negotiation will succeed to the first matching one.

I understand what you say, however, it feels strange. If a client opens a new stream, it provides the PeerId to which it want to connect, together with a set of protocols. Using ms-select a negotiation takes place and one of the provided protocols is selected (or none if nothing matches). The provided set of protocols is in my understanding the requested protocols, as in: "Dear peer, I want to open a connection with you and can we agree on the following protocols?".

I was logging on my side what was happening during negotiation. The peer that was dialing to me (I was the listener) requested(/offered?) certain protocols. In my view the dialer (or better: the one that opens a stream) request some protocols, and the listener offers protocols, as in "I don't mind you can try to connect with my with any protocol you like, but I only offer these".

Probably we are talking about the same thing. Sorry for the miscommunication.

The rust-libp2p interop test binary will attempt to open a stream with the identify protocol to you. We had to add that because there is / was(?) a race condition in nim-libp2p that required it. Your implementation can simply ignore that stream though.

Yes, this is what I thought I saw. I added a dummy identify protocol, doing nothing. Only accepting the stream associated with the identify protocol tag and closing it immediately. i.e. not really implementing the protocol. Once I added this, it continued and succeeded.

The only thing that is required for the interop test is that you correctly handle the ping protocol.

Ok, thanks. I thought that every client, as a minimum, had to implement the identify protocol. I was mistaken.

[thomaseizinger]

The rust-libp2p interop test binary will attempt to open a stream with the identify protocol to you. We had to add that because there is / was(?) a race condition in nim-libp2p that required it. Your implementation can simply ignore that stream though.

Yes, this is what I thought I saw. I added a dummy identify protocol, doing nothing. Only accepting the stream associated with the identify protocol tag and closing it immediately. i.e. not really implementing the protocol. Once I added this, it continued and succeeded.

This is very surprising to me and I can hardly believe that that is the issue. If the remote doesn't support identify, we report that and try again: https://github.com/libp2p/rust-libp2p/blob/e8759c85c278006f5fc94e823c2a3620abaaf697/protocols/identify/src/handler.rs#L200

Which version were you testing against?

[thomaseizinger]

to see which protocols are requested.

Perhaps there is a misunderstanding here but libp2p does not have a notion of requesting protocols. Instead, nodes offer certain protocols. This basically means that if you open a stream and run multistream-select with one or more of the offered protocols, the negotiation will succeed to the first matching one.

I understand what you say, however, it feels strange. If a client opens a new stream, it provides the PeerId to which it want to connect, together with a set of protocols. Using ms-select a negotiation takes place and one of the provided protocols is selected (or none if nothing matches). The provided set of protocols is in my understanding the requested protocols, as in: "Dear peer, I want to open a connection with you and can we agree on the following protocols?".

I was logging on my side what was happening during negotiation. The peer that was dialing to me (I was the listener) requested(/offered?) certain protocols. In my view the dialer (or better: the one that opens a stream) request some protocols, and the listener offers protocols, as in "I don't mind you can try to connect with my with any protocol you like, but I only offer these".

Probably we are talking about the same thing. Sorry for the miscommunication.

I think we are talking about the same thing. However, most nodes will only send a single protocol when they want to open a stream so they rarely are requesting a list of protocols.

Sending multiple usually only happens for the muxer or security protocol handshake.

[erwin-kok]

The rust-libp2p interop test binary will attempt to open a stream with the identify protocol to you. We had to add that because there is / was(?) a race condition in nim-libp2p that required it. Your implementation can simply ignore that stream though.

Yes, this is what I thought I saw. I added a dummy identify protocol, doing nothing. Only accepting the stream associated with the identify protocol tag and closing it immediately. i.e. not really implementing the protocol. Once I added this, it continued and succeeded.

This is very surprising to me and I can hardly believe that that is the issue. If the remote doesn't support identify, we report that and try again: https://github.com/libp2p/rust-libp2p/blob/e8759c85c278006f5fc94e823c2a3620abaaf697/protocols/identify/src/handler.rs#L200

Which version were you testing against?

Ok, I will retry this scenario again (probably tomorrow/weekend) and come back to you.

[erwin-kok]

The rust-libp2p interop test binary will attempt to open a stream with the identify protocol to you. We had to add that because there is / was(?) a race condition in nim-libp2p that required it. Your implementation can simply ignore that stream though.

Yes, this is what I thought I saw. I added a dummy identify protocol, doing nothing. Only accepting the stream associated with the identify protocol tag and closing it immediately. i.e. not really implementing the protocol. Once I added this, it continued and succeeded.

This is very surprising to me and I can hardly believe that that is the issue. If the remote doesn't support identify, we report that and try again: https://github.com/libp2p/rust-libp2p/blob/e8759c85c278006f5fc94e823c2a3620abaaf697/protocols/identify/src/handler.rs#L200
Which version were you testing against?

Ok, I will retry this scenario again (probably tomorrow/weekend) and come back to you.

@thomaseizinger @mxinden

I looked at bit better at the issue. When rust (v0.52) opens a stream with a protocol that I do not support, the following happens:

When I receive a protocol that I do not support, I wait in the negotiating phase. Until either: a protocol is received that I do support, or a timeout expires. (This looks like the same behaviour as in https://github.com/multiformats/go-multistream/blob/master/multistream.go#L194). In the current scenario, no matching protocol comes by so I wait until the timeout expires. When the timeout expires (1 minute) I reset the stream from my side.

It seems that rust also has a timeout (a few seconds) and when this expires you reset the stream from your side. If this happens, it seems to hang. I receive the stream resets from your side. Perhaps you wait for something that I forgot to send? Also, when you receive a "NA", what happens on your side (before you reset the stream)? Do you expect when an implementation does not support a protocol, it resets the stream immediately?

If I lower my timeout to occur before your timeout happens, I reset the stream before you do. Then everything is without problems.

[mxinden]

Do you expect when an implementation does not support a protocol, it resets the stream immediately?

It should not reset the stream. The initiator might still have other protocols to suggest.

If this happens, it seems to hang.

Can you describe what exactly "hang"s?

[erwin-kok]

Do you expect when an implementation does not support a protocol, it resets the stream immediately?

It should not reset the stream. The initiator might still have other protocols to suggest.

Yes, okay makes sense.

If this happens, it seems to hang.

Can you describe what exactly "hang"s?

@mxinden
I found the problem! The problem was that the rust implementation tries an identify stream (that I do not support) and my parts waits for another request, and the rust part eventually reset the stream.

However, due to a bug on my side, when I receive a stream reset an exception was thrown which caused that also the connection was closed where the stream was part of.

Side note (I'm not sure, don't spend too much time on this): After the rust implementation resets the identify stream it tries to continue with opening a ping stream. However, the connection was closed from my side (by mistake). So also the ping fails. It seemed that the rust implementation was "hanging" (waiting for something). Probably a timeout. However, after 20 minutes, it was still waiting.

Can someone please trigger?:
https://github.com/libp2p/test-plans/actions/runs/6025353126

[erwin-kok]

I added a docker-compose file for convenience to be able to test a ping locally on your own system. However, I put it in the wrong directory (impl), thats why the builded failed. Btw, if this is not wanted/needed I can remove it.

Can somebody trigger a build? @MarcoPolo @p-shahi @mxinden ?
https://github.com/libp2p/test-plans/actions/runs/6037351379

Thanks!

[erwin-kok]

All green! 😃

I set the PR to "ready for review". If somebody can have a look... @mxinden @MarcoPolo

Btw,I added a docker-compose file for convenience to be able to test a ping locally on your own system. I thought it was handy for testing. If this is not wanted/needed I can remove it.

[mxinden]

Cool to see this happening.

Do you know how much additional time compiling and running this adds?

[erwin-kok]

Cool to see this happening.

Do you know how much additional time compiling and running this adds?

Almost 3 min