fix: use correct id sequence (dialer even/listener odd) #3

dryajov · 2018-04-11T07:53:52Z

There is an inconsistency in the spec. It says odd IDs are used for initiators and even ids for receiver. This is reverced from the current js implementation - https://github.com/libp2p/js-libp2p-mplex/blob/master/src/internals/index.js#L83-L91.

Stebalien · 2018-04-12T00:14:32Z

Crap. I now finally know why this protocol has separate tags for messages from the initiator and the receiver but have no idea why Go and JS can even talk to each other. Go does use odd numbers.

It looks like the original protocol didn't specify odd/even stream IDs. Instead, each endpoint had two separate sets of streams, ones that the endpoint initiated and ones that the endpoint received. That's why there are two different message tags for each type of message. Sending a message with the MessageInitiator tag means that it's a message on a stream that I initiated. That means we can use the same stream IDs and everything "just works".

It looks like JS does this so, really, the even-odd distinction shouldn't make a difference there but go doesn't.

So, I'd expect streams from Go to JS to work but not the other way around. I'll chat with @diasdavid about this.

dryajov · 2018-04-12T23:48:38Z

It looks like the original protocol didn't specify odd/even stream IDs

yeah, actually it looks like it doesn't care about even/odd ids at all and for some reason it would just always use even ids for initiators in the original implementation - https://github.com/maxogden/multiplex/blob/master/index.js#L162...L164

We might have introduced the the even/odd numbering when porting that over to libp2p, but it still doesn't matter since we keep two separate lists for initiators and receivers, which are selected based on the tag type - https://github.com/libp2p/js-libp2p-mplex/blob/master/src/internals/index.js#L199.

The reason it works, seems to be because the tag eve/odd'nes matches - https://github.com/libp2p/go-mplex/blob/master/multiplex.go#L36...L42, odd for initiator, even for receiver, which is reversed again from the id numbering 🙃

Stebalien · 2018-04-13T00:10:23Z

The reason it works, seems to be because the tag eve/odd'nes matches

....? I'd expect that to break it completely...

Luckily, we only use this for go/js interop so simply fix the spec and the go implementation (don't care about go/go interop).

So, spec updates:

IDs are allocated out of the local pool.
Use tag + 1 when sending a message on a stream initiated by the other side.

Does the session (not stream) initiator even matter?

dryajov · 2018-04-13T00:17:00Z

I think there was a misunderstanding, the go tags are correct, the IDs were reversed. But the IDs didn't affect anything in js because it uses the tag to select the list where to look the stream in.

That said, I like the even/odd Id distinction because it allows to use the same list without clashing.

daviddias · 2018-04-13T00:25:27Z

@dryajov you might have found the reason why DHT interop breaks. Could you had a set of tests to interop that actively test the creation of multiple muxed streams (lots of different bitswap, ping, dht and other streams?)

dryajov · 2018-04-13T00:25:28Z

Does the session (not stream) initiator even matter?

It doesn't matter unless we care about the Id even/oddess. I think the original intent with that was precisely not having to use two lists, but it probably changed during implementation and the initator flag was left kinda dangling.

dryajov · 2018-04-13T00:56:25Z

So, I would still like to keep even/odd numbering of steams - even for initiator odd for receiver, same as the tags. It would keep it backwards compatible with the original js implementation, and fixing the ids would make it work with the new pull-stream implementation https://github.com/dryajov/pull-plex.

So the changes to the spec would be:

change the id to even for initiators odd for receivers
use even tags for initiator and odd for receiver

Stebalien · 2018-04-16T12:42:17Z

With respect to the even/odd IDs, I'm pretty sure the none of the JS implementations relied on this at any point (the current version uses even/odd IDs but still stores the streams in separate tables). I can make the go implementation use even IDs for the initiator but I don't think there's much of a reason to do so (and, on go at least, it won't simplify anything).

See: libp2p/mplex#3 (massive thanks to @dyajov for finding this)

Stebalien · 2018-04-16T12:47:36Z

Fix here (without the alternating stream IDs for now): libp2p/go-mplex#26

Can someone test it?

dryajov · 2018-04-17T22:35:35Z

Can someone test it?

I'll give it a go.

With respect to the even/odd IDs, I'm pretty sure the none of the JS implementations relied on this at any point (the current version uses even/odd IDs but still stores the streams in separate tables). I can make the go implementation use even IDs for the initiator but I don't think there's much of a reason to do so (and, on go at least, it won't simplify anything).

Fair enough - it changes little on my side as well.

Stebalien · 2018-04-23T17:43:23Z

@dryajov I've written a short interop test (checked into that branch) and everything appears to work. I believe the new update to the spec will be:

Remove mentions of ID selection (each side just selects an ID that doesn't correspond to a channel that they've opened).
The initiator uses even tags to indicate that the stream is theirs. That is, the full stream ID is a tuple (id, mine) = (header >> 3, (header&1) == 1)

