fix: deduplicate NAT port mapping requests

Multiple libp2p transports can share the same port (TCP, QUIC,
WebTransport, WebRTC-direct), causing duplicate AddMapping calls
for the same protocol/port combination. This fix adds deduplication
in NAT.AddMapping to prevent redundant NAT device operations and
reduce log spam.
This commit is contained in:
Marcin Rataj
2025-08-16 03:36:14 +02:00
committed by Marco Munizaga
parent 8b798777dc
commit 9afd260b32
2 changed files with 44 additions and 1 deletions

View File

@@ -163,13 +163,21 @@ func (nat *NAT) AddMapping(ctx context.Context, protocol string, port int) error
return errors.New("closed")
}
// Check if the mapping already exists to avoid duplicate work
e := entry{protocol: protocol, port: port}
if _, exists := nat.mappings[e]; exists {
return nil
}
log.Info("Starting maintenance of port mapping", "protocol", protocol, "port", port)
// do it once synchronously, so first mapping is done right away, and before exiting,
// allowing users -- in the optimistic case -- to use results right after.
extPort := nat.establishMapping(ctx, protocol, port)
// Don't validate the mapping here, we refresh the mappings based on this map.
// We can try getting a port again in case it succeeds. In the worst case,
// this is one extra LAN request every few minutes.
nat.mappings[entry{protocol: protocol, port: port}] = extPort
nat.mappings[e] = extPort
return nil
}

View File

@@ -110,6 +110,41 @@ func TestAddMappingInvalidPort(t *testing.T) {
require.False(t, found, "didn't expect a port mapping for invalid nat-ed port")
}
// TestAddMappingDeduplication tests that duplicate AddMapping calls don't trigger duplicate NAT operations.
// This is a regression test for a bug where multiple libp2p listeners sharing the same port
// (e.g., TCP, QUIC, WebTransport, WebRTC-direct all on the same port) would cause duplicate NAT
// port mapping requests - resulting in 5+ duplicate mapping attempts for the same protocol/port combination.
func TestAddMappingDeduplication(t *testing.T) {
mockNAT, reset := setupMockNAT(t)
defer reset()
mockNAT.EXPECT().GetExternalAddress().Return(net.IPv4(1, 2, 3, 4), nil)
nat, err := DiscoverNAT(context.Background())
require.NoError(t, err)
// Expect only ONE call to AddPortMapping despite multiple AddMapping calls
expectPortMappingSuccess(mockNAT, "tcp", 10000, 1234)
// First call should trigger NAT operation
require.NoError(t, nat.AddMapping(context.Background(), "tcp", 10000))
// Verify mapping was created
mapped, found := nat.GetMapping("tcp", 10000)
require.True(t, found, "expected port mapping")
addr, _ := netip.AddrFromSlice(net.IPv4(1, 2, 3, 4))
require.Equal(t, netip.AddrPortFrom(addr, 1234), mapped)
// Second and third calls should NOT trigger NAT operations (no additional expectations)
// This simulates what happens when multiple transports use the same port
require.NoError(t, nat.AddMapping(context.Background(), "tcp", 10000))
require.NoError(t, nat.AddMapping(context.Background(), "tcp", 10000))
// Mapping should still exist
mapped, found = nat.GetMapping("tcp", 10000)
require.True(t, found, "expected port mapping")
require.Equal(t, netip.AddrPortFrom(addr, 1234), mapped)
}
// TestNATRediscoveryOnConnectionError tests automatic NAT rediscovery after router restart
// to ensure mappings are restored when router's NAT service (e.g. miniupnpd) changes its listening port
// (a regression test for https://github.com/libp2p/go-libp2p/issues/3224#issuecomment-2866844723).