Skip to content

Conversation

zdave-parity
Copy link

@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.
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
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?

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.

2 participants