p2p/nat: fix UPnP port reset

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 client.AddAnyPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS)
+		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)
-	if err == nil {
-		return uint16(extport), nil
+	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,28 +156,49 @@ 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 {
-					log.Debug("Couldn't add port mapping", "err", err)
-					m.extPort = 0
+					// 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.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 {
-					log.Info("NAT mapped port")
-				}
 
-				// Update port in local ENR.
-				switch m.protocol {
-				case "TCP":
-					srv.localnode.Set(enr.TCP(m.extPort))
-				case "UDP":
-					srv.localnode.SetFallbackUDP(m.extPort)
+				// It was mapped!
+				m.retries = 0
+				log = newLogger(m.protocol, int(p), m.port)
+				if int(p) != m.extPort {
+					m.extPort = int(p)
+					if m.port != m.extPort {
+						log.Info("NAT mapped alternative port")
+					} else {
+						log.Info("NAT mapped port")
+					}
+
+					// Update port in local ENR.
+					switch m.protocol {
+					case "TCP":
+						srv.localnode.Set(enr.TCP(m.extPort))
+					case "UDP":
+						srv.localnode.SetFallbackUDP(m.extPort)
+					}
 				}
+				m.nextTime = srv.clock.Now().Add(portMapRefreshInterval)
 			}
 		}
 	}