p2p/nat: fix UPnP port reset (#31566)

Make UPnP more robust

- Once a random port was mapped, we try to stick to it even if a UPnP
refresh fails. Previously we were immediately moving back to try the
default port, leading to frequent ENR changes.

- We were deleting port mappings before refresh as a possible
workaround. This created issues in some UPnP servers. The UPnP (and PMP)
specification is explicit about the refresh requirements, and delete is
clearly not needed (see
https://github.com/ethereum/go-ethereum/pull/30265#issuecomment-2766987859).
From now on we only delete when closing.

- We were trying to add port mappings only once, and then moved on to
random ports. Now we insist a bit more, so that a simple failed request
won't lead to ENR changes.

Fixes https://github.com/ethereum/go-ethereum/issues/31418

---------

Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>

p2p/nat/natupnp.go

@@ -26,6 +26,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/ethereum/go-ethereum/log"
 	"github.com/huin/goupnp"
 	"github.com/huin/goupnp/dcps/internetgateway1"
 	"github.com/huin/goupnp/dcps/internetgateway2"
@@ -34,6 +35,8 @@ import (
 const (
 	soapRequestTimeout = 3 * time.Second
 	rateLimit          = 200 * time.Millisecond
+	retryCount         = 3 // number of retries after a failed AddPortMapping
+	randomCount        = 3 // number of random ports to try
 )
 
 type upnp struct {
@@ -89,42 +92,43 @@ func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, li
 
 	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 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 {
-			extport = int(p)
-		}
-		return err
-	})
-	return uint16(extport), err
+	return n.addAnyPortMapping(protocol, extport, intport, ip, desc, lifetimeS)
 }
 
 // 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 n.portWithRateLimit(func() (uint16, error) {
 			return client.AddAnyPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS)
+		})
 	}
 	// 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)
+	for i := 0; i < retryCount+1; i++ {
+		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
 		}
+		log.Debug("Failed to add port mapping", "protocol", protocol, "extport", extport, "intport", intport, "err", err)
+	}
 
 	// If above fails, we retry with a random port.
 	// We retry several times because of possible port conflicts.
-	for i := 0; i < 3; i++ {
+	var err error
+	for i := 0; i < randomCount; i++ {
 		extport = n.randomPort()
-		err := n.client.AddPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS)
+		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
 		}
+		log.Debug("Failed to add random port mapping", "protocol", protocol, "extport", extport, "intport", intport, "err", err)
 	}
 	return 0, err
 }
@@ -169,6 +173,17 @@ func (n *upnp) String() string {
 	return "UPNP " + n.service
 }
 
+func (n *upnp) portWithRateLimit(pfn func() (uint16, error)) (uint16, error) {
+	var port uint16
+	var err error
+	fn := func() error {
+		port, err = pfn()
+		return err
+	}
+	n.withRateLimit(fn)
+	return port, err
+}
+
 func (n *upnp) withRateLimit(fn func() error) error {
 	n.mu.Lock()
 	defer n.mu.Unlock()

p2p/server_nat.go

@@ -31,12 +31,14 @@ const (
 	portMapRefreshInterval = 8 * time.Minute
 	portMapRetryInterval   = 5 * time.Minute
 	extipRetryInterval     = 2 * time.Minute
+	maxRetries             = 5 // max number of failed attempts to refresh the mapping
 )
 
 type portMapping struct {
 	protocol string
 	name     string
 	port     int
+	retries  int // number of failed attempts to refresh the mapping
 
 	// for use by the portMappingLoop goroutine:
 	extPort  int // the mapped port returned by the NAT interface
@@ -154,15 +156,34 @@ func (srv *Server) portMappingLoop() {
 				log.Trace("Attempting port mapping")
 				p, err := srv.NAT.AddMapping(m.protocol, m.extPort, m.port, m.name, portMapDuration)
 				if err != nil {
+					// Failed to add or refresh port mapping.
+					if m.extPort == 0 {
 						log.Debug("Couldn't add port mapping", "err", err)
+					} else {
+						// Failed refresh. Since UPnP implementation are often buggy,
+						// and lifetime is larger than the retry interval, this does not
+						// mean we lost our existing mapping. We do not reset the external
+						// port, as it is still our best chance, but we do retry soon.
+						// We could check the error code, but UPnP implementations are buggy.
+						log.Debug("Couldn't refresh port mapping", "err", err)
+						m.retries++
+						if m.retries > maxRetries {
+							m.retries = 0
+							err := srv.NAT.DeleteMapping(m.protocol, m.extPort, m.port)
+							log.Debug("Couldn't refresh port mapping, trying to delete it:", "err", err)
 							m.extPort = 0
+						}
+					}
 					m.nextTime = srv.clock.Now().Add(portMapRetryInterval)
+					// Note ENR is not updated here, i.e. we keep the last port.
 					continue
 				}
+
 				// It was mapped!
+				m.retries = 0
+				log = newLogger(m.protocol, int(p), m.port)
+				if int(p) != m.extPort {
 					m.extPort = int(p)
-				m.nextTime = srv.clock.Now().Add(portMapRefreshInterval)
-				log = newLogger(m.protocol, m.extPort, m.port)
 					if m.port != m.extPort {
 						log.Info("NAT mapped alternative port")
 					} else {
@@ -177,6 +198,8 @@ func (srv *Server) portMappingLoop() {
 						srv.localnode.SetFallbackUDP(m.extPort)
 					}
 				}
+				m.nextTime = srv.clock.Now().Add(portMapRefreshInterval)
+			}
 		}
 	}
 }