MTU Path discovery by Wunka · Pull Request #3633 · PixelGuys/Cubyz

Wunka · 2026-09-23T19:07:30Z

Closes #819

This has still one big problem:

Only client or server increase their mtu estimate. For example if client increases it, the server can't anymore for whatever reason. It seems like the probe is send, the other gets it and does a confirmation, but that confirmation is never received.
After fixing the above issue, debug information needs to be deleted and the setting for mtu also needs to be removed. Also the timers and in general constants need to be adjusted to right values. Currently they are made better for debugging.

Changes:

Connections now have a probingState. This + the connectionState about represent the State machine in RFC 8899. The only state missing is the Error state.
The slow channel now also acts as a ProbeChannel. That means it will query on the connections probingState if it should send a probe and if yes it queries which size it should have and gives the probingState the SequenzIndex of the probe. With this we can then identify when a confirmation is for the probe
Probes are for now only send when no other should be send over the slow channel, as this is easier to implement
Probes are done with the mtu estimate + 50. (here there is no number mentioned in the RFC)
On a double loss of packets in general, the mtu estimate is reseted again to the mtuMin. Here we can maybe in the future make better heuristics. But I don't think networking is such a bottleneck that we need that.
variables where in general created with the corresponding RFC 8899 name in mind.

Wunka · 2026-09-27T16:47:40Z

So, a problem we have:
We kinda want to pad the normal messages, but for that we would have to go very weird around our Sendbuffer. As we don't need probes to be always received, so resending / confirmation is something we don't that much care about. That's why in this pr I have gone the way of doing a seperate channel for probe messages, which don't have a send / receive buffer. The other side just sends a confirmation packet that it was received and nothing more. This sadly means we can't pad messages, but instead have to send special probe messages which only contain nonsense data.

IntegratedQuantum · 2026-09-27T17:04:36Z

This sadly means we can't pad messages, but instead have to send special probe messages which only contain nonsense data.

At the end of the data it probably doesn't matter. Both techniques have performance impacts, and as long as the algorithm guarantees that only a small fraction of the throughput is probes, it shouldn't matter.

IntegratedQuantum

Haven't reviewed the actual MTU discovery code yet, I'll check it another day

IntegratedQuantum · 2026-10-25T20:23:39Z

 						try windowsError(c.WSAGetLastError());
 						return error.UNKNOWN;
 					}
+					const setting: c_int = 1;


Instead of numbers, coud you maybe use c.TRUE?

IntegratedQuantum · 2026-10-25T20:28:37Z

 	lossyChannel: Channel, // TODO: Actually allow it to be lossy
 	secureChannel: SecureChannel,
-	slowChannel: Channel,
+	slowChannel: ProbeChannel,


As mentioned in the call, the slow channel is independent, and instead you should just add a separate check at the callsite of slowChannel.sendNextPacketAndGetSize

Then you also don't need to introduce this wrapper channel, instead you can just add another helping function to the MtuHandler struct (which is where this functionality belongs anyways).

Oh. I was not yet done with doing everything from the call. That's why this is still there

Ah, I just saw it on the PR board and thought it was ready

okay, now it's done.

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

IntegratedQuantum · 2026-10-26T19:08:04Z

