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

Fix AirLiquidPlace falses (and BadPacketsX false) #1822

Open
wants to merge 10 commits into
base: 2.0
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -1,27 +1,83 @@
package ac.grim.grimac.checks.impl.scaffolding;

import ac.grim.grimac.GrimAPI;
import ac.grim.grimac.api.config.ConfigManager;
import ac.grim.grimac.checks.CheckData;
import ac.grim.grimac.checks.type.BlockPlaceCheck;
import ac.grim.grimac.player.GrimPlayer;
import ac.grim.grimac.utils.anticheat.update.BlockPlace;
import ac.grim.grimac.utils.change.BlockModification;
import ac.grim.grimac.utils.nmsutil.Materials;
import com.github.retrooper.packetevents.protocol.player.GameMode;
import com.github.retrooper.packetevents.protocol.world.states.type.StateType;
import com.github.retrooper.packetevents.util.Vector3i;

@CheckData(name = "AirLiquidPlace")
public class AirLiquidPlace extends BlockPlaceCheck {

public AirLiquidPlace(GrimPlayer player) {
super(player);
}

/*
* This check has been plagued by falses for ages, and I've finally figured it out.
* When breaking and placing on the same tick in the same tick, I believe the vanilla client always sends DIGGING ACTION packets first
* This check's falses all seem to stem from processing DiggingAction.START_DIGGING before PacketType.Play.Client.PLAYER_BLOCK_PLACEMENT in the same tick
* Since we process the break first, when we go to process the place it looks like the player placed against air in the async world
*
* We will often see:
* Async world updated: short_grass -> air at X: -32, Y: 69, Z: -240, tick 0, cause/source: DiggingAction.START_DIGGING
* AirLiquidPlace Check: Block state at X: -32, Y: 69, Z: -240 is air (valid: false), tick +0-1, cause/source: PacketType.Play.Client.PLAYER_BLOCK_PLACEMENT <---- previously falsed here
* Async world updated: air -> short_grass at X: -32, Y: 69, Z: -240, tick +3-4, cause: realtime task in applyBlockChanges(List<Vector3i> toApplyBlocks) source: PacketType.Play.Client.PONG
* Async world updated: short_grass -> air at X: -32, Y: 69, Z: -240, tick +0-1, cause: handleNettySyncTransaction(LatencyUtils.java:56) source: PacketType.Play.Client.PONG
*
* In addition, it is possible for:
* Async world updated: short_grass -> air at X: -49, Y: 69, Z: -190, tick 0, cause: realtime task in applyBlockChanges(List<Vector3i> toApplyBlocks) source: PacketType.Play.Client.PONG
* AirLiquidPlace Check: Block state at X: -49, Y: 69, Z: -190 is air (valid=false), tick 0 <--- false due to change from applyBlockChanges()
* Async world updated: grass_block[snowy=false] -> grass_block[snowy=false] at X: -49, Y: 69, Z: -189, tick 0, cause: handleNettySyncTransaction(LatencyUtils.java:56) source: PacketType.Play.Client.PONG
*
* And in even more rare cases:
* Async world updated: air -> air at X: -51, Y: 71, Z: -179, tick 0, cause: handleNettySyncTransaction(LatencyUtils.java:56) source: PacketType.Play.Client.PONG
* AirLiquidPlace Check: Block state at X: -49, Y: 70, Z: -180 is short_grass (valid=true), tick 0
* Async world updated: short_grass -> air at X: -49, Y: 70, Z: -180, tick 1, cause/source: DiggingAction.START_DIGGING <--- double dig here (see my BadPacketsX patch) this is legit behaviour. Can only be up to 2 in 1 tick though.
* Async world updated: air -> short_grass at X: -49, Y: 70, Z: -180, tick 1, cause/source: DiggingAction.START_DIGGING
* Async world updated: short_grass -> air at X: -51, Y: 70, Z: -179, tick 1, cause: realtime task in applyBlockChanges(List<Vector3i> toApplyBlocks) source: PacketType.Play.Client.PONG
* AirLiquidPlace Check: Block state at X: -49, Y: 70, Z: -179 is air (valid=false), tick 2 <--- falses here due to double dig if we only check the latest changed blockstate. We have to check all changes at the location in same tick.
* AirLiquidPlace Check: Block state at X: -49, Y: 70, Z: -179 is air (valid=true), tick 2
*
* All of which previously would've caused a false.
* To solve this we store recently changed blocks caused by DiggingAction.START_DIGGING (instant breaking) and check against the old block.
* Lots of other checks have similar issues, and with the new player.blockHistory we can patch those.
*
* So that's it right? It's unfalsable?
* Very close but not quite. Vanilla's client game desyncs, especially on a laggy connection where a player is breaking and placing grass 20 cps/sec in the same tick
* it is possible for short grass to be interacted with even if server-side the block is air much later, and it won't be accounted for because the modification isn't recent.
* This is incredibly rare, unreliable and is only triggerable if you intentionally want to false the check. I consider a violation lvl of 2-3 to be reliable
* and 5-6 to be autobannable (if you don't care about people who are deliberately trying to get themselves false banned)
*/
@Override
public void onBlockPlace(final BlockPlace place) {
if (player.gamemode == GameMode.CREATIVE) return;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Vector3i blockPos = place.getPlacedAgainstBlockLocation();
StateType placeAgainst = player.compensatedWorld.getStateTypeAt(blockPos.getX(), blockPos.getY(), blockPos.getZ());

int currentTick = GrimAPI.INSTANCE.getTickManager().currentTick;
// this is actual more lenient then we need to be, We can check up to 1 ticks for all changes at location sand up to 0 ticks for first change
// But for such tiny differences in legitness its not worth it.
Iterable<BlockModification> blockModifications = player.blockHistory.getRecentModifications((blockModification) -> currentTick - blockModification.getTick() < 2
&& blockPos.equals(blockModification.getLocation())
&& (blockModification.getCause() == BlockModification.Cause.START_DIGGING || blockModification.getCause() == BlockModification.Cause.HANDLE_NETTY_SYNC_TRANSACTION));

// Check if old block from instant breaking in same tick as the current placement was valid
// There should only be one block here for legit clients
for (BlockModification blockModification : blockModifications) {
StateType stateType = blockModification.getOldBlockContents().getType();
if (!(stateType.isAir() || Materials.isNoPlaceLiquid(stateType))) {
return;
}
}

if (placeAgainst.isAir() || Materials.isNoPlaceLiquid(placeAgainst)) { // fail
if (flagAndAlert() && shouldModifyPackets() && shouldCancel()) {
place.resync();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import ac.grim.grimac.utils.anticheat.update.*;
import ac.grim.grimac.utils.blockplace.BlockPlaceResult;
import ac.grim.grimac.utils.blockplace.ConsumesBlockPlace;
import ac.grim.grimac.utils.change.BlockModification;
import ac.grim.grimac.utils.collisions.HitboxData;
import ac.grim.grimac.utils.collisions.datatypes.CollisionBox;
import ac.grim.grimac.utils.collisions.datatypes.SimpleCollisionBox;
Expand Down Expand Up @@ -476,6 +477,15 @@ public void onPacketReceive(PacketReceiveEvent event) {
// Instant breaking, no damage means it is unbreakable by creative players (with swords)
if (damage >= 1) {
player.compensatedWorld.startPredicting();
player.blockHistory.add(
new BlockModification(
player.compensatedWorld.getWrappedBlockStateAt(blockBreak.position),
WrappedBlockState.getByGlobalId(0),
blockBreak.position,
GrimAPI.INSTANCE.getTickManager().currentTick,
BlockModification.Cause.START_DIGGING
)
);
if (player.getClientVersion().isNewerThanOrEquals(ClientVersion.V_1_13) && Materials.isWaterSource(player.getClientVersion(), blockBreak.block)) {
// Vanilla uses a method to grab water flowing, but as you can't break flowing water
// We can simply treat all waterlogged blocks or source blocks as source blocks
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/ac/grim/grimac/manager/TickManager.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ac.grim.grimac.manager;

import ac.grim.grimac.manager.tick.Tickable;
import ac.grim.grimac.manager.tick.impl.ClearRecentlyUpdatedBlocks;
import ac.grim.grimac.manager.tick.impl.ClientVersionSetter;
import ac.grim.grimac.manager.tick.impl.ResetTick;
import ac.grim.grimac.manager.tick.impl.TickInventory;
Expand All @@ -22,6 +23,7 @@ public TickManager() {
asyncTick = new ImmutableClassToInstanceMap.Builder<Tickable>()
.put(ClientVersionSetter.class, new ClientVersionSetter()) // Async because permission lookups might take a while, depending on the plugin
.put(TickInventory.class, new TickInventory()) // Async because I've never gotten an exception from this. It's probably safe.
.put(ClearRecentlyUpdatedBlocks.class, new ClearRecentlyUpdatedBlocks())
.build();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package ac.grim.grimac.manager.tick.impl;

import ac.grim.grimac.GrimAPI;
import ac.grim.grimac.manager.tick.Tickable;
import ac.grim.grimac.player.GrimPlayer;

public class ClearRecentlyUpdatedBlocks implements Tickable {

private static final int maxTickAge = 2;

@Override
public void tick() {
for (GrimPlayer player : GrimAPI.INSTANCE.getPlayerDataManager().getEntries()) {
player.blockHistory.cleanup(GrimAPI.INSTANCE.getTickManager().currentTick - maxTickAge);
}
}
}
2 changes: 2 additions & 0 deletions src/main/java/ac/grim/grimac/player/GrimPlayer.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import ac.grim.grimac.predictionengine.PointThreeEstimator;
import ac.grim.grimac.predictionengine.UncertaintyHandler;
import ac.grim.grimac.utils.anticheat.LogUtil;
import ac.grim.grimac.utils.change.PlayerBlockHistory;
import ac.grim.grimac.utils.collisions.datatypes.SimpleCollisionBox;
import ac.grim.grimac.utils.data.*;
import ac.grim.grimac.utils.data.packetentity.PacketEntity;
Expand Down Expand Up @@ -214,6 +215,7 @@ public void onPacketCancel() {

public int totalFlyingPacketsSent;
public Queue<BlockPlaceSnapshot> placeUseItemPackets = new LinkedBlockingQueue<>();
public PlayerBlockHistory blockHistory = new PlayerBlockHistory();
// This variable is for support with test servers that want to be able to disable grim
// Grim disabler 2022 still working!
public boolean disableGrim = false;
Expand Down
39 changes: 39 additions & 0 deletions src/main/java/ac/grim/grimac/utils/change/BlockModification.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package ac.grim.grimac.utils.change;

import com.github.retrooper.packetevents.protocol.world.states.WrappedBlockState;
import com.github.retrooper.packetevents.util.Vector3i;
import lombok.Getter;

@Getter
public class BlockModification {
private final WrappedBlockState oldBlockContents;
private final WrappedBlockState newBlockContents;
private final Vector3i location;
private final int tick;
private final Cause cause; // Optional enum for cause
// private final long time; // System time in milliseconds or nanoseconds for ordering
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// private final long time; // System time in milliseconds or nanoseconds for ordering


public BlockModification(WrappedBlockState oldBlockContents, WrappedBlockState newBlockContents,
Vector3i location, int tick, Cause cause) {
this.oldBlockContents = oldBlockContents;
this.newBlockContents = newBlockContents;
this.location = location;
this.tick = tick;
this.cause = cause;
}

@Override
public String toString() {
return String.format(
"BlockModification{location=%s, old=%s, new=%s, tick=%d, cause=%s}",
location, oldBlockContents, newBlockContents, tick, cause
);
}

public enum Cause {
START_DIGGING,
APPLY_BLOCK_CHANGES,
HANDLE_NETTY_SYNC_TRANSACTION,
OTHER
}
}
35 changes: 35 additions & 0 deletions src/main/java/ac/grim/grimac/utils/change/PlayerBlockHistory.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package ac.grim.grimac.utils.change;

import java.util.concurrent.ConcurrentLinkedDeque;
import java.util.function.Predicate;

public class PlayerBlockHistory {
private final ConcurrentLinkedDeque<BlockModification> blockHistory = new ConcurrentLinkedDeque<>();

// Add a new block modification to the history.
public void add(BlockModification modification) {
blockHistory.add(modification);
}

// Get all recent modifications (optionally filtered by a condition).
public Iterable<BlockModification> getRecentModifications(Predicate<BlockModification> filter) {
return blockHistory.stream().filter(filter).toList(); // Java 8+ compatible
}

// Remove old modifications older than maxTick
public void cleanup(int maxTick) {
while (!blockHistory.isEmpty() && maxTick - blockHistory.peekFirst().getTick() > 0) {
blockHistory.removeFirst();
}
}

// Get the size of the block history
public int size() {
return blockHistory.size();
}

// Clear all block modifications
public void clear() {
blockHistory.clear();
}
}
11 changes: 10 additions & 1 deletion src/main/java/ac/grim/grimac/utils/latency/CompensatedWorld.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import ac.grim.grimac.GrimAPI;
import ac.grim.grimac.player.GrimPlayer;
import ac.grim.grimac.utils.change.BlockModification;
import ac.grim.grimac.utils.chunks.Column;
import ac.grim.grimac.utils.collisions.CollisionData;
import ac.grim.grimac.utils.collisions.datatypes.SimpleCollisionBox;
Expand Down Expand Up @@ -139,8 +140,16 @@ private void applyBlockChanges(List<Vector3i> toApplyBlocks) {
private void handleAck(Vector3i vector3i, int originalBlockId, Vector3d playerPosition) {
// If we need to change the world block state
if (getWrappedBlockStateAt(vector3i).getGlobalId() != originalBlockId) {
player.blockHistory.add(
new BlockModification(
player.compensatedWorld.getWrappedBlockStateAt(vector3i),
WrappedBlockState.getByGlobalId(originalBlockId),
vector3i,
GrimAPI.INSTANCE.getTickManager().currentTick,
BlockModification.Cause.HANDLE_NETTY_SYNC_TRANSACTION
)
);
updateBlock(vector3i.getX(), vector3i.getY(), vector3i.getZ(), originalBlockId);

WrappedBlockState state = WrappedBlockState.getByGlobalId(blockVersion, originalBlockId);

// The player will teleport themselves if they get stuck in the reverted block
Expand Down