IPv6 support for ENet networking module

[akien-mga]

Now that we have IPv6 support in the engine, we need to extend the ENet module to support it too in the NetworkedMultiplayerEnet API.

AFAIK upstream ENet has no IPv6 support, and there is still no sign upstream of willing to add it. There are two pending PRs adding it (not reviewed):

• https://github.com/lsalzman/enet/pull/21 (needs rebased)
• https://github.com/lsalzman/enet/pull/51 (merges cleanly, recently rebased)

As IPv6 support is a growing concern and ENet is quite popular, I imagine that there might be many forks out there that add the feature. We should try to find one which is well maintained, and use it as a replacement to our upstream ENet - hacking it ourselves would be a waste of time (unless there are really no good forks implementing it).

In this issue, we should thus list all forks/branches we can find and try to assess them. Then we could try to get in touch with those developers and see if they'd be up for making their repo an alternative upstream that can be used by Godot (and maybe eventually packaged in Linux distros as new enet version if the lsalzman/enet repo continues ignoring PRs).

[akien-mga]

Forks / IPv6 branches I found so far:

• https://github.com/freeminer/enet/tree/ipv6 (used in the Freeminer game, recent activity).
• https://github.com/Ericson2314/enet (needs rebasing, inactive)
• https://github.com/cgutman/enet (recent activity)
• https://github.com/thewolfwillcome/enet (uses PR21 mentioned above + some more commits)

That's it for the GH forks AFAICT.

Edit: Also this one which is not registered as "GitHub fork", and quite old:

• https://github.com/csm/enet

[akien-mga]

The Dolphin emulator had plans to use the freeminer fork, but as far as I can tell it hasn't happened yet.

At any rate, from the above list, I think the most relevant to review are @proller's @freeminer fork, and @cgutman's.

[Faless]

I hope I'm not hijacking this issue. I've successfully merged lsalzman/enet PR 51 into godot enet version and updated the enet module to the new IPv6 API. It seems to work so far. It should support both ipv4 and ipv6 (since the original patch uses dual stack sockets). @vnen , @akien-mga , @punto- can you have a look? https://github.com/Faless/godot/tree/enet_ipv6

[akien-mga]

@Faless I'd prefer we don't modify thirdparty/enet further than syncing with https://github.com/freeminer/enet/tree/ipv6, so if it needs more work, it would be nice to make a PR to the freeminer repo.

I don't want us (yet) to become maintainers of yet another enet fork, and I would very much prefer that everything we need to modify is also proposed upstream via PR 51, so that we can tell distro maintainers that if they want to build against system enet, they need to cherry-pick this PR.

[Faless]

