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

Simpler negotiation #41

Open
martinthomson opened this issue Feb 12, 2025 · 7 comments
Open

Simpler negotiation #41

martinthomson opened this issue Feb 12, 2025 · 7 comments

Comments

@martinthomson
Copy link
Member

I like the design overall, but I find the transport parameter needlessly complex.

Here's a simpler design proposal:

  1. The transport parameter is made empty.
  2. Endpoints advertise the transport parameter if they understand the transport parameter AND they want to receive the frame.
  3. Endpoints can send the frame if their peer sends the transport parameter.

That's it.

Looking at the existing proposal:

  • A value of 0 might as well not be signaled at all.
  • A value of 1 and 2 both are achieved by the above proposal by including the transport parameter.
  • The aspect of 1 where an endpoint is not willing to send the frame is achieved by not sending the frame.

I understand that endpoints might want to know in advance whether a peer is going to send the frame to them. The design I propose doesn't achieve that. Receiving the frame does. If endpoints that are willing to send the frame send the frame (which can be done as soon as application data keys are available) that uncertainty goes away.

@huitema
Copy link
Collaborator

huitema commented Feb 12, 2025

I wonder whether tracking "did the peer send a frame" is simpler than "did the peer set the bit in the TP".

If we follow your design, we need to add some text such as "if the peer accepts address discovery frames and the local endpoint supports them, it should send an address discovery frame promptly after the beginning of the connection." And deal with the morass hiding behind "promptly".

@martinthomson
Copy link
Member Author

Yes, "promptly". (I would prioritize this lower than some of the other frame types, which might mean that "prompt" means "eventually".

We have a similar concern with HANDSHAKE_DONE and we somehow muddle through, so that wouldn't bother me even if it was "eventually" in practice. I'd have said that if you are tracking the observed/reflexive addresses you have received, then you have the information you need if you care about receiving that information if another path opened up.

@flub
Copy link

flub commented Feb 13, 2025

This change would make it a bit annoying to figure out when to give up on a peer if you only connect because you want to get an address. "promptly" or "eventually" are... vague. The original design makes it very simple to make that decision and you can disconnect if the peer does not want to send you the discovered address.

@martinthomson
Copy link
Member Author

I seriously doubt that this is how you would operate in practice. Peers who support this will likely indicate a willingness to receive the frame.

Separately, many of the deployments that I would consider useful here are essentially static. That is, you know that this particular server supports the feature, therefore you integrate it into your process for managing NAT traversal.

@huitema
Copy link
Collaborator

huitema commented Feb 14, 2025

Of course, even if a server sets a bit to promise that it will send discovery frames, that does not mean that it will actually do it "immediately". But then, not setting the bit means that there is no point waiting for the frame, and that's valuable information.

@marten-seemann
Copy link
Collaborator

I see @martinthomson’s concern about the added complexity, but I’m curious how significant it is in practice.

For those who’ve implemented the draft: What complexity did you encounter beyond simply replacing a bool with an enum?

@huitema
Copy link
Collaborator

huitema commented Feb 15, 2025

I did not find that overly complex. Here is the actual code in picoquic:

                case picoquic_tp_address_discovery: {
                    uint64_t address_discovery_mode =
                        picoquic_transport_param_varint_decode(cnx, bytes + byte_index, extension_length, &ret);
                    if (ret == 0) {
                        if (address_discovery_mode > 2) {
                            ret = picoquic_connection_error_ex(cnx, PICOQUIC_TRANSPORT_PARAMETER_ERROR, 0, "Address discovery parameter");
                        }
                        else {
                            /* After doing +1, we get the following:
                            * address_discovery_mode == 0: nothing goes (TP is absent)
                            * address_discovery_mode == 1: send only (TP value 0)
                            * address_discovery_mode == 2: receive only (TP value 1)
                            * address_discovery_mode == 3: both (TP value 2)
                            */
                            cnx->remote_parameters.address_discovery_mode = (int)(address_discovery_mode + 1);
                            cnx->is_address_discovery_provider = ((cnx->remote_parameters.address_discovery_mode & 2) != 0 &&
                                (cnx->local_parameters.address_discovery_mode & 1) != 0);
                            cnx->is_address_discovery_receiver = ((cnx->remote_parameters.address_discovery_mode & 1) != 0 &&
                                (cnx->local_parameters.address_discovery_mode & 2) != 0);
                        }
                    }
                    break;
                }

But yes, a boolean is simpler. See for example the equivalent code for enabling greasing of the Quic bit:

                case picoquic_tp_grease_quic_bit:
                    if (extension_length != 0) {
                        ret = picoquic_connection_error_ex(cnx, PICOQUIC_TRANSPORT_PARAMETER_ERROR, 0, "Grease TP");
                    }
                    else {
                        cnx->remote_parameters.do_grease_quic_bit = 1;
                    }
                    break;

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

4 participants