cmd/bootnode, p2p: use an alternate port mapped to NAT

[dbadoy]

The purpose of that PR is efficient handling for NAT that return by allocating an alternate port if the requested port is already in use. So I changed the interface because I need to handle the port number returned from the method for AddPortMapping. I tried not to change the interface, but I couldn't solve it without using channels, and I wanted to avoid using channels because I thought it would be fatal if the caller didn't handle them properly.

The refresh process for NAT mapping is handled by the user himself. This allows free control over each calling module (bootnode, p2p.Server). In the current implementation, duplication of code doesn't seem too bad.

[dbadoy]

Would you mind briefly checking this idea before writing the test script and detailing? @fjl

[p9595jh]

LGTM

[fjl]

I still don't quite understand why you want to process port changes on the server main loop. You can just change the port on LocalNode, it is safe for concurrent use.

[dbadoy]

I vaguely imagined something like server.ChangePort(), or rpc method net_changePort...
But I can't come up with a clear scenario(+barely imagined, ports are not properly mapped, and don't want to wait until the next refresh (10 minutes)).

As you said, I want to change it to the following. Is my understanding correct?

// in Server nat refresh loop
for {
	select {
	case _, ok := <-srv.quit:
		if !ok {
			return
		}
	case <-refresh.C:
		log.Trace("Refreshing port mapping")
		p, err := natm.AddMapping(protocol, external, internal, name, mapTimeout)
		if err != nil {
			log.Debug("Couldn't add port mapping", "err", err)
		} else {
			if p != uint16(external) {
				log.Debug("Already mapped port", external, "use alternative port", p)
				log = newLogger(protocol, int(p), internal, natm)
				external = int(p)

				switch protocol {
				case "tcp":
					srv.localnode.Set(enr.TCP(external))
				case "udp":
					srv.localnode.SetFallbackUDP(external)
				}
			}
		}
		refresh.Reset(mapTimeout)
	}
}

[fjl]

Yes that looks better.

[fjl]

Actually, the flow can be a bit simpler than what you have there. Whether or not AddMapping returns a port that is equal to the local port, we can just always call Set or SetFallbackUDP on LocalNode. Idempotent updates will simply be ignored.

[dbadoy]

Good. I applied that.
Additionally, changed log.Trace message to remove duplicate code. 7adc65f

[dbadoy]

@fjl Thank you so much for your kind review. For the rest, I'll submit a PR to geth repo along with adding some test script and proceed.