dryajov · 2018-04-23T18:01:53Z

@Stebalien LGTM!

Thanks for the interop, I'll try it out with my new implementation as well 👍

remove mention of even/odd id numbering

dryajov · 2018-04-23T18:29:05Z

@Stebalien I've added the clarification to the spec.

Stebalien · 2018-04-23T18:53:00Z

README.md

@@ -52,13 +52,13 @@ necessarily send the first packet, this distinction is just made to make the all

 ### Opening a new stream

-To open a new stream, first allocate a new unique stream ID; the session initiator allocates odd IDs and the session receiver allocates even IDs. Then, send a message with the flag set to `NewStream`, the ID set to the newly
+To open a new stream, first allocate a new unique stream ID. Then, send a message with the flag set to `NewStream`, the ID set to the newly


Stream IDs are no longer unique session-wide.

Hmm, good point. I think we can word it as unique for each end, as in reusing IDs is not supported? Suggestions welcome.

Each endpoint has an independent pool of stream IDs. To open a new stream, send a message with the flag set to NewStream, the ID set to the next available ID (for this endpoint), and the data of the message set to the name of the stream.

?

Note: We should still note somewhere that streams are identified by the (streamID, initiatedByMe) tuple.

who is me? The peerId?

Stebalien · 2018-04-23T18:54:42Z

README.md

 allocated stream ID, and the data of the message set to the name of the stream.

 Stream names are purely for interfaces and are not otherwise considered by the protocol. An empty string may also be used for the stream name, and they may also be repeated (using the same stream name for every stream is valid). Reusing
 a stream ID after closing a stream may result in undefined behaviour.

-The party that opens a stream is called the stream initiator. This is used for numbering the streams.
+The party that opens a stream is called the stream initiator. This is used for numbering the streams as well as identifying whether the message comes from a channel opened locally or remotely. Thus, the stream initiator always uses even flags and stream receivers uses odd flags.


This is used for numbering the streams

Not anymore (in go at least).

True, I'll change it.

What's the trick to number streams then?

Stebalien · 2018-04-23T23:48:08Z

README.md

@@ -52,13 +52,13 @@ necessarily send the first packet, this distinction is just made to make the all

 ### Opening a new stream


In the section above ^^, we should also get rid of all information about stream IDs, initiators, and receivers.

we should also get rid of all information about stream IDs, initiators, and receivers.

There is one part where mentioning initiators makes sense - when talking about flag's oddness.

daviddias · 2018-04-24T04:58:23Z

I've written a short interop test (checked into that branch)

How short? The issue only started to appear with multiple streams were open. Can we have a serious interop test that tests with hundreds to thousands of streams?

Stebalien · 2018-04-25T00:05:24Z

How short? The issue only started to appear with multiple streams were open. Can we have a serious interop test that tests with hundreds to thousands of streams?

Short in terms of code length/complexity. Each side opens 100 streams (in parallel) and read/write 100 different messages. The test fails with all previous versions of mplex (that I've tested).

A longer test would hammer on stream open/close events, resets, slow reads, small writes, large writes, etc. This test doesn't do all that.

daviddias · 2018-04-25T07:52:51Z

@dryajov is there a PR coming with these changes for libp2p-mplex?

Stebalien · 2018-04-25T19:14:47Z

@diasdavid this PR is the spec change, the go fix is here: libp2p/go-mplex#26

@dryajov said that the interop tests didn't work for him, any help with that would be appreciated (they work for me 100% of the time).

dryajov · 2018-04-25T19:35:24Z

We shouldn't need to make any changes to the js code, it should already work as expected. I'll look into what's breaking with the interop tests and see if it works with @Stebalien's changes, but what's breaking seems to be specific to my env.

Stebalien · 2018-05-04T18:23:43Z

I've merged the go changes (@dryajov found the issue with the test). The only thing left is to finish this up and bubble the go changes.

daviddias · 2018-05-05T23:30:38Z

@Stebalien that is awesome to hear! Is it coming on next go-ipfs? Sign me up for that release :)

Stebalien · 2018-05-06T00:14:16Z

@diasdavid the one after the next one. IIRC, we have an RC1 for the next release already cut.

daviddias · 2018-05-06T13:29:17Z

That really drags things for us. Are we looking into something like 6~8 weeks?

Stebalien · 2018-05-06T19:02:20Z

@diasdavid the answer is ASAP. We're cutting this release because we want to fix a burning windows bug.

vyzo · 2018-05-06T19:06:00Z

also, relays; we need rapid deployment for that.

fix: use correct id sequence (dialer even/listener odd)

fix: use correct id sequence (dialer even/listener odd)

2aa08b9

Stebalien added a commit to libp2p/go-mplex that referenced this pull request Apr 16, 2018

make spec compliant

bc5bcdd

See: libp2p/mplex#3 (massive thanks to @dyajov for finding this)

Stebalien mentioned this pull request Apr 16, 2018

make spec compliant libp2p/go-mplex#26

Merged

clarify even/odd tag ussage

506fb65

remove mention of even/odd id numbering

Stebalien reviewed Apr 23, 2018

View reviewed changes

fix: corrections based on review

ec210bd

ghost assigned dryajov May 30, 2018

ghost added the in progress label May 30, 2018

ghost mentioned this pull request Jun 20, 2018

Implement IPNS ipfs/js-ipfs#209

Closed

twittner mentioned this pull request Aug 28, 2018

ping-client example is not working properly. libp2p/rust-libp2p#434

Closed

dryajov mentioned this pull request Oct 1, 2018

fix stream ids to match mplex protocol spec libp2p/js-libp2p-mplex#83

Closed

Stebalien merged commit e43bb01 into master Oct 1, 2018

ghost removed the in progress label Oct 1, 2018

Stebalien deleted the fix/id-sequence branch October 1, 2018 18:56

jacobheun mentioned this pull request Oct 1, 2018

mplex stream numbers ipfs/js-ipfs#1601

Closed

Stebalien added a commit that referenced this pull request Oct 10, 2018

Merge pull request #3 from libp2p/fix/id-sequence

b515fe9

fix: use correct id sequence (dialer even/listener odd)

Stebalien added a commit that referenced this pull request Oct 10, 2018

Merge pull request #3 from libp2p/fix/id-sequence

3820f9a

fix: use correct id sequence (dialer even/listener odd)

tomaka mentioned this pull request Oct 10, 2018

Add specs for mplex libp2p/specs#96

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use correct id sequence (dialer even/listener odd) #3

fix: use correct id sequence (dialer even/listener odd) #3

dryajov commented Apr 11, 2018

Stebalien commented Apr 12, 2018

dryajov commented Apr 12, 2018

Stebalien commented Apr 13, 2018

dryajov commented Apr 13, 2018

daviddias commented Apr 13, 2018

dryajov commented Apr 13, 2018

dryajov commented Apr 13, 2018

Stebalien commented Apr 16, 2018

Stebalien commented Apr 16, 2018

dryajov commented Apr 17, 2018

Stebalien commented Apr 23, 2018 •

edited

Loading

dryajov commented Apr 23, 2018

dryajov commented Apr 23, 2018

Stebalien Apr 23, 2018

dryajov Apr 23, 2018

Stebalien Apr 23, 2018 •

edited

Loading

Stebalien Apr 23, 2018

daviddias Apr 25, 2018 •

edited

Loading

Stebalien Apr 23, 2018

dryajov Apr 23, 2018

daviddias Apr 25, 2018

Stebalien Apr 23, 2018

dryajov May 30, 2018

daviddias commented Apr 24, 2018

Stebalien commented Apr 25, 2018

daviddias commented Apr 25, 2018 •

edited

Loading

Stebalien commented Apr 25, 2018

dryajov commented Apr 25, 2018

Stebalien commented May 4, 2018

daviddias commented May 5, 2018

Stebalien commented May 6, 2018

daviddias commented May 6, 2018

Stebalien commented May 6, 2018

vyzo commented May 6, 2018

		@@ -52,13 +52,13 @@ necessarily send the first packet, this distinction is just made to make the all

		### Opening a new stream

fix: use correct id sequence (dialer even/listener odd) #3

fix: use correct id sequence (dialer even/listener odd) #3

Conversation

dryajov commented Apr 11, 2018

Stebalien commented Apr 12, 2018

dryajov commented Apr 12, 2018

Stebalien commented Apr 13, 2018

dryajov commented Apr 13, 2018

daviddias commented Apr 13, 2018

dryajov commented Apr 13, 2018

dryajov commented Apr 13, 2018

Stebalien commented Apr 16, 2018

Stebalien commented Apr 16, 2018

dryajov commented Apr 17, 2018

Stebalien commented Apr 23, 2018 • edited Loading

dryajov commented Apr 23, 2018

dryajov commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Apr 24, 2018

Stebalien commented Apr 25, 2018

daviddias commented Apr 25, 2018 • edited Loading

Stebalien commented Apr 25, 2018

dryajov commented Apr 25, 2018

Stebalien commented May 4, 2018

daviddias commented May 5, 2018

Stebalien commented May 6, 2018

daviddias commented May 6, 2018

Stebalien commented May 6, 2018

vyzo commented May 6, 2018

Stebalien commented Apr 23, 2018 •

edited

Loading

Stebalien Apr 23, 2018 •

edited

Loading

daviddias Apr 25, 2018 •

edited

Loading

daviddias commented Apr 25, 2018 •

edited

Loading