@akien-mga , I see your point, and I agree. I don't want to patch enet either. I guess we could just use the freeminer fork. I've synced the source and added a single commit (mostly defines) to get the godot build system to work properly. We could try to get the small UWP/WINRT fix into the freeminer branch (but not having windows I can't really try UWP builds). Also, defines could be added to scons somehow, but I don't know how that works either :8ball: .

[akien-mga]

Also, defines could be added to scons somehow, but I don't know how that works either :8ball: .

env_enet.Append(CFLAGS=["-DYOUR_DEFINE"])

[vnen]

BTW, I got the UWP patch from here: https://github.com/ahrnbom/enet-uwp

[akien-mga]

If @proller is up to it, we could probably ask him to merge his own ipv6 branch into his master branch (or into a new "development" branch or similar) where we could make new PRs without polluting the pending ipv6 PR to the upstream repo.

[akien-mga]

Alternatively, we could have a godotengine/enet repo with all the further developments we need in one public place, if the freeminer guys don't wish to become an upstream for another project.

[punto-]

How hard would it be to patch ENet to be able to provide our own socket connections? That way we eliminate the duplicate code (and the bsd and winsock network code from ENet), and it'll work with any OS that we support with our own sockets (including websockets for example)

On 2 November 2016 at 18:55, Rémi Verschelde notifications@github.com wrote:

Alternatively, we could have a godotengine/enet repo with all the further developments we need in one public place, if the freeminer guys don't wish to become an upstream for another project.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6992#issuecomment-258011500, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVmPRSRKzST2qXRUDBCQETTJ_s9pHVXks5q6QbpgaJpZM4Kk3BW .

[reduz]

There is a file that needs to be reimplemented, did not seem that hard but its work

On Nov 2, 2016 21:33, "punto-" notifications@github.com wrote:

How hard would it be to patch ENet to be able to provide our own socket connections? That way we eliminate the duplicate code (and the bsd and winsock network code from ENet), and it'll work with any OS that we support with our own sockets (including websockets for example)

On 2 November 2016 at 18:55, Rémi Verschelde notifications@github.com wrote:

Alternatively, we could have a godotengine/enet repo with all the further developments we need in one public place, if the freeminer guys don't wish to become an upstream for another project.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/godotengine/godot/issues/6992#issuecomment-258011500 , or mute the thread https://github.com/notifications/unsubscribe-auth/ AGVmPRSRKzST2qXRUDBCQETTJ_s9pHVXks5q6QbpgaJpZM4Kk3BW .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6992#issuecomment-258040266, or mute the thread https://github.com/notifications/unsubscribe-auth/AF-Z2yquAfb2wf1V-d1wRcbSHRGJ3f9Pks5q6SvigaJpZM4Kk3BW .

[Faless]

Well, it will require us to maintain our own patched version of ENet which I think is what we are trying to avoid.

[Faless]

it'll work with any OS that we support with our own sockets (including websockets for example)

ENet over websocket does not make much sense. ENet is build to provide reliability over an unreliable transport protocol (eg. UDP). Websockets uses TCP which is already reliable. It might be useful in WebRTC (which uses UDP), but for what I see the WebRTC protocol already provide both reliable/unreliable modes for data channels ( https://developer.mozilla.org/en-US/docs/Games/Techniques/WebRTC_data_channels ).

[akien-mga]

@Faless How should we proceed in the end? Do you think you could get your changes upstreamed in the Freeminer fork? I see you've used some Godot-specific macros, so I guess you'd have to find universal equivalents to identify UWP, among other things.

[Faless]

@Faless How should we proceed in the end? Do you think you could get your changes upstreamed in the Freeminer fork?

TL;DR;

1. UWP patch makes ping calculation and timeout slightly off due to difference in precision (no clock time adjustment). But it shouldn't be a problem.
2. UWP macros are more complex then they should, It would be nice if someone with a Windows Phone could try them.
3. OpenBSD will not work with ENet-IPv6 freeminer fork.

Longer description

I've been digging a bit, sadly though I don't have a windows phone to actually try it.

That said, the code makes sense, and it should definitely work (although latency and timeout calculations might be off by few milliseconds due to the difference in precision, see https://msdn.microsoft.com/en-us/library/windows/desktop/ms724411(v=vs.85).aspx and https://blogs.msdn.microsoft.com/larryosterman/2009/09/02/whats-the-difference-between-gettickcount-and-timegettime/ ).

That said, I could find these macro, but Microsoft docs sucks and maybe @vnen which is more experienced than me when it comes to UWP can help me understand if I'm correct ( see https://blogs.msdn.microsoft.com/chuckw/2012/09/17/dual-use-coding-techniques-for-games-part-1/ ): I think the check should be:

#if WINAPI_FAMILY==WINAPI_FAMILY_PHONE_APP || WINAPI_FAMILY==WINAPI_FAMILY_PC_APP || WINAPI_FAMILY==WINAPI_FAMILY_APP
  // UWP, should I add more stuff for XBox?!
#else
  // Windows
#endif

Easy right?! I will probably also need a series of #ifdef WINAPI_FAMILY_PHONE_APP #ifdef WINAPI_FAMILY_PC_APP and WINAPI_FAMILY_APP I suppose, or it won't compile on older system (i.e. XP/Vista/7).

One last thing. The Freeminer fork uses IPv6 dual stacking. So in OpenBSD it will only work if the machine is connected to an IPv6 network since OpenBSD developers deliberately decided not to comply with RFC 3493 .

Recap and Options

So in conclusion, as a recap:

1. Macros: I'd love someone with a windows phone / better windows experience to test/confirm them (my windows machine is utterly slow and compiling there is quite nightmarish)
2. The patch for UWP is really small (13 lines, so I'm pretty sure we could maintain it downstream)
3. OpenBSD will be out of the game if we update to IPv6. The only solution here would be to do a totally different fork that can use both IPv4 and IPv6 sockets (possibly using godot sockets at that point, since the big amount of work required).

It's probably not what @akien-mga wants to hear, but for what I see this is the situation.

P.S.: I would go for 2 and just tell OpenBSD users that their system is broken, and they should complain with the devs. But that's just me.

[punto-]

Enet using godot sockets would be great, it means a lot less network code to maintain. Also, it wouldn't be a big deal to lock a udp socket to a particular protocol after the first use, you can open a second one for the other protocol

On 11 January 2017 at 13:58, Fabio Alessandrelli notifications@github.com wrote:

@Faless https://github.com/Faless How should we proceed in the end? Do you think you could get your changes upstreamed in the Freeminer fork?

TL;DR;

1. UWP patch makes ping calculation and timeout slightly off due to difference in precision (no clock time adjustment). But it shouldn't be a problem.
2. UWP macros are more complex then they should, It would be nice if someone with a Windows Phone could try them.
3. OpenBSD will not work with ENet-IPv6 freeminer fork.

Longer description

I've been digging a bit, sadly though I don't have a windows phone to actually try it.

That said, the code makes sense, and it should definitely work (although latency and timeout calculations might be off by few milliseconds due to the difference in precision, see https://msdn.microsoft.com/en- us/library/windows/desktop/ms724411(v=vs.85).aspx and https://blogs.msdn.microsoft.com/larryosterman/2009/09/02/ whats-the-difference-between-gettickcount-and-timegettime/ ).

That said, I could find these macro, but Microsoft docs sucks and maybe @vnen https://github.com/vnen which is more experienced than me when it comes to UWP can help me understand if I'm correct ( see https://blogs.msdn.microsoft.com/chuckw/2012/09/17/dual- use-coding-techniques-for-games-part-1/ ): I think the check should be:

if WINAPI_FAMILY==WINAPI_FAMILY_PHONE_APP || WINAPI_FAMILY==WINAPI_FAMILY_PC_APP || WINAPI_FAMILY==WINAPI_FAMILY_APP

// UWP, should I add more stuff for XBox?!

else

// Windows

endif

Easy right?! I will probably also need a series of #ifdef WINAPI_FAMILY_PHONE_APP #ifdef WINAPI_FAMILY_PC_APP and WINAPI_FAMILY_APP I suppose, or it won't compile on older system (i.e. XP/Vista/7).

One last thing. The Freeminer fork uses IPv6 dual stacking. So in OpenBSD it will only work if the machine is connected to an IPv6 network since OpenBSD developers deliberately decided not to comply with RFC 3493 https://tools.ietf.org/html/rfc3493 . Recap and Options

So in conclusion, as a recap:

1. Macros: I'd love someone with a windows phone / better windows experience to test/confirm them (my windows machine is utterly slow and compiling there is quite nightmarish)
2. The patch for UWP is really small (13 line, so I'm pretty sure we could maintain it downstream)
3. OpenBSD will be out of the game if we update to IPv6. The only solution here would be to do a totally different fork that can use both IPv4 and IPv6 sockets (possibly using godot sockets at that point, since the big amount of work required).

It's probably not what @akien-mga https://github.com/akien-mga wants to hear, but for what I see this is the situation.

P.S.: I would go for 2 and just tell OpenBSD users that their system is broken, and they should complain with the devs. But that's just me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6992#issuecomment-271925750, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVmPRdtG6XqoO4XnMx0KPn9FbNps0vOks5rRQpKgaJpZM4Kk3BW .

[ghost]

So in OpenBSD it will only work if the machine is connected to an IPv6 network since OpenBSD developers deliberately decided not to comply with RFC 3493 .

That's interesting, but as I guessed it comes down to security concerns. I found some reading about it here. It doesn't sound like they'll ever implement it.

[Faless]

it comes down to security concerns

Yes, there is a lot of debate about it. I might not have understood the full extent of the security issues reported, so please correct me if I'm wrong.

For what I understood. Considering a security issue the fact that the user might not have an IPv6 firewall configured is a bit far fetched in my opinion. At the same time, saying that it's a security issue because a broken implementation might consider a network-received packet from a mapped address as a local IPv4 is utterly wrong, because the OS must not pass those addresses to the application (same as if it receives from eth a packet from 127.0.0.1). So that would mean the OS implementation is broken. Also, regarding IP based ACLs, I don't see how IP spoofing would be different between supporting IPv4 mapped addresses over IPv6 and using plain old IPv4 addresses.

I haven't read about any other "security concern" beside those two. All discussions on this matter seems to be really vague, and somehow driven by personal beliefs. Also the fact that any other OS implemented it should mean something IMHO.

That said, agreed, OpenBSD is not going to implement it ever. For this reason, it will be impossible to listen to wildcard addresses on both stacks (unless you use two different sockets, but that would bring back at least the first "security issue"). I'm currently implementing socket binding on specific addresses for UDP/TCP, my proposal is to default to IPv4 on OpenBSD for wildcard addresses (i.e. 0.0.0.0) and use the appropriate socket when provided with a specific bind address (eg. ::1 or 127.0.0.1). This also prevents us from using freeminer Enet fork since it will not work on OpenBSD leaving the only feasible option of patching ENet to use Godot sockets if we want to support OpenBSD. I'll probably be working on that when I finish the socket binding patch.

[akien-mga]

This also prevents us from using freeminer Enet fork since it will not work on OpenBSD leaving the only feasible option of patching ENet to use Godot sockets if we want to support OpenBSD.

Or we disable the enet module on OpenBSD :)

[proller]

why enet? enet almost dead.

Maybe try use https://github.com/sctplab/usrsctp ? it have much more features than enet

[punto-]

I'm for it if it's portable (ie allows us to provide our own sockets)

On 17 January 2017 at 16:21, proller notifications@github.com wrote:

why enet? enet almost dead.

Maybe try use https://github.com/sctplab/usrsctp ? it have much more features than enet

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6992#issuecomment-273270700, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVmPfiPqkWRHQwcKoMGLL1FRMOgYGhqks5rTRTWgaJpZM4Kk3BW .

[proller]

like this? https://github.com/sctplab/usrsctp/blob/master/programs/ekr_loop_offload.c#L413

[punto-]

No, that's horrible, it's full of #ifdefs :p I was thinking something like a struct with pointers to functions like socket_open, socket_send, socket_set_mode, etc, with a default implementation for unix, windows, etc. Then we provide one that uses our network classes.. Similar to how many libraries do with filesystem access and memory allocation.

On 17 January 2017 at 17:55, proller notifications@github.com wrote:

like this? https://github.com/sctplab/usrsctp/blob/master/programs/ ekr_loop_offload.c#L413

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6992#issuecomment-273296689, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVmPYev0cl_-9eYTaKt2nZTe5w-BnDkks5rTSrVgaJpZM4Kk3BW .

[Faless]

@punto- I made a small patch to ENet to use godot socket. The patch itself is not big (just adds our implementation source and header and change a few lines) and we could also remove unix.c and win32.c (anyway the UWP patch won't be needed anymore, so keeping in sync in case won't be hard).

There is a testable 2.2 version here (eg. with Bomber Multiplayer from demo). The tree also contains cherry pick of 7581, the full diff for ENet is this (https://github.com/Faless/godot/compare/a39bef90fa811a5588710f8aa822e7e85781d39e...Faless:enet_2.2_godot). We also keep compatibility with builtin enet library as per @akien-mga request (of course falling back to IPv4 in that case).

In case anyone wants to take a look while I wait for 7581 :wink: .

[punto-]

That's amazing! is it using the sockets directly in enet, or is there like a layer where you provide your socket implementation to enet (which happens to be godot sockets in this case)?

On 23 January 2017 at 07:22, Fabio Alessandrelli notifications@github.com wrote:

@punto- https://github.com/punto- I made a small patch to ENet to use godot socket. The patch itself is not big (just adds our implementation source and header and change a few lines) and we could also remove unix.c and win32.c (anyway the UWP patch won't be needed anymore, so keeping in sync in case won't be hard).

There is a testable 2.2 version here http:///faless/godot/tree/enet_2.2_godot (eg. with Bomber Multiplayer from demo). The tree also contains cherry pick of 7581, the full diff for ENet is this (Faless@a39bef9...Faless:enet_2.2_godot https://github.com/Faless/godot/compare/a39bef90fa811a5588710f8aa822e7e85781d39e...Faless:enet_2.2_godot ). We also keep compatibility with builtin enet library as per @akien-mga https://github.com/akien-mga request (of course falling back to IPv4 in that case).

In case anyone wants to take a look while I wait for 7581 😉 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6992#issuecomment-274449875, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVmPRajKYtbZ45pPz7ruLMA-uDZAg6nks5rVH-DgaJpZM4Kk3BW .

[Faless]

is it using the sockets directly in enet, or is there like a layer where you provide your socket implementation to enet (which happens to be godot sockets in this case)?

Well, apparently ENet was already designed to easily provide your own socket implementation instead of the default one.

The library internally calls functions like enet_socket_create and enet_socket_send, so instead of including unix.h or win32.h you can include your own myimpl.h and myimpl.c[pp] where you typdef ENetAddress and implement your own enet_socket_send. enet_socket_create, etc... See this file.

The only few modification I had to do to the library itself (~10 lines of code) was to allow setting and comparing IPv6 address.

[punto-]

Oh that's amazing.. great job!

On 23 January 2017 at 15:28, Fabio Alessandrelli notifications@github.com wrote:

is it using the sockets directly in enet, or is there like a layer where you provide your socket implementation to enet (which happens to be godot sockets in this case)?

Well, apparently ENet was already designed to easily provide your own socket implementation instead of the default one.

The library internally calls functions like enet_socket_create and enet_socket_send, so instead of including unix.h or win32.h you can include your own myimpl.h and myimpl.c[pp] where you typdef ENetAddress and implement your own enet_socket_send. enet_socket_create, etc... See this file https://github.com/Faless/godot/blob/ee67fc6e1fc095fdd250e09ed01e536e6f0a1ba9/thirdparty/enet/godot.cpp#L81 .

The only few modification I had to do to the library itself (~10 lines of code) was to allow setting and comparing IPv6 address.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/6992#issuecomment-274574356, or mute the thread https://github.com/notifications/unsubscribe-auth/AGVmPc24q6mSR64tXLg3mz5boULIHasmks5rVPFTgaJpZM4Kk3BW .

[Faless]

Oh that's amazing.. great job!

Thanks! :smile: