diff --git a/p2p/nat/natupnp.go b/p2p/nat/natupnp.go index ed00b8eeb6..d79677db55 100644 --- a/p2p/nat/natupnp.go +++ b/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() diff --git a/p2p/server_nat.go b/p2p/server_nat.go index 5830f950e1..298c454a4a 100644 --- a/p2p/server_nat.go +++ b/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) } } }