Skip to content
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

add autoblock check #1467

Open
wants to merge 5 commits into
base: 2.0
Choose a base branch
from
Open

add autoblock check #1467

wants to merge 5 commits into from

Conversation

K0itoyuu
Copy link

No description provided.

@rejomy
Copy link

rejomy commented May 10, 2024

Hello man! What does this line do in the config?

AutoBlock:
  setbackvl: -1 # silent

@K0itoyuu
Copy link
Author

你好,朋友!这行在配置中有什么作用?

AutoBlock:
  setbackvl: -1 # silent

Personal preference

@Anthony01M
Copy link
Contributor

Was this tested? And the one in the config fairly sure it does nothing??

@K0itoyuu
Copy link
Author

Was this tested? And the one in the config fairly sure it does nothing??

It has been tested in 0ms, 30ms, 80ms, 160ms, and 200ms environments

@Anthony01M
Copy link
Contributor

Was this tested? And the one in the config fairly sure it does nothing??

It has been tested in 0ms, 30ms, 80ms, 160ms, and 200ms environments

Server & client version??

@K0itoyuu
Copy link
Author

Was this tested? And the one in the config fairly sure it does nothing??

Was this tested? And the one in the config fairly sure it does nothing??

It has been tested in 0ms, 30ms, 80ms, 160ms, and 200ms environments

Server & client version??
test on 1.8.9,1.12.2,1.20.4(no viaversion)

@K0itoyuu
Copy link
Author

Was this tested? And the one in the config fairly sure it does nothing??

I see, I used to develop other anti-cheats when the flagAndAlert() method can be used to setback directly, but grim is different, I'm going to re-push the code

@AoElite
Copy link
Collaborator

AoElite commented May 11, 2024

When you cancel packets you need to check if it flags & use the method that checks if it should modify packets. Also since you are using bukkit methods to check if they are blocking it's probably going to false with latency.


