muxers/mplex: Allow up to 60 bit long stream IDs

[whereistejas]

hi @mxinden,
I have opened this PR for closing issue: #2061

Do we also need write a test to check if we can actually use 60 bit stream ID?

[mxinden]

Do we also need write a test to check if we can actually use 60 bit stream ID?

Yes please, that would be great!

[whereistejas]

Hi @mxinden,
I'm having a hard time figuring out how to write a test for this. I tried to go through the programs in the examples directory, looking for something that I could use. Can you please give me a little guidance about how I should proceed with writing this test?

[mxinden]

Codec implements both Encode and Decode. To test your patch I would encode a Frame::Open with a stream ID > 2^32 into a BytesMut and then decode said BytesMut expecting the two stream IDs to equal.

Does that help @whereistejas?

[whereistejas]

Hi @mxinden ,
I have finished writing the test. Your suggestion helped me a bunch, thanks! :)

[mxinden]

Test looks great. Just a couple of smaller suggestions.

Would you mind including a changelog entry in muxers/mplex/CHANGELOG.md?

[whereistejas]

Would you mind including a changelog entry in muxers/mplex/CHANGELOG.md?

Are we currently on version 0.39.0? I'm asking so that I may add the version in the change logs.

[mxinden]

Please move this line under the # 0.29.0 [...] section below the - Update dependencies line.

[mxinden]

Please revert the code formatting introduced in a016222.

I am not opposed to using rustfmt, though I think, in case we do decide to use it across the repository, it should be a separate pull request. See #1668 for details.

[whereistejas]

I had accidentally used rustfmt in the commit ead5d97. The commit a016222 reverted the formatting that was done by rustfmt.

[whereistejas]

I think, we are pretty much finished. I would love to take up rustfmt issue in a seperate PR. The least I can do is open the PR. I will leave the decision of whether or not to merge it to the reviewers.

[mxinden]

Thanks for the help @whereistejas.

[whereistejas]

Thanks @mxinden! :)

Can we close this PR now?

[senden9]

Hi!
I know that this pull request is already merged. But if I look at the changed files I see no restriction to 60 Bit. As far as I can see there is nothing that would stop me from creating a 61, 62, 63 or 64 Bit ID. Or overlook/misunderstood I something here?

[whereistejas]

@senden9
I can be completely wrong here, but my understanding is that the first 3 bits are reserved for the message flag. Please refer to this document: https://github.com/libp2p/specs/blob/master/mplex/README.md

@mxinden can you please help out here? Also, I think it makes sense to add a test, that fails if we try to use a stream ID greater than 60bits.

[senden9]

Right. It seams that the first three bits are reserved for flags. And the lowest three bits are shifted away in encoding. So LocalStreamId::num seams not to be the ID, but the Header (as called your linked document).

Anyway as I want introduce any more "noise" in the notification inboxes of the people of this PR: I will think a bit about that and then create a new Discussion or Issue if anything seems strange to me. Thanks for the fast reply @whereistejas

[mxinden]

I know that this pull request is already merged.

Feedback is always welcome, no matter what status the pull request is in ;)

But if I look at the changed files I see no restriction to 60 Bit. As far as I can see there is nothing that would stop me from creating a 61, 62, 63 or 64 Bit ID.

Correct. You can still call RemoteStreamId::dialer(u64::MAX). I agree that this is not ideal and that there should be something stopping you, e.g. through an assert.

On the other hand:

• I doubt anyone will ever reach stream IDs of more than 60 bit given that it is monotonically increased by 1.
• LocalStreamId is not exposed by the crate, and thus an internal construct only.

All that said, the goal of this pull request is to support stream IDs of up to 60 bit coming from a remote peer. Using a u64 instead of a u32 does allow that.

This is the relevant excerpt from the specification:

The maximum header length is 9 bytes (per the unsigned-varint spec). With 9 continuation bits and 3 message flag bits the maximum stream ID is 60 bits (maximum value of 2^60 - 1).

https://github.com/libp2p/specs/blob/master/mplex/README.md#message-format

@senden9 does the above make sense?

[senden9]

@mxinden yes, thanks for the explanation. The "Internal construct only"-part is also a good argument for why it isn't too bad to not block the constructing or reaching of that invalid state.