search

libp2p / specs

Technical specifications for the libp2p networking stack
https://libp2p.io
1.56k stars 273 forks source link

fix: specify max mplex stream ID size #317

Closed Stebalien closed 3 years ago

Stebalien commented 3 years ago

NOTE: go-mplex technically supports 61bit IDs, but those violate our unsigned-varint spec and may not work on all implementations.

fixes #259

Stebalien commented 3 years ago

Do I understand correctly that we deem 2^28 bit stream ID space to small for long running nodes?

It should be enough. But I don't see any reason to have a restriction like that given that we already need to decode up to 9 byte varints.

mxinden commented 3 years ago

Do I understand correctly that we deem 2^28 bit stream ID space to small for long running nodes?

It should be enough. But I don't see any reason to have a restriction like that given that we already need to decode up to 9 byte varints.

That depends on the implementation of the varint decoder right? E.g. in unsigned-varint it will at most decode 4 bytes when decoding for a u32.

Either way, I am fine with allowing up to 2^60 stream IDs, especially since in most cases (< 2^28) less bytes are needed on the wire.

Stebalien commented 3 years ago

That depends on the implementation of the varint decoder right?

Yeah... it just seems like there's no good reason not to use 64bits. If you have a 32bit decoder, you probably have a 64bit one.

How about this:

  1. Implementations MUST support up to 32bit headers.
  2. Implementations MAY support up to 64bit headers.

Worst case, the connection will break after a few years...

Stebalien commented 3 years ago

Changed. Does that work for you?

arnetheduck commented 3 years ago

I'm reading this PR and I'm not sure what it brings - introducing SHOULD for 64-bit values is of no value basically, because the whole purpose of libp2p is interoperability and in order to be interoperable, you must be able to depend on what goes on at the other end - which with a SHOULD in the middle is.. 32 bits.

mxinden commented 3 years ago

I understand your reasoning @arnetheduck, though I am having difficulties coming up with a probable use-case of > 2^28 streams.

Again, I am fine with either (A) 32 bit, (B) 64 bit or (C) requiring 32 bit and recommending 64 bit. If you feel strongly about a specific option @arnetheduck I am fine with altering the wording once more, as long as @Stebalien agrees as well.

tomaka commented 3 years ago

I think @arnetheduck's point is that option (C) doesn't make sense. If a node A supports 64 bits substreams and another node B only supports 32 bits substreams, then node A has no choice but to only generate 32 bits substreams just in case.

Stebalien commented 3 years ago

In almost all cases, the connection will break before we form 2**28 connections. My assumption is that a peer will break the connection after it exhausts all stream IDs.

From an interop standpoint, this should be fine. If at least one peer supports 2**28 stream IDs, the peers will be able to interop up to the point where they open 2**28 streams. If both happen to support 2**60 streams, they'll just be able to keep the connection open longer.

Also note: SHOULD specifically means "all implementations SHOULD do this unless they know the repercussions of not doing so". REQUIRED/MUST means "you need to do this to interop", which isn't the case here.

tomaka commented 3 years ago

In almost all cases, the connection will break before we form 2**28 connections. My assumption is that a peer will break the connection after it exhausts all stream IDs.

You seem to also assume that stream IDs start at 0 or 1 and increase linearly, while I don't think the specs mention this.

Also note: SHOULD specifically means "all implementations SHOULD do this unless they know the repercussions of not doing so". REQUIRED/MUST means "you need to do this to interop", which isn't the case here.

What I don't understand is: why not settle on either 32 or 64? Why leave the door open for uncertainties?

Stebalien commented 3 years ago

You seem to also assume that stream IDs start at 0 or 1 and increase linearly, while I don't think the specs mention this.

Fair enough.

What I don't understand is: why not settle on either 32 or 64? Why leave the door open for uncertainties?

I want implementations to be able to support up to 64 bit headers. Otherwise, connections will break after a few hundred thousand streams.

But yeah, there's no reason not to just require 64bit headers (other than the fact that rust will need to make a change).

mxinden commented 3 years ago

But yeah, there's no reason not to just require 64bit headers (other than the fact that rust will need to make a change).

I don't think the quirks of a single implementation should influence our choice here (said as one of the maintainers of rust-libp2p). At this point, to simplify the spec, I am in favor of just requiring implementations to support a 64bit header.

Stebalien commented 3 years ago

https://github.com/libp2p/specs/pull/322