Apply Style Changes From Code Review

Co-authored-by: ManInMyVan <113963219+ManInMyVan@users.noreply.github.com>

src/main/java/ac/grim/grimac/checks/impl/scaffolding/AirLiquidPlace.java

@@ -1,10 +1,12 @@
 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;
@@ -16,12 +18,65 @@ public class AirLiquidPlace extends BlockPlaceCheck {
         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;
+
         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();

src/main/java/ac/grim/grimac/events/packets/CheckManagerListener.java

@@ -6,6 +6,7 @@ import ac.grim.grimac.player.GrimPlayer;
 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;
@@ -476,6 +477,15 @@ public class CheckManagerListener extends PacketListenerAbstract {
                         // 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

src/main/java/ac/grim/grimac/manager/TickManager.java

@@ -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;
@@ -22,6 +23,7 @@ public class 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();
     }
 

src/main/java/ac/grim/grimac/manager/tick/impl/ClearRecentlyUpdatedBlocks.java

@@ -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);
+        }
+    }
+}

src/main/java/ac/grim/grimac/player/GrimPlayer.java

@@ -14,6 +14,7 @@ import ac.grim.grimac.predictionengine.MovementCheckRunner;
 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;
@@ -214,6 +215,7 @@ public class GrimPlayer implements GrimUser {
 
     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;

src/main/java/ac/grim/grimac/utils/change/BlockModification.java

@@ -0,0 +1,38 @@
+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;
+
+    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
+    }
+}

src/main/java/ac/grim/grimac/utils/change/PlayerBlockHistory.java

@@ -0,0 +1,46 @@
+package ac.grim.grimac.utils.change;
+
+import java.util.concurrent.ConcurrentLinkedDeque;
+import java.util.function.Predicate;
+
+/**
+ * Tracks block modifications made by a player over time.
+ */
+public class PlayerBlockHistory {
+    private final ConcurrentLinkedDeque<BlockModification> blockHistory = new ConcurrentLinkedDeque<>();
+
+    /**
+     * Adds a new block modification to the history.
+     * @param modification The block modification to add
+     */
+    public void add(BlockModification modification) {
+        blockHistory.add(modification);
+    }
+
+    /**
+     * Retrieves recent modifications that match the given filter.
+     * @param filter Predicate to filter modifications
+     * @return Filtered list of block modifications
+     */
+    public Iterable<BlockModification> getRecentModifications(Predicate<BlockModification> filter) {
+        return blockHistory.stream().filter(filter).toList();
+    }
+
+    /**
+     * Removes modifications older than the specified tick.
+     * @param maxTick The maximum tick age to keep
+     */
+    public void cleanup(int maxTick) {
+        while (!blockHistory.isEmpty() && maxTick - blockHistory.peekFirst().getTick() > 0) {
+            blockHistory.removeFirst();
+        }
+    }
+
+    public int size() {
+        return blockHistory.size();
+    }
+
+    public void clear() {
+        blockHistory.clear();
+    }
+}

src/main/java/ac/grim/grimac/utils/latency/CompensatedWorld.java

@@ -2,6 +2,7 @@ package ac.grim.grimac.utils.latency;
 
 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;
@@ -139,8 +140,16 @@ public class CompensatedWorld {
     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