From ff365afc63f047c09ff04f41e12678c6b6698b72 Mon Sep 17 00:00:00 2001
From: Nathan Jo <162083209+qqqeck@users.noreply.github.com>
Date: Fri, 4 Apr 2025 17:56:55 +0900
Subject: [PATCH] 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.
---
 p2p/nat/natpmp.go  |  3 +++
 p2p/nat/natupnp.go | 36 +++++++++++++++++++++++-------------
 p2p/server_nat.go  | 13 ++++---------
 3 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/p2p/nat/natpmp.go b/p2p/nat/natpmp.go
index 4a9644ac1a..ee07eb4ff6 100644
--- a/p2p/nat/natpmp.go
+++ b/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))
diff --git a/p2p/nat/natupnp.go b/p2p/nat/natupnp.go
index 1014dc69d2..ed00b8eeb6 100644
--- a/p2p/nat/natupnp.go
+++ b/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
 	}
-	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 0, err
 }
 
 func (n *upnp) randomPort() int {
diff --git a/p2p/server_nat.go b/p2p/server_nat.go
index 933993bc1f..5830f950e1 100644
--- a/p2p/server_nat.go
+++ b/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")