Spottedleaf

Hi,

It's important to remember how this behaviour (update suppression) works in the current release version of the game (1.18.2). By recursively performing updates, the block physics propagation code is vulnerable to a stack overflow if enough updates are performed. This is what causes the suppression to happen. While most block update code can fail at any point during its execution without corrupting the world, this is not always the case. If a chunk load were to occur, you are in monstrous trouble. I have seen crashes on Paper servers (which do try and catch the StackOverflow to prevent a total crash, but this fix is from Spigot) that have resulted from exactly that. Modded servers typically have plugins that listen and have behaviours on the block physics as well, and it is not unlikely that a stack overflow may corrupt a plugin's state. However, any logic called by the block tick is vulnerable to crash unpredictably and as such this issue has a very wide surface area of attack for a wide variety of purposes - duplication, crashing, etc. To summarize, this behaviour is dangerous and prone to cause all kinds of problems, especially on modded servers - and honestly, this should be a private issue due to that. We shouldn't be inviting this kind of attention to a bug with a scope like this.

I am glad to see that this bug is modified so that the recursive updates no longer occur. And yes it is a bug, nobody is insane enough to write recursive code like this with the intention of allowing physics to break.

The block update order can be changed with the current code to be DFS (as it is currently in 1.18.2) instead of BFS with the current code as-is (i.e by polling the last added item, rather than the oldest), without requiring recursion to take place. However a BFS update order can be optimised quite well compared to the old. But that is a discussion not meant for this specific issue.

There are no random ticking changes in 21w43a, so this still applies.

I created a fresh 1.7.10 world and loaded it (and attached it here), almost every chunk failed to load

Minecraft intentionally will not tick chunks that are ~128 blocks away from a player, however, an entity can tick as long as its in an entity ticking chunk. This is why this occurs.

It's very clear from the code that this is the behaviour:

If the gamerule is false, then spectators do not add player tickets to chunks. This means that there is no attempt to load the chunks around the player, whether or not they are generated.

If the gamerule is true, then spectators do add player tickets to chunks, like how normal players do.

Forceloaded chunks will still be sent to the player, as the gamerule does not affect whether chunks are sent to the player, or how.

That is, there is NO ATTEMPT to try and load already generated chunks if the gamerule is false. I suspect the actual issue is with the naming of the gamerule.

Sorry, pressing enter multiple times created multiple issues - please close this.

https://bugs.mojang.com/browse/MC-158900

Please re-open this issue, it's still present in 1.14.4. Reproduction steps are the same - set a ban that will expire, wait until expire time, and then join. The exception will only occur once though.

I have submitted a patch to Paper that Mojang is free to use - https://github.com/PaperMC/Paper/pull/2458

(the code is a bit different due to CraftBukkit changes but the same logic applies)

The relevant code is just this one line (spigot mappings), marked // NPE in PlayerList:

if (this.k.isBanned(gameprofile)) {
            GameProfileBanEntry gameprofilebanentry = (GameProfileBanEntry) this.k.get(gameprofile);

            chatmessage = new ChatMessage("multiplayer.disconnect.banned.reason", new Object[]{gameprofilebanentry.getReason()}); // NPE
            if (gameprofilebanentry.getExpires() != null) {
                chatmessage.addSibling(new ChatMessage("multiplayer.disconnect.banned.expiration", new Object[]{PlayerList.g.format(gameprofilebanentry.getExpires())}));
            }

the get() call will remove the relevant entry if it has expired, and then return null.  Then it will clearly NPE on the marked line.

The stacktrace of this occurring on the vanilla server:

[06:30:45] [Server thread/WARN]: Failed to handle packet for /127.0.0.1:50701
java.lang.NullPointerException: null
        at xv.a(SourceFile:357) ~[minecraft_server.jar:?]
        at we.c(SourceFile:97) ~[minecraft_server.jar:?]
        at we.b(SourceFile:63) ~[minecraft_server.jar:?]
        at jc.a(SourceFile:230) ~[minecraft_server.jar:?]
        at wb.c(SourceFile:171) [minecraft_server.jar:?]
        at net.minecraft.server.MinecraftServer.b(SourceFile:847) [minecraft_server.jar:?]
        at uk.b(SourceFile:343) [minecraft_server.jar:?]
        at net.minecraft.server.MinecraftServer.a(SourceFile:774) [minecraft_server.jar:?]
        at net.minecraft.server.MinecraftServer.run(SourceFile:642) [minecraft_server.jar:?]
        at java.lang.Thread.run(Thread.java:748) [?:1.8.0_222]
[06:30:45] [Server thread/INFO]: com.mojang.authlib.GameProfile@664932ca[id=9a9949cc-1987-3d04-b56b-cf3286adfe51,name=searchndstroy,properties={},legacy=false] (/127.0.0.1:50701) lost connection: Internal server error

It's similar in that we both keep double precision on the client/server - but mine changes the entity tracker serverside & clientside so that when relative move packets come in they do not discard the full precision data the client already has (Panda's will do that if the rel movement for the axis is not 0).

I'm also not familiar with the entity tracker 2 years ago but I know that applying his changes on the server entity tracker for 1.14 has an issue with the fact that it will not correctly record what position the client has - making relative move a use the wrong positions for delta (however you wont tend to notice it given how small the error would be). In fairness to Panda this might not have even been an issue 2 years ago, as I'm not familiar with the tracker then. But that issue stands for 1.14's tracker.

After some testing I've found a partial solution.

The current issue with the tracker is how it updates positions to the client. When an absolute position packet comes in (whether it be due to the spawn packet or teleport packet), the client will use the full precision to update the entity position. It will also update the fixed precision "server position" packets (serverPosX MCP 1.14 mappings) on the entity. This is good until a relative move packet comes along.

The handling for relative move is it will update the "server position" fields on the entity, and then re-set the position of the entity using those fields. This is where issues come in: The teleport packet data is rounded off within 3 seconds of it being sent!

I've updated clientside & server side trackers to handle this here:

https://gist.github.com/Spottedleaf/2b7fa048e5bfdf3b804a0864d7dca19d

I've lightly tested this to confirm it works - tested moving around, lots of entities, moving in and out of boats/horses, etc. The patch is for both server & clients, using oldish 1.14.3 MCP mappings. Ignore the // leaf comments - that's just a habit from working on paper.

This patch will ensure correct behavior if you use the following command in a command block:

summon item ~ ~1 ~-0.6249 {Item:{id:"stone",Count:1b}}

The item will fall on top of the command block and will stay there until removed.

My diff basically drops fixed precision positions on client/server - however it still sends relative move with fixed precision. It's just that now clients will store server positions as full precision double values. So when adding relative move, there's no casting away of precision - which was the original problem here.

The server has been modified accordingly to ensure that the positions do not get out of sync. The positions sent to the client are also now stored as full precision in the entity tracker, however I have ensured that we correctly update these positions when sending the fixed precision relative move packets so that they should be exactly what they are on the client.

Now this does NOT entirely solve the issue with MC-4, if an entity moves (on the x/z axis) to the edge of a block it's still possible for them to fall off clientside. However the server eventually sends a teleport packet (full precision) which would resolve it.

I think it is a good idea to also disable gravity client-side if the server has explicitly marked the entity as being on the ground - however that has its own mess to solve and inconsistencies elsewhere, but my patch is a good start to this issue.

EDIT:

I'd like to thank Billy from PaperMC for notifying me relative move packets were causing this issue.