Skip to content

Commit 9afd260

Browse files
lidelMarcoPolo
authored andcommitted
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.
1 parent 8b79877 commit 9afd260

File tree

2 files changed

+44
-1
lines changed

2 files changed

+44
-1
lines changed

p2p/net/nat/nat.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,13 +163,21 @@ func (nat *NAT) AddMapping(ctx context.Context, protocol string, port int) error
163163
return errors.New("closed")
164164
}
165165

166+
// Check if the mapping already exists to avoid duplicate work
167+
e := entry{protocol: protocol, port: port}
168+
if _, exists := nat.mappings[e]; exists {
169+
return nil
170+
}
171+
172+
log.Info("Starting maintenance of port mapping", "protocol", protocol, "port", port)
173+
166174
// do it once synchronously, so first mapping is done right away, and before exiting,
167175
// allowing users -- in the optimistic case -- to use results right after.
168176
extPort := nat.establishMapping(ctx, protocol, port)
169177
// Don't validate the mapping here, we refresh the mappings based on this map.
170178
// We can try getting a port again in case it succeeds. In the worst case,
171179
// this is one extra LAN request every few minutes.
172-
nat.mappings[entry{protocol: protocol, port: port}] = extPort
180+
nat.mappings[e] = extPort
173181
return nil
174182
}
175183

p2p/net/nat/nat_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,41 @@ func TestAddMappingInvalidPort(t *testing.T) {
110110
require.False(t, found, "didn't expect a port mapping for invalid nat-ed port")
111111
}
112112

113+
// TestAddMappingDeduplication tests that duplicate AddMapping calls don't trigger duplicate NAT operations.
114+
// This is a regression test for a bug where multiple libp2p listeners sharing the same port
115+
// (e.g., TCP, QUIC, WebTransport, WebRTC-direct all on the same port) would cause duplicate NAT
116+
// port mapping requests - resulting in 5+ duplicate mapping attempts for the same protocol/port combination.
117+
func TestAddMappingDeduplication(t *testing.T) {
118+
mockNAT, reset := setupMockNAT(t)
119+
defer reset()
120+
121+
mockNAT.EXPECT().GetExternalAddress().Return(net.IPv4(1, 2, 3, 4), nil)
122+
nat, err := DiscoverNAT(context.Background())
123+
require.NoError(t, err)
124+
125+
// Expect only ONE call to AddPortMapping despite multiple AddMapping calls
126+
expectPortMappingSuccess(mockNAT, "tcp", 10000, 1234)
127+
128+
// First call should trigger NAT operation
129+
require.NoError(t, nat.AddMapping(context.Background(), "tcp", 10000))
130+
131+
// Verify mapping was created
132+
mapped, found := nat.GetMapping("tcp", 10000)
133+
require.True(t, found, "expected port mapping")
134+
addr, _ := netip.AddrFromSlice(net.IPv4(1, 2, 3, 4))
135+
require.Equal(t, netip.AddrPortFrom(addr, 1234), mapped)
136+
137+
// Second and third calls should NOT trigger NAT operations (no additional expectations)
138+
// This simulates what happens when multiple transports use the same port
139+
require.NoError(t, nat.AddMapping(context.Background(), "tcp", 10000))
140+
require.NoError(t, nat.AddMapping(context.Background(), "tcp", 10000))
141+
142+
// Mapping should still exist
143+
mapped, found = nat.GetMapping("tcp", 10000)
144+
require.True(t, found, "expected port mapping")
145+
require.Equal(t, netip.AddrPortFrom(addr, 1234), mapped)
146+
}
147+
113148
// TestNATRediscoveryOnConnectionError tests automatic NAT rediscovery after router restart
114149
// to ensure mappings are restored when router's NAT service (e.g. miniupnpd) changes its listening port
115150
// (a regression test for https://github.com/libp2p/go-libp2p/issues/3224#issuecomment-2866844723).

0 commit comments

Comments
 (0)