fix: deterministic resolution of colliding argument-type ids - #1869
Open
bejiihiu wants to merge 1 commit into
Open
fix: deterministic resolution of colliding argument-type ids#1869bejiihiu wants to merge 1 commit into
bejiihiu wants to merge 1 commit into
Conversation
Fixes PaperMC#1868. ArgumentPropertyRegistry resolved a numeric argument id to an ArgumentIdentifier by linearly scanning byIdentifier.keySet() and returning the first match. For protocol versions where a Forge/CrossStitch identifier reuses a vanilla id (e.g. forge:enum and minecraft:template_rotation both claim id 50 on 1.21.11), the returned identifier depended on HashMap iteration order, which is seeded per JVM start. On unlucky boots the wrong serializer read the argument properties, desyncing the packet parser and surfacing as Unknown node type 3 for OP players - cured only by a restart. Changes: - Give ArgumentIdentifier stable equals/hashCode based on its identifier string, so map behaviour no longer depends on identity hashes - Build a per-version numeric id -> identifier index once at class init instead of scanning the registry on every packet; lookups are O(1) and order-independent - Resolve id collisions deterministically: vanilla (minecraft:*/brigadier:*) namespaces win over mod namespaces, with a lexicographic tie-break; collisions are logged and fail fast at startup if a positive id is ever reused - readIdentifier now throws CorruptedFrameException for an unknown id, matching the rest of the packet decoder - Harden AvailableCommandsPacket against truncated frames (missing bytes / out-of-bounds root index) and include the node index in the unknown node type message Added ArgumentPropertyRegistryTests covering collision resolution, determinism across registration order, and round-tripping every registered identifier (including the new pre-1.19 string path). Validation: - All proxy tests pass - Checkstyle and Spotless clean - Full Gradle build successful
Contributor
Author
|
hi, sorry my english is very bad, i am not know english at all. i write this explain with help of llm and google translate lol. if words is weird its google translate fault not mine hehe. hope you understand what bug is about. thank for reading my pr :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1868.
ArgumentPropertyRegistryresolved a numeric argument id to anArgumentIdentifierby linearly scanningbyIdentifier.keySet()and returning the first match. For protocol versions where a Forge/CrossStitch identifier reuses a vanilla id (e.g.forge:enumandminecraft:template_rotationboth claim id 50 on 1.21.11), the returned identifier depended onHashMapiteration order, which is seeded per JVM start. On unlucky boots the wrong serializer read the argument properties, desyncing the packet parser and surfacing asUnknown node type 3for OP players — cured only by a restart.Changes:
ArgumentIdentifierstableequals/hashCodebased on its identifier string, so map behaviour no longer depends on identity hashesminecraft:*/brigadier:*) namespaces win over mod namespaces, with a lexicographic tie-break; collisions are logged and fail fast at startup if a positive id is ever reusedreadIdentifiernow throwsCorruptedFrameExceptionfor an unknown id, matching the rest of the packet decoderAvailableCommandsPacketagainst truncated frames (missing bytes / out-of-bounds root index) and include the node index in the unknown node type messageAdded
ArgumentPropertyRegistryTestscovering collision resolution, determinism across registration order, and round-tripping every registered identifier (including the new pre-1.19 string path).Validation: