Add UPnP support.

[shaunren]

Added UPnP support using libminiupnpc.

[irungentoo]

An issue I see about this pull request is that upnp should be optional (travis is failing because configure can't find the upnp library).

I also need someone to test it and confirm that it works.

@mk21 upnp is a clusterfuck so we kind of have to depend on a library to support it.

[shaunren]

You can disable UPnP support in ./configure (./configure --disable-miniupnp).

[fcore117]

UPnP as optional library is good choice for lot of routers so people without knowledge can be directly connected if ISP has open ports.

[shaunren]

Some of the tests fail currently because it times out on allocating UPnP ports. Should there be an argument in new_networking that disables UPnP port mapping?

[irungentoo]

even if upnp is enabled in the build tox needs to work if the NAT doesn't support upnp.

[shaunren]

Tox does work when NAT doesn't support UPnP. However, the function upnpDiscover will block for 1 second before it gives up searching for UPnP devices. For the same reason the tests timeout, since new_networking was called many times on those tests.

[iShift]

And what about NAT PMP (Apple UPNP)?

[fcore117]

MiniUPnP C library should support NAT-PNP too or i am wrong?

[nurupo]

@fcore117 you are wrong. see http://miniupnp.tuxfamily.org/

[irungentoo]

Has anyone tested this and can confirm that it works?

uPnP support on my router is broken so unfortunately I can't really test it.

[shaunren]

I have tested it on a few computers running on different routers, and it works well. I have only tested on Linux though.

[ittner]

@fcore117 @nurupo

miniupnp have a companion library to support NAT-PMP: http://miniupnp.tuxfamily.org/libnatpmp.html . Transmission uses both (https://trac.transmissionbt.com/browser/trunk/third-party?order=name), so we can consider them well tested.

[cebe]

I could help testing this if someone can write a little bit about what needs to be done and how to verify that it works...

[irungentoo]

you need to confirm that it does actually forward the port and keeps it forwarded for the time Tox is running.

[cebe]

okay but tox works without uPNP currently, what is the point of adding it? Or does it not work in all environments and uPNP will enable that?

[irungentoo]

It might improve performance of Tox on some routers.

[towlie]

It works on windows.

[LittleVulpix]

What's happening with this one? In general, tox (udp) seems to work better when ports are opened. Can I help with testing this? What needs to be done to merge this?

[BiTOk]

Why this PR was not merged to master?

[GrayHatter]

@BiTOk for a few reasons. The first few that comes to mind is that it merge conflics. no one else has tested it and posted here that it works. It's been silent for almost a year. And the submitter hasn't done anything with it for awhile

[towlie]

I tested it on windows and it worked. I saw that it forwarded ports via UPnP.

[GrayHatter]

@shaunren any chance we could get you to rebase or merge master into this PR?

@towlie @BiTOk @LittleVulpix @cebe any of you want to take a stab at it if shaunren isn't around?
If so just use git format-patch and past the output here.

[nurupo]

If so just use git format-patch and past the output here.

What about running Travis on it? A separate cross-referenced PR might be better.

[towlie]

The only problem I saw with this was that static linking failed. I could only use shared dll for linking. The static .a file was never used/found by toxcore configure.

[LittleVulpix]

I'll try to see if I can get it going somehow.

[GrayHatter]

@nurupo is right, a separate PR would be best

[GrayHatter]

And now that I think about it. What do you think about defaulting to compiling without this? Tox isn't exactly quiet on a network. But UPnP broadcasts would be quite telling as to who's running what on the network... Thoughts?

[BiTOk]

Maybe present it as a setting "Enable UPnP" and disable it by default?

[GrayHatter]

@BiTOk yeah, there's already a flag to disable it, just need to make that the default, and change the flag to optionally enable it

[BiTOk]

@GrayHatter I said about client setting, not about compiling setting.

[nurupo]

What does this UPnP support actually bring to Tox? How it is useful?

As far as I understand, it's used only for discovery of Tox nodes/clients running in LAN, i.e. it's functionality greatly overlaps with the one of LAN discovery Tox currently has. If so, then I don't see it bringing anything useful to justify a new library dependency (perhaps optional one).

[nurupo]

Also, there must be a way to enable/disable it, so a new field in Tox_Options struct would be required. (That essentially means that a Tox must be restarted in order to enable/disable it.)

[LittleVulpix]

@nurupo upnp configures upnp-compatible(and enabled) routers for port forwarding. In other words, it will automatically make your router forward 33445 to you (or whichever port your tox decides to use), which improves tox networking, especially in udp mode.

[GrayHatter]

@nurupo much more reliable then UDP hole punching

[nurupo]

Ok, then I have misunderstood it. Sounds good to me, the dependency is justified :)

[BiTOk]

As I can understand, this can increase file transfer speed and decrease tox network load (transfer without tcp relay. I correctly understood?

[fcore117]

towlie: what do you mean cannot statically linked? does it mean we should forget single .exe or binary releases or it is something else? i am not programmer.

[towlie]

@fcore117 It means that I had to use an extra dll file. So no single .exe binary. I assume that this is fixable with some research. Because it produces an static linkable .a file but somehow toxcore wasn't able to use it.

[LittleVulpix]

I've been defeated by rebasing, I can't do it. It's up to @shaunren or the other warriors now!

[GrayHatter]

Closing as stale, I'll re-open this if anyone wants to take over. @irungentoo @shaunren