forked from quinn-rs/quinn
-
Notifications
You must be signed in to change notification settings - Fork 4
add n0's version of quic nat hole punching #177
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
Draft
divagant-martian
wants to merge
57
commits into
main-iroh
Choose a base branch
from
protocol-simplification
base: main-iroh
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
otherwise we're opening paths to/from the wrong remotes and packets remotes do not match the PathData remote.
…theus23/rtt-from-path-response
…enge resending (#178) - Adds a timer to resend another path challenge if the old one was lost and the path isn't validated. - The resent challenges now have different tokens. We keep track of all of them to verify incoming path responses. - The logic for when to send a path challenge is simplified to only send challenges when `send_new_challenge` is set to `true` (instead of triggering *always* when we're in path validation mode)
We can receive a PATH_ABANDON frame for which we do not yet have any PathData. In this case we still want to abandon that path, and not accidentally think we might be closing the last path.
- span for timers - span for packets, this means we get path_id for some thing that are not on the path of processing frames. the recv span can now lose the path_id.
…d` instead of `Connection::max_path_id` (#183) This adds a previously failing test case that best illustrates the problem. Here's a picture anyways: <img width="1733" height="1044" alt="image" src="https://github.com/user-attachments/assets/684d66de-f80a-4561-93a2-7391e5372734" /> When the server receives a `MAX_PATH_ID` frame and agrees to increase it, it'll increase it after sending a `MAX_PATH_ID` frame itself. Now, from the server's perspective the maximum path ID might be set to 14 (a value I saw in practice). Then it starts issuing NEW_CONNECTION_ID frames with path IDs up to what it thinks is the max path id. However, these two datagrams might get reordered. This is what we synthetically make happen in the test `issue_max_path_id_reordered`. Then, the client will receive newer NEW_CONNECTION_ID frames while it still thinks the max path ID might be something like 12 and promptly close the connection with a protocol violation. The fix I propose is to be more lenient when reading incoming packets. The `NEW_CONNECTION_ID` frame we got IMO is *fine* because we've already sent out a `MAX_PATH_ID` frame with 14. The fact that the other side hasn't sent us a `MAX_PATH_ID` frame is kind of secondary. The other possible fix would be to delay the remote using any higher path IDs until it made sure the `MAX_PATH_ID` frame was acknowledged. I think this adds unnecessary delays. The specification doesn't really seem to agree with me on this. Section 4 says: > Receipt of multipath-specific frames that use a path ID that is > greater than the announced Maximum Paths value in the MAX_PATH_ID > frame or in the initial_max_path_id transport parameter, if no > MAX_PATH_ID frame was received yet, MUST be treated as a connection > error of type PROTOCOL_VIOLATION. I interpret the `MAX_PATH_ID` frames as the ones that are *received*, due to it referring to having received them or not in the next sentence. This means the situation from the picture above would be the connection's flow according to the specification. (But I think that's a specification bug.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RttEstimatorfrom path challenge responses