//Even blocking due to a delay will send Interact before then
if (wrapper.getAction().equals(WrapperPlayClientInteractEntity.InteractAction.ATTACK)) {
if (this.player.bukkitPlayer.isBlocking()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use player.packetStateData.slowedByUsingItem to avoid falses due to desync like https://github.com/GrimAnticheat/Grim/blob/2.0/src/main/java/ac/grim/grimac/events/packets/PacketPlayerAttack.java#L48

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried slowedByUseItem, but it doesn't work

Copy link
Contributor

@ManInMyVan ManInMyVan May 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried slowedByUseItem, but it doesn't work

Try handling it right after BadPacketsW in https://github.com/GrimAnticheat/Grim/blob/2.0/src/main/java/ac/grim/grimac/events/packets/PacketPlayerAttack.java

if (wrapper.getEntityId() != this.lastInteractEntity) {
if (isAboveSetbackVl()) this.player.getSetbackTeleportUtil().executeNonSimulatingSetback();
flagAndAlert();
event.setCancelled(true);
Copy link
Contributor

@ManInMyVan ManInMyVan May 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this with

if (shouldModifyPackets()) {
    event.setCancelled(true);
    player.onPacketCancel();
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

import com.github.retrooper.packetevents.protocol.packettype.PacketType;
import com.github.retrooper.packetevents.wrapper.play.client.WrapperPlayClientInteractEntity;

@CheckData(name = "AutoBlockA (Interact)",configName = "AutoBlock",setback = 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add spaces between the commas

if (wrapper.getAction().equals(WrapperPlayClientInteractEntity.InteractAction.ATTACK)) {
if (this.player.bukkitPlayer.isBlocking()) {
if (this.lastUseItem == -1L && this.useItem == -1L) {
this.useItem = this.player.lastBlockPlaceUseItem;
Copy link
Contributor

@ManInMyVan ManInMyVan May 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works on 1.8 servers iirc

if (event.getPacketType().equals(PacketType.Play.Client.INTERACT_ENTITY)) {
WrapperPlayClientInteractEntity wrapper = new WrapperPlayClientInteractEntity(event);
if (wrapper.getAction().equals(WrapperPlayClientInteractEntity.InteractAction.ATTACK)) {
if (this.player.bukkitPlayer.isBlocking()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, use player.packetStateData.slowedByUsingItem to avoid falses due to desync like https://github.com/GrimAnticheat/Grim/blob/2.0/src/main/java/ac/grim/grimac/events/packets/PacketPlayerAttack.java#L48


//vanilla is unable to do blocking when sending more than two interact_entity packets
if (this.useItem == this.lastUseItem) {
setbackIfAboveSetbackVL();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why setback?

import com.github.retrooper.packetevents.protocol.packettype.PacketType;
import com.github.retrooper.packetevents.wrapper.play.client.WrapperPlayClientInteractEntity;

@CheckData(name = "AutoBlockB (MultiActions)",configName = "AutoBlock",setback = 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, add spaces

//vanilla is unable to do blocking when sending more than two interact_entity packets
if (this.useItem == this.lastUseItem) {
setbackIfAboveSetbackVL();
flagAndAlert("Duplicate tick");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add verbose when the verbose is always the same?

if (wrapper.getAction().equals(WrapperPlayClientInteractEntity.InteractAction.ATTACK)) {
if (this.player.bukkitPlayer.isBlocking()) {
if (wrapper.getEntityId() != this.lastInteractEntity) {
setbackIfAboveSetbackVL();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why setback?

Comment on lines 31 to 33
setbackIfAboveSetbackVL();
flagAndAlert();
event.setCancelled(true);
Copy link
Contributor

@ManInMyVan ManInMyVan May 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flagAndAlert() returns something, check it before you do anything

if (flagAndAlert()) {
    ...
}

Comment on lines 33 to 34
setbackIfAboveSetbackVL();
flagAndAlert("Duplicate tick");
Copy link
Contributor

@ManInMyVan ManInMyVan May 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (flagAndAlert("Duplicate tick")) {
    setbackIfAboveSetbackVL();
}

@K0itoyuu
Copy link
Author

When you cancel packets you need to check if it flags & use the method that checks if it should modify packets. Also since you are using bukkit methods to check if they are blocking it's probably going to false with latency.

i will recode it


//Even blocking due to a delay will send Interact before then
if (wrapper.getAction().equals(WrapperPlayClientInteractEntity.InteractAction.ATTACK)) {
if (this.player.packetStateData.slowedByUsingItem || this.player.packetStateData.wasSlowedByUsingItem) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasSlowedByUsingItem was added to patch switchitem noslow, and is only updated every client tick and on slot changes, this could cause false positives.

@ManInMyVan
Copy link
Contributor

AutoBlockB is easily falsable https://streamable.com/yto3z0

this.lastUseItem = this.useItem;
return;
}
alert("useItem=" + this.useItem + ", lastUseItem=" + this.lastUseItem);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless if this is for debug, this should be flagAndAlert, not just alert

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I couldn't find a debug method in grim, I just used alert instead

import com.github.retrooper.packetevents.protocol.packettype.PacketType;
import com.github.retrooper.packetevents.wrapper.play.client.WrapperPlayClientInteractEntity;

@CheckData(name = "AutoBlockB (MultiActions)", configName = "AutoBlock", setback = 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate AutoBlockA and AutoBlockB in the config

Copy link
Author

@K0itoyuu K0itoyuu May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

@Anthony01M
Copy link
Contributor

Anthony01M commented May 12, 2024

Please solve the conflicts and put this PR as draft; and you should probably handle 1.9+ servers/clients (second hand; shield, etcetera) at the same time.

@K0itoyuu
Copy link
Author

Please solve the conflicts and put this PR as draft; and you should probably handle 1.9+ servers/clients (second hand; shield, etcetera) at the same time.

This check has been tested in 1.9+.

@Anthony01M
Copy link
Contributor

Anthony01M commented May 15, 2024

Please solve the conflicts and put this PR as draft; and you should probably handle 1.9+ servers/clients (second hand; shield, etcetera) at the same time.

This check has been tested in 1.9+.

I'm saying with ViaVersion, you previously said without ViaVersion/Backwards/Rewind.
Please review your conflicts and also remove buffer in AutoBlockA (since it's useless?), it should look something like this:
image
player.onPacketCancel("AutoBlockA (InteractEntity)"); should be changed to player.onPacketCancel(); since there's no "reason" in Grim forgot to remove that part.

@K0itoyuu K0itoyuu closed this May 16, 2024
@K0itoyuu K0itoyuu reopened this May 16, 2024
@ManInMyVan
Copy link
Contributor

If this gets merged we should rename BadPacketsV to AutoBlockC

@ManInMyVan
Copy link
Contributor

3 things:

  • Fix merge conflicts
  • Add these to the punishments config ("AutoBlock" can be used instead of specifying them individually)
  • Do this:

If this gets merged we should rename BadPacketsV to AutoBlockC

@ManInMyVan ManInMyVan added the status: rebase required The pull request needs rebasing onto the merge branch label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: rebase required The pull request needs rebasing onto the merge branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants