Fixed mismatched predictions due to NBT key order differences

this is a pain :(
It appears the client always sorts the keys in alphabetical order due to use of std::map. However I'm not sure of the exact ordering behaviour, so it needs to be investigated.

src/network/mcpe/InventoryManager.php

@@ -423,6 +423,41 @@ class InventoryManager{
 		}
 	}
 
+	/**
+	 * Compares itemstack extra data for equality. This is used to verify legacy InventoryTransaction slot predictions.
+	 *
+	 * TODO: It would be preferable if we didn't have to deserialize this, to improve performance and reduce attack
+	 * surface. However, the raw data may not match due to differences in ordering. Investigate whether the
+	 * client-provided NBT is consistently sorted.
+	 */
+	private function itemStackExtraDataEqual(ItemStack $left, ItemStack $right) : bool{
+		if($left->getRawExtraData() === $right->getRawExtraData()){
+			return true;
+		}
+
+		$typeConverter = $this->session->getTypeConverter();
+		$leftExtraData = $typeConverter->deserializeItemStackExtraData($left->getRawExtraData(), $left->getId());
+		$rightExtraData = $typeConverter->deserializeItemStackExtraData($right->getRawExtraData(), $right->getId());
+
+		$leftNbt = $leftExtraData->getNbt();
+		$rightNbt = $rightExtraData->getNbt();
+		return
+			$leftExtraData->getCanPlaceOn() === $rightExtraData->getCanPlaceOn() &&
+			$leftExtraData->getCanDestroy() === $rightExtraData->getCanDestroy() && (
+				$leftNbt === $rightNbt || //this covers null === null and fast object identity
+				($leftNbt !== null && $rightNbt !== null && $leftNbt->equals($rightNbt))
+			);
+	}
+
+	private function itemStacksEqual(ItemStack $left, ItemStack $right) : bool{
+		return
+			$left->getId() === $right->getId() &&
+			$left->getMeta() === $right->getMeta() &&
+			$left->getBlockRuntimeId() === $right->getBlockRuntimeId() &&
+			$left->getCount() === $right->getCount() &&
+			$this->itemStackExtraDataEqual($left, $right);
+	}
+
 	public function onSlotChange(Inventory $inventory, int $slot) : void{
 		$inventoryEntry = $this->inventories[spl_object_id($inventory)] ?? null;
 		if($inventoryEntry === null){
@@ -432,7 +467,7 @@ class InventoryManager{
 		}
 		$currentItem = $this->session->getTypeConverter()->coreItemStackToNet($inventory->getItem($slot));
 		$clientSideItem = $inventoryEntry->predictions[$slot] ?? null;
-		if($clientSideItem === null || !$clientSideItem->equals($currentItem)){
+		if($clientSideItem === null || !$this->itemStacksEqual($currentItem, $clientSideItem)){
 			//no prediction or incorrect - do not associate this with the currently active itemstack request
 			$this->trackItemStack($inventoryEntry, $slot, $currentItem, null);
 			$inventoryEntry->pendingSyncs[$slot] = $currentItem;

src/network/mcpe/convert/TypeConverter.php

@@ -248,10 +248,7 @@ class TypeConverter{
 		if($itemStack->getId() === 0){
 			return VanillaItems::AIR();
 		}
-		$extraDataDeserializer = PacketSerializer::decoder($itemStack->getRawExtraData(), 0);
+		$extraData = $this->deserializeItemStackExtraData($itemStack->getRawExtraData(), $itemStack->getId());
-		$extraData = $itemStack->getId() === $this->shieldRuntimeId ?
-			ItemStackExtraDataShield::read($extraDataDeserializer) :
-			ItemStackExtraData::read($extraDataDeserializer);
 
 		$compound = $extraData->getNbt();
 
@@ -272,4 +269,11 @@ class TypeConverter{
 
 		return $itemResult;
 	}
+
+	public function deserializeItemStackExtraData(string $extraData, int $id) : ItemStackExtraData{
+		$extraDataDeserializer = PacketSerializer::decoder($extraData, 0);
+		return $id === $this->shieldRuntimeId ?
+			ItemStackExtraDataShield::read($extraDataDeserializer) :
+			ItemStackExtraData::read($extraDataDeserializer);
+	}
 }

src/network/mcpe/handler/InGamePacketHandler.php

@@ -444,9 +444,18 @@ class InGamePacketHandler extends PacketHandler{
 			return false;
 		}
 		$serverItemStack = $this->session->getTypeConverter()->coreItemStackToNet($sourceSlotItem);
-		//because the client doesn't tell us the expected itemstack ID, we have to deep-compare our known
+		//Sadly we don't have itemstack IDs here, so we have to compare the basic item properties to ensure that we're
-		//itemstack info with the one the client sent. This is costly, but we don't have any other option :(
+		//dropping the item the client expects (inventory might be out of sync with the client).
-		if(!$serverItemStack->equals($clientItemStack)){
+		if(
+			$serverItemStack->getId() !== $clientItemStack->getId() ||
+			$serverItemStack->getMeta() !== $clientItemStack->getMeta() ||
+			$serverItemStack->getCount() !== $clientItemStack->getCount() ||
+			$serverItemStack->getBlockRuntimeId() !== $clientItemStack->getBlockRuntimeId()
+			//Raw extraData may not match because of TAG_Compound key ordering differences, and decoding it to compare
+			//is costly. Assume that we're in sync if id+meta+count+runtimeId match.
+			//NB: Make sure $clientItemStack isn't used to create the dropped item, as that would allow the client
+			//to change the item NBT since we're not validating it.
+		){
 			return false;
 		}