godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 97 forks source link

MultiplayerAPI's `connection_failed` signal should state reason if due to a Godot version mismatch #3533

Open goatchurchprime opened 2 years ago

goatchurchprime commented 2 years ago

Describe the project you are working on

Multiplayer VR system

Describe the problem or limitation you are having in your project

I spent most of a day trying to work out why I couldn't connect the Godot app on my computer to the server I have running.

It turned out that I had downloaded the latest Godot version just a few days after it had been upgraded to v3.4 and I didn't notice.

Godot on the server is pretty stable, so my v3.3 app has been running on a server for months accepting multiplayer connections.

I kept getting connection_failed signals and I couldn't work out why, and was hunting down port blocking, running wireshark etc before I recognized the problem.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

During development like this it's easy to get out of sync versions of Godot trying to make multiplayer connections, especially when the version numbers are being upgraded on a frequent schedule and you don't necessarily realize where all the copies trying to connect are.

A simple error code that printed out the message: "Warning, Godot v3.4 MultiplayerENET attempting to connect to Godot v3.3" printed on either the client or the server would help to track down such bugs more quickly.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The first ENET packet that connects to the server should state the Godot version number. This gives it the chance to print the warning if there is a mismatch before unexpected behavior subsequently happens.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No

Is there a reason why this should be core and not an add-on in the asset library?

Networking is core

Faless commented 2 years ago

The problem between Godot 3.3 and 3.4 is that the default ENet compression was changed. Since the server cannot decode the packet it wouldn't be able to detect if the packet contains a different version, or simply junk anyway.

On Thu, Nov 11, 2021, 19:39 Hugo Locurcio @.***> wrote:

cc @Faless https://github.com/Faless

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot-proposals/issues/3533#issuecomment-966535970, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM4C3VH4QIX6ED43ISAUQLULQEWHANCNFSM5H275WIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

mhilbrunner commented 2 years ago

Yeah, to be robust this would require a out of band handshake before the real connection is initialized, I think. (Which could also be used for things like user auth before actually letting the user connect to ENet/other peers - so it would not be just for this, but still quite some additional complexity.)

goatchurchprime commented 2 years ago

That's almost too minimal to not have a fragment of plain text at the start of the binary stream.

A lot of binary file formats have some ascii letters in the first few bytes.

For example, the GLB file format begins with:

It doesn't serve any purpose, but it can really help you out when you're debugging something where completely wrong or corrupted files are the source of an error.

Is this a feature of the ENET library to not have this helpful ascii header junk, or the Godot implementation?

I would avoid having any feature creep (eg user auth) that can anyway be done in GDScript shortly after the connection. This is about leaving some clues in the right place for programmers who are resorting to using Wireshark to diagnose why their networking is failing.

mhilbrunner commented 2 years ago

Is this a feature of the ENET library to not have this helpful ascii header junk, or the Godot implementation?

ENet doesn't have any kind of version check, afaik, and doesn't send such information.

I would avoid having any feature creep (eg user auth) that can anyway be done in GDScript shortly after the connection

The idea would be to avoid exposing the entirety of our RPC/Networking features and GDScript/Godot to unauthenticated users, thus reducding the possible attack surface for them.

dominiks commented 2 years ago

As far as I understand the problem behind this issue as described by @Faless Godot 3.3 and 3.4 cannot event start communicating since they are using different compression modes for ENet. So no information exchange is possible in the first place. I see two possible solutions for the original problem:

  1. Revert the compression change so 3.3 can talk to 3.4
  2. Have 3,4 try to decode an unreadable packet with the old algorithm

Though I don't know what the disadvantages of the old compression are or if option 2 is even possible.

mhilbrunner commented 2 years ago

It's really not expected for different Godot Engine versions to work together in multiplayer. We should add a version check to make sure this is handled with clear errors though I guess.

dominiks commented 2 years ago

Oh maybe this idea No 3 would help there:

When the server receives a connection attempt it cannot decode (because of version differences or its something else than Godot entirely) give out a warning or signal instead of just timing out. That should help identify such an issue when it occurs.

goatchurchprime commented 2 years ago

That idea would be a help -- an error hint. (Although this ticket thread will also serve that purpose if people find it when they search for the problem having guessed the right keywords.)

It does remain an interesting question of a lack of future proofing. It is common practice in networking code for a server to detect the version number of the client and support earlier protocols since you don't always have control of all the nodes. Might not apply to game applications where you can usually force all the clients to upgrade or they don't get their game. (This is hard to enforce if you have embedded devices, maybe mini-game pads running on raspberry Pis that users who can download a new copy.)

But, additionally, the clients need to know when an upgrade required. Obviously they'll have to connect to the game server through some other method (eg http) to find out when an upgrade is due before they try to connect to the ENET server and fail. So in the real world we will always have to design this second channel using a protocol that never upgrades (eg http) in order to communicate the version "it's-time-to-update-your-software" information to the client.

I don't see a way round it, and it feels like it might be quite important during the operational phase of a networked game, not just the development phase when this is simply a bug. We've either got to get into the habit of an http poll of the server before connection to check the version (ie it's upgrade time), or have some way to get the facts from the CONNECTION_FAILED attempt. This might even generalize to a version mismatch error to be emitted even when the ENET library is compatible.