Page MenuHomeWildfire Games

Embed zpl-c's version of enet library
Needs ReviewPublic

Authored by Lefo on Nov 18 2018, 6:43 PM.

Details

Reviewers
Imarok
vladislavbelov
wraitii
Trac Tickets
#4301
Summary

The main purpose of this patch is to add initial support for IPv6.
With this patch you can host on both IPv4 and IPv6 simultaneously as well as
joining games via both IPv4 and IPv6 protocols.
Since this patch you need your system to support AF_INET6. Also to be able to
use IPv4 you need your system to support IPv4-mapped address feature even when
using IPv4 only.

New option --with-system-enet was added when executing update-workspaces script
so you can switch back to your system's enet library which should allow you to
compile even with no IPv6 support in your system. This will disable any IPv6
support if your system's enet library does not support IPv6.

Lobby still lacks the necessary IPv6 support so for now joining games via lobby
is always IPv4 only.

See https://github.com/zpl-c/enet for more informations about zpl-c's version
of enet library.

v1->v2

  • addressed vladislavbelov's and elexis's requests for syntax adjustments
  • updated premake4 so it should at least compile with system's enet
  • fixed bugs causing crash when peer gets disconnected due to timeout

v2->v3

  • dropped conflicting premake4 support
  • addressed Stan's requests for syntax adjustments
  • fixed some bugs
Test Plan

Tested playing games over IPv6 and also playing over IPv4 with both game compiled with system's enet and the new default embedded enet.
Tested hosting as well as joining other games.
Tested backward compatibility with players without this patch.
Tested banning feature.
Tested hosting and joining games via lobby with both stun enabled and disabled.
So far everything seemed to work as expected.

I did not test the game on any system other than Debian Stretch. But I'm not aware of anything that could break the game on other system.

