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

One packet number space as mandatory to implement? #4

Closed
mirjak opened this issue Oct 15, 2021 · 8 comments
Closed

One packet number space as mandatory to implement? #4

mirjak opened this issue Oct 15, 2021 · 8 comments

Comments

@mirjak
Copy link
Collaborator

mirjak commented Oct 15, 2021

I took the following text from draft-liu:

Note: Even if use of multiple packet number spaces is negotiated but a peer uses an zero length connection ID, then all packets sent to that peer MUST be numbered in a single number space (as specified in the previous section), because the packet level decryption implementation will only see one Connection ID sequence number (the default number 0).

This basically makes the one packet number handling mandatory to implement as you also need it if multiple packet number spaces were negotiated but zero length CID is used.

The alternative would be to say that you can't used zero length CID if multiple packet number spaces are negotiated.

Also I'm currently proposing enable_multipath=1 for multiple number space and enable_multipath=2 for one number space to be compatible with the draft-liu and the alibaba implementation. However if we think one packet number space is mandatory to implement, it would make more sense to have it the other way around and allow fallback to one packet number space, namely if the client ask for two packet number spaces but the server supports only one but would also lead to a successful negotiation.

What do people think about this?

@yfmascgy
Copy link
Contributor

As long as inter-op is allowed, I think it might be better to keep both PN space choices optional (Mirja's proposal of using enable_multipath=1 or 2 works for me). The first thing is to allow people more freedom. If some implementors only want to use one option, they just need to implement one code branch. Second, single packet number space requires the modification of loss detection algorithm, the way how rtt is sampled (the overall largest ack may not be the largest acked packet on a given path), and some delayed ACK strategies, which might take additional effort. People may also want to avoid round-robin scheduling in single PN as it creates a lot of holes in the ack range, so certain things regarding performance get coupled. Christian introduced many good practices and we are currently learning from it, so we probably need more experience on that.

@Yanmei-Liu
Copy link
Contributor

Adding the option to negotiate is a very good solution at this point. It would be helpful to hear more people's implementation experience. To avoid failure for interop tests, I suggest that we could negotiate the transport parameter like this: use 2-bits in the value field for negotiating one or more PN spaces.

  • 0x0: don't support multi-path
  • 0x1: only support multiple PN spaces for multi-path
  • 0x2: only support one PN space for multi-path
  • 0x3: support both one PN space and multiple PN space

The client could send all these 4 values in the enable_multipath transport parameter which depends on what kind of modes it supports. If the server just support one mode(such as one PN space), it could return 0x2 so that the client knows how to do next. If the server support both, it could choose 0x1 or 0x2 as it prefers, and make sure that the server don't return 0x3 as it need to decide which mode to choose. In 0x2 mode, the endpoints must not use 0-length CID.

Then for people who just want one PN space, it would be ok to just support 0x2 mode. For the other guys who don't use 0-length Connection ID and want to get less overhead for ack ranges, they could choose to use 0x1 mode. And for people who want to support both 0-length CID and multiple PN spaces, they could choose 0x3 and of course it will add more modification in the implementation.
Maybe this could meet everyone's need for experiments.

@mirjak
Copy link
Collaborator Author

mirjak commented Oct 15, 2021

@huitema any views?

@huitema
Copy link
Contributor

huitema commented Oct 15, 2021

I agree with Yanmei's proposal of 4 options 0, 1, 2 or 3, although I have a slight preference for "the other way around":

Client Option Definition Allowed server responses
0x0 don't support multi-path 0x0
0x1 only support one PN space for multi-path 0x0 or 0x1
0x2 only support multiple PN spaces for multi-path 0x0 or 0x2
0x3 support both one PN space and multiple PN space 0x0, 0x1 or 0x2

Receiving an unexpected value from the server should be a protocol violation. If the transport parameter is not present, the value defaults to zero.

@Yanmei-Liu
Copy link
Contributor

Agree with Christian's design for value definitions.
Submitted PR #20 for this issue.

@qdeconinck
Copy link
Contributor

As #20 got merged, I think this is now resolved.

@mirjak mirjak reopened this Oct 18, 2021
@mirjak
Copy link
Collaborator Author

mirjak commented Oct 18, 2021

Actually I think this is not fully resolved. The initial question was about use of multiple packet number spaces and zero length connection ID.

Do we now say that if multiple packet number spaces are used, zero connection ID cannot be used? If yes, that means we need to remove the text I've cited in the initial issue description from the draft. And also add some text that "zero-length CID MUST not be used with enable_multipath=2".

Or if we want to keep that option that basically means that you need to always implement also singe packet number handling even if use of multiple packet number spaces is negotiated, and then we should say that more explicitly in the negotiation section.

To me it's not fully clear which option you have agreed to now...?

@mirjak
Copy link
Collaborator Author

mirjak commented Oct 25, 2021

I opened #43 to track the remaining point.

@mirjak mirjak closed this as completed Oct 25, 2021
qdeconinck pushed a commit that referenced this issue Mar 6, 2023
Align with draft March 2023
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

5 participants