Fixed a whole bunch of issues with legacy transactions

src/network/mcpe/InventoryManager.php

@@ -51,6 +51,7 @@ use pocketmine\network\mcpe\protocol\MobEquipmentPacket;
 use pocketmine\network\mcpe\protocol\types\BlockPosition;
 use pocketmine\network\mcpe\protocol\types\inventory\ContainerIds;
 use pocketmine\network\mcpe\protocol\types\inventory\CreativeContentEntry;
+use pocketmine\network\mcpe\protocol\types\inventory\ItemStack;
 use pocketmine\network\mcpe\protocol\types\inventory\ItemStackWrapper;
 use pocketmine\network\mcpe\protocol\types\inventory\NetworkInventoryAction;
 use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset;
@@ -100,6 +101,8 @@ class InventoryManager{
 	private ?\Closure $pendingOpenWindowCallback = null;
 
 	private int $nextItemStackId = 1;
+	private ?int $currentItemStackRequestId = null;
+
 	/**
 	 * @var int[][]
 	 * @phpstan-var array<int, array<int, ItemStackInfo>>
@@ -186,7 +189,7 @@ class InventoryManager{
 		return null;
 	}
 
-	public function addPredictedSlotChange(Inventory $inventory, int $slot, Item $item) : void{
+	private function addPredictedSlotChange(Inventory $inventory, int $slot, Item $item) : void{
 		$predictions = ($this->initiatedSlotChanges[spl_object_id($inventory)] ??= new InventoryManagerPredictedChanges($inventory));
 		$predictions->add($slot, $item);
 	}
@@ -223,6 +226,10 @@ class InventoryManager{
 		}
 	}
 
+	public function setCurrentItemStackRequestId(?int $id) : void{
+		$this->currentItemStackRequestId = $id;
+	}
+
 	/**
 	 * When the server initiates a window close, it does so by sending a ContainerClose to the client, which causes the
 	 * client to behave as if it initiated the close itself. It responds by sending a ContainerClose back to the server,
@@ -390,8 +397,25 @@ class InventoryManager{
 					//BDS (Bedrock Dedicated Server) also seems to work this way.
 					$this->session->sendDataPacket(InventoryContentPacket::create($windowId, [$itemStackWrapper]));
 				}else{
+					if($this->currentItemStackRequestId !== null){
+						//TODO: HACK!
+						//When right-clicking to equip armour, the client predicts the content of the armour slot, but
+						//doesn't report it in the transaction packet. The server then sends an InventorySlotPacket to
+						//the client, assuming the slot changed for some other reason, since there is no prediction for
+						//the slot.
+						//However, later requests involving that itemstack will refer to the request ID in which the
+						//armour was equipped, instead of the stack ID provided by the server in the outgoing
+						//InventorySlotPacket. (Perhaps because the item is already the same as the client actually
+						//predicted, but didn't tell us?)
+						//We work around this bug by setting the slot to air and then back to the correct item. In
+						//theory, setting a different count and then back again (or changing any other property) would
+						//also work, but this is simpler.
+						$this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, new ItemStackWrapper(0, ItemStack::null())));
+					}
 					$this->session->sendDataPacket(InventorySlotPacket::create($windowId, $netSlot, $itemStackWrapper));
 				}
+			}elseif($this->currentItemStackRequestId !== null){
+				$this->trackItemStack($inventory, $slot, $currentItem, $this->currentItemStackRequestId);
 			}
 			$predictions?->remove($slot);
 		}
@@ -498,11 +522,15 @@ class InventoryManager{
 		return $this->nextItemStackId++;
 	}
 
-	public function trackItemStack(Inventory $inventory, int $slotId, Item $item, ?int $itemStackRequestId) : ItemStackInfo{
+	public function getItemStackInfo(Inventory $inventory, int $slot) : ?ItemStackInfo{
+		return $this->itemStackInfos[spl_object_id($inventory)][$slot] ?? null;
+	}
+
+	private function trackItemStack(Inventory $inventory, int $slotId, Item $item, ?int $itemStackRequestId) : ItemStackInfo{
 		$existing = $this->itemStackInfos[spl_object_id($inventory)][$slotId] ?? null;
 		$typeConverter = TypeConverter::getInstance();
 		$itemStack = $typeConverter->coreItemStackToNet($item);
-		if($existing !== null && $existing->getItemStack()->equals($itemStack)){
+		if($existing !== null && $existing->getItemStack()->equals($itemStack) && $existing->getRequestId() === $itemStackRequestId){
 			return $existing;
 		}
 
@@ -515,7 +543,7 @@ class InventoryManager{
 		return new ItemStackWrapper($info->getStackId(), $info->getItemStack());
 	}
 
-	public function matchItemStack(Inventory $inventory, int $slotId, int $itemStackId) : bool{
+	public function matchItemStack(Inventory $inventory, int $slotId, int $clientItemStackId) : bool{
 		$inventoryObjectId = spl_object_id($inventory);
 		if(!isset($this->itemStackInfos[$inventoryObjectId])){
 			$this->session->getLogger()->debug("Attempted to match item preimage unsynced inventory " . get_class($inventory) . "#" . $inventoryObjectId);
@@ -527,10 +555,10 @@ class InventoryManager{
 			return false;
 		}
 
-		if(!($itemStackId < 0 ? $info->getRequestId() === $itemStackId : $info->getStackId() === $itemStackId)){
+		if(!($clientItemStackId < 0 ? $info->getRequestId() === $clientItemStackId : $info->getStackId() === $clientItemStackId)){
 			$this->session->getLogger()->debug(
 				"Mismatched expected itemstack: " . get_class($inventory) . "#" . $inventoryObjectId . ", " .
-				"slot: $slotId, expected: $itemStackId, actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none")
+				"slot: $slotId, client expected: $clientItemStackId, server actual: " . $info->getStackId() . ", last modified by request: " . ($info->getRequestId() ?? "none")
 			);
 			return false;
 		}

src/network/mcpe/handler/InGamePacketHandler.php

@@ -97,6 +97,8 @@ use pocketmine\network\mcpe\protocol\types\inventory\MismatchTransactionData;
 use pocketmine\network\mcpe\protocol\types\inventory\NetworkInventoryAction;
 use pocketmine\network\mcpe\protocol\types\inventory\NormalTransactionData;
 use pocketmine\network\mcpe\protocol\types\inventory\ReleaseItemTransactionData;
+use pocketmine\network\mcpe\protocol\types\inventory\stackrequest\ItemStackRequest;
+use pocketmine\network\mcpe\protocol\types\inventory\stackresponse\ItemStackResponse;
 use pocketmine\network\mcpe\protocol\types\inventory\UIInventorySlotOffset;
 use pocketmine\network\mcpe\protocol\types\inventory\UseItemOnEntityTransactionData;
 use pocketmine\network\mcpe\protocol\types\inventory\UseItemTransactionData;
@@ -258,6 +260,8 @@ class InGamePacketHandler extends PacketHandler{
 			if(count($useItemTransaction->getTransactionData()->getActions()) > 100){
 				throw new PacketHandlingException("Too many actions in item use transaction");
 			}
+
+			$this->inventoryManager->setCurrentItemStackRequestId($useItemTransaction->getRequestId());
 			$this->inventoryManager->addRawPredictedSlotChanges($useItemTransaction->getTransactionData()->getActions());
 			if(!$this->handleUseItemTransaction($useItemTransaction->getTransactionData())){
 				$packetHandled = false;
@@ -265,14 +269,13 @@ class InGamePacketHandler extends PacketHandler{
 			}else{
 				$this->inventoryManager->syncMismatchedPredictedSlotChanges();
 			}
+			$this->inventoryManager->setCurrentItemStackRequestId(null);
 		}
 
 		$itemStackRequest = $packet->getItemStackRequest();
 		if($itemStackRequest !== null){
-			$executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $itemStackRequest);
+			$result = $this->handleSingleItemStackRequest($itemStackRequest);
-			$transaction = $executor->generateInventoryTransaction();
+			$this->session->sendDataPacket(ItemStackResponsePacket::create([$result]));
-			$result = $this->executeInventoryTransaction($transaction);
-			$this->session->sendDataPacket(ItemStackResponsePacket::create([$executor->buildItemStackResponse($result)]));
 		}
 
 		$blockActions = $packet->getBlockActions();
@@ -330,10 +333,11 @@ class InGamePacketHandler extends PacketHandler{
 			throw new PacketHandlingException("Too many actions in inventory transaction");
 		}
 
+		$this->inventoryManager->setCurrentItemStackRequestId($packet->requestId);
 		$this->inventoryManager->addRawPredictedSlotChanges($packet->trData->getActions());
 
 		if($packet->trData instanceof NormalTransactionData){
-			$result = $this->handleNormalTransaction($packet->trData);
+			$result = $this->handleNormalTransaction($packet->trData, $packet->requestId);
 		}elseif($packet->trData instanceof MismatchTransactionData){
 			$this->session->getLogger()->debug("Mismatch transaction received");
 			$this->inventoryManager->syncAll();
@@ -349,22 +353,14 @@ class InGamePacketHandler extends PacketHandler{
 		if($this->craftingTransaction === null){ //don't sync if we're waiting to complete a crafting transaction
 			$this->inventoryManager->syncMismatchedPredictedSlotChanges();
 		}
-		if($packet->requestId !== 0){
+		$this->inventoryManager->setCurrentItemStackRequestId(null);
-			$itemStackResponseBuilder = new ItemStackResponseBuilder($packet->requestId, $this->inventoryManager);
-			foreach($packet->requestChangedSlots as $requestChangedSlotsEntry){
-				foreach($requestChangedSlotsEntry->getChangedSlotIndexes() as $slotId){
-					$itemStackResponseBuilder->addSlot($requestChangedSlotsEntry->getContainerId(), $slotId);
-				}
-			}
-			$itemStackResponse = $itemStackResponseBuilder->build($result);
-			$this->session->sendDataPacket(ItemStackResponsePacket::create([$itemStackResponse]));
-			$this->session->getLogger()->debug("Sent item stack response for fake stack request " . $packet->requestId);
-		}
 		return $result;
 	}
 
-	private function executeInventoryTransaction(InventoryTransaction $transaction) : bool{
+	private function executeInventoryTransaction(InventoryTransaction $transaction, int $requestId) : bool{
 		$this->player->setUsingItem(false);
+
+		$this->inventoryManager->setCurrentItemStackRequestId($requestId);
 		$this->inventoryManager->addTransactionPredictedSlotChanges($transaction);
 		try{
 			$transaction->execute();
@@ -373,12 +369,15 @@ class InGamePacketHandler extends PacketHandler{
 			$logger->debug("Failed to execute inventory transaction: " . $e->getMessage());
 
 			return false;
+		}finally{
+			$this->inventoryManager->syncMismatchedPredictedSlotChanges();
+			$this->inventoryManager->setCurrentItemStackRequestId(null);
 		}
 
 		return true;
 	}
 
-	private function handleNormalTransaction(NormalTransactionData $data) : bool{
+	private function handleNormalTransaction(NormalTransactionData $data, int $itemStackRequestId) : bool{
 		/** @var InventoryAction[] $actions */
 		$actions = [];
 
@@ -426,7 +425,7 @@ class InGamePacketHandler extends PacketHandler{
 				return true;
 			}
 			try{
-				return $this->executeInventoryTransaction($this->craftingTransaction);
+				return $this->executeInventoryTransaction($this->craftingTransaction, $itemStackRequestId);
 			}finally{
 				$this->craftingTransaction = null;
 			}
@@ -443,7 +442,7 @@ class InGamePacketHandler extends PacketHandler{
 				return true;
 			}
 
-			return $this->executeInventoryTransaction(new InventoryTransaction($this->player, $actions));
+			return $this->executeInventoryTransaction(new InventoryTransaction($this->player, $actions), $itemStackRequestId);
 		}
 	}
 
