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")