From 994603292405dbb5765e66130605d1a5d5808a13 Mon Sep 17 00:00:00 2001 From: Seungbae Yu Date: Wed, 14 Dec 2022 22:11:52 +0900 Subject: [PATCH 1/3] squash cmd/bootnode, p2p: use an alternate port mapped to NAT * temp * rede * remove meaningless field * temp comment * set refresh time to param * server: use channel enr.Entry * server: rename `changedport` to `changeport` * server: apply new nat-refresh `ethereum discovery` * p2p/nat: add some comment * p2p/nat: use `DefaultMapTimeout` * p2p: fixed * p2p/server: impr * p2p/server: syntax change * cmd/bootnode: add comments * add p2p.Server changeport * all: fix comments * p2p/server: syntax changes * cmd/bootnode: do not change UDPAddr Port * p2p: remove incorrect `for` context * p2p: add what needs to be addressed * p2p/nat: use AddPortMapping if supported * asynchronous port set * remove unused variables * change return format in `changePort` * cmd/bootnode: consistent log * p2p/server: rename `refreshLogger` to `newLogger` * cmd/bootnode: fix incorrect if-else * fix * change log orders * rename `natRefresh` to `natMapLoop` * p2p: remove `changeport` in Server main loop * server: remove duplicate code * typo * typo * cmd/bootnode: consistent log message * p2p: fix comments test code about port change on localnode cmd/bootnode: add log improve test logic , add some comments p2p/nat: add comments about not supported p2p: remove unnecessary conversion in test --- cmd/bootnode/main.go | 53 ++++++++++++++++- p2p/nat/nat.go | 22 +++---- p2p/nat/natpmp.go | 24 ++++---- p2p/nat/natupnp.go | 34 ++++++++++- p2p/server.go | 60 +++++++++++++++++-- p2p/server_test.go | 139 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 298 insertions(+), 34 deletions(-) diff --git a/cmd/bootnode/main.go b/cmd/bootnode/main.go index 036b968ef83d..768a9bc64213 100644 --- a/cmd/bootnode/main.go +++ b/cmd/bootnode/main.go @@ -23,6 +23,7 @@ import ( "fmt" "net" "os" + "time" "github.com/ethereum/go-ethereum/cmd/utils" "github.com/ethereum/go-ethereum/crypto" @@ -110,11 +111,57 @@ func main() { realaddr := conn.LocalAddr().(*net.UDPAddr) if natm != nil { + var ( + protocol = "udp" + name = "ethereum discovery" + intport = realaddr.Port + extport = realaddr.Port + mapTimeout = nat.DefaultMapTimeout + + newLogger = func(p string, e int, i int, n nat.Interface) log.Logger { + return log.New("proto", p, "extport", e, "intport", i, "interface", n) + } + ) + if !realaddr.IP.IsLoopback() { - go nat.Map(natm, nil, "udp", realaddr.Port, realaddr.Port, "ethereum discovery") + log := newLogger(protocol, extport, intport, natm) + + p, err := natm.AddMapping(protocol, extport, intport, name, mapTimeout) + if err != nil { + log.Debug("Couldn't add port mapping", "err", err) + } else { + log.Info("Mapped network port") + if p != uint16(extport) { + log.Debug("Already mapped port", extport, "use alternative port", p) + log = newLogger(protocol, int(p), intport, natm) + extport = int(p) + } + } + + go func() { + refresh := time.NewTimer(mapTimeout) + for { + <-refresh.C + log.Trace("Start port mapping") + p, err := natm.AddMapping(protocol, extport, intport, name, mapTimeout) + if err != nil { + log.Debug("Couldn't add port mapping", "err", err) + } else { + if p != uint16(extport) { + // If the port mapping is changed after the boot node is executed and the + // URL is shared, there is no point in continuing the node. In this case, + // re-execute with an available port and share the URL again. + natm.DeleteMapping(protocol, int(p), intport) + panic(fmt.Errorf("port %d already mapped to another address (hint: use %d", extport, p)) + } + log.Info("Mapped network port") + } + refresh.Reset(mapTimeout) + } + }() } - if ext, err := natm.ExternalIP(); err == nil { - realaddr = &net.UDPAddr{IP: ext, Port: realaddr.Port} + if extip, err := natm.ExternalIP(); err == nil { + realaddr = &net.UDPAddr{IP: extip, Port: extport} } } diff --git a/p2p/nat/nat.go b/p2p/nat/nat.go index ad4c36582ae7..534134ba494c 100644 --- a/p2p/nat/nat.go +++ b/p2p/nat/nat.go @@ -38,7 +38,7 @@ type Interface interface { // protocol is "UDP" or "TCP". Some implementations allow setting // a display name for the mapping. The mapping may be removed by // the gateway when its lifetime ends. - AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) error + AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) DeleteMapping(protocol string, extport, intport int) error // ExternalIP should return the external (Internet-facing) @@ -91,20 +91,22 @@ func Parse(spec string) (Interface, error) { } const ( - mapTimeout = 10 * time.Minute + DefaultMapTimeout = 10 * time.Minute ) // Map adds a port mapping on m and keeps it alive until c is closed. // This function is typically invoked in its own goroutine. +// +// This does not support using alternate ports. func Map(m Interface, c <-chan struct{}, protocol string, extport, intport int, name string) { log := log.New("proto", protocol, "extport", extport, "intport", intport, "interface", m) - refresh := time.NewTimer(mapTimeout) + refresh := time.NewTimer(DefaultMapTimeout) defer func() { refresh.Stop() log.Debug("Deleting port mapping") m.DeleteMapping(protocol, extport, intport) }() - if err := m.AddMapping(protocol, extport, intport, name, mapTimeout); err != nil { + if _, err := m.AddMapping(protocol, extport, intport, name, DefaultMapTimeout); err != nil { log.Debug("Couldn't add port mapping", "err", err) } else { log.Info("Mapped network port") @@ -117,10 +119,10 @@ func Map(m Interface, c <-chan struct{}, protocol string, extport, intport int, } case <-refresh.C: log.Trace("Refreshing port mapping") - if err := m.AddMapping(protocol, extport, intport, name, mapTimeout); err != nil { + if _, err := m.AddMapping(protocol, extport, intport, name, DefaultMapTimeout); err != nil { log.Debug("Couldn't add port mapping", "err", err) } - refresh.Reset(mapTimeout) + refresh.Reset(DefaultMapTimeout) } } } @@ -135,8 +137,8 @@ func (n ExtIP) String() string { return fmt.Sprintf("ExtIP(%v)", ne // These do nothing. -func (ExtIP) AddMapping(string, int, int, string, time.Duration) error { return nil } -func (ExtIP) DeleteMapping(string, int, int) error { return nil } +func (ExtIP) AddMapping(string, int, int, string, time.Duration) (uint16, error) { return 0, nil } +func (ExtIP) DeleteMapping(string, int, int) error { return nil } // Any returns a port mapper that tries to discover any supported // mechanism on the local network. @@ -193,9 +195,9 @@ func startautodisc(what string, doit func() Interface) Interface { return &autodisc{what: what, doit: doit} } -func (n *autodisc) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) error { +func (n *autodisc) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) { if err := n.wait(); err != nil { - return err + return 0, err } return n.found.AddMapping(protocol, extport, intport, name, lifetime) } diff --git a/p2p/nat/natpmp.go b/p2p/nat/natpmp.go index 40f2aff44e7a..567d2260e240 100644 --- a/p2p/nat/natpmp.go +++ b/p2p/nat/natpmp.go @@ -44,28 +44,24 @@ func (n *pmp) ExternalIP() (net.IP, error) { return response.ExternalIPAddress[:], nil } -func (n *pmp) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) error { +func (n *pmp) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) { if lifetime <= 0 { - return fmt.Errorf("lifetime must not be <= 0") + return 0, fmt.Errorf("lifetime must not be <= 0") } // 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)) if err != nil { - return err + return 0, err } - // NAT-PMP maps an alternative available port number if the requested - // port is already mapped to another address and returns success. In this - // case, we return an error because there is no way to return the new port - // to the caller. - if uint16(extport) != res.MappedExternalPort { - // Destroy the mapping in NAT device. - n.c.AddPortMapping(strings.ToLower(protocol), intport, 0, 0) - return fmt.Errorf("port %d already mapped to another address (%s)", extport, protocol) - } - - return nil + // NAT-PMP maps an alternative available port number if the requested port + // is already mapped to another address and returns success. Handling of + // alternate port numbers is done by the caller. + // + // note: The result of AddPortMapping has several fields, but returns only + // MappedExternalPort(no other fields are used). + return res.MappedExternalPort, nil } func (n *pmp) DeleteMapping(protocol string, extport, intport int) (err error) { diff --git a/p2p/nat/natupnp.go b/p2p/nat/natupnp.go index a8de00e978b9..2053fdcdfb61 100644 --- a/p2p/nat/natupnp.go +++ b/p2p/nat/natupnp.go @@ -19,6 +19,8 @@ package nat import ( "errors" "fmt" + "math" + "math/rand" "net" "strings" "sync" @@ -76,18 +78,39 @@ func (n *upnp) ExternalIP() (addr net.IP, err error) { return ip, nil } -func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, lifetime time.Duration) error { +func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, lifetime time.Duration) (uint16, error) { ip, err := n.internalAddress() if err != nil { - return nil // TODO: Shouldn't we return the error? + return 0, nil // TODO: Shouldn't we return the error? } protocol = strings.ToUpper(protocol) lifetimeS := uint32(lifetime / time.Second) n.DeleteMapping(protocol, extport, intport) - return n.withRateLimit(func() error { + 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 + } + + return uint16(extport), n.withRateLimit(func() error { + p, err := n.addAnyPortMapping(protocol, extport, intport, ip, desc, lifetimeS) + if err == nil { + extport = int(p) + } + return err + }) +} + +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 = randomPort() + return uint16(extport), n.client.AddPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS) } func (n *upnp) internalAddress() (net.IP, error) { @@ -213,3 +236,8 @@ func discover(out chan<- *upnp, target string, matcher func(goupnp.ServiceClient out <- nil } } + +func randomPort() int { + rand.Seed(time.Now().UnixNano()) + return rand.Intn(math.MaxUint16-10000) + 10000 +} diff --git a/p2p/server.go b/p2p/server.go index 19f7935ffcae..002df532192a 100644 --- a/p2p/server.go +++ b/p2p/server.go @@ -571,16 +571,16 @@ func (srv *Server) setupDiscovery() error { } realaddr := conn.LocalAddr().(*net.UDPAddr) srv.log.Debug("UDP listener up", "addr", realaddr) + if srv.NAT != nil { if !realaddr.IP.IsLoopback() { srv.loopWG.Add(1) go func() { - nat.Map(srv.NAT, srv.quit, "udp", realaddr.Port, realaddr.Port, "ethereum discovery") + srv.natMapLoop(srv.NAT, "udp", realaddr.Port, realaddr.Port, "ethereum discovery", nat.DefaultMapTimeout) srv.loopWG.Done() }() } } - srv.localnode.SetFallbackUDP(realaddr.Port) // Discovery V4 var unhandled chan discover.ReadPacket @@ -678,11 +678,10 @@ func (srv *Server) setupListening() error { // Update the local node record and map the TCP listening port if NAT is configured. if tcp, ok := listener.Addr().(*net.TCPAddr); ok { - srv.localnode.Set(enr.TCP(tcp.Port)) if !tcp.IP.IsLoopback() && srv.NAT != nil { srv.loopWG.Add(1) go func() { - nat.Map(srv.NAT, srv.quit, "tcp", tcp.Port, tcp.Port, "ethereum p2p") + srv.natMapLoop(srv.NAT, "tcp", tcp.Port, tcp.Port, "ethereum p2p", nat.DefaultMapTimeout) srv.loopWG.Done() }() } @@ -693,6 +692,59 @@ func (srv *Server) setupListening() error { return nil } +// natMapLoop performs initialization mapping for nat and repeats refresh. +func (srv *Server) natMapLoop(natm nat.Interface, protocol string, intport, extport int, name string, interval time.Duration) { + var ( + internal = intport + external = extport + mapTimeout = interval + + newLogger = func(p string, e int, i int, n nat.Interface) log.Logger { + return log.New("proto", p, "extport", e, "intport", i, "interface", n) + } + ) + + log := newLogger(protocol, external, internal, natm) + + // Set to 0 to perform initial port mapping. This will return C + // immediately and set it to mapTimeout in the next loop. + refresh := time.NewTimer(time.Duration(0)) + defer func() { + refresh.Stop() + log.Debug("Deleting port mapping") + natm.DeleteMapping(protocol, external, internal) + }() + + for { + select { + case _, ok := <-srv.quit: + if !ok { + return + } + case <-refresh.C: + log.Trace("Start port mapping") + p, err := natm.AddMapping(protocol, external, internal, name, mapTimeout) + if err != nil { + log.Debug("Couldn't add port mapping", "err", err) + } else { + log.Info("Mapped network port") + 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) + } + } +} + // doPeerOp runs fn on the main loop. func (srv *Server) doPeerOp(fn peerOpFunc) { select { diff --git a/p2p/server_test.go b/p2p/server_test.go index f6f5700c5efc..8dddfe056634 100644 --- a/p2p/server_test.go +++ b/p2p/server_test.go @@ -24,6 +24,8 @@ import ( "math/rand" "net" "reflect" + "strconv" + "strings" "testing" "time" @@ -32,6 +34,7 @@ import ( "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/p2p/enode" "github.com/ethereum/go-ethereum/p2p/enr" + "github.com/ethereum/go-ethereum/p2p/nat" "github.com/ethereum/go-ethereum/p2p/rlpx" ) @@ -224,6 +227,14 @@ func TestServerRemovePeerDisconnect(t *testing.T) { srv2.Start() defer srv2.Stop() + s := strings.Split(srv2.ListenAddr, ":") + if len(s) != 2 { + t.Fatal("invalid ListenAddr") + } + if port, err := strconv.Atoi(s[1]); err == nil { + srv2.localnode.Set(enr.TCP(uint16(port))) + } + if !syncAddPeer(srv1, srv2.Self()) { t.Fatal("peer not connected") } @@ -455,6 +466,134 @@ func TestServerSetupConn(t *testing.T) { } } +// TestChangeLocalNodePort tests for localnode port change in Server.natMapLoop. +func TestChangeLocalNodePort(t *testing.T) { + tests := []struct { + protocol string + request int // The port number that requested by the server. + response int // The port number that responded with mock NAT. + }{ + { + protocol: "udp", + request: 30303, + response: 30303, + }, + { + protocol: "udp", + request: 30303, + response: 30000, + }, + { + protocol: "tcp", + request: 30303, + response: 30303, + }, + { + protocol: "tcp", + request: 30303, + response: 30000, + }, + } + + srvkey := newkey() + srv := &Server{ + Config: Config{ + PrivateKey: srvkey, + MaxPeers: 0, + NoDial: true, + NoDiscovery: true, + Protocols: []Protocol{discard}, + Logger: testlog.Logger(t, log.LvlTrace), + }, + newTransport: nil, + } + + srv.quit = make(chan struct{}) + + if err := srv.setupLocalNode(); err != nil { + t.Fatal(err) + } + + for _, test := range tests { + mock := &mockNAT{uint16(test.response)} + go srv.natMapLoop(mock, test.protocol, test.request, test.request, "mock discovery", nat.DefaultMapTimeout) + time.Sleep(5 * time.Millisecond) + + close(srv.quit) + srv.quit = make(chan struct{}) + + var port int + switch test.protocol { + case "tcp": + port = srv.localnode.Node().TCP() + case "udp": + port = srv.localnode.Node().UDP() + } + if port != test.response { + t.Fatalf("couldn't change the port number on localnode: got %d want %d", port, test.response) + } + } +} + +// TestMapLoop checks if the alternate port is actually used by Server.natMapLoop. +func TestMapLoop(t *testing.T) { + srvkey := newkey() + srv := &Server{ + Config: Config{ + PrivateKey: srvkey, + MaxPeers: 0, + NoDial: true, + NoDiscovery: true, + Protocols: []Protocol{discard}, + Logger: testlog.Logger(t, log.LvlTrace), + }, + newTransport: nil, + } + + srv.quit = make(chan struct{}) + + if err := srv.setupLocalNode(); err != nil { + t.Fatal(err) + } + + mock := &mockNAT{30000} + interval := 20 * time.Millisecond + + go srv.natMapLoop(mock, "udp", 30303, 30303, "mock discovery", interval) + time.Sleep(5 * time.Millisecond) + + // Watch the interval 3 times. UDP on localnode should be 30000. + for i := 0; i < 3; i++ { + if srv.localnode.Node().UDP() != int(mock.mapped) { + t.Fatalf("natMapLoop does not make requests to alternate ports: got %d want %d", srv.localnode.Node().UDP(), int(mock.mapped)) + } + time.Sleep(interval) + + // In the last interval, the alternate port is changed once more. + if i == 2 { + mock.mapped = 50000 + } + } + close(srv.quit) +} + +type mockNAT struct { + mapped uint16 +} + +func (m *mockNAT) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) { + return m.mapped, nil +} +func (m *mockNAT) DeleteMapping(protocol string, extport, intport int) error { + return nil +} +func (m *mockNAT) ExternalIP() (net.IP, error) { + panic("not implemented") +} +func (m *mockNAT) String() string { + panic("not implemented") +} + type setupTransport struct { pubkey *ecdsa.PublicKey encHandshakeErr error From faeac3be16bbf03cac5f19ed6214d723c0b1174c Mon Sep 17 00:00:00 2001 From: dbadoy4874 Date: Fri, 23 Dec 2022 14:48:18 +0900 Subject: [PATCH 2/3] Revert "squash" This reverts commit 994603292405dbb5765e66130605d1a5d5808a13. --- cmd/bootnode/main.go | 53 +---------------- p2p/nat/nat.go | 22 ++++--- p2p/nat/natpmp.go | 24 ++++---- p2p/nat/natupnp.go | 34 +---------- p2p/server.go | 60 ++----------------- p2p/server_test.go | 139 ------------------------------------------- 6 files changed, 34 insertions(+), 298 deletions(-) diff --git a/cmd/bootnode/main.go b/cmd/bootnode/main.go index 768a9bc64213..036b968ef83d 100644 --- a/cmd/bootnode/main.go +++ b/cmd/bootnode/main.go @@ -23,7 +23,6 @@ import ( "fmt" "net" "os" - "time" "github.com/ethereum/go-ethereum/cmd/utils" "github.com/ethereum/go-ethereum/crypto" @@ -111,57 +110,11 @@ func main() { realaddr := conn.LocalAddr().(*net.UDPAddr) if natm != nil { - var ( - protocol = "udp" - name = "ethereum discovery" - intport = realaddr.Port - extport = realaddr.Port - mapTimeout = nat.DefaultMapTimeout - - newLogger = func(p string, e int, i int, n nat.Interface) log.Logger { - return log.New("proto", p, "extport", e, "intport", i, "interface", n) - } - ) - if !realaddr.IP.IsLoopback() { - log := newLogger(protocol, extport, intport, natm) - - p, err := natm.AddMapping(protocol, extport, intport, name, mapTimeout) - if err != nil { - log.Debug("Couldn't add port mapping", "err", err) - } else { - log.Info("Mapped network port") - if p != uint16(extport) { - log.Debug("Already mapped port", extport, "use alternative port", p) - log = newLogger(protocol, int(p), intport, natm) - extport = int(p) - } - } - - go func() { - refresh := time.NewTimer(mapTimeout) - for { - <-refresh.C - log.Trace("Start port mapping") - p, err := natm.AddMapping(protocol, extport, intport, name, mapTimeout) - if err != nil { - log.Debug("Couldn't add port mapping", "err", err) - } else { - if p != uint16(extport) { - // If the port mapping is changed after the boot node is executed and the - // URL is shared, there is no point in continuing the node. In this case, - // re-execute with an available port and share the URL again. - natm.DeleteMapping(protocol, int(p), intport) - panic(fmt.Errorf("port %d already mapped to another address (hint: use %d", extport, p)) - } - log.Info("Mapped network port") - } - refresh.Reset(mapTimeout) - } - }() + go nat.Map(natm, nil, "udp", realaddr.Port, realaddr.Port, "ethereum discovery") } - if extip, err := natm.ExternalIP(); err == nil { - realaddr = &net.UDPAddr{IP: extip, Port: extport} + if ext, err := natm.ExternalIP(); err == nil { + realaddr = &net.UDPAddr{IP: ext, Port: realaddr.Port} } } diff --git a/p2p/nat/nat.go b/p2p/nat/nat.go index 534134ba494c..ad4c36582ae7 100644 --- a/p2p/nat/nat.go +++ b/p2p/nat/nat.go @@ -38,7 +38,7 @@ type Interface interface { // protocol is "UDP" or "TCP". Some implementations allow setting // a display name for the mapping. The mapping may be removed by // the gateway when its lifetime ends. - AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) + AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) error DeleteMapping(protocol string, extport, intport int) error // ExternalIP should return the external (Internet-facing) @@ -91,22 +91,20 @@ func Parse(spec string) (Interface, error) { } const ( - DefaultMapTimeout = 10 * time.Minute + mapTimeout = 10 * time.Minute ) // Map adds a port mapping on m and keeps it alive until c is closed. // This function is typically invoked in its own goroutine. -// -// This does not support using alternate ports. func Map(m Interface, c <-chan struct{}, protocol string, extport, intport int, name string) { log := log.New("proto", protocol, "extport", extport, "intport", intport, "interface", m) - refresh := time.NewTimer(DefaultMapTimeout) + refresh := time.NewTimer(mapTimeout) defer func() { refresh.Stop() log.Debug("Deleting port mapping") m.DeleteMapping(protocol, extport, intport) }() - if _, err := m.AddMapping(protocol, extport, intport, name, DefaultMapTimeout); err != nil { + if err := m.AddMapping(protocol, extport, intport, name, mapTimeout); err != nil { log.Debug("Couldn't add port mapping", "err", err) } else { log.Info("Mapped network port") @@ -119,10 +117,10 @@ func Map(m Interface, c <-chan struct{}, protocol string, extport, intport int, } case <-refresh.C: log.Trace("Refreshing port mapping") - if _, err := m.AddMapping(protocol, extport, intport, name, DefaultMapTimeout); err != nil { + if err := m.AddMapping(protocol, extport, intport, name, mapTimeout); err != nil { log.Debug("Couldn't add port mapping", "err", err) } - refresh.Reset(DefaultMapTimeout) + refresh.Reset(mapTimeout) } } } @@ -137,8 +135,8 @@ func (n ExtIP) String() string { return fmt.Sprintf("ExtIP(%v)", ne // These do nothing. -func (ExtIP) AddMapping(string, int, int, string, time.Duration) (uint16, error) { return 0, nil } -func (ExtIP) DeleteMapping(string, int, int) error { return nil } +func (ExtIP) AddMapping(string, int, int, string, time.Duration) error { return nil } +func (ExtIP) DeleteMapping(string, int, int) error { return nil } // Any returns a port mapper that tries to discover any supported // mechanism on the local network. @@ -195,9 +193,9 @@ func startautodisc(what string, doit func() Interface) Interface { return &autodisc{what: what, doit: doit} } -func (n *autodisc) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) { +func (n *autodisc) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) error { if err := n.wait(); err != nil { - return 0, err + return err } return n.found.AddMapping(protocol, extport, intport, name, lifetime) } diff --git a/p2p/nat/natpmp.go b/p2p/nat/natpmp.go index 567d2260e240..40f2aff44e7a 100644 --- a/p2p/nat/natpmp.go +++ b/p2p/nat/natpmp.go @@ -44,24 +44,28 @@ func (n *pmp) ExternalIP() (net.IP, error) { return response.ExternalIPAddress[:], nil } -func (n *pmp) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) { +func (n *pmp) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) error { if lifetime <= 0 { - return 0, fmt.Errorf("lifetime must not be <= 0") + return fmt.Errorf("lifetime must not be <= 0") } // 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)) if err != nil { - return 0, err + return err } - // NAT-PMP maps an alternative available port number if the requested port - // is already mapped to another address and returns success. Handling of - // alternate port numbers is done by the caller. - // - // note: The result of AddPortMapping has several fields, but returns only - // MappedExternalPort(no other fields are used). - return res.MappedExternalPort, nil + // NAT-PMP maps an alternative available port number if the requested + // port is already mapped to another address and returns success. In this + // case, we return an error because there is no way to return the new port + // to the caller. + if uint16(extport) != res.MappedExternalPort { + // Destroy the mapping in NAT device. + n.c.AddPortMapping(strings.ToLower(protocol), intport, 0, 0) + return fmt.Errorf("port %d already mapped to another address (%s)", extport, protocol) + } + + return nil } func (n *pmp) DeleteMapping(protocol string, extport, intport int) (err error) { diff --git a/p2p/nat/natupnp.go b/p2p/nat/natupnp.go index 2053fdcdfb61..a8de00e978b9 100644 --- a/p2p/nat/natupnp.go +++ b/p2p/nat/natupnp.go @@ -19,8 +19,6 @@ package nat import ( "errors" "fmt" - "math" - "math/rand" "net" "strings" "sync" @@ -78,39 +76,18 @@ func (n *upnp) ExternalIP() (addr net.IP, err error) { return ip, nil } -func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, lifetime time.Duration) (uint16, error) { +func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, lifetime time.Duration) error { ip, err := n.internalAddress() if err != nil { - return 0, nil // TODO: Shouldn't we return the error? + return nil // TODO: Shouldn't we return the error? } protocol = strings.ToUpper(protocol) lifetimeS := uint32(lifetime / time.Second) n.DeleteMapping(protocol, extport, intport) - err = n.withRateLimit(func() error { + return 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 - } - - return uint16(extport), n.withRateLimit(func() error { - p, err := n.addAnyPortMapping(protocol, extport, intport, ip, desc, lifetimeS) - if err == nil { - extport = int(p) - } - return err - }) -} - -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 = randomPort() - return uint16(extport), n.client.AddPortMapping("", uint16(extport), protocol, uint16(intport), ip.String(), true, desc, lifetimeS) } func (n *upnp) internalAddress() (net.IP, error) { @@ -236,8 +213,3 @@ func discover(out chan<- *upnp, target string, matcher func(goupnp.ServiceClient out <- nil } } - -func randomPort() int { - rand.Seed(time.Now().UnixNano()) - return rand.Intn(math.MaxUint16-10000) + 10000 -} diff --git a/p2p/server.go b/p2p/server.go index 002df532192a..19f7935ffcae 100644 --- a/p2p/server.go +++ b/p2p/server.go @@ -571,16 +571,16 @@ func (srv *Server) setupDiscovery() error { } realaddr := conn.LocalAddr().(*net.UDPAddr) srv.log.Debug("UDP listener up", "addr", realaddr) - if srv.NAT != nil { if !realaddr.IP.IsLoopback() { srv.loopWG.Add(1) go func() { - srv.natMapLoop(srv.NAT, "udp", realaddr.Port, realaddr.Port, "ethereum discovery", nat.DefaultMapTimeout) + nat.Map(srv.NAT, srv.quit, "udp", realaddr.Port, realaddr.Port, "ethereum discovery") srv.loopWG.Done() }() } } + srv.localnode.SetFallbackUDP(realaddr.Port) // Discovery V4 var unhandled chan discover.ReadPacket @@ -678,10 +678,11 @@ func (srv *Server) setupListening() error { // Update the local node record and map the TCP listening port if NAT is configured. if tcp, ok := listener.Addr().(*net.TCPAddr); ok { + srv.localnode.Set(enr.TCP(tcp.Port)) if !tcp.IP.IsLoopback() && srv.NAT != nil { srv.loopWG.Add(1) go func() { - srv.natMapLoop(srv.NAT, "tcp", tcp.Port, tcp.Port, "ethereum p2p", nat.DefaultMapTimeout) + nat.Map(srv.NAT, srv.quit, "tcp", tcp.Port, tcp.Port, "ethereum p2p") srv.loopWG.Done() }() } @@ -692,59 +693,6 @@ func (srv *Server) setupListening() error { return nil } -// natMapLoop performs initialization mapping for nat and repeats refresh. -func (srv *Server) natMapLoop(natm nat.Interface, protocol string, intport, extport int, name string, interval time.Duration) { - var ( - internal = intport - external = extport - mapTimeout = interval - - newLogger = func(p string, e int, i int, n nat.Interface) log.Logger { - return log.New("proto", p, "extport", e, "intport", i, "interface", n) - } - ) - - log := newLogger(protocol, external, internal, natm) - - // Set to 0 to perform initial port mapping. This will return C - // immediately and set it to mapTimeout in the next loop. - refresh := time.NewTimer(time.Duration(0)) - defer func() { - refresh.Stop() - log.Debug("Deleting port mapping") - natm.DeleteMapping(protocol, external, internal) - }() - - for { - select { - case _, ok := <-srv.quit: - if !ok { - return - } - case <-refresh.C: - log.Trace("Start port mapping") - p, err := natm.AddMapping(protocol, external, internal, name, mapTimeout) - if err != nil { - log.Debug("Couldn't add port mapping", "err", err) - } else { - log.Info("Mapped network port") - 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) - } - } -} - // doPeerOp runs fn on the main loop. func (srv *Server) doPeerOp(fn peerOpFunc) { select { diff --git a/p2p/server_test.go b/p2p/server_test.go index 8dddfe056634..f6f5700c5efc 100644 --- a/p2p/server_test.go +++ b/p2p/server_test.go @@ -24,8 +24,6 @@ import ( "math/rand" "net" "reflect" - "strconv" - "strings" "testing" "time" @@ -34,7 +32,6 @@ import ( "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/p2p/enode" "github.com/ethereum/go-ethereum/p2p/enr" - "github.com/ethereum/go-ethereum/p2p/nat" "github.com/ethereum/go-ethereum/p2p/rlpx" ) @@ -227,14 +224,6 @@ func TestServerRemovePeerDisconnect(t *testing.T) { srv2.Start() defer srv2.Stop() - s := strings.Split(srv2.ListenAddr, ":") - if len(s) != 2 { - t.Fatal("invalid ListenAddr") - } - if port, err := strconv.Atoi(s[1]); err == nil { - srv2.localnode.Set(enr.TCP(uint16(port))) - } - if !syncAddPeer(srv1, srv2.Self()) { t.Fatal("peer not connected") } @@ -466,134 +455,6 @@ func TestServerSetupConn(t *testing.T) { } } -// TestChangeLocalNodePort tests for localnode port change in Server.natMapLoop. -func TestChangeLocalNodePort(t *testing.T) { - tests := []struct { - protocol string - request int // The port number that requested by the server. - response int // The port number that responded with mock NAT. - }{ - { - protocol: "udp", - request: 30303, - response: 30303, - }, - { - protocol: "udp", - request: 30303, - response: 30000, - }, - { - protocol: "tcp", - request: 30303, - response: 30303, - }, - { - protocol: "tcp", - request: 30303, - response: 30000, - }, - } - - srvkey := newkey() - srv := &Server{ - Config: Config{ - PrivateKey: srvkey, - MaxPeers: 0, - NoDial: true, - NoDiscovery: true, - Protocols: []Protocol{discard}, - Logger: testlog.Logger(t, log.LvlTrace), - }, - newTransport: nil, - } - - srv.quit = make(chan struct{}) - - if err := srv.setupLocalNode(); err != nil { - t.Fatal(err) - } - - for _, test := range tests { - mock := &mockNAT{uint16(test.response)} - go srv.natMapLoop(mock, test.protocol, test.request, test.request, "mock discovery", nat.DefaultMapTimeout) - time.Sleep(5 * time.Millisecond) - - close(srv.quit) - srv.quit = make(chan struct{}) - - var port int - switch test.protocol { - case "tcp": - port = srv.localnode.Node().TCP() - case "udp": - port = srv.localnode.Node().UDP() - } - if port != test.response { - t.Fatalf("couldn't change the port number on localnode: got %d want %d", port, test.response) - } - } -} - -// TestMapLoop checks if the alternate port is actually used by Server.natMapLoop. -func TestMapLoop(t *testing.T) { - srvkey := newkey() - srv := &Server{ - Config: Config{ - PrivateKey: srvkey, - MaxPeers: 0, - NoDial: true, - NoDiscovery: true, - Protocols: []Protocol{discard}, - Logger: testlog.Logger(t, log.LvlTrace), - }, - newTransport: nil, - } - - srv.quit = make(chan struct{}) - - if err := srv.setupLocalNode(); err != nil { - t.Fatal(err) - } - - mock := &mockNAT{30000} - interval := 20 * time.Millisecond - - go srv.natMapLoop(mock, "udp", 30303, 30303, "mock discovery", interval) - time.Sleep(5 * time.Millisecond) - - // Watch the interval 3 times. UDP on localnode should be 30000. - for i := 0; i < 3; i++ { - if srv.localnode.Node().UDP() != int(mock.mapped) { - t.Fatalf("natMapLoop does not make requests to alternate ports: got %d want %d", srv.localnode.Node().UDP(), int(mock.mapped)) - } - time.Sleep(interval) - - // In the last interval, the alternate port is changed once more. - if i == 2 { - mock.mapped = 50000 - } - } - close(srv.quit) -} - -type mockNAT struct { - mapped uint16 -} - -func (m *mockNAT) AddMapping(protocol string, extport, intport int, name string, lifetime time.Duration) (uint16, error) { - return m.mapped, nil -} -func (m *mockNAT) DeleteMapping(protocol string, extport, intport int) error { - return nil -} -func (m *mockNAT) ExternalIP() (net.IP, error) { - panic("not implemented") -} -func (m *mockNAT) String() string { - panic("not implemented") -} - type setupTransport struct { pubkey *ecdsa.PublicKey encHandshakeErr error From cb983db9aabc6fa130bc423322278246609fd003 Mon Sep 17 00:00:00 2001 From: dbadoy4874 Date: Fri, 23 Dec 2022 14:54:08 +0900 Subject: [PATCH 3/3] add content in flag description --- cmd/bootnode/main.go | 2 +- cmd/utils/flags.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/bootnode/main.go b/cmd/bootnode/main.go index 036b968ef83d..748113aa4841 100644 --- a/cmd/bootnode/main.go +++ b/cmd/bootnode/main.go @@ -40,7 +40,7 @@ func main() { writeAddr = flag.Bool("writeaddress", false, "write out the node's public key and quit") nodeKeyFile = flag.String("nodekey", "", "private key filename") nodeKeyHex = flag.String("nodekeyhex", "", "private key as hex (for testing)") - natdesc = flag.String("nat", "none", "port mapping mechanism (any|none|upnp|pmp|extip:)") + natdesc = flag.String("nat", "none", "port mapping mechanism (any|none|upnp|pmp|pmp:|extip:)") netrestrict = flag.String("netrestrict", "", "restrict network communication to the given IP networks (CIDR masks)") runv5 = flag.Bool("v5", false, "run a v5 topic discovery bootnode") verbosity = flag.Int("verbosity", int(log.LvlInfo), "log verbosity (0-5)") diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 5c0d984cbf16..77c5ff74ad4f 100644 --- a/cmd/utils/flags.go +++ b/cmd/utils/flags.go @@ -833,7 +833,7 @@ var ( } NATFlag = &cli.StringFlag{ Name: "nat", - Usage: "NAT port mapping mechanism (any|none|upnp|pmp|extip:)", + Usage: "NAT port mapping mechanism (any|none|upnp|pmp|pmp:|extip:)", Value: "any", Category: flags.NetworkingCategory, }