Topic: [PULL] UPnP - page 2. (Read 5054 times)

In a remote buffer overflow scenario all nodes participating in the normal p2p network are pretty much fucked, no matter if they accept incoming connections or not.

Consider the following scenario:
Attacker exploits bug and disables all public nodes allowing incoming connections.
Nodes *not* allowing incoming connections see their outgoing connections count dropping and try to connect to new peers.
Only nodes left accepting incoming connections are under control of the attacker and exploit same bug in nodes connecting to them.
Whoops.

Who knows enough about wxWidgets to add a checkbox to the Settings UI for "Enable UpNP" (grayed out #ifndef UPNP...)?  Wanna coordinate with BlueMatt to get that done?

It seems reasonable to me to start with UPnP disabled by default.  As long as enabling it in the client is easy enough to do, the network will benefit because some nodes will "opt-in" that wouldn't have before.  We can change the default later, after we gain more confidence about remote vulnerabilities.

Erm, no. Services may be disabled by default, certainly, but once you start (for example) Apache, it listens to port 80 by default. You don't have to jump through extra hoops to configure a port. Likewise, distros won't auto-launch bitcoind by default, but when the user does so, they should reasonably expect it to listen.

An unfounded possible vulnerability is no excuse to make things harder for the user than they have to be. There could just as well be a vulnerability in the transaction code, or anywhere else that is going to be exposed to all nodes regardless. If you don't trust the bitcoin wallet you're using to be secure, you shouldn't be using it period.

Think of all those users in Canada and Australia who will have their limited bandwidth siphoned away without any warning...

After Bitcoin runs in lightweight client mode by default, it would make sense to ask the user about triggering UPnP if they enable "supernode" mode.

I completely disagree.  The more nodes which are connectible the harder any kind of Sybil attack becomes which just benefits the network.  Any user who has a legitimate reason why they don't want incoming connections will already either have UPnP off on their router or be able to disable it in the client.

Well, it benefits the network, which benefits the node.

But I agree:  it should be off by default.

It should not be enabled by default. Opening 8333 significantly increases network usage without any benefit to the node.

I wrote a patch for UPnP via miniupnpc.  Currently it is just for UNIX, but shouldn't be too hard to get running on OSX.  Some people have mentioned that Windows has its own native library for UPnP which we should use.  Currently UPnP is on off by default and can be easily turned on with -upnp but that is up for debate (actually poll).  See the pull request here: https://github.com/bitcoin/bitcoin/pull/133.

UPDATE: The patch now works for OSX and Windows and includes a WXUI switch for enabling/disabling UPnP at runtime.  Note that the windows version still uses miniupnpc (not the native windows libraries, does anyone feel very strongly about this?).  
As I couldn't get miniupnpc to compile on Windows, the makefile is designed to use the binary version published on the website (which includes the library against which bitcoin is linked, upnpc-exe-win32-20110215.zip).  To get the relevant headers, however you need to download the original source archive (miniupnpc-1.5.20110215.tar.gz) and copy *.h to C:\upnpc-exe-win32-20110215\miniupnpc.  Note that the UNIX/OSX versions expect version 1.5 (slightly different UPNP_AddPortMapping definition).  
Also, the translations of the copyright notice in the about dialog need changed before this should be pulled.  Currently only dutch has been translated (thanks joepie91).

UPDATE 2: Added translations by community members for French, Spanish and German (plus existing Dutch) and added Russian, Portuguese and Italian translations from Google Translate.  Anyone had time to review the code and have suggestions or think it is ready?

UPDATE 3: Italian translation updated thanks to Joozero.  Anyone have comments on the code?