@@ -558,14 +557,19 @@ class InGamePacketHandler extends PacketHandler{
 		return false;
 	}
 
+	private function handleSingleItemStackRequest(ItemStackRequest $request) : ItemStackResponse{
+		$executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $request);
+		$transaction = $executor->generateInventoryTransaction();
+		$result = $this->executeInventoryTransaction($transaction, $request->getRequestId());
+		$this->session->getLogger()->debug("Item stack request " . $request->getRequestId() . " result: " . ($result ? "success" : "failure"));
+
+		return $executor->buildItemStackResponse($result);
+	}
+
 	public function handleItemStackRequest(ItemStackRequestPacket $packet) : bool{
 		$responses = [];
 		foreach($packet->getRequests() as $request){
-			$executor = new ItemStackRequestExecutor($this->player, $this->inventoryManager, $request);
+			$responses[] = $this->handleSingleItemStackRequest($request);
-			$transaction = $executor->generateInventoryTransaction();
-			$result = $this->executeInventoryTransaction($transaction);
-			$this->session->getLogger()->debug("Item stack request " . $request->getRequestId() . " result: " . ($result ? "success" : "failure"));
-			$responses[] = $executor->buildItemStackResponse($result);
 		}
 
 		$this->session->sendDataPacket(ItemStackResponsePacket::create($responses));

src/network/mcpe/handler/ItemStackResponseBuilder.php

@@ -70,14 +70,23 @@ final class ItemStackResponseBuilder{
 			foreach($slotIds as $slotId){
 				[$inventory, $slot] = $this->getInventoryAndSlot($containerInterfaceId, $slotId);
 
+				$itemStackInfo = $this->inventoryManager->getItemStackInfo($inventory, $slot);
+				if($itemStackInfo === null){
+					//TODO: what if a plugin closes the inventory while the transaction is ongoing?
+					throw new \LogicException("ItemStackInfo should never be null for an open inventory");
+				}
+				if($itemStackInfo->getRequestId() !== $this->requestId){
+					//the itemstack may have been synced due to transaction producing results that the client did not
+					//predict correctly, which will wipe out the tracked request ID (intentionally)
+					continue;
+				}
 				$item = $inventory->getItem($slot);
-				$info = $this->inventoryManager->trackItemStack($inventory, $slot, $item, $this->requestId);
 
 				$responseInfosByContainer[$containerInterfaceId][] = new ItemStackResponseSlotInfo(
 					$slotId,
 					$slotId,
-					$info->getItemStack()->getCount(),
+					$item->getCount(),
-					$info->getStackId(),
+					$itemStackInfo->getStackId(),
 					$item->hasCustomName() ? $item->getCustomName() : "",
 					0
 				);