From c3002824372365a6a3189357cc57a56c267b31b0 Mon Sep 17 00:00:00 2001 From: Arnaud Date: Wed, 17 Jun 2026 18:11:44 +0400 Subject: [PATCH] Remove port mapper: PCP on another port is a limitation --- storage/nat.nim | 28 -------------- storage/storage.nim | 2 - tests/storage/testnatdetection.nim | 59 ------------------------------ tests/storage/testnatreaction.nim | 42 --------------------- 4 files changed, 131 deletions(-) diff --git a/storage/nat.nim b/storage/nat.nim index 02b54a34..22f99ef9 100644 --- a/storage/nat.nim +++ b/storage/nat.nim @@ -151,34 +151,6 @@ method hasMappingIds*(m: NatPortMapper): bool {.base, gcsafe.} = # (use hasMapping() for liveness check). m.tcpMappingId.isSome and m.udpMappingId.isSome -proc setupMappedAddrMapper*(switch: Switch, natMapper: NatPortMapper) = - ## We define a custom mapper that adds the externally-mapped address to - ## peerInfo.addrs when a port mapping is active, so AutoNAT tests that port. - ## PCP/NAT-PMP may grant an external port different from the listen port. - let mapper: AddressMapper = proc( - addrs: seq[MultiAddress] - ): Future[seq[MultiAddress]] {.gcsafe, async: (raises: [CancelledError]).} = - result = addrs - - if natMapper.activeTcpPort.isNone: - return result - - let mappedPort = natMapper.activeTcpPort.get - for listenAddr in switch.peerInfo.listenAddrs: - # Dialable IP (observed public, or the listen IP if already public) - # used with the mapped port. - let mappedAddr = switch.peerStore.guessDialableAddr(listenAddr).remapAddr( - port = some(mappedPort) - ) - if mappedAddr.isPublicMA(): - # Insert first so AutoNAT dials it before the listen-port candidate (the - # server tests only the first dialable address). - result.insert(mappedAddr, 0) - - return result.deduplicate() - - switch.peerInfo.addressMappers.add(mapper) - method handleNatStatus*( m: NatPortMapper, networkReachability: NetworkReachability, diff --git a/storage/storage.nim b/storage/storage.nim index 4088dcb8..e3b87704 100644 --- a/storage/storage.nim +++ b/storage/storage.nim @@ -469,8 +469,6 @@ proc new*( ) ) - setupMappedAddrMapper(switch, natMapper.get) - autonatService.get.setStatusAndConfidenceHandler( proc( networkReachability: NetworkReachability, diff --git a/tests/storage/testnatdetection.nim b/tests/storage/testnatdetection.nim index c4b75f6e..c58b10d8 100644 --- a/tests/storage/testnatdetection.nim +++ b/tests/storage/testnatdetection.nim @@ -256,62 +256,3 @@ asyncchecksuite "NAT detection - dial request candidates": await autonat.stop(sw) await sw2.stop() - - test "after a port mapping, the mapped address is AutoNAT's first dial candidate": - # Temporary skipped because it might not work if PCP creates a mapping on a different port - return - - let mapper = MockMappingNatPortMapper() - - setupMappedAddrMapper(sw, mapper) - - # Reach the observation quorum so guessDialableAddr trusts 8.8.8.8 - let observed = MultiAddress.init("/ip4/8.8.8.8/tcp/4001").expect("valid") - let quorum = 3 - for _ in 0 ..< quorum: - discard sw.peerStore.identify.observedAddrManager.addObservation(observed) - - # Setup AutoRelayService - let relay = AutoRelayService.new( - 1, relayClientModule.RelayClient.new(), nil, Rng.instance().libp2pRng - ) - autorelayservice.setup(relay, sw) - - # Define our handleNatStatus callback - let disc = Discovery.new( - PrivateKey.random(Rng.instance().libp2pRng).get(), announceAddrs = @[] - ) - let dialBack = MultiAddress.init("/ip4/8.8.8.8/tcp/8080").expect("valid") - await mapper.handleNatStatus( - NotReachable, Opt.some(dialBack), discoveryPort, disc, sw, relay - ) - - # Define our AutonatV2Service - let mockClient = MockAutonatV2Client() - let autonat = AutonatV2Service.new( - Rng.instance().libp2pRng, - mockClient, - AutonatV2ServiceConfig.new( - enableDialableCandidates = true, maxQueueSize = 1, minConfidence = 0.5 - ), - ) - service.setup(autonat, sw) - await autonat.start(sw) - - # Connect to a second switch to test NAT detection - let sw2 = newStandardSwitch() - await sw2.start() - await sw.connect(sw2.peerInfo.peerId, sw2.peerInfo.addrs) - - # The expected mapped address should be the guessDialableAddr (8.8.8.8) - # using the mapping mocked port (40000) because a mapping was created. - let mapped = - MultiAddress.init("/ip4/8.8.8.8/tcp/" & $mockMappedTcpPort).expect("valid") - check eventually(mapped in mockClient.reqAddrs) - # Ensute that it comes first (because AutonatV2 test only the first candidate) - check mockClient.reqAddrs[0] == mapped - - await autonat.stop(sw) - await sw2.stop() - if relay.isRunning: - await relay.stop(sw) diff --git a/tests/storage/testnatreaction.nim b/tests/storage/testnatreaction.nim index 4e229f6e..76887506 100644 --- a/tests/storage/testnatreaction.nim +++ b/tests/storage/testnatreaction.nim @@ -215,39 +215,6 @@ asyncchecksuite "NAT reaction - address announcing": check disc.announceAddrs == newSeq[MultiAddress]() - test "mapped-addr mapper injects the mapped port as the first candidate": - const mockMappedTcpPort = 40000 - - setupMappedAddrMapper( - sw, NatPortMapper(activeTcpPort: some(Port(mockMappedTcpPort))) - ) - - # Reach the observation quorum so guessDialableAddr trusts 8.8.8.8 - let observed = MultiAddress.init("/ip4/8.8.8.8/tcp/4001").expect("valid") - let quorum = 3 - for _ in 0 ..< quorum: - discard sw.peerStore.identify.observedAddrManager.addObservation(observed) - - await sw.peerInfo.update() - - # Ensure that the address mapper injects the mapped port as the first candidate - # after peer info update - check sw.peerInfo.addrs[0] == - MultiAddress.init("/ip4/8.8.8.8/tcp/" & $mockMappedTcpPort).expect("valid") - - test "mapped-addr mapper is a no-op without an active mapping": - setupMappedAddrMapper(sw, NatPortMapper()) - - let observed = MultiAddress.init("/ip4/8.8.8.8/tcp/4001").expect("valid") - let quorum = 3 - for _ in 0 ..< quorum: - discard sw.peerStore.identify.observedAddrManager.addObservation(observed) - - await sw.peerInfo.update() - - # Ensure that nothing is injected because there is no active mapping - check sw.peerInfo.addrs == sw.peerInfo.listenAddrs - test "handleNatStatus clears the DHT routing addresses when it becomes NotReachable": let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid") let mapper = MockNatPortMapper(mappedPorts: none((Port, Port, MappingProtocol))) @@ -266,15 +233,6 @@ asyncchecksuite "NAT reaction - address announcing": ) check disc.dhtAddrs.len == 0 - test "mapped-addr mapper does not inject a non-public mapped address": - # Active mapping, but no public observed address: the candidate stays private - # and must not be injected. - setupMappedAddrMapper(sw, NatPortMapper(activeTcpPort: some(Port(40000)))) - - await sw.peerInfo.update() - - check sw.peerInfo.addrs == sw.peerInfo.listenAddrs - test "announceRelayReservation announces only the publicly dialable circuit address": disc.announceRelayReservation( @[circuitAddr("127.0.0.1"), circuitAddr("204.168.234.45")]