p2p/nat: remove forceful port mapping in upnp (#30265)

Here we are modifying the port mapping logic so that existing port
mappings will only be removed when they were previously created by geth.

The AddAnyPortMapping functionality has been adapted to work consistently
between the IGDv1 and IGDv2 backends.

Author: qqqeck

p2p/nat/natpmp.go

@@ -49,6 +49,9 @@ func (n *pmp) AddMapping(protocol string, extport, intport int, name string, lif
 	if lifetime <= 0 {
 		return 0, errors.New("lifetime must not be <= 0")
 	}
+	if extport == 0 {
+		extport = intport
+	}
 	// Note order of port arguments is switched between our
 	// AddMapping and the client's AddPortMapping.
 	res, err := n.c.AddPortMapping(strings.ToLower(protocol), intport, extport, int(lifetime/time.Second))

p2p/nat/natupnp.go

@@ -86,15 +86,15 @@ func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, li
 	}
 	protocol = strings.ToUpper(protocol)
 	lifetimeS := uint32(lifetime / time.Second)
-	n.DeleteMapping(protocol, extport, intport)
 
-	err = n.withRateLimit(func() error {
-		return n.client.AddPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS)
-	})
-	if err == nil {
-		return uint16(extport), nil
+	if extport == 0 {
+		extport = intport
+	} else {
+		// Only delete port mapping if the external port was already used by geth.
+		n.DeleteMapping(protocol, extport, intport)
 	}
-	// Try addAnyPortMapping if mapping specified port didn't work.
+
+	// Try to add port mapping, preferring the specified external port.
 	err = n.withRateLimit(func() error {
 		p, err := n.addAnyPortMapping(protocol, extport, intport, ip, desc, lifetimeS)
 		if err == nil {
@@ -105,18 +105,28 @@ func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, li
 	return uint16(extport), err
 }
 
+// addAnyPortMapping tries to add a port mapping with the specified external port.
+// If the external port is already in use, it will try to assign another port.
 func (n *upnp) addAnyPortMapping(protocol string, extport, intport int, ip net.IP, desc string, lifetimeS uint32) (uint16, error) {
 	if client, ok := n.client.(*internetgateway2.WANIPConnection2); ok {
 		return client.AddAnyPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS)
 	}
-	// It will retry with a random port number if the client does
-	// not support AddAnyPortMapping.
-	extport = n.randomPort()
+	// For IGDv1 and v1 services we should first try to add with extport.
 	err := n.client.AddPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS)
-	if err != nil {
-		return 0, err
+	if err == nil {
+		return uint16(extport), nil
+	}
+
+	// If above fails, we retry with a random port.
+	// We retry several times because of possible port conflicts.
+	for i := 0; i < 3; i++ {
+		extport = n.randomPort()
+		err := n.client.AddPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS)
+		if err == nil {
+			return uint16(extport), nil
+		}
 	}
-	return uint16(extport), nil
+	return 0, err
 }
 
 func (n *upnp) randomPort() int {

p2p/server_nat.go

@@ -150,14 +150,9 @@ func (srv *Server) portMappingLoop() {
 					continue
 				}
 
-				external := m.port
-				if m.extPort != 0 {
-					external = m.extPort
-				}
-				log := newLogger(m.protocol, external, m.port)
-
+				log := newLogger(m.protocol, m.extPort, m.port)
 				log.Trace("Attempting port mapping")
-				p, err := srv.NAT.AddMapping(m.protocol, external, m.port, m.name, portMapDuration)
+				p, err := srv.NAT.AddMapping(m.protocol, m.extPort, m.port, m.name, portMapDuration)
 				if err != nil {
 					log.Debug("Couldn't add port mapping", "err", err)
 					m.extPort = 0
@@ -167,8 +162,8 @@ func (srv *Server) portMappingLoop() {
 				// It was mapped!
 				m.extPort = int(p)
 				m.nextTime = srv.clock.Now().Add(portMapRefreshInterval)
-				if external != m.extPort {
-					log = newLogger(m.protocol, m.extPort, m.port)
+				log = newLogger(m.protocol, m.extPort, m.port)
+				if m.port != m.extPort {
 					log.Info("NAT mapped alternative port")
 				} else {
 					log.Info("NAT mapped port")