diff --git a/storage/nat.nim b/storage/nat.nim index 11aa4961..a7e76389 100644 --- a/storage/nat.nim +++ b/storage/nat.nim @@ -133,38 +133,6 @@ method hasMappingIds*(m: NatPortMapper): bool {.base, gcsafe.} = # (use hasMapping() for liveness check). m.tcpMappingId.isSome and m.udpMappingId.isSome -proc announcePeerInfoAddrs*(discovery: Discovery, peerInfo: PeerInfo, udpPort: Port) = - ## Announces peerInfo.addrs to the DHT, excluding relay circuit addresses: - ## they are announced via onReservation and must not enter the DHT routing - ## record. No-op when the addresses are already announced, so peerInfo - ## updates that only touch filtered-out addresses do not re-announce. - let addrs = peerInfo.addrs.filterIt(not it.isCircuitRelayMA()).deduplicate() - if addrs.len == 0 or addrs == discovery.announceAddrs: - return - discovery.announceDirectAddrs(addrs, udpPort = udpPort) - -proc setupPeerInfoObserver*( - switch: Switch, - autonat: AutonatV2Service, - discovery: Discovery, - natMapper: NatPortMapper, -): PeerInfoObserver = - ## AutoNAT's address mapper resolves peerInfo.addrs into public addresses - ## once the node is Reachable; peerInfo.update() then notifies observers. - ## Keep the DHT records in sync with what libp2p announces. - let observer: PeerInfoObserver = proc(peerInfo: PeerInfo) {.gcsafe, raises: [].} = - info "PeerInfo updated", - addrs = peerInfo.addrs, reachability = autonat.networkReachability - if autonat.networkReachability != NetworkReachability.Reachable: - return - # When a NAT mapping is active, announce its external UDP port: the router - # may have assigned a different port than the requested discoveryPort. - let udpPort = natMapper.activeUdpPort.get(natMapper.discoveryPort) - announcePeerInfoAddrs(discovery, peerInfo, udpPort) - - switch.peerInfo.addObserver(observer) - observer - proc setupMappedAddrMapper*(switch: Switch, natMapper: NatPortMapper) = ## We define a custom mapper that adds the external port to peerInfo.addrs when ## a port mapping is active, so AutoNAT tests that port. @@ -213,10 +181,12 @@ method handleNatStatus*( await autoRelayService.stop(switch) debug "AutoRelayService stopped" - # No announce here: AutoNAT refreshes peerInfo right after this handler, - # its address mapper (active now that the node is Reachable) resolves the - # public addresses and the peerInfo observer announces them. discovery.protocol.clientMode = false + + if dialBackAddr.isSome: + discovery.announceDirectAddrs( + @[dialBackAddr.get], udpPort = m.activeUdpPort.get(discoveryPort) + ) of NotReachable: var hasPortMapping = false diff --git a/storage/storage.nim b/storage/storage.nim index 8b623de5..a57705ea 100644 --- a/storage/storage.nim +++ b/storage/storage.nim @@ -67,7 +67,6 @@ type autoRelayService*: Option[AutoRelayService] natMapper*: Option[NatPortMapper] holePunchHandler: Option[connmanager.PeerEventHandler] - peerInfoObserver: Option[PeerInfoObserver] bootstrapNodes: seq[SignedPeerRecord] isStarted: bool @@ -170,9 +169,6 @@ proc stop*(s: StorageServer) {.async.} = s.holePunchHandler.get, PeerEventKind.Joined ) - if s.peerInfoObserver.isSome: - s.storageNode.switch.peerInfo.removeObserver(s.peerInfoObserver.get) - var futures = @[ s.storageNode.switch.stop(), s.storageNode.stop(), @@ -451,7 +447,6 @@ proc new*( var natMapper: Option[NatPortMapper] var autoRelayService: Option[AutoRelayService] var holePunchHandler: Option[connmanager.PeerEventHandler] - var peerInfoObserver: Option[PeerInfoObserver] if autonatService.isSome: let relayService = AutoRelayService.new( @@ -487,9 +482,6 @@ proc new*( ) ) - peerInfoObserver = - some(setupPeerInfoObserver(switch, autonatService.get, discovery, natMapper.get)) - setupMappedAddrMapper(switch, natMapper.get) autonatService.get.setStatusAndConfidenceHandler( @@ -535,6 +527,5 @@ proc new*( autoRelayService: autoRelayService, natMapper: natMapper, holePunchHandler: holePunchHandler, - peerInfoObserver: peerInfoObserver, bootstrapNodes: bootstrapNodes, ) diff --git a/tests/storage/testnatreaction.nim b/tests/storage/testnatreaction.nim index 468a3267..fe265285 100644 --- a/tests/storage/testnatreaction.nim +++ b/tests/storage/testnatreaction.nim @@ -2,7 +2,6 @@ import std/[net] import pkg/chronos import pkg/libp2p/[multiaddress, multihash, multicodec] import pkg/libp2p/protocols/connectivity/autonat/types -import pkg/libp2p/protocols/connectivity/autonatv2/service except setup import pkg/libp2p/protocols/connectivity/relay/client as relayClientModule import pkg/libp2p/services/autorelayservice except setup import pkg/libp2p/observedaddrmanager @@ -53,7 +52,7 @@ asyncchecksuite "NAT reaction - port mapping": let discoveryPort = Port(8090) - test "handleNatStatus announces mapped address when NotReachable and UPnP succeeds": + test "handleNatStatus keeps relay off when NotReachable and mapping succeeds": let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/8080").expect("valid") let mapper = MockNatPortMapper( mappedPorts: some((Port(9000), Port(9001), MappingProtocol.UPnP)) @@ -64,8 +63,8 @@ asyncchecksuite "NAT reaction - port mapping": NotReachable, Opt.some(dialBack), discoveryPort, disc, sw, autoRelay ) - check disc.announceAddrs == - @[MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid")] + # A mapping doesn't guarantee reachability, so the relay stays off until + # AutoNAT confirms Reachable. check not autoRelay.isRunning check disc.protocol.clientMode @@ -149,8 +148,12 @@ asyncchecksuite "NAT reaction - address announcing": var sw: Switch var key: PrivateKey var disc: Discovery + var autoRelay: AutoRelayService setup: + autoRelay = AutoRelayService.new( + 1, relayClientModule.RelayClient.new(), nil, Rng.instance().libp2pRng + ) key = PrivateKey.random(Rng.instance().libp2pRng).get() disc = Discovery.new(key, announceAddrs = @[]) sw = newStandardSwitch() @@ -158,70 +161,40 @@ asyncchecksuite "NAT reaction - address announcing": teardown: await sw.stop() + if autoRelay.isRunning: + await autoRelay.stop(sw) let discoveryPort = Port(8090) - test "announcePeerInfoAddrs excludes relay circuit addresses": - let circuitAddr = MultiAddress - .init("/ip4/1.2.3.4/tcp/4040/p2p/" & $sw.peerInfo.peerId & "/p2p-circuit") - .expect("valid") - sw.peerInfo.addrs.add(circuitAddr) + test "handleNatStatus announces the dial-back address when Reachable": + let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid") - announcePeerInfoAddrs(disc, sw.peerInfo, discoveryPort) - - check circuitAddr notin disc.announceAddrs - check disc.announceAddrs == sw.peerInfo.addrs.filterIt(it != circuitAddr) - - test "announcePeerInfoAddrs does nothing when addresses are already announced": - announcePeerInfoAddrs(disc, sw.peerInfo, discoveryPort) - let seqNo = disc.getSpr().data.seqNo - - announcePeerInfoAddrs(disc, sw.peerInfo, discoveryPort) - - check disc.getSpr().data.seqNo == seqNo - - test "peerInfo observer announces addresses when Reachable": - let autonat = AutonatV2Service.new(Rng.instance().libp2pRng) - discard setupPeerInfoObserver( - sw, autonat, disc, NatPortMapper(discoveryPort: discoveryPort) + let mapper = NatPortMapper(discoveryPort: discoveryPort) + await mapper.handleNatStatus( + Reachable, Opt.some(dialBack), discoveryPort, disc, sw, autoRelay ) - autonat.networkReachability = Reachable - sw.peerInfo.listenAddrs.add( - MultiAddress.init("/ip4/1.2.3.4/tcp/9999").expect("valid") - ) - await sw.peerInfo.update() + check disc.announceAddrs == @[dialBack] - check disc.announceAddrs == sw.peerInfo.addrs + test "handleNatStatus announces the mapped external UDP port when a mapping is active": + let dialBack = MultiAddress.init("/ip4/1.2.3.4/tcp/9000").expect("valid") - test "peerInfo observer announces the mapped external UDP port when a mapping is active": - let autonat = AutonatV2Service.new(Rng.instance().libp2pRng) let mapper = NatPortMapper(discoveryPort: discoveryPort, activeUdpPort: some(Port(40001))) - discard setupPeerInfoObserver(sw, autonat, disc, mapper) - autonat.networkReachability = Reachable - - sw.peerInfo.listenAddrs.add( - MultiAddress.init("/ip4/1.2.3.4/tcp/9999").expect("valid") + await mapper.handleNatStatus( + Reachable, Opt.some(dialBack), discoveryPort, disc, sw, autoRelay ) - await sw.peerInfo.update() let sprAddrs = disc.getSpr().data.addresses.mapIt(it.address) check MultiAddress.init("/ip4/1.2.3.4/udp/40001").expect("valid") in sprAddrs check MultiAddress.init("/ip4/1.2.3.4/udp/" & $discoveryPort).expect("valid") notin sprAddrs - test "peerInfo observer does not announce when the node is not Reachable": - let autonat = AutonatV2Service.new(Rng.instance().libp2pRng) - discard setupPeerInfoObserver( - sw, autonat, disc, NatPortMapper(discoveryPort: discoveryPort) + test "handleNatStatus does not announce when Reachable without a dial-back address": + let mapper = NatPortMapper(discoveryPort: discoveryPort) + await mapper.handleNatStatus( + Reachable, Opt.none(MultiAddress), discoveryPort, disc, sw, autoRelay ) - autonat.networkReachability = NotReachable - - sw.peerInfo.listenAddrs.add( - MultiAddress.init("/ip4/1.2.3.4/tcp/9999").expect("valid") - ) - await sw.peerInfo.update() check disc.announceAddrs == newSeq[MultiAddress]()