+			searching: struct {
+				firstProbe: bool = true,
+				probedSize: u16 = undefined,
+				probeSequenceIndex: ?SequenceIndex = null,


Do we really need this? The RFC mentions it should be used to protect against off-path data. We should maybe explicitly handle this threat for the whole protocol. Either way a sequence number that starts at 0 and is inspectible from any client that joins is not providing any protection anyways. Maybe make a new issue for this.
Here I'd suggest to just send the probe size in the confirmation, that's enough to identify it, and makes the whole thing a little simpler.

This was actually originally to go around the need to have a SequenceIndex.

Huh? This answer makes no sense. You use a SequenceIndex to go around the need for a SequenceIndex???

... okay maybe was wrongly explained. If I remember correctly I added that because on the receiving side it would especially try to read for a sequence Index. Before giving the rest of the date the the correct channel. So that's the reason I even added the sequenceIndex

As far as I can see it only does that for lossy/secure/slow, I don't see anywhere where it reads this from the buffer.
And in the Confirmation packet you can jsut send the length instead of the sequence index.

IntegratedQuantum · 2026-10-27T20:31:53Z

Has conflicts now, due to the currentFrame refactor #3733

IntegratedQuantum · 2026-10-28T19:21:42Z

+
+				self.searching.probeCount = 0;
+				self.searching.nextProbeTime = 0; // next probe can be send
+				conn.mtuHandler.mtuEstimate = @intCast(probeLen);


I'd suggest to still check the length against the length of the probe sent, otherwise this may cause the value to reset to a lower one.

IntegratedQuantum · 2026-10-28T19:41:04Z

-			return self.probingState.receiveConfirmationAndGetTimestamp(conn, sequenceIndex);
+		fn receiveConfirmationAndGetTimestamp(self: *MtuHandler, sequenceIndex: SequenceIndex) ?SendBuffer.ReceiveConfirmationResult {
+			self.lossCounter = 0;
+			self.maxLossSize = 0;


These can only be reset if the received probe length actually was >= the maxLossSize

I would expect it to be bigger, but yes. that if is good.

IntegratedQuantum

you forgot about https://github.com/PixelGuys/Cubyz/pull/3633/changes#r4372262506

also now that a few things have changed, I think it would make sense to make another test over the wire this weekend.

IntegratedQuantum · 2026-10-30T19:59:55Z

 						try windowsError(c.WSAGetLastError());
 						return error.UNKNOWN;
 					}
+					const setting: c_int = c.true;


Where does true come from? Windows API uses TRUE

It was given to me by zls. I wanted to try out if it works, that's why I triggered the CI. Seems to. interesting. But to stay consistent. I changed it to TRUE for windows

But macos does expect you to use true here, or is that also something that came from autocomplete?

autocomplete. And works on macos, linux and windows

maybe someting translate-c just adds?

I'd suggest to just hardcode 1 here then.

What about @intFromBool(true); or do you not like this?

either way. I changed to = 1

I think we should be explicit, when the API wants a number we should use a number, or the corresponding macro for that number.

understandable

Wunka added 4 commits September 23, 2026 19:56

first edition

9b53a47

remove debug print and add a slider to simulate mtu changes

92f7205

reset on double loss

83c962f

one more debug information

11a2a38

Wunka added this to PRs to review Sep 23, 2026

Wunka moved this to WIP/not ready for review in PRs to review Sep 23, 2026

Wunka added 10 commits September 23, 2026 21:11

remove error, I kinda don't need it

5be7ae7

add comment for handshake

e016aac

add RFC recommandation comment

12ff10e

rename ProbeStatus -> ProbingState

66cb525

add padding protocal and don't resend probes

8d6f6db

instead of just poping it, we confirm the probe

5380c19

remove mtu blocker

7e13a8f

don't reset when handshake is not done and fix dumb mistake

ca47075

remove debug setting

73948fe

remove probingState from debug window

3d7d4cf

Wunka marked this pull request as ready for review September 24, 2026 11:16

Wunka moved this from WIP/not ready for review to Low Priority in PRs to review Sep 24, 2026

Wunka and others added 4 commits September 24, 2026 13:18

space

58928e0

make implementation cleaner

a7ab7ca

improve

d76c617

Update version number

a51a7fd

Merge remote-tracking branch 'upstream/0.4.x' into mtu

9404030

Wunka marked this pull request as draft October 1, 2026 21:31

Wunka moved this from Low Priority to WIP/not ready for review in PRs to review Oct 1, 2026

Wunka added 3 commits October 1, 2026 23:55

setsocketopt

fab6f7c

add windows support

999b264

ups

655b4ca

Wunka added 8 commits October 25, 2026 15:59

mark

9ae2f36

add -14

aba0e18

test

5302c44

test more

c9e9224

do more tests

15188ec

remove spliting

107cb01

remove return

a816e19

make mtuHandler struct

dcbe8d3

IntegratedQuantum requested changes Oct 25, 2026

View reviewed changes

Wunka and others added 2 commits October 25, 2026 20:35

Update src/network.zig

a87c75a

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

Update src/network.zig

1e2b10c

Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>

IntegratedQuantum moved this from Low Priority to In review in PRs to review Oct 25, 2026

Wunka added 2 commits October 26, 2026 09:27

small changes

bc27fb3

slow channel is not probeChannel anymore

5c75e85

IntegratedQuantum requested changes Oct 26, 2026

View reviewed changes

Wunka added 2 commits October 26, 2026 20:43

move around

615a832

even more moving

7b34d95

Wunka added 2 commits October 27, 2026 20:38

Merge branch 'master' into mtu

59b7c53

remove probeSequenceIndex

e2ef5cc

IntegratedQuantum requested changes Oct 28, 2026

View reviewed changes

just a few changes

723a1b6

IntegratedQuantum requested changes Oct 28, 2026

View reviewed changes

Wunka added 2 commits October 28, 2026 20:58

yes

71ca989

how did it work without the reseting here?

6b17d02

IntegratedQuantum requested changes Oct 30, 2026

View reviewed changes

huh

2dc4aa9

IntegratedQuantum requested changes Oct 30, 2026

View reviewed changes

Wunka added 2 commits October 30, 2026 21:09

true => TRUE

d379007

1

39ce76a

Conversation

Wunka commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wunka commented Sep 27, 2026

Uh oh!

IntegratedQuantum commented Sep 27, 2026

Uh oh!

IntegratedQuantum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IntegratedQuantum commented Oct 27, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IntegratedQuantum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wunka Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wunka commented Sep 23, 2026 •

edited

Loading

Wunka Oct 30, 2026 •

edited

Loading