Unfortunetely I can't test playing via lobby with latest git version since it's now A24 which is incompatible with currently played game.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9375
Build 15562: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Lefo added inline comments.Nov 21 2018, 12:18 AM
lobby/XmppClient.cpp
1210 ↗(On Diff #6992)

static_cast didn't work:
error: invalid static_cast from type ‘sockaddr_in*’ to type ‘sockaddr*’
So I used reinterpret_cast.

network/StunClient.cpp
240 ↗(On Diff #6992)

What you mean by old version of initialization?
I agree it does not look good.
How about this?
const u16 v4_mapped_prefix[] = {0, 0, 0, 0, 0, 0xFFFF};
bool sender_is_ipv4 = !memcmp(addr.sin6_addr.s6_addr, v4_mapped_prefix, sizeof(v4_mapped_prefix));
It's much longer but seems to be more clear.

436 ↗(On Diff #6992)

Condition of if depends on status so I can't do that.
I could declare status inside if condition and drop the part "!= 0" if that's what you want.
But that would make code inconsistent with getaddrinfo usage above. Unless I adjust it there too.

premake/premake5.lua
184 ↗(On Diff #6992)

You mean replacing WITH_SYSTEM_ENET with something like CONFIG2_SYSTEM_ENET?
But these defines are related to source/lib/config2.h aren't they?
I guess that would require to modify config2.h to default CONFIG2_SYSTEM_ENET to 0 and defines { "WITH_SYSTEM_ENET" } would become defines { "CONFIG2_SYSTEM_ENET=1" }.
enet.h would then need to be adjusted as well to use #if instead of #ifdef.
Is that what you want?

Do I understand it correctly, that IPv4 won't work when IPv6 is enabled?

lobby/XmppClient.cpp
1210 ↗(On Diff #6992)

So, reinterpret_cast then :)

network/StunClient.cpp
240 ↗(On Diff #6992)

Yep, something like that.

436 ↗(On Diff #6992)

I meant, that if (status != 0) equals to if (status), so it could be written so:

if (int status = getaddrinfo(serverAddress, nullptr, &hints, &res))
{
    // ...
}
premake/premake5.lua
184 ↗(On Diff #6992)

I was talking about CONFIG2_SYSTEM_ENET, yes. But I don't mind how it's defined, with =0 or without.

Lefo marked 12 inline comments as done.Nov 22 2018, 8:18 AM
Lefo added inline comments.
network/StunClient.cpp
436 ↗(On Diff #6992)

Ok, I also updated previously untouched code above to make it consistent with this.

Lefo marked an inline comment as done.Nov 22 2018, 8:29 AM

Do I understand it correctly, that IPv4 won't work when IPv6 is enabled?

No, it seems you don't. Read the summary.

Lefo updated this revision to Diff 6996.Nov 22 2018, 8:58 AM
Lefo edited the summary of this revision. (Show Details)
Lefo edited the test plan for this revision. (Show Details)
Stan added a subscriber: Stan.

Could you guys have a look at this. It's starting to be a really wanted feature.

Stan added inline comments.Jan 21 2019, 6:41 PM
source/network/StunClient.cpp
174

Not sure what the coding conventions say about this. https://trac.wildfiregames.com/wiki/Coding_Conventions

254

static_cast

410

Could be a function maybe to avoid duplication.

Lefo added inline comments.Jan 21 2019, 8:45 PM
source/network/StunClient.cpp
174

I also preferred the old version. But I changed it to this form on vladislavbelov's request.
See this: https://code.wildfiregames.com/D1676?id=6992#inline-31309
I can't really satisfy both of your opinions at the same time, so either you guys should agree on what is right or tell me which one of you two has higher priority.

254

would make it inconsistent with current IPv4 code, wanna change that too?

410

Ok, use of getaddrinfo could be converted to function as well.

For me: fix ntohl -> htonl.

Stan added inline comments.Jan 21 2019, 8:53 PM
source/network/StunClient.cpp
174

@vladislavbelov saying so is enough for me :) Was just wondering because I didn't even know that could be done. Wonder what the scope of that status is in this case.

254

Yeah in that case. It's better to know which type of cast we use :)

410

Not sure what ntohl is so can't really tell :)

Stan added a comment.Jan 21 2019, 8:53 PM

You'll have to bump the file years too btw we are in 2019. Sorry about that.

vladislavbelov added a comment.EditedJan 21 2019, 9:02 PM
In D1676#70736, @Stan wrote:

Could you guys have a look at this. It's starting to be a really wanted feature.

As I said before:

It'd be good to attach a diff between the added version and a usual system version of enet.

Because we don't see what was changed.

Also I see that the fork isn't used for any long live opensource project. It's the low level library, so the supportability is really important for us. Does it work for all our supported platforms/compilers/hardwares?

Also I pinged @fcxSanya, because he had some experience with that.

source/network/StunClient.cpp
174

Why I vote this. Because you don't need to share the variable with all other code, only with the space where it's required.

If there would be another similar block, you would need to call all these variables by some meaning names.

Lefo marked 5 inline comments as done.Jan 21 2019, 9:46 PM
Lefo added inline comments.
source/network/StunClient.cpp
174

@Stan the scope is the consequent of if statement (code inside {}). It is similar to for cycle in which you can also define variable in initialization part.

@vladislavbelov I prefer creating another {} block for limiting scope of variables. This seems ugly because it is not C compatible and it does not work in any other comparison case (such as equal instead of not equal or comparing to any other number different from zero).
And it also can lead to situations where one thinks he can modify the comparison such as

if (int status = getaddrinfo(...) == 0)

which doesn't work because status variable gets assigned 0 or 1 depending on result of comparison and the original result of getaddrinfo gets dropped.

Lefo marked an inline comment as done.Jan 21 2019, 9:58 PM

It'd be good to attach a diff between the added version and a usual system version of enet.

You can run diff between zpl-c's and lsalzman's versions of enet on github. But I'm sure you will find that it makes no sense to do it.

Also I see that the fork isn't used for any long live opensource project. It's the low level library, so the supportability is really important for us.

Both lsalzman's and zpl-c's versions of enet does not appear to be maintained anymore.

Does it work for all our supported platforms/compilers/hardwares?

I think it should run on all systems capable of running 0ad as well as on many other systems incapable of running 0ad. But I did not test anything except debian.

source/network/StunClient.cpp
174

I prefer creating another {} block for limiting scope of variables.

I can say that it's ugly about useless {}. But it's just tastes I think. I just prefer the if variable while it's compilable.

it is not C compatible.

Then you shouldn't use reinterpret_cast, STL and other C++ stuff here.

it does not work in any other comparison case (such as equal instead of not equal or comparing to any other number different from zero).

You're right until C++17 :)

And it also can lead to situations where one thinks he can modify the comparison such as

Yes, I agree. But it's a more complex problem, that people don't know the language. It should be solved on the review stage.

elexis added a subscriber: elexis.Jan 22 2019, 12:05 PM

The program should inform the lobby player or IP-joiner if he can't join because of an IPv4 vs IPv6 issue and advise him what he would have to do in order to solve this problem.
(We also miss an explanation why players can't join the STUN game if their ISP prohibits that traffic. We find these questions being asked on a daily basis in the lobby and noone is willing to believe that it's not a configuration issue, or bug with pyrogenesis.)
Can be done in a separate revision proposal afterwards if it's more than one or two JS hunks.

Lefo added a comment.Jan 22 2019, 1:22 PM
In D1676#70760, @elexis wrote:

The program should inform the lobby player or IP-joiner if he can't join because of an IPv4 vs IPv6 issue and advise him what he would have to do in order to solve this problem.

This patch does not support lobby at all. We need to support lobby first. Before we start with lobby support we should first apply this and let people test if there are no yet unrevealed issues such as bugs that only affects specific systems. AFAIK now I'm the only one who tested this.

(We also miss an explanation why players can't join the STUN game if their ISP prohibits that traffic. We find these questions being asked on a daily basis in the lobby and noone is willing to believe that it's not a configuration issue, or bug with pyrogenesis.)
Can be done in a separate revision proposal afterwards if it's more than one or two JS hunks.

Yeah I think this is completely different issue and should be solved elsewhere. It would be best to have lobby server attempt somehow to connect to host and check whether it's possible. If not we should inform hosting player that noone can really join his game so it's useless to wait for players.

We probably need to setup a team moment to test this easily, as it won't work with the lobby obviously.

Lefo added a comment.Jan 22 2019, 3:07 PM

We probably need to setup a team moment to test this easily, as it won't work with the lobby obviously.

Not sure if that's necessary. Just testing whether IPv4 still works for everyone should be enough for now.

lobby

The gamelist bot would already provide IPv6 addresses if we hadn't configured it to IPv4 (see lobby README.md).
Just keep it in mind that users should be informed why they can't connect.

It would be best to have lobby server attempt somehow to connect to host and check whether it's possible. If not we should inform hosting player that noone can really join his game so it's useless to wait for players.

STUN is by policy disabled by some ISPs, so some clients will be able to join, others won't. So such a join-test would only be informative for non-STUN hosts.
Point was only about a better string in the already existing messagebox. Previously there were 2 reasons why people couldn't join (host didn't forward, STUN), now there would be 3 with IPv4/6.

Lefo added a comment.Jan 22 2019, 3:37 PM
In D1676#70771, @elexis wrote:

lobby

The gamelist bot would already provide IPv6 addresses if we hadn't configured it to IPv4 (see lobby README.md).
Just keep it in mind that users should be informed why they can't connect.

As we already discussed before, this is more complicated. We need to know both IPv4 and IPv6 addresses of host and we need to provide them both to joining users.

It would be best to have lobby server attempt somehow to connect to host and check whether it's possible. If not we should inform hosting player that noone can really join his game so it's useless to wait for players.

STUN is by policy disabled by some ISPs, so some clients will be able to join, others won't. So such a join-test would only be informative for non-STUN hosts.

Server should connect via STUN just as any other joining player would if host has STUN enabled.

Point was only about a better string in the already existing messagebox. Previously there were 2 reasons why people couldn't join (host didn't forward, STUN), now there would be 3 with IPv4/6.

Yeah we should probably somehow detect what could the problem be to better inform user what could be wrong rather than just showing all errors at once and let user choose what could it be. But this we should solve in some future patch which will provide IPv6 support for lobby.

Server should connect via STUN just as any other joining player would if host has STUN enabled.

(As far as I know, the missing STUN connectivity for some clients is a clientside problem, not a serverside one (we can observe something like 90% of the people can join STUN games and something like 10% can't); what information does it yield to test with one client?
I suppose it could detect a serverside firewall issue, but the problem is to explain to the user why all the other players could join but he individually could not. Whether a host uses STUN or not, whether it's an IPv4 or IPv6, whether anyone else could join before should be known already at the point of displaying it in the lobby gamelist at least.)

Lefo added a comment.Jan 22 2019, 5:37 PM
In D1676#70774, @elexis wrote:

Server should connect via STUN just as any other joining player would if host has STUN enabled.

(As far as I know, the missing STUN connectivity for some clients is a clientside problem, not a serverside one (we can observe something like 90% of the people can join STUN games and something like 10% can't); what information does it yield to test with one client?
I suppose it could detect a serverside firewall issue, but the problem is to explain to the user why all the other players could join but he individually could not. Whether a host uses STUN or not, whether it's an IPv4 or IPv6, whether anyone else could join before should be known already at the point of displaying it in the lobby gamelist at least.)

Seems you don't understand the point. Some users can't host on IPv4 even with STUN enabled because they have some NAT or firewall on the way which just won't allow it. That's why it might be a good idea to have some server which would attempt to connect to client and check whether it works (either directly or via STUN depending on host settings). When it does not work host could be informed that he just can't host so that he knows it's his problem which he should fix or give up on hosting and it's not that just nobody wants to join his game.

In D1676#70775, @Lefo wrote:

Seems you don't understand the point. Some users can't host

The point I was making was not about the problem where someone hosts a game that noone can join, but the problem where 9/10 users can join but the 10th cannot.
That issue is, from the number of complaints on the lobby, much more frequent, and it's in particular more frustrating to that player;
(because the player who can't host at all will give up and find another host, while the 10th client now is unable to participate in the game where all his buddies are in).
A testclient can't yield any information about this situation, but only inform the users who can't host that they can't host (which they usually find out quickly, that I'm not sure that it would be worth writing such a testclient as opposed to just displaying a short sentence hint in the dialog box, but if someone wants to implement it, maybe).

some server which would attempt to connect to client

<->

Lefo added a comment.Jan 22 2019, 8:23 PM
In D1676#70776, @elexis wrote:

the problem where 9/10 users can join but the 10th cannot.
That issue is, from the number of complaints on the lobby, much more frequent, and it's in particular more frustrating to that player;
(because the player who can't host at all will give up and find another host, while the 10th client now is unable to participate in the game where all his buddies are in).

Perhaps the best approach would be to classify hosts and clients. If you host only with stun you get S. If you host via IPv4 directly, you get 4. If you host via IPv6 you get 6. This would get combined, e.g. when you host via IPv4 STUN and also have IPv6, you get S6, if you host IPv4 without STUN and also have working IPv6 you get 46. Then you also classify clients. If you can connect via IPv4 you get 4, if you can also connect via STUN you get S, so you can also have S4, if you can connect via IPv6 you get 6. So possible client's combinations are 4, 6, S4, S46 (S and S6 seems meaningless for client). These codes would be displayed next to every hosted game in lobby. And everyone should also see his own code somewhere. Then the semantics would be simple: If you as a client have at least one character shared with the host you should be able to join. This should make it more clear for users to understand who can they join and who they can't and why.
Then we can make some FAQ why user don't have 4 or 6 or something.
Perhaps it would be better to use some letters instead of 4 and 6 to make it less misinterpretable.

Lefo marked 8 inline comments as done.Jan 23 2019, 8:19 PM
Lefo added inline comments.
source/network/StunClient.cpp
254

reinterpret_cast

Lefo updated this revision to Diff 7377.Jan 23 2019, 8:29 PM
Lefo marked an inline comment as done.
Lefo edited the test plan for this revision. (Show Details)
Lefo edited the summary of this revision. (Show Details)Jan 23 2019, 8:33 PM
Lefo edited the test plan for this revision. (Show Details)

Re connection compatibility UI side/OTissue:
(I think symbols like 4, 6, S might not be understood by many players.
Either these indicators give some circumstancial evidence, or they indicate clear causal relationships.
In the first case, it doesn't really inform the user.
In the second case, it could be used to to disable / grayout games that are unjoinable (like we do with games that have incompatible mods) without introducing custom symbols or abbreviations.
If there was a failed connection attempt, the messagebox that already pops up could inform the user which types of connection issues it could be to begin with (as it knows whether the host has IPv4/6, whether it uses STUN, whether someone else was able to join.).
The difference between informing before connecting and after connecting is that in the latter case, we know for sure whether the client can connect, in the former case we don't. So we can conclude a little bit more about the possible connection issues after an attempt.
It could be similar to getDisconnectReason in network.js, it'd just be an set of conditions and strings.)

source/lobby/XmppClient.cpp
1311

(IPTools.cpp?)

source/network/NetSession.cpp
160

(Disconnect reasons are informative pieces of information to the GUI. If we can distinguish two events, the GUI should be able to provide two different presentations too ideally. I guess the event.type is not carried in event.data?)

source/network/StunClient.cpp
157

I'd suggest moving these helpers to a new file source/network/IPTools.cpp or similar, as done in D1746, so that it can be used by all networking code without being located in an unrelated file.

(Also notice in the new enet version ENet 1.3.14 (January 27, 2019):, enet does use getaddrinfo and getnameinfo where available https://github.com/lsalzman/enet/blob/master/ChangeLog but that doesn't relate to the StunClient, so we still need these functions I guess)

174
if (foo)\n
{

Why does this hardcode IPv4 and not have some condition for IPv6?

450

(This freezes the mainthread for seconds. Should become async)

Lefo marked 8 inline comments as done.Jan 31 2019, 9:02 PM
In D1676#71458, @elexis wrote:

(I think symbols like 4, 6, S might not be understood by many players.

The idea is to provide users with enough information so that they can figure out not just why they can't join but also why their friend can't.
Perhaps not everyone would understand them but it would allow user to easily pick some host which not only he but also some his friend can connect.
Also it might help hosts to know which other users it makes sense to invite to their games and which they wouldn't be able to connect anyway.
We could also show some green circle or something on hosts which are compatible with everything (both direct IPv4 and IPv6). That would be more simple to understand. But a lot of information would be dropped.

If there was a failed connection attempt, the messagebox that already pops up could inform the user which types of connection issues it could be to begin with (as it knows whether the host has IPv4/6, whether it uses STUN, whether someone else was able to join.).
The difference between informing before connecting and after connecting is that in the latter case, we know for sure whether the client can connect, in the former case we don't. So we can conclude a little bit more about the possible connection issues after an attempt.

Users usually want to see whether they're gonna be able to connect before they really attempt to do so. And not to try every host and wait few seconds to get connection failure for each before they finally find some host which they can connect to.
But sure error message can be improved.

But note that this is all a bit off topic anyway since this patch does not aim to touch lobby code in any way. Maybe it could be discussed in following patch for IPv6 lobby support.

source/lobby/XmppClient.cpp
1311
source/network/NetSession.cpp
160

I'm not sure if it is in event.data, seems more like it isn't.
HandleDisconnect treats the passed argument as reason.
From looking to enet library and searching for keyword ENET_EVENT_TYPE_DISCONNECT it seems there is always 0. But there is also same case when there is peer->eventData, don't know if it's not only for client or something though. It might be easier to test rather than trying to figure out from source.
I can add some new argument to HandleDisconnect or modify the one that already is there. Just tell me what you want.

source/network/StunClient.cpp
157

That would make collision with D1746, wouldn't it? I'd wait after D1746 is done, then we can change this. We're gonna need to change some things anyway when we start implementing lobby support.
I'd rather not use any conversion functions from enet to keep code as generic and compatible as possible. Currently used getnameinfo and getaddrinfo is abstract and simple to use enough already. Would be nice to use inet_pton and inet_ntop, but well you know, there is that ugly stupid self-proclaimed operating system which can't make itself compatible with standards even after 18 years.
What surprises me more is that there is some new commit in enet. Perhaps it's not as unmaintained as I thought after all.

174

Becase this code is related to STUN. There is no STUN for IPv6. One of the reasons we want IPv6 is that there is no need for ugly stuff such as STUN.

450

Yes, I agree. But this is way out of scope of this patch.

elexis added inline comments.Feb 1 2019, 8:24 AM
source/network/NetSession.cpp
160

None of this has to be in this patch. Just pointed it out in principle that we should pass as much intel as possible to the GUI.

If we can't derive from event.data that this was a timeout, perhaps we can just pass the event itself or both event.data and event.type to HandleDisconnect. (Might also introduce merge conflicts with #3700 but that should be easy to solve as well. (Individual merge conflicts are only terrible when there are two patches that change the logic in mutually exclusive ways and one of the patches has to be redesigned rather than refactored.))

Just tell me what you want.

(We just try to uncover the truth™, my opinion doesn't matter, I'm just noise that might communicate observations).

I suppose it shouldn't become a new disconnect reason number, because then a server could send this number in the disconnect message and it would appear to the client like a timeout when in fact it could have been a ban with a modified disconnect reason. Guess that argument isn't so convincing because the host can still disconnect with other fake reasons. meh.
But by definition the "disconnect reason" is an u32 that the server sent, so it seems more correct to stick to that. (And I shouldn't have followed the request to revert rP17566 in rP17601, because the intention was to make it more clear that this function should be the direct equivalent to the disconnect reason in NetHost.h. Perhaps it would be cleaner to specify the strings in NetHost.cpp and pass them instead of the number to JS. The downside would be that string changes would require compiling the program again, wheras JS string changes are very simple).
If we're handling timeout separately, we might also add the number of seconds of the timeout as an argument (not so important either, but wouldn't hurt so much to add it).

source/network/StunClient.cpp
157

(Either after that is committed or just before this is committed in case D1746 is let rot.)
(Certainly we won't use enet functions if we don't already have an enet peer.)

What surprises me more is that there is some new commit in enet. Perhaps it's not as unmaintained as I thought after all.

Would be very good to fix this upstream. Technically we didn't try to communicate yet.
In theory it shouldn't be so hard to get a patch committed if it's really just a couple of lines to be changed for IPv4 vs IPv6, if its uploaded in a minimialistic clean way where the devs can't find anything to disagree with.

174

There is no STUN for IPv6

Is that absolutely true or is there only one of the main historic use cases for NAT dropped?
STUN is used for NAT traversal, but IPv6 doesn't necessarily kill every NAT?
https://www.quora.com/Will-the-IPv6-result-in-the-death-of-STUN-and-ICE

Also since the StunClient was imported from supertuxkart, we might want to inform them in of the update once we merged it and if we find an active maintainer of this file.
(Also we still have a probable memory leak in the glooxwrapper since the StunClient commit (rP19703).)

450

(Just trying to get skilled people interested in related bugs in order to optimize the engine and my laziness simultaneously)

Lefo marked 11 inline comments as done.Feb 1 2019, 11:22 AM
Lefo added inline comments.
source/network/NetSession.cpp
160

I don't think it's a good idea to pass event type directly. There is a mess in those constants because they are defined as enums and not as macros. I already had to do some hacks in order to make the code compatible with all versions of enet libraries.
Perhaps just adding new bool timeout argument might do the trick?

source/network/StunClient.cpp
157

The problem also is that with zpl-c's version of enet these conversion functions are no longer available for IPv4. That's the main reason I removed use of enet to convert ip addresses.

Would be very good to fix this upstream. ...

I'm sure it would but it seems to be hard:

https://github.com/lsalzman/enet/issues/78
https://github.com/lsalzman/enet/pull/21

From what I can see, people already tried with no luck.
That's why there are forks with IPv6 support in the first place.

174

I did not ever see any NAT on IPv6. And I hope I ever won't. Let's hope that people realize that it breaks way too many things and it is bad by design soon enough to not to implement it anywhere. If you send packet from IP address X and recipient receives this packet from IP address Y then there is something fundamentally wrong with the brain of the one who is responsible.

elexis added inline comments.Feb 1 2019, 5:23 PM
source/network/NetSession.cpp
160

Sure, just a matter of encoding. Could be a boolean until we have a third type (then it could become a string "timeout", "disconnect", "x") (or different GUI event types).

source/network/StunClient.cpp
157

Just because something has failed by others doesn't mean that one can't try to look at the underyling problem and try to solve it. (Like, in case we wanted to).

From what I read of lsalzman, the problem is that it's a lot of work to determine what the best ipv6 enet implementaiton would be to satisfy every stakeholder and in how far the existing patches qualify as such; including discussion in the mailing list.

So maybe fixing the issue would be to oneself review all the 4 pull requests, figure out where they fail and where they succeed, coming up with the ideal implementation that satisfies everyone, minimizes backwards compatibility problems, is approved by the mailinglist, writing a summary why the patches 1-4 are wrong and the new one the right way to go, laying out all the evidence, and only then passing it on as a pull request.
Like, trying to do as much review and redesign work for them as possible, so that they have no other way than to effortless understand and accept.

Certainly seems cleaner and more rewarrding to fix the upstream review / communication knot than addressing a library problem in the application layer.
Once the new enet version was out, pyrogenesis could rely on it (as the OSX script downloads it, as we compile and bundle the windows enet library, and linux distributions have packet managers that keep enet up to date for their dependent programs).

(Which doesn't mean one must not start with Plan B)

174

Mostly wondering whether this could use a protocol version agnostic function. Otherwise a code comment explaining the implementation decision would be useful.

Lefo marked 7 inline comments as done.Feb 2 2019, 8:47 PM
Lefo added inline comments.
source/network/StunClient.cpp
157

Well I'm a bit sceptical in this. From what I see it seems that lsalzman just has no time to deal with IPv6: https://github.com/lsalzman/enet/pull/21#issuecomment-236402694

It might be possible to implement it better so it does not break backward compatibility. But that would be a lot of work and I'm not really much motivated to do it when I don't even know whether it would ever got upstream.

It's also a lot complicated by the fact that enet relies on passing huge structures to user such as ENetPeer https://github.com/lsalzman/enet/blob/master/include/enet/enet.h#L267
And user is expected to access that structure and even modify some attributes. You can't change anything in that structure because that would break ABI compatibility. This is a poor design. You can't even add new fields to that structure because it is expected to be allocated on user's stack using structure size known to user at compile time. Any attempt to implement IPv6 without breaking ABI would probably lead to very ugly code. So it just seems to be better to drop everything and start over and do it right this time.

174

I think that IPv6 will kill NAT. I don't know of any ISP or router that would use NAT for any reason. When it comes to privacy random temporary addresses are usually used. When it comes to security stateful firewalls are usually used.
If we're ever gonna need some ugly helper such as STUN is on IPv6. It's probably gonna be to punch holes through stateful firewalls. You'd need to send packet from both peers first to establish working bidirectional UDP communication. But that would be much easier than STUN since no crazy things such as public ip addrees and public port detection would be necessary.
I don't know of any ISP that would implement such stateful firewall on their side. This is usually done in user's router which user usually has in control and can allow incoming connection on necessary port.
And it's probably better to lead users to learn how to setup their firewall rather than trying to overcome this somehow. Users will probably need this also for TCP and that's not that easy to punch through firewall as UDP is.
Anyway stateful firewalls are gonna be a problem only for hosts. They should not cause problems when joining a game.
Hopefully we're not ever gonna need to do anything and it will just work.

Stan added a comment.Sep 10 2019, 4:23 PM

This may be a stupid question, but does this work in the following situations:

  • player1 IPV4 + STUN → player2 IPV6
  • player1 IPV4 + STUN ← player2 IPV6
Lefo marked an inline comment as done.Sep 10 2019, 8:34 PM
In D1676#94736, @Stan wrote:

This may be a stupid question, but does this work in the following situations:

  • player1 IPV4 + STUN → player2 IPV6
  • player1 IPV4 + STUN ← player2 IPV6

I'm not sure what you mean by this question.
You cannot communicate from IPv4 address to IPv6 address and viceversa unless some translation is involved.
If you mean compatibility between players which one of them has IPv6 patch applied and one does not. Then you can for sure connect with this patch to any IPv4 host using STUN.
As for hosting with STUN enabled and IPv6 patch applied, I think it should work too.
There's even a line addressing this in what was tested: "Tested hosting and joining games via lobby with both stun enabled and disabled.".
But it's been a while since I made this patch. I don't remember whether I really tested it and I don't use STUN when hosting since I have port forwarded.

elexis added inline comments.Sep 14 2019, 9:23 PM
source/lib/external_libraries/enet.h
68

typedef a b; -> using b = a;

source/lobby/XmppClient.cpp
29

Does the "else" case trigger for other windows versions?

1312

sockaddr_in addr = {}; -> sockaddr_in addr;
sockaddr * -> sockaddr*

source/network/StunClient.cpp
154

c-style casts -> static_cast,reinterpret_cast etc.

sockaddr_in * -> sockaddr_in*

438

serverAddress.c_str()

source/third_party/enet/enet.h
2

How does one review this file?

A good test would be to download the official source, then apply a diff to that, then generate this file, and only review the diff.

15

-> 2016 That sounds a lot like this is outdated by three years (even if only few commits)

elexis added a comment.Oct 6 2019, 4:02 PM

Published with permission of Lefo, following lobby discussion yesterday:

Date: Mon, 23 Sep 2019 19:09:11 +0200
From: Filip Moc
To: Lee Salzman
Subject: Re: IPv6 support for enet

Hi Lee,

Well, here's the idea.
Let's put aside implementation for now and focus on API ABI and header files.

First we need to create some new variant of ENetAddress to hold either IPv4 or
IPv6 address together with some AF information. Let's name it ENetSockAddr for
example.
Perhaps we could completely reuse sockaddr structures here as they should be
portable enough for enet's needs.
To address compatibility with other families with bigger address lengths
possibly supported in the future it would be best to always allocate separate
memory for it and always reference it via pointer and never embed it in any
structure like it's unfortunetely currently done for ENetAddress in ENetPeer
and ENetHost.

Now we need to add this new ENetSockAddr somehow (via pointer reference) to
ENetPeer and ENetHost as we need to know what is the address of the peer and
such.
Since we don't want to break ABI the only option is to add it to the end of
ENetPeer and ENetHost structures.
By doing this we could break some crazy apps which do something strange with
ENetHost/ENetPeer such as memcpy it somewhere else and then pass it back to
enet since we'd access these new fields while the app wouldn't even allocate
memory for it. We could solve even this by introducing some global variable
which app would use to explicitly state it is compatible with new enet and we'd
only touch these new fields when this global variable is enabled. But AFAIK
sane applications are never expected to assemble custom ENetPeer/ENetHost and
should only pass pointers they got from enet back to ENetPeer/ENetHost
arguments. So I think we can just let apps which do strange things to crash (if
they are lucky) and avoid any complications.
But according to my understanding it is completely reasonable to assume that
applications may want to access currently existing ENetAddress fields (e.g. to
read remote address) so even if we have new ENetSockAddr fields we need to keep
old ENetAddress fields updated for backward compatibility.

Now we also need to provide application with new functions supporting new
ENetSockAddr arguments so app can e.g. connect to some host via IPv6 and
accept IPv6 client knowing it's address, etc.
There are curretly 12 functions expecting ENetAddress argument.
We could either overload them. This again introduces some global variable.
Application sets this global variable (probably via some function call) to
state it wants to use new enet API/ABI and if this global flag is enabled all
these functions would treat argument as ENetSockAddr instead of ENetAddress.
This should keep both API and ABI compatibility but will probably result in
compilation warnings unless explicitely taken care of (e.g. using some
preprocessor substitutions).
Another option is to create new functions (maybe less than 12 if we don't need
all of them / some are deprecated already?). These new functions would expect
ENetSockAddr as argument. Old functions expecting ENetAddress could then be
reimplemented as simple wrappers to ENetSockAddr alternatives.
I prefer creating new functions instead of overloading as it does not require
introducing any global variables and enables application to switch to new API
more smoothly using old and new function variants at the same time.

So far implementing this seems to me to be all the work needed which seems
reasonably easy.

So tell me what you think. I certainly did not examine all the details so there
might be some problems I'm still not aware of. But if you don't know about any
problems and you agree this approach is acceptable I can try to look into it in
more detail and perhaps even do some first testing and prepare some initial
patch.

Filip