-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement TransactionOrder #1057
Conversation
Hello! This check needs to verify if transaction initialy sent by player (server should anwser the transaction), I think it can be done by not processing ID if its higher than 0, or checking if player sent not accepted transaction. |
can u explain why u think this should be it shouldnt also be a problem to check for all send transactions by the server and not only grims since every sent transaction will get received and responded by vanilla (unless there is something i am missing) |
Sure, because when client interacting with inventory it sends transaction to server for synchronization. In that case client sends thansaction that does not expected by the server and flags. I tested this on a 1.12.2 server, joining from a 1.8.9 vanilla client. (With ViaBackwards, ViaRewind and ViaVersion installed) Small PoC (i changed messages for a bit): https://i.imgur.com/D8OSkDy.png |
vanilla only sends positive transaction ID's and grim only sends negative. |
Did you test this on a proxy? Between switching servers sometimes a transaction packet can leak over to the server they switched to when it was intended for the other server (at least I encountered this issue on Velocity). |
Yes, this problem happens because of packets leaking during server switching, and this happens on Velocity and bungee. An alternative would be to set it to disregard transaction errors for a few seconds after entering the server. |
Implemented both suggestions (join exception and only tracking grims transactions) |
@c0dingnoobi Can you rename the field name so its not named atomic boolean & make sure the check is set to experimental. You could add the check to the configuration as well if you want. |
Done, was already set to experimental sooo didn't had to change it |
@AoElite |
Imo having LinkedHashSet instead of ArrayList is kind of redundant since i am using ArrayList to get the first element of that LinkedHashSet to avoid iterator |
Transaction order is a bit strange. Sometimes the transactions are not being sent in the correct order sometimes even for vanilla clients. |
Never came across to something like that, check has been running for months on 1.8 and 1.19 (since last month even 1.20). |
This check looks for transactions being cancelled by the client.
Vanilla will never cancel sent transactions.
When client cancels a transaction and starts to resend them, the order will be off. By implementing
This has been tested from 1.8 to 1.19 on different servers without falses (has been running the past two months). I have still added this as expiremntal check to make sure.
Credits to @iFlamingoz for making this 1.8+ compatible
Additional credits to @f4n74z14 for making this extremely simplified