[Bug] Incorrect game protocol assumptions

[rubidium42]

The implementation of the game protocol assumes that the 40th packet type is a shut down. However, this lies far within the range of the packet types for which "ordering is unimportant from here".

As a result of changes this will now parse the "server company update" packet, and when the company passwords are gone the "client quit" packet. Those are not necessarily a big issue on itself, as the only user of the "server shutdown" packet is the game-coordinator, and that does not login so it won't get the "server company update" packet and it should never get the "client quit" packet from the server. It might receive an actual shutdown that causes a PacketInvalidType to be raised though.

https://github.com/OpenTTD/py-protocol/blob/7fc1da26c7a3fcf4d1290647567ac184d5a75745/openttd_protocol/protocol/game.py#L170C4-L174C7

So might it maybe make sense, since we know it's "broken" in any case to fix this for the future by moving a number of the more useful packets to the "stable" range such as "server shutdown" and maybe some others? Or would only "server shutdown" be enough?

As I understand the OpenTTD code at least "shut down", "new game", "send chat" and "send external chat" are called for all connections.

I would consider both chats to be bugs in OpenTTD; it should not send those unless at least identified (having logged in) => OpenTTD/OpenTTD#12377.

I would suggest moving "server shut down" and "server new game" to just after the 8 original stable packet types and before the range of unstable packet types. That way those packets will remain stable in the future. Since the packets immediately after the original 8 stable packets will not be called without triggering a join, it is unlikely that any server will be sending anything with those packet numbers that isn't a shut down or new game packet without (the game-coordinator) sending a join. So it wouldn't break the game-coordinator any more than it already is => OpenTTD/OpenTTD#12379.

[TrueBrain]

As I understand the code at least "shut down", "new game", "send chat" and "send external chat" are called for all connections.

It seems this comment only makes sense in the context of the OpenTTD bug, and seems unrelated to this repository? At least it is confusing me :)

Anyway, makes total sense to move packets handled by game-coordinator to the stable range; that makes life a bit easier. Would still need glue to fix it for pre-15 servers, but I will take care of that.

[rubidium42]

As I understand the code at least "shut down", "new game", "send chat" and "send external chat" are called for all connections.

It seems this comment only makes sense in the context of the OpenTTD bug, and seems unrelated to this repository? At least it is confusing me :)

Anyway, makes total sense to move packets handled by game-coordinator to the stable range; that makes life a bit easier. Would still need glue to fix it for pre-15 servers, but I will take care of that.

Sorry for the confusing comment. I started writing down what I saw in OpenTTD's code without explicitly noting that. It was all triggered by reading the linked bit of py-protocol's code which was definitely wrong, and trying to find good long term solution. In any case, I have made the appropriate OpenTTD PRs. I marked both 'backport requested' as they are essentially about a broken protocol, but feel free to change that for the change of the packet numbers if that makes more sense in the realm of py-protocol.

[TrueBrain]

Doing it for 14.0 is fine. Better sooner than later :D Nice find btw, I totally forgot we also monitor the shutdown command :)

[TrueBrain]

Now seeing this ticket, I guess I have to take some kind of action. I just kinda forgot what action. Can you briefly refresh my memory? Otherwise I will delve into this later myself :)

[TrueBrain]

Ah, right. When querying a server it might return SERVER_SHUTDOWN or SERVER_NEWGAME. These were floating, as in, every version of OpenTTD could have them assigned to another number, as we never promised anyone they would be stable.

Since ... 14.0? they now have an assigned spot, and are guaranteed to be that number.

So I guess I should fix up this repository to reflect that. Not a big priority, as the code currently using the value, isn't all that likely to receive it anyway.

The biggest "issue" is that there isn't actually a protocol-version or anything. So it is hard to know whether the server is a 14.0 server, and in what position SERVER_SHUTDOWN is. Additionally, SERVER_SHUTDOWN can be sent at any moment from when the TCP connection is created. It is sent to all connections, no matter their state.

Adding to that, as the only "versioning" for the game protocol is a string, we can't even say: if above this value, start using it like that.

Basically, this is nearly impossible to implement in a way that is guaranteed to work in all cases; what-ever we will do, it will be fuzzy logic. Which means this will never be a generic game-protocol implementation, but rather a "only works for querying server information".

In that scenario, it is easier. As if you receive any (!) packet after connection that is above ID 7 (CLIENT_GAME_INFO), it must be either SERVER_NEWGAME or SERVER_SHUTDOWN. And from the querying perceptive, neither is relevant, as in all cases we cannot query the game.

Either way, this is all a lot of work for very little benefit. As in both cases, the TCP connection will be terminated too, so we can just monitor for a disconnect, and deal with that. And we already do that. So we are better of just removing this code completely.

A very long talk to say: let's remove the SHUTDOWN part, and not try to fix this; I don't see any sane way to do so.