From de33af6096716081e0db05b6dd23bd6cc62ef381 Mon Sep 17 00:00:00 2001 From: Chris Ziogas Date: Thu, 1 Feb 2024 23:53:49 +0200 Subject: [PATCH 1/2] p2p: fix portMappingLoop unwanted termination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit which was causing the defer function to be called and trigger “Deleting port mapping” 2 minutes after the initial launch --- p2p/server_nat.go | 2 +- p2p/server_nat_test.go | 59 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/p2p/server_nat.go b/p2p/server_nat.go index 354597cc7a44..299d27549005 100644 --- a/p2p/server_nat.go +++ b/p2p/server_nat.go @@ -127,7 +127,7 @@ func (srv *Server) portMappingLoop() { } else if !ip.Equal(lastExtIP) { log.Debug("External IP changed", "ip", extip, "interface", srv.NAT) } else { - return + continue } // Here, we either failed to get the external IP, or it has changed. lastExtIP = ip diff --git a/p2p/server_nat_test.go b/p2p/server_nat_test.go index de935fcfc56d..264e980c1902 100644 --- a/p2p/server_nat_test.go +++ b/p2p/server_nat_test.go @@ -75,6 +75,65 @@ func TestServerPortMapping(t *testing.T) { } } +func TestExtipLoop(t *testing.T) { + clock := new(mclock.Simulated) + mockNAT := &mockNAT{mappedPort: 30000} + srv := Server{ + Config: Config{ + PrivateKey: newkey(), + NoDial: true, + ListenAddr: ":0", + NAT: mockNAT, + Logger: testlog.Logger(t, log.LvlTrace), + clock: clock, + }, + } + err := srv.Start() + if err != nil { + t.Fatal(err) + } + defer srv.Stop() + + // Wait for the port mapping to be registered. Synchronization with the port mapping + // goroutine works like this: For each iteration, we allow other goroutines to run and + // also advance the virtual clock by 1 second. Waiting stops when the NAT interface + // has received some requests, or when the clock reaches a timeout. + deadline := clock.Now().Add(portMapRefreshInterval + extipRetryInterval) + for clock.Now() < deadline && mockNAT.mapRequests.Load() < 2 { + clock.Run(1 * time.Second) + time.Sleep(10 * time.Millisecond) + } + + if mockNAT.ipRequests.Load() != 1 { + t.Fatal("external IP was never requested") + } + + reqCount := mockNAT.mapRequests.Load() + if reqCount != 2 { + t.Error("wrong request count:", reqCount) + } + + // Wait for the External IP to be checked. + for clock.Now() < deadline && mockNAT.ipRequests.Load() < 2 { + clock.Run(extipRetryInterval) + time.Sleep(10 * time.Millisecond) + } + + ipRequestsCount := mockNAT.ipRequests.Load() + if ipRequestsCount != 2 { + t.Fatal("wrong external IP request count:", ipRequestsCount) + } + + // Verify that no unmapRequests were sent. + // Handles an edge case where the port mapping loop was prematurely terminated, + // causing the defer function to be called and delete the UPnP mappings. + // ref: https://github.com/etclabscore/core-geth/pull/611 + unmapReqCount := mockNAT.unmapRequests.Load() + if unmapReqCount != 0 { + t.Fatal("wrong unmap request count:", unmapReqCount) + } +} + type mockNAT struct { mappedPort uint16 mapRequests atomic.Int32 From b2eb0c848f5b1f9f03164276a3d33a83aea95703 Mon Sep 17 00:00:00 2001 From: Chris Ziogas Date: Fri, 2 Feb 2024 08:36:43 +0200 Subject: [PATCH 2/2] remove test (fragile) --- p2p/server_nat_test.go | 59 ------------------------------------------ 1 file changed, 59 deletions(-) diff --git a/p2p/server_nat_test.go b/p2p/server_nat_test.go index 264e980c1902..de935fcfc56d 100644 --- a/p2p/server_nat_test.go +++ b/p2p/server_nat_test.go @@ -75,65 +75,6 @@ func TestServerPortMapping(t *testing.T) { } } -func TestExtipLoop(t *testing.T) { - clock := new(mclock.Simulated) - mockNAT := &mockNAT{mappedPort: 30000} - srv := Server{ - Config: Config{ - PrivateKey: newkey(), - NoDial: true, - ListenAddr: ":0", - NAT: mockNAT, - Logger: testlog.Logger(t, log.LvlTrace), - clock: clock, - }, - } - err := srv.Start() - if err != nil { - t.Fatal(err) - } - defer srv.Stop() - - // Wait for the port mapping to be registered. Synchronization with the port mapping - // goroutine works like this: For each iteration, we allow other goroutines to run and - // also advance the virtual clock by 1 second. Waiting stops when the NAT interface - // has received some requests, or when the clock reaches a timeout. - deadline := clock.Now().Add(portMapRefreshInterval + extipRetryInterval) - for clock.Now() < deadline && mockNAT.mapRequests.Load() < 2 { - clock.Run(1 * time.Second) - time.Sleep(10 * time.Millisecond) - } - - if mockNAT.ipRequests.Load() != 1 { - t.Fatal("external IP was never requested") - } - - reqCount := mockNAT.mapRequests.Load() - if reqCount != 2 { - t.Error("wrong request count:", reqCount) - } - - // Wait for the External IP to be checked. - for clock.Now() < deadline && mockNAT.ipRequests.Load() < 2 { - clock.Run(extipRetryInterval) - time.Sleep(10 * time.Millisecond) - } - - ipRequestsCount := mockNAT.ipRequests.Load() - if ipRequestsCount != 2 { - t.Fatal("wrong external IP request count:", ipRequestsCount) - } - - // Verify that no unmapRequests were sent. - // Handles an edge case where the port mapping loop was prematurely terminated, - // causing the defer function to be called and delete the UPnP mappings. - // ref: https://github.com/etclabscore/core-geth/pull/611 - unmapReqCount := mockNAT.unmapRequests.Load() - if unmapReqCount != 0 { - t.Fatal("wrong unmap request count:", unmapReqCount) - } -} - type mockNAT struct { mappedPort uint16 mapRequests atomic.Int32