ViaVersion / ViaRewind

ViaBackwards addon to allow 1.8.x and 1.7.x clients on newer server versions.
https://hangar.papermc.io/ViaVersion/ViaRewind
GNU General Public License v3.0
245 stars 77 forks source link

Ping - Pong packets not being translated to transactions (1.17+) #360

Closed NikV2 closed 2 years ago

NikV2 commented 2 years ago

Describe the bug, provide any errors ViaRewind does not translate ping - pong packets to transactions for clients < 1.17. The reason i'm reporting this as a bug report is that im guessing it's designed to be handled somehow. However from my testing and a lot of debugging these are the results i see. ProtocolSupport seems to work properly and older clients are properly receiving ping packets as transactions and vice versa. Note, this also applied for ViaBackwards, I simply decided to report this on one of them

How can we reproduce it? Steps to reproduce the behavior:

  1. Make a 1.17.1 server with ViaVersion, ViaBackwards and ViaRewind.
  2. Join the server with any client that's older than 1.17
  3. Try to send a ping packet to the player by using an external plugin, Any packet listener-handler will do such as ProtocolLib
  4. The client should never receive that packet, or very rarely only receive it once and then never again. (You can use a custom client in order to check what packets the player sends-receives from the client's side)

Expected behaviour Ping and Pong packets should be translated to transactions for legacy versions

Additional server info Spigot + PaperSpigot 1.17.1 Plugins being ProtocolLib, ViaVersion, ViaRewind, ViaBackwards and a plugin that sends ping packets to the player every now and then.

Checklist (mark with [X] to check)

Barvalg commented 2 years ago

Error: Invalid or missing dump

Camotoy commented 2 years ago

This is handled in the dev builds in ViaBackwards; see this commit: https://github.com/ViaVersion/ViaBackwards/commit/455bb546f0606927038a0a86cda0de1cd474bb2d

NikV2 commented 2 years ago

Then this should be set to true by default. since its a very important feature.

99.9% of people usually don't touch the config or they may be updating from an older version without looking at the changes, And get in trouble thinking it's the anticheat.

Actually this shouldn't even be configurable, it should always act like that since every single anticheat uses the ping packet as a replacement for transactions.

NikV2 commented 2 years ago

Apologies for opening an issue even though this was already fixed, Just got confused since i wasn't aware of this.

Take care!

Camotoy commented 2 years ago

Then this should be set to true by default. since its a very important feature.

You're able to set the system property to enable it, if your anticheat relies on it: https://github.com/ViaVersion/ViaBackwards/commit/455bb546f0606927038a0a86cda0de1cd474bb2d#diff-50aa7552a280f8d1bca67614f0f43d66c98d805c62c28c458ed8aa459442996aR76

It's not enabled by default because inventory transactions use a short, whereas the new ping packets use an integer. Larger numbers in the ping packet will not be translated correctly - having this as a setting means you're aware of the inconsistencies with translating when enabling.