Skip to content

Introduce plaintext buffer for SecureChannel - #3628

Open
realugi wants to merge 1 commit into
PixelGuys:masterfrom
realugi:fix/3250
Open

realugi wants to merge 1 commit into
PixelGuys:masterfrom
realugi:fix/3250

Conversation

@realugi

@realugi realugi commented Sep 22, 2026

Copy link
Copy Markdown

attempts to fix #3250

I added an additional buffer to SecureChannel, which stores the data-to-send in a temporary plaintext buffer. after the buffer has reached the negotiated TLS record size, it flushes/encrypts the plaintext buffer into the SendBuffer.

The maximum buffer size (comptime) is determined by c.MBEDTLS_SSL_OUT_CONTENT_LEN. however, the negotiated TLS record size can differ between TLS versions. that is why I explicitly get the negotiated TLS record length instead of relying on c.MBEDTLS_SSL_OUT_CONTENT_LEN or c.MBEDTLS_SSL_OUT_CONTENT_LEN-1, in order to guarantee correctness for future TLS versions.

Furthermore, I reckon there should be a flush point for the plaintext buffer, so that data-to-send does not stay in plaintext buffer for too long. I chose to place it in Connection.processNextPackets, because that is the place where we determine whether the next UDP packet should be sent or not.

That might not be optimal in all cases though, because of two things:

  1. SendBuffer might still choose to not send a package (secureChannel.sendNextPacketAndGetSize can return null). But SendBuffer also decides whether to send based on the length of the send buffer. So we actually need the ciphertext size before SendBuffer can decide. (kinda a chicken/egg problem). one could try to estimate ciphertext size based on plaintext size and let the SendBuffer decide based on that. this adds quite some complexity. so I chose not to do it.
  2. Connection.processNextPackets is called in the network thread, so it would do the encryption in the network thread, while encryption normally happens in the game/update thread. That might have consequences for the responsiveness of the network thread, if a particularly big plaintext buffer has to be flushed/encrypted at that stage.

As for testing/benchmarking this, I did the following method (on both master and this branch, debug build):

  1. Disable BlockUpdateTask (so that I dont get OOM crashes)
  2. Create flat world
  3. /tp 0 0 0, /pos1, /tp 63 63 63, /pos2
  4. spam alternate between /set cubyz:dirt and /set cubyz:air

You can then observe, that the external message overhead grows around 2x faster on master than on this branch.
Furthermore, you can observe, that the game loop does not lag behind as much. That may be because some of the encryption is now done in network thread. But another contributing factor could be, that the TLS encryption is faster, because it is more likely to encrypt multiple block updates within a single record.

I also realize, that the benchmark does not really represent what usually happens in a cubyz world. #2585 would have been a good candidate, since #3250 comes out of it, but it's not based on current master.

Lastly, this PR may have consequences for latency under certain conditions.
Before this PR, N blockupdates would have all had their own TLS record (encrypted and decrypted individually), and that TLS record would usually be sent in 1-2 UDP packets. So the first blockupdate would be perceived right when that UDP packet comes around.
But now, those N blockupdates might all land in the same TLS record, which in turn could be spread over many UDP packets, which in turn means that the first blockupdate is only seen, once all those UDP packets have been sent.
That would be particularly pathological for small UDP sizes and large TLS record sizes, which is the case for localhost connection. I didn't notice any latency differences between master and this branch though.

@Wunka Wunka moved this to Low Priority in PRs to review Sep 22, 2026
@Wunka

Wunka commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Thanks for looking at this! I don't know if you are on the discord, but just for you information: With the 0.4.0 release this week, there is a review pause until the release is over, so only expect the earliest a review after the release on saturday.
That's probably also why your comment on the issue didn't get an answer. And to "kinda" answer that comment: Quantum always likes when people take up issues, there are only a very few where he reserves them for himself and I think I can with confident say that network related issues are not part of that.

@IntegratedQuantum

Copy link
Copy Markdown
Member

Disable BlockUpdateTask (so that I dont get OOM crashes)

What? Is this #182 (fake out of memory)? Or an actual leak that we should be concerned about?

But now, those N blockupdates might all land in the same TLS record, which in turn could be spread over many UDP packets, which in turn means that the first blockupdate is only seen, once all those UDP packets have been sent.
That would be particularly pathological for small UDP sizes and large TLS record sizes, which is the case for localhost connection. I didn't notice any latency differences between master and this branch though.

That last part makes no sense. localhost is the best case scenario, as the network latency is quite small the bandwidth large and the chance of packet loss is quite big. Of course you are not seeing any latency difference.

Also we could easily cut down the latency cost by choosing a size similar to the MTU. I wonder what the actual impact of this is.

spam alternate between /set cubyz:dirt and /set cubyz:air

Honestly this does not sound like a solid benchmarking strategy. Ideally to have reproducible results, I'd suggest to choose a larger area and then only do a single change and measure the total network data (F7) and total server time (printed to console)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Low Priority

Development

Successfully merging this pull request may close these issues.

Batch network updates before encrypting them with mbedtls

3 participants