Allow ENetConnection to send a packet to an arbitrary destination
#77627
Conversation
|
Do you have a proposal related to this? And what's the reason this is only exposed to scripting? |
There is not a proposal for this. I also wasn't aware of any other mechanisms for exposing C++ functionality other than the ClassDB. Unless you mean why there's no Node workflow for this? |
|
The function is private, so you can't call it (directly) from c++, I was asking if there was any reason you made that choice, or that you weren't aware This is the kind of change that needs a proposal, explaining why this is needed, and allowing discussions, you need to follow the contribution workflow, so that things can be looked into and worked out, see here |
|
No, I was not aware. I put it next to I opened the PR at this point to start getting feedback from actual C++ devs (beyond the Godot rocket chat, where I've been discussing the change for a little while now). |
How do you mean? This can be called from GDScript so how do you control that? But yes, please open a proposal, while discussion on chat is great it doesn't offer the opportunity to everyone in the community here to respond and give their support or criticism or opinions |
|
Oh, just with one of the |
|
But that will work if you call from c++ too, and it'll make it possible to use it from extensions and modules |
|
Oh, I'm totally open to switching it from a private method to a public method, I just figured since there were so many private methods in the class that it had enough friends that it wasn't going to matter. I can also open a proposal, though it's pretty much going to be the same text as the PR here. |
|
Proposals are the place to discuss the "if" and "why" and "what", but a PR is better served by mainly discussing the "how" of the code itself, separating the two is important |
|
The thing with this particular feature is that the 'if' and 'why' are fundamental functions of internet traffic. Without this, ICE via STUN is simply impossible with ENet, and users must fall back to a PacketPeerUDP implementation and re-do all of it themselves. It also railroads the solution pretty well: you have to send a packet from the same exact socket that you are going to receive the connection on. There is no other answer, that's just how NAT and sockets work. There's very little to discuss for a proposal conversation. It all ends up boiling down to the 'how'. I'll open the proposal when I have time to do it, maybe some time this evening. |
There was a problem hiding this comment.
Hi and thanks for your first contribution.
This is something I agree should be included in the module.
The PR seems overall good, I left some comments to fix the style so CI passes (for future PRs, you can configure git pre-commit hooks to automate the task https://github.com/godotengine/godot/blob/master/misc/hooks/README.md , we are in the process of better documenting those ;-) ).
As I mention in a comment, I also think the method should be named socket_send to better reflect the underlying ENet method used, and allow devs to specify their own data.
One last thing this PR does not address is the DTLS variant, since the method will not work once the DTLS server has been setup. I don't think that's a blocker to the merge, so don't worry too much about that, but it's something we should address at some point in the future.
|
All of that sounds completely fair. I'll make the requested changes when I get back from traveling on Sunday or Monday. For the DTLS server variant, I wasn't sure what it is or how it works very well, I had looked at the sendto implementation for it and saw it was much more complex, and figured it's probably not something that would see use in a home network self hosting sort of situation. I'd love to know more about the use cases for the DTLS upgrade, if you're willing to explain more? |
Well, the use case for DTLS is encrypted communication between the server and the clients (like in WebRTC or WebSocket Secure). If you want man-in-the-middle resistance your ICE/STUN infrastructure would need to provide the public certificate to the clients along with the server IP address/port. |
TestSubject06
force-pushed
the
reciprocal-conns
branch
from
b8575c9
to
6fca27c
Compare
Am I supposed to run something to generate the XML, or am I supposed to be hand writing it? Because I'm hand writing it. |
|
Thank you. I knew I had read it somewhere but was losing my mind thinking I imagined it because I couldn't find it again. |
There was a problem hiding this comment.
This is looking great 👍 .
I requested a small change on the C++ type for the packet, and added a nitpick on argument names, then I think it's good to go after a rebase (see https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase )
modules/enet/enet_connection.cpp
Outdated
| @@ -342,6 +342,39 @@ void ENetConnection::_broadcast(int p_channel, PackedByteArray p_packet, int p_f | |||
| broadcast(p_channel, pkt); | |||
| } | |||
|
|
|||
| void ENetConnection::socket_send(const String &p_address, int p_port, PackedByteArray p_packet) { | |||
There was a problem hiding this comment.
| void ENetConnection::socket_send(const String &p_address, int p_port, PackedByteArray p_packet) { | |
| void ENetConnection::socket_send(const String &p_address, int p_port, const PackedByteArray &p_packet) { |
|
I'll get it done this evening, and get it rebased as well. Thanks Faless! |
TestSubject06
force-pushed
the
reciprocal-conns
branch
2 times, most recently
from
cac5a20
to
578593f
Compare
* Sends a given packet toward a given destination address and port, using the current ENetHost's socket.
TestSubject06
force-pushed
the
reciprocal-conns
branch
from
578593f
to
9e2f48c
Compare
|
Thanks! |
|
🎉 |
YuriSizov
changed the title
ENetConnection to send a packet to an arbitrary destination
Problem
Godot's ENet implementation currently requires server hosts to be publicly addressable on the internet, and also requires all ports be forwarded if the host is behind a NAT. Godot implements some UPnP features, but many computers are behind multiple NAT devices - especially those with ISPs in rural areas that do not have enough IPv4 addresses to serve their customers.
This means that some users cannot possibly have an "Open" NAT resolution, meaning they can never host a game under the current ENet solution. We want users to be able to host while they are either behind an "Open" or "Limited" NAT firewall.
In order for two computers behind NATs to communicate with each other, they must both send a packet toward each other at the same time. The tolerance here is pretty high, around 5 minutes on average. However, it's not enough for Machine A to send a packet to Machine B - the packet must be sent from an anchored port. If Machine A sends a packet to Machine B from port 19020, Machine B will receive return information and be able to communicate with Machine A on the received port - but will always map back to port 19020 on Machine A.
If Machine A is hosting an ENetMultilplayerPeer server on port 19020, then in order to enable a reciprocal P2P connection between A and prospective Client B, then A must send a packet toward client B from port 19020.
Solution
Implement a simple method that takes in a target destination and port and sends a dummy packet to that address. The content of the packet is completely arbitrary and not meant to be consumed by anything. Its purpose is only to establish NAT routing table entries on the way out toward the client, so that messages from the client can be received by the running host instance. Once the NAT routing table entries are set, any future client connection requests from B, within the tolerance window, will be able to reach A as if it were a stock-standard request/response a la any other service on the internet.
Limitations
This doesn't solve the issue by itself, and will never be able to. It still requires the assistance of a third party service to handle identification of public-facing IP and communication ports, as well as a service to hand off this information to the host so that it can prepare the NAT. The first part is solved by free and public STUN servers. The second part requires developers to create or use a shared service that can store and retrieve these pieces of data.
The second limitation is that a client behind a Symmetric NAT cannot have its IP:port information known before a request to a host is made, by design. A client behind a Symmetric NAT cannot ever connect to a host behind a "Limited" NAT. And a server hosted behind a Symmetric NAT cannot ever have any clients connect to it.
Definitions
"Open" NAT - A machine with an "Open" NAT is publicly addressable on the internet. They have a clear path, ports are forwarded and/or the host is DMZd. A reciprocal connection isn't required.
"Limited" NAT - A machine with a "Limited" NAT is not publicly addressable, but is behind a NAT that has a funnel-type NAT allocation strategy, meaning its communication ip and ports are stable when reaching out to different services. Some NAT devices in this category still require that the machine has sent a request to a particular destination before accepting packets from that as a source. So although A -> B and A -> C have the same address and port, if A has not sent a request to C, then it cannot receive a packet from C.
"Closed" NAT / Symmetric NAT - A machine with a "Closed" or Symmetric NAT is like attempting to mark a sand dune. Every time a request is made to a new destination the request comes from a different IP:port combination. Cell phone internet is a pretty common example of a Symmetric NAT.