p2p: remove forceful port mapping in upnp

[qqqeck]

Since addAnyPortMapping is implemented, we don't need DeleteMapping to remove an existing port mapping and make a forceful port mapping (which somewhat goes against the concept of upnp).

Also there is some chance n.randomPort() in addAnyPortMapping returns an occupied port, so it's safer to try it couple of times. Three looks like enough according to libp2p.

Edit: Fixed so it would only delete port mapping only if the external port was originally used by geth.

[lightclient]

DeleteMapping was added in #15222 to fix issue #1024, but this is before addAnyPortMapping was added in #26359. I'm not really sure the point of removing it? But at the same time it seems like it isn't needed now.

Running addAnyPortMapping(..) three times before giving up looks okay to me.

Let's see what @fjl thinks.

[qqqeck]

I just thought it could cause some problems if it deletes port mappings used by other users(processes) using the same upnp.

Not sure about the full issue #1024 but the internal ip changing case is probably covered by addAnyPortMapping. I guess in this case geth does leave behind an abandoned port mapping, but upnp only maintains each port mapping for a certain time so it's probably okay.

[panicalways]

Basically, the resources of NAT are not sufficient. Therefore, related protocols recommend minimizing the usage of NAT resources. I think it would be better to remove unused ports.

[qqqeck]

I agree it would be better to remove unused ports. Still, I think deleting maybe-in-use port mappings is a much bigger problem. I'll add the code to delete ports that were abandoned by geth.

[cskiraly]

DeleteMapping was added in #15222 to fix issue #1024, but this is before addAnyPortMapping was added in #26359. I'm not really sure the point of removing it? But at the same time it seems like it isn't needed now.

Regarding the discussion on delete, it is not needed according to spec. Neither in UPnP IGDv1, nor in v2, nor in NAT-PMP. Implementations are notoriously buggy, so we might keep it as a workaround, but I would not use it as part of the base renewal mechanism, especially if we would risk side-effects.

Also, we do set a lifetime (10 minutes, which is well below the recommended 1-2 hours), so delete should happen even without an explicit call if we shutdown abruptly, while we could easily call it in normal shutdown (I didn't yet check if it is happening). But even if it was not deleted, the new request should count as an overwrite.

There are some corner cases. I will check what's handled and what's not:

1. Lifetime might not be supported. There should be a specific error code for this. In that case we could redo the mapping with lifetime 0 (permanent), but I think this is very rare in practive. 2, If a node restarted without deleting the mapping, and it restarts on a different internal IP, then it would not be able to get the same port. In this case addAnyPortMapping() would map to a different port. But mapping to a different port is perfectly fine, our protocol just needs a port, not that specific port anyway.

We should also anticipate the renewal. Currently we are doing it with the same time as the lifetime, which can result in some service outage.

Here are the relevant specs:

Spec

UPnP Internet Gateway Device v1 (IGD:1)

https://upnp.org/specs/gw/UPnP-gw-WANIPConnection-v1-Service.pdf

UPnP Internet Gateway Device v2 (IGD:2).

https://upnp.org/specs/gw/UPnP-gw-WANIPConnection-v2-Service.pdf

PMP spec

https://www.rfc-editor.org/rfc/rfc6886.txt

PCP (PMP next version) spec

https://www.rfc-editor.org/rfc/rfc6887.txt

Renawal

And here are the relevant parts about renewal (called overwrite in UPnP)

UPnP IGD v1

Overwriting Previous / Existing Port Mappings: If the RemoteHost, ExternalPort, PortMappingProtocol and InternalClient are exactly the same as an existing mapping, the existing mapping values for InternalPort, PortMappingDescription, PortMappingEnabled and PortMappingLeaseDuration are overwritten.

UPnP IGD v2

2.5.16 AddPortMapping() This action creates a new port mapping or overwrites an existing mapping with the same internal client. If the ExternalPort and PortMappingProtocol pair is already mapped to another internal client, an error is returned.

Figure 2-3: Summary of AddPortMapping() results

Protocol	ExternalPort	RemoteHost	InternalClient	Result
=	=	=	=	Success (overwrite)

PMP

A renewal packet is formatted identically to an initial mapping request packet, except that for renewals the client sets the Suggested External Port field to the port the gateway actually assigned, rather than the port the client originally wanted.