Merge branch 'PaperMC:dev/3.0.0' into forwarding-mode

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/TransitionSessionHandler.java

@@ -137,12 +137,14 @@ public class TransitionSessionHandler implements MinecraftSessionHandler {
           smc.setActiveSessionHandler(StateRegistry.PLAY,
               new BackendPlaySessionHandler(server, serverConn));
 
-          // Clean up disabling auto-read while the connected event was being processed.
-          smc.setAutoReading(true);
-
           // Now set the connected server.
           serverConn.getPlayer().setConnectedServer(serverConn);
 
+          // Clean up disabling auto-read while the connected event was being processed.
+          // Do this after setting the connection, so no incoming packets are processed before
+          // the API knows which server the player is connected to.
+          smc.setAutoReading(true);
+
           // Send client settings. In 1.20.2+ this is done in the config state.
           if (smc.getProtocolVersion().lessThan(ProtocolVersion.MINECRAFT_1_20_2)
               && player.getClientSettingsPacket() != null) {

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java

@@ -54,6 +54,7 @@ import com.velocitypowered.proxy.protocol.packet.ServerboundCookieResponsePacket
 import com.velocitypowered.proxy.protocol.packet.TabCompleteRequestPacket;
 import com.velocitypowered.proxy.protocol.packet.TabCompleteResponsePacket;
 import com.velocitypowered.proxy.protocol.packet.TabCompleteResponsePacket.Offer;
+import com.velocitypowered.proxy.protocol.packet.chat.ChatAcknowledgementPacket;
 import com.velocitypowered.proxy.protocol.packet.chat.ChatHandler;
 import com.velocitypowered.proxy.protocol.packet.chat.ChatTimeKeeper;
 import com.velocitypowered.proxy.protocol.packet.chat.CommandHandler;
@@ -423,6 +424,15 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
     return true;
   }
 
+  @Override
+  public boolean handle(ChatAcknowledgementPacket packet) {
+    if (player.getCurrentServer().isEmpty()) {
+      return true;
+    }
+    player.getChatQueue().handleAcknowledgement(packet.offset());
+    return true;
+  }
+
   @Override
   public boolean handle(ServerboundCookieResponsePacket packet) {
     server.getEventManager()

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java

@@ -83,6 +83,7 @@ import com.velocitypowered.proxy.protocol.packet.chat.ChatType;
 import com.velocitypowered.proxy.protocol.packet.chat.ComponentHolder;
 import com.velocitypowered.proxy.protocol.packet.chat.PlayerChatCompletionPacket;
 import com.velocitypowered.proxy.protocol.packet.chat.builder.ChatBuilderFactory;
+import com.velocitypowered.proxy.protocol.packet.chat.builder.ChatBuilderV2;
 import com.velocitypowered.proxy.protocol.packet.chat.legacy.LegacyChatPacket;
 import com.velocitypowered.proxy.protocol.packet.config.ClientboundServerLinksPacket;
 import com.velocitypowered.proxy.protocol.packet.config.StartUpdatePacket;
@@ -1108,11 +1109,12 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player,
         "input cannot be greater than " + LegacyChatPacket.MAX_SERVERBOUND_MESSAGE_LENGTH
             + " characters in length");
     if (getProtocolVersion().noLessThan(ProtocolVersion.MINECRAFT_1_19)) {
-      this.chatQueue.hijack(getChatBuilderFactory().builder().asPlayer(this).message(input),
+      ChatBuilderV2 message = getChatBuilderFactory().builder().asPlayer(this).message(input);
-          (instant, item) -> {
+      this.chatQueue.queuePacket(chatState -> {
-            item.setTimestamp(instant);
+        message.setTimestamp(chatState.lastTimestamp);
-            return item.toServer();
+        message.setLastSeenMessages(chatState.createLastSeen());
-          });
+        return message.toServer();
+      });
     } else {
       ensureBackendConnection().write(getChatBuilderFactory().builder()
           .asPlayer(this).message(input).toServer());

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/ChatAcknowledgementPacket.java

@@ -54,4 +54,8 @@ public class ChatAcknowledgementPacket implements MinecraftPacket {
                 "offset=" + offset +
                 '}';
     }
+
+    public int offset() {
+        return offset;
+    }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/ChatQueue.java

@@ -23,7 +23,9 @@ import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import io.netty.channel.ChannelFuture;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import java.time.Instant;
+import java.util.BitSet;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
 
 /**
@@ -32,9 +34,10 @@ import java.util.function.Function;
  */
 public class ChatQueue {
 
-  private final Object internalLock;
+  private final Object internalLock = new Object();
   private final ConnectedPlayer player;
-  private CompletableFuture<WrappedPacket> packetFuture;
+  private final ChatState chatState = new ChatState();
+  private CompletableFuture<Void> head = CompletableFuture.completedFuture(null);
 
   /**
    * Instantiates a {@link ChatQueue} for a specific {@link ConnectedPlayer}.
@@ -43,8 +46,19 @@ public class ChatQueue {
    */
   public ChatQueue(ConnectedPlayer player) {
     this.player = player;
-    this.packetFuture = CompletableFuture.completedFuture(new WrappedPacket(Instant.EPOCH, null));
+  }
-    this.internalLock = new Object();
+
+  private void queueTask(Task task) {
+    synchronized (internalLock) {
+      MinecraftConnection smc = player.ensureAndGetCurrentServer().ensureConnected();
+      head = head.thenCompose(v -> {
+        try {
+          return task.update(chatState, smc).exceptionally(ignored -> null);
+        } catch (Throwable ignored) {
+          return CompletableFuture.completedFuture(null);
+        }
+      });
+    }
   }
 
   /**
@@ -52,121 +66,115 @@ public class ChatQueue {
    * packets. This maintains order on the server-level for the client insertions of commands
    * and messages. All entries are locked through an internal object lock.
    *
-   * @param nextPacket the {@link CompletableFuture} which will provide the next-processed packet.
+   * @param nextPacket       a function mapping {@link LastSeenMessages} state to a {@link CompletableFuture} that will
-   * @param timestamp  the {@link Instant} timestamp of this packet so we can allow piggybacking.
+   *                         provide the next-processed packet. This should include the fixed {@link LastSeenMessages}.
+   * @param timestamp        the new {@link Instant} timestamp of this packet to update the internal chat state.
+   * @param lastSeenMessages the new {@link LastSeenMessages} last seen messages to update the internal chat state.
    */
-  public void queuePacket(CompletableFuture<MinecraftPacket> nextPacket, Instant timestamp) {
+  public void queuePacket(Function<LastSeenMessages, CompletableFuture<MinecraftPacket>> nextPacket, @Nullable Instant timestamp, @Nullable LastSeenMessages lastSeenMessages) {
-    synchronized (internalLock) { // wait for the lock to resolve - we don't want to drop packets
+    queueTask((chatState, smc) -> {
-      MinecraftConnection smc = player.ensureAndGetCurrentServer().ensureConnected();
+      LastSeenMessages newLastSeenMessages = chatState.updateFromMessage(timestamp, lastSeenMessages);
-
+      return nextPacket.apply(newLastSeenMessages).thenCompose(packet -> writePacket(packet, smc));
-      CompletableFuture<WrappedPacket> nextInLine = WrappedPacket.wrap(timestamp, nextPacket);
+    });
-      this.packetFuture = awaitChat(smc, this.packetFuture,
-          nextInLine); // we await chat, binding `this.packetFuture` -> `nextInLine`
-    }
   }
 
   /**
-   * Hijacks the latest sent packet's timestamp to provide an in-order packet without polling the
+   * Hijacks the latest sent packet's chat state to provide an in-order packet without polling the
    * physical, or prior packets sent through the stream.
    *
-   * @param packet        the {@link MinecraftPacket} to send.
+   * @param packetFunction a function that maps the prior {@link ChatState} into a new packet.
-   * @param instantMapper the {@link InstantPacketMapper} which maps the prior timestamp and current
+   * @param <T>            the type of packet to send.
-   *                      packet to a new packet.
-   * @param <K>           the type of base to expect when mapping the packet.
-   * @param <V>           the type of packet for instantMapper type-checking.
    */
-  public <K, V extends MinecraftPacket> void hijack(K packet,
+  public <T extends MinecraftPacket> void queuePacket(Function<ChatState, T> packetFunction) {
-      InstantPacketMapper<K, V> instantMapper) {
+    queueTask((chatState, smc) -> {
-    synchronized (internalLock) {
+      T packet = packetFunction.apply(chatState);
-      CompletableFuture<K> trueFuture = CompletableFuture.completedFuture(packet);
+      return writePacket(packet, smc);
-      MinecraftConnection smc = player.ensureAndGetCurrentServer().ensureConnected();
+    });
-
-      this.packetFuture = hijackCurrentPacket(smc, this.packetFuture, trueFuture, instantMapper);
-    }
   }
 
-  private static Function<WrappedPacket, WrappedPacket> writePacket(MinecraftConnection connection) {
+  public void handleAcknowledgement(int offset) {
-    return wrappedPacket -> {
+    queueTask((chatState, smc) -> {
-      if (!connection.isClosed()) {
+      int ackCountToForward = chatState.accumulateAckCount(offset);
-        ChannelFuture future = wrappedPacket.write(connection);
+      if (ackCountToForward > 0) {
+        return writePacket(new ChatAcknowledgementPacket(ackCountToForward), smc);
+      }
+      return CompletableFuture.completedFuture(null);
+    });
+  }
+
+  private static <T extends MinecraftPacket> CompletableFuture<Void> writePacket(T packet, MinecraftConnection smc) {
+    return CompletableFuture.runAsync(() -> {
+      if (!smc.isClosed()) {
+        ChannelFuture future = smc.write(packet);
         if (future != null) {
           future.awaitUninterruptibly();
         }
       }
-
+    }, smc.eventLoop());
-      return wrappedPacket;
-    };
   }
 
-  private static <T extends MinecraftPacket> CompletableFuture<WrappedPacket> awaitChat(
+  private interface Task {
-      MinecraftConnection connection,
+    CompletableFuture<Void> update(ChatState chatState, MinecraftConnection smc);
-      CompletableFuture<WrappedPacket> binder,
-      CompletableFuture<WrappedPacket> future
-  ) {
-    // the binder will run -> then the future will get the `write packet` caller
-    return binder.thenCompose(ignored -> future.thenApply(writePacket(connection)));
-  }
-
-  private static <K, V extends MinecraftPacket> CompletableFuture<WrappedPacket> hijackCurrentPacket(
-      MinecraftConnection connection,
-      CompletableFuture<WrappedPacket> binder,
-      CompletableFuture<K> future,
-      InstantPacketMapper<K, V> packetMapper
-  ) {
-    CompletableFuture<WrappedPacket> awaitedFuture = new CompletableFuture<>();
-    // the binder will complete -> then the future will get the `write packet` caller
-    binder.whenComplete((previous, ignored) -> {
-      // map the new packet into a better "designed" packet with the hijacked packet's timestamp
-      WrappedPacket.wrap(previous.timestamp,
-              future.thenApply(item -> packetMapper.map(previous.timestamp, item)))
-          .thenApplyAsync(writePacket(connection), connection.eventLoop())
-          .whenComplete(
-              (packet, throwable) -> awaitedFuture.complete(throwable != null ? null : packet));
-    });
-    return awaitedFuture;
   }
 
   /**
-   * Provides an {@link Instant} based timestamp mapper from an existing object to create a packet.
+   * Tracks the last Secure Chat state that we received from the client. This is important to always have a valid 'last
+   * seen' state that is consistent with future and past updates from the client (which may be signed). This state is
+   * used to construct 'spoofed' command packets from the proxy to the server.
+   * <ul>
+   *     <li>If we last forwarded a chat or command packet from the client, we have a known 'last seen' that we can
+   *     reuse.</li>
+   *     <li>If we last forwarded a {@link ChatAcknowledgementPacket}, the previous 'last seen' cannot be reused. We
+   *     cannot predict an up-to-date 'last seen', as we do not know which messages the client actually saw.</li>
+   *     <li>Therefore, we need to hold back any acknowledgement packets so that we can continue to reuse the last valid
+   *     'last seen' state.</li>
+   *     <li>However, there is a limit to the number of messages that can remain unacknowledged on the server.</li>
+   *     <li>To address this, we know that if the client has moved its 'last seen' window far enough, we can fill in the
+   *     gap with dummy 'last seen', and it will never be checked.</li>
+   * </ul>
    *
-   * @param <K> The base object type to map.
+   * Note that this is effectively unused for 1.20.5+ clients, as commands without any signature do not send 'last seen'
-   * @param <V> The resulting packet type.
+   * updates.
    */
-  public interface InstantPacketMapper<K, V extends MinecraftPacket> {
+  public static class ChatState {
+    private static final int MINIMUM_DELAYED_ACK_COUNT = LastSeenMessages.WINDOW_SIZE;
+    private static final BitSet DUMMY_LAST_SEEN_MESSAGES = new BitSet();
 
-    /**
+    public volatile Instant lastTimestamp = Instant.EPOCH;
-     * Maps a value into a packet with it and a timestamp.
+    private volatile BitSet lastSeenMessages = new BitSet();
-     *
+    private final AtomicInteger delayedAckCount = new AtomicInteger();
-     * @param nextInstant   the {@link Instant} timestamp to use for tracking.
-     * @param currentObject the current item to map to the packet.
-     * @return The resulting packet from the mapping.
-     */
-    V map(Instant nextInstant, K currentObject);
-  }
 
-  private static class WrappedPacket {
+    private ChatState() {
-
-    private final Instant timestamp;
-    private final MinecraftPacket packet;
-
-    private WrappedPacket(Instant timestamp, MinecraftPacket packet) {
-      this.timestamp = timestamp;
-      this.packet = packet;
     }
 
     @Nullable
-    public ChannelFuture write(MinecraftConnection connection) {
+    public LastSeenMessages updateFromMessage(@Nullable Instant timestamp, @Nullable LastSeenMessages lastSeenMessages) {
-      if (packet != null) {
+      if (timestamp != null) {
-        return connection.write(packet);
+        this.lastTimestamp = timestamp;
+      }
+      if (lastSeenMessages != null) {
+        // We held back some acknowledged messages, so flush that out now that we have a known 'last seen' state again
+        int delayedAckCount = this.delayedAckCount.getAndSet(0);
+        this.lastSeenMessages = lastSeenMessages.getAcknowledged();
+        return lastSeenMessages.offset(delayedAckCount);
       }
       return null;
     }
 
-    private static CompletableFuture<WrappedPacket> wrap(Instant timestamp,
+    public int accumulateAckCount(int ackCount) {
-        CompletableFuture<MinecraftPacket> nextPacket) {
+      int delayedAckCount = this.delayedAckCount.addAndGet(ackCount);
-      return nextPacket
+      int ackCountToForward = delayedAckCount - MINIMUM_DELAYED_ACK_COUNT;
-          .thenApply(pkt -> new WrappedPacket(timestamp, pkt))
+      if (ackCountToForward >= LastSeenMessages.WINDOW_SIZE) {
-          .exceptionally(ignored -> new WrappedPacket(timestamp, null));
+        // Because we only forward acknowledgements above the window size, we don't have to shift the previous 'last seen' state
+        this.lastSeenMessages = DUMMY_LAST_SEEN_MESSAGES;
+        this.delayedAckCount.set(MINIMUM_DELAYED_ACK_COUNT);
+        return ackCountToForward;
+      }
+      return 0;
+    }
+
+    public LastSeenMessages createLastSeen() {
+      return new LastSeenMessages(0, lastSeenMessages);
     }
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/CommandHandler.java

@@ -23,11 +23,13 @@ import com.velocitypowered.proxy.connection.client.ConnectedPlayer;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import java.time.Instant;
 import java.util.concurrent.CompletableFuture;
+import java.util.function.BiFunction;
 import java.util.function.Function;
 import net.kyori.adventure.text.Component;
 import net.kyori.adventure.text.format.NamedTextColor;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 public interface CommandHandler<T extends MinecraftPacket> {
 
@@ -53,11 +55,12 @@ public interface CommandHandler<T extends MinecraftPacket> {
   }
 
   default void queueCommandResult(VelocityServer server, ConnectedPlayer player,
-      Function<CommandExecuteEvent, CompletableFuture<MinecraftPacket>> futurePacketCreator,
+      BiFunction<CommandExecuteEvent, LastSeenMessages, CompletableFuture<MinecraftPacket>> futurePacketCreator,
-      String message, Instant timestamp) {
+      String message, Instant timestamp, @Nullable LastSeenMessages lastSeenMessages) {
-    player.getChatQueue().queuePacket(
+      CompletableFuture<CommandExecuteEvent> eventFuture = server.getCommandManager().callCommandEvent(player, message);
-        server.getCommandManager().callCommandEvent(player, message)
+      player.getChatQueue().queuePacket(
-            .thenComposeAsync(futurePacketCreator)
+        newLastSeenMessages -> eventFuture
+            .thenComposeAsync(event -> futurePacketCreator.apply(event, newLastSeenMessages))
             .thenApply(pkt -> {
               if (server.getConfiguration().isLogCommandExecutions()) {
                 logger.info("{} -> executed command /{}", player, message);
@@ -68,6 +71,6 @@ public interface CommandHandler<T extends MinecraftPacket> {
               player.sendMessage(
                   Component.translatable("velocity.command.generic-error", NamedTextColor.RED));
               return null;
-            }), timestamp);
+            }), timestamp, lastSeenMessages);
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/LastSeenMessages.java

@@ -24,7 +24,8 @@ import java.util.BitSet;
 
 public class LastSeenMessages {
 
-  private static final int DIV_FLOOR = -Math.floorDiv(-20, 8);
+  public static final int WINDOW_SIZE = 20;
+  private static final int DIV_FLOOR = -Math.floorDiv(-WINDOW_SIZE, 8);
   private int offset;
   private BitSet acknowledged;
 
@@ -33,6 +34,11 @@ public class LastSeenMessages {
     this.acknowledged = new BitSet();
   }
 
+  public LastSeenMessages(int offset, BitSet acknowledged) {
+    this.offset = offset;
+    this.acknowledged = acknowledged;
+  }
+
   public LastSeenMessages(ByteBuf buf) {
     this.offset = ProtocolUtils.readVarInt(buf);
 
@@ -46,14 +52,18 @@ public class LastSeenMessages {
     buf.writeBytes(Arrays.copyOf(acknowledged.toByteArray(), DIV_FLOOR));
   }
 
-  public boolean isEmpty() {
-    return acknowledged.isEmpty();
-  }
-
   public int getOffset() {
     return this.offset;
   }
 
+  public BitSet getAcknowledged() {
+    return acknowledged;
+  }
+
+  public LastSeenMessages offset(final int offset) {
+    return new LastSeenMessages(this.offset + offset, acknowledged);
+  }
+
   @Override
   public String toString() {
     return "LastSeenMessages{" +

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/builder/ChatBuilderV2.java

@@ -21,6 +21,7 @@ import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.api.proxy.Player;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.packet.chat.ChatType;
+import com.velocitypowered.proxy.protocol.packet.chat.LastSeenMessages;
 import java.time.Instant;
 import net.kyori.adventure.identity.Identity;
 import net.kyori.adventure.text.Component;
@@ -36,6 +37,7 @@ public abstract class ChatBuilderV2 {
   protected @Nullable Identity senderIdentity;
   protected Instant timestamp;
   protected ChatType type = ChatType.CHAT;
+  protected @Nullable LastSeenMessages lastSeenMessages;
 
   protected ChatBuilderV2(ProtocolVersion version) {
     this.version = version;
@@ -77,6 +79,11 @@ public abstract class ChatBuilderV2 {
     return this;
   }
 
+  public ChatBuilderV2 setLastSeenMessages(LastSeenMessages lastSeenMessages) {
+    this.lastSeenMessages = lastSeenMessages;
+    return this;
+  }
+
   public abstract MinecraftPacket toClient();
 
   public abstract MinecraftPacket toServer();

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/keyed/KeyedChatHandler.java

@@ -91,11 +91,12 @@ public class KeyedChatHandler implements
       });
     }
     chatQueue.queuePacket(
-        chatFuture.exceptionally((ex) -> {
+        newLastSeen -> chatFuture.exceptionally((ex) -> {
           logger.error("Exception while handling player chat for {}", player, ex);
           return null;
         }),
-        packet.getExpiry()
+        packet.getExpiry(),
+        null
     );
   }
 

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/keyed/KeyedCommandHandler.java

@@ -43,7 +43,7 @@ public class KeyedCommandHandler implements CommandHandler<KeyedPlayerCommandPac
 
   @Override
   public void handlePlayerCommandInternal(KeyedPlayerCommandPacket packet) {
-    queueCommandResult(this.server, this.player, event -> {
+    queueCommandResult(this.server, this.player, (event, newLastSeenMessages) -> {
       CommandExecuteEvent.CommandResult result = event.getResult();
       IdentifiedKey playerKey = player.getIdentifiedKey();
       if (result == CommandExecuteEvent.CommandResult.denied()) {
@@ -111,6 +111,6 @@ public class KeyedCommandHandler implements CommandHandler<KeyedPlayerCommandPac
         }
         return null;
       });
-    }, packet.getCommand(), packet.getTimestamp());
+    }, packet.getCommand(), packet.getTimestamp(), null);
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/legacy/LegacyCommandHandler.java

@@ -42,7 +42,7 @@ public class LegacyCommandHandler implements CommandHandler<LegacyChatPacket> {
   @Override
   public void handlePlayerCommandInternal(LegacyChatPacket packet) {
     String command = packet.getMessage().substring(1);
-    queueCommandResult(this.server, this.player, event -> {
+    queueCommandResult(this.server, this.player, (event, newLastSeenMessages) -> {
       CommandExecuteEvent.CommandResult result = event.getResult();
       if (result == CommandExecuteEvent.CommandResult.denied()) {
         return CompletableFuture.completedFuture(null);
@@ -62,6 +62,6 @@ public class LegacyCommandHandler implements CommandHandler<LegacyChatPacket> {
         }
         return null;
       });
-    }, command, Instant.now());
+    }, command, Instant.now(), null);
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/session/SessionChatBuilder.java

@@ -41,6 +41,7 @@ public class SessionChatBuilder extends ChatBuilderV2 {
 
   @Override
   public MinecraftPacket toServer() {
+    LastSeenMessages lastSeenMessages = this.lastSeenMessages != null ? this.lastSeenMessages : new LastSeenMessages();
     if (message.startsWith("/")) {
       if (version.noLessThan(ProtocolVersion.MINECRAFT_1_20_5)) {
         UnsignedPlayerCommandPacket command = new UnsignedPlayerCommandPacket();
@@ -52,7 +53,7 @@ public class SessionChatBuilder extends ChatBuilderV2 {
         command.salt = 0L;
         command.timeStamp = timestamp;
         command.argumentSignatures = new SessionPlayerCommandPacket.ArgumentSignatures();
-        command.lastSeenMessages = new LastSeenMessages();
+        command.lastSeenMessages = lastSeenMessages;
         return command;
       }
     } else {
@@ -62,8 +63,8 @@ public class SessionChatBuilder extends ChatBuilderV2 {
       chat.signature = new byte[0];
       chat.timestamp = timestamp;
       chat.salt = 0L;
-      chat.lastSeenMessages = new LastSeenMessages();
+      chat.lastSeenMessages = lastSeenMessages;
       return chat;
     }
   }
-}
+}

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/session/SessionChatHandler.java

@@ -29,6 +29,8 @@ import com.velocitypowered.proxy.protocol.packet.chat.ChatQueue;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
+import java.util.concurrent.CompletableFuture;
+
 public class SessionChatHandler implements ChatHandler<SessionPlayerChatPacket> {
 
   private static final Logger logger = LogManager.getLogger(SessionChatHandler.class);
@@ -51,8 +53,9 @@ public class SessionChatHandler implements ChatHandler<SessionPlayerChatPacket>
     ChatQueue chatQueue = this.player.getChatQueue();
     EventManager eventManager = this.server.getEventManager();
     PlayerChatEvent toSend = new PlayerChatEvent(player, packet.getMessage());
+    CompletableFuture<PlayerChatEvent> eventFuture = eventManager.fire(toSend);
     chatQueue.queuePacket(
-        eventManager.fire(toSend)
+        newLastSeenMessages -> eventFuture
             .thenApply(pme -> {
               PlayerChatEvent.ChatResult chatResult = pme.getResult();
               if (!chatResult.isAllowed()) {
@@ -70,15 +73,17 @@ public class SessionChatHandler implements ChatHandler<SessionPlayerChatPacket>
                 }
                 return this.player.getChatBuilderFactory().builder().message(packet.message)
                     .setTimestamp(packet.timestamp)
+                    .setLastSeenMessages(newLastSeenMessages)
                     .toServer();
               }
-              return packet;
+              return packet.withLastSeenMessages(newLastSeenMessages);
             })
             .exceptionally((ex) -> {
               logger.error("Exception while handling player chat for {}", player, ex);
               return null;
             }),
-        packet.getTimestamp()
+        packet.getTimestamp(),
+        packet.getLastSeenMessages()
     );
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/session/SessionCommandHandler.java

@@ -18,13 +18,14 @@
 package com.velocitypowered.proxy.protocol.packet.chat.session;
 
 import com.velocitypowered.api.event.command.CommandExecuteEvent;
-import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.VelocityServer;
 import com.velocitypowered.proxy.connection.client.ConnectedPlayer;
+import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.packet.chat.ChatAcknowledgementPacket;
 import com.velocitypowered.proxy.protocol.packet.chat.CommandHandler;
 import java.util.concurrent.CompletableFuture;
 import net.kyori.adventure.text.Component;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 public class SessionCommandHandler implements CommandHandler<SessionPlayerCommandPacket> {
 
@@ -41,78 +42,81 @@ public class SessionCommandHandler implements CommandHandler<SessionPlayerComman
     return SessionPlayerCommandPacket.class;
   }
 
+  @Nullable
+  private MinecraftPacket consumeCommand(SessionPlayerCommandPacket packet) {
+    if (packet.lastSeenMessages == null) {
+      return null;
+    }
+    if (packet.isSigned()) {
+      // Any signed message produced by the client *must* be passed through to the server in order to maintain a
+      // consistent state for future messages.
+      logger.fatal("A plugin tried to deny a command with signable component(s). "
+          + "This is not supported. "
+          + "Disconnecting player " + player.getUsername() + ". Command packet: " + packet);
+      player.disconnect(Component.text(
+          "A proxy plugin caused an illegal protocol state. "
+              + "Contact your network administrator."));
+      return null;
+    }
+    // An unsigned command with a 'last seen' update will not happen as of 1.20.5+, but for earlier versions - we still
+    // need to pass through the acknowledgement
+    final int offset = packet.lastSeenMessages.getOffset();
+    if (offset != 0) {
+      return new ChatAcknowledgementPacket(offset);
+    }
+    return null;
+  }
+
+  @Nullable
+  private MinecraftPacket forwardCommand(SessionPlayerCommandPacket packet, String newCommand) {
+    if (newCommand.equals(packet.command)) {
+      return packet;
+    }
+    return modifyCommand(packet, newCommand);
+  }
+
+  @Nullable
+  private MinecraftPacket modifyCommand(SessionPlayerCommandPacket packet, String newCommand) {
+    if (packet.isSigned()) {
+      logger.fatal("A plugin tried to change a command with signed component(s). "
+          + "This is not supported. "
+          + "Disconnecting player " + player.getUsername() + ". Command packet: " + packet);
+      player.disconnect(Component.text(
+          "A proxy plugin caused an illegal protocol state. "
+              + "Contact your network administrator."));
+      return null;
+    }
+
+    return this.player.getChatBuilderFactory()
+        .builder()
+        .setTimestamp(packet.timeStamp)
+        .setLastSeenMessages(packet.lastSeenMessages)
+        .asPlayer(this.player)
+        .message("/" + newCommand)
+        .toServer();
+  }
+
   @Override
   public void handlePlayerCommandInternal(SessionPlayerCommandPacket packet) {
-    queueCommandResult(this.server, this.player, event -> {
+    queueCommandResult(this.server, this.player, (event, newLastSeenMessages) -> {
+      SessionPlayerCommandPacket fixedPacket = packet.withLastSeenMessages(newLastSeenMessages);
+
       CommandExecuteEvent.CommandResult result = event.getResult();
       if (result == CommandExecuteEvent.CommandResult.denied()) {
-        if (packet.isSigned()) {
+        return CompletableFuture.completedFuture(consumeCommand(fixedPacket));
-          logger.fatal("A plugin tried to deny a command with signable component(s). "
-              + "This is not supported. "
-              + "Disconnecting player " + player.getUsername() + ". Command packet: " + packet);
-          player.disconnect(Component.text(
-              "A proxy plugin caused an illegal protocol state. "
-                  + "Contact your network administrator."));
-        }
-        // We seemingly can't actually do this if signed args exist, if not, we can probs keep stuff happy
-        if (player.getProtocolVersion().noLessThan(ProtocolVersion.MINECRAFT_1_19_3) && packet.lastSeenMessages != null) {
-          return CompletableFuture.completedFuture(new ChatAcknowledgementPacket(packet.lastSeenMessages.getOffset()));
-        }
-        return CompletableFuture.completedFuture(null);
       }
 
-      String commandToRun = result.getCommand().orElse(packet.command);
+      String commandToRun = result.getCommand().orElse(fixedPacket.command);
       if (result.isForwardToServer()) {
-        if (packet.isSigned() && commandToRun.equals(packet.command)) {
+        return CompletableFuture.completedFuture(forwardCommand(fixedPacket, commandToRun));
-          return CompletableFuture.completedFuture(packet);
-        } else {
-          if (packet.isSigned()) {
-            logger.fatal("A plugin tried to change a command with signed component(s). "
-                + "This is not supported. "
-                + "Disconnecting player " + player.getUsername() + ". Command packet: " + packet);
-            player.disconnect(Component.text(
-                "A proxy plugin caused an illegal protocol state. "
-                    + "Contact your network administrator."));
-            return CompletableFuture.completedFuture(null);
-          }
-
-          return CompletableFuture.completedFuture(this.player.getChatBuilderFactory()
-              .builder()
-              .setTimestamp(packet.timeStamp)
-              .asPlayer(this.player)
-              .message("/" + commandToRun)
-              .toServer());
-        }
       }
 
       return runCommand(this.server, this.player, commandToRun, hasRun -> {
-        if (!hasRun) {
+        if (hasRun) {
-          if (packet.isSigned() && commandToRun.equals(packet.command)) {
+          return consumeCommand(fixedPacket);
-            return packet;
-          } else {
-            if (packet.isSigned()) {
-              logger.fatal("A plugin tried to change a command with signed component(s). "
-                  + "This is not supported. "
-                  + "Disconnecting player " + player.getUsername() + ". Command packet: " + packet);
-              player.disconnect(Component.text(
-                  "A proxy plugin caused an illegal protocol state. "
-                      + "Contact your network administrator."));
-              return null;
-            }
-
-            return this.player.getChatBuilderFactory()
-                .builder()
-                .setTimestamp(packet.timeStamp)
-                .asPlayer(this.player)
-                .message("/" + commandToRun)
-                .toServer();
-          }
         }
-        if (player.getProtocolVersion().noLessThan(ProtocolVersion.MINECRAFT_1_19_3) && packet.lastSeenMessages != null) {
+        return forwardCommand(fixedPacket, commandToRun);
-          return new ChatAcknowledgementPacket(packet.lastSeenMessages.getOffset());
-        }
-        return null;
       });
-    }, packet.command, packet.timeStamp);
+    }, packet.command, packet.timeStamp, packet.lastSeenMessages);
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/session/SessionPlayerChatPacket.java

@@ -99,4 +99,15 @@ public class SessionPlayerChatPacket implements MinecraftPacket {
     buf.readBytes(signature);
     return signature;
   }
+
+  public SessionPlayerChatPacket withLastSeenMessages(LastSeenMessages lastSeenMessages) {
+    SessionPlayerChatPacket packet = new SessionPlayerChatPacket();
+    packet.message = message;
+    packet.timestamp = timestamp;
+    packet.salt = salt;
+    packet.signed = signed;
+    packet.signature = signature;
+    packet.lastSeenMessages = lastSeenMessages;
+    return packet;
+  }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/session/SessionPlayerCommandPacket.java

@@ -25,6 +25,8 @@ import com.velocitypowered.proxy.protocol.ProtocolUtils;
 import com.velocitypowered.proxy.protocol.packet.chat.LastSeenMessages;
 import com.velocitypowered.proxy.util.except.QuietDecoderException;
 import io.netty.buffer.ByteBuf;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
 import java.time.Instant;
 import java.util.List;
 
@@ -63,8 +65,7 @@ public class SessionPlayerCommandPacket implements MinecraftPacket {
   }
 
   public boolean isSigned() {
-    if (salt == 0) return false;
+    return !argumentSignatures.isEmpty();
-    return !lastSeenMessages.isEmpty() || !argumentSignatures.isEmpty();
   }
 
   @Override
@@ -83,6 +84,21 @@ public class SessionPlayerCommandPacket implements MinecraftPacket {
             '}';
   }
 
+  public SessionPlayerCommandPacket withLastSeenMessages(@Nullable LastSeenMessages lastSeenMessages) {
+    if (lastSeenMessages == null) {
+      UnsignedPlayerCommandPacket packet = new UnsignedPlayerCommandPacket();
+      packet.command = command;
+      return packet;
+    }
+    SessionPlayerCommandPacket packet = new SessionPlayerCommandPacket();
+    packet.command = command;
+    packet.timeStamp = timeStamp;
+    packet.salt = salt;
+    packet.argumentSignatures = argumentSignatures;
+    packet.lastSeenMessages = lastSeenMessages;
+    return packet;
+  }
+
   public static class ArgumentSignatures {
 
     private final List<ArgumentSignature> entries;

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/chat/session/UnsignedPlayerCommandPacket.java

@@ -19,7 +19,9 @@ package com.velocitypowered.proxy.protocol.packet.chat.session;
 
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
+import com.velocitypowered.proxy.protocol.packet.chat.LastSeenMessages;
 import io.netty.buffer.ByteBuf;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 public class UnsignedPlayerCommandPacket extends SessionPlayerCommandPacket {
 
@@ -33,6 +35,11 @@ public class UnsignedPlayerCommandPacket extends SessionPlayerCommandPacket {
     ProtocolUtils.writeString(buf, this.command);
   }
 
+  @Override
+  public SessionPlayerCommandPacket withLastSeenMessages(@Nullable LastSeenMessages lastSeenMessages) {
+    return this;
+  }
+
   public boolean isSigned() {
     return false;
   }