Skip to content

Conversation

zdave-parity
Copy link
Contributor

@zdave-parity zdave-parity commented Sep 29, 2025

Relax an MTU discovery state assertion: while we will never start probing before the connection has been established, it is possible for black hole detection to trigger, which will put MTU discovery into the Phase::Complete state.

Fix some comparisons in the black hole detector:

  • Lost packets with size exactly matching the minimum MTU should not be treated as suspicious; by definition the MTU will never be reduced below this.
  • Similarly, lost packets with size exactly matching a more recent successfully transmitted packet should not be treated as suspicious.

Lost packets with size exactly matching the minimum MTU should not be
treated as suspicious; by definition the MTU will never be reduced below
this.

Similarly, lost packets with size exactly matching a more recent
successfully transmitted packet should not be treated as suspicious.
@zdave-parity
Copy link
Contributor Author

There is a test which picks up this issue here, but it's not really suitable for including in the test suite. It iterates over many patterns of handshake packet loss and so takes quite a while to run. Even with this bug fix the test doesn't actually pass because the static CLIENT_PORTS range in tests/util.rs gets exhausted :). It would be possible to check just the pattern which currently fails, but it's unclear to me how useful that would be. I think this is the sort of thing best caught by fuzz testing.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Can you squash the test commit into the commit that broke it, and the formatting fix into the commit that originated the formatting deviation?

While we will never start probing before the connection has been
established, it is possible for black hole detection to trigger, which
will put MTU discovery into the Complete state.
@zdave-parity
Copy link
Contributor Author

Can you squash the test commit into the commit that broke it, and the formatting fix into the commit that originated the formatting deviation?

Sure, done.

@djc
Copy link
Member

djc commented Oct 9, 2025

@Ralith would be nice if you could take a look at this.

@zdave-parity main is currently aiming for a 0.12.0 release. Would you want/need this backported to 0.11.x?

@zdave-parity
Copy link
Contributor Author

@zdave-parity main is currently aiming for a 0.12.0 release. Would you want/need this backported to 0.11.x?

I think we'd be happy to just upgrade to 0.12 when it is released.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes make perfect sense to me. Thanks for finding these!

@Ralith Ralith added this pull request to the merge queue Oct 11, 2025
@Ralith
Copy link
Collaborator

Ralith commented Oct 11, 2025

There is a test which picks up this issue here, but it's not really suitable for including in the test suite

I'm not sure we should exclude such tests -- exhaustive searches and fuzzing are important, as this PR illustrates! Might want a separate test binary that gets built in release mode, or something, though...

Merged via the queue into quinn-rs:main with commit e05a8e5 Oct 11, 2025
20 checks passed
@zdave-parity zdave-parity deleted the mtud-fixes branch October 13, 2025 00:30
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

Successfully merging this pull request may close these issues.

3 participants