chore: more concise comments

Author: gmelodie

libp2p/services/nat/plum_mapper.nim

@@ -3,18 +3,10 @@
 
 ## NAT port mapper backed by libplum (PCP / NAT-PMP / UPnP-IGD).
 ##
-## libplum owns a single process-wide client with its own internal thread that
-## discovers the gateway, selects a working protocol, and keeps mappings
-## refreshed on its own — so there is no per-mapping lease and no refresh loop
-## on our side. The C `plum_init`/`plum_cleanup` pair is a global singleton
-## (a second `plum_init` fails while a client is live), so this module
-## ref-counts init across every live `PlumMapper` and keeps the protocol filter
-## of the first mapper that initialized it.
-##
-## Precondition: every `PlumMapper` is created and closed from the same chronos
-## event-loop thread. The ref-count and active-filter globals below are plain
-## (unsynchronized) vars, so driving mappers from multiple threads would race
-## them and double-init libplum.
+## `plum_init`/`plum_cleanup` is a process-global singleton, so this module
+## ref-counts init across every live `PlumMapper` and keeps the first mapper's
+## protocol filter. The ref-count/filter globals are unsynchronized: create and
+## close every `PlumMapper` from the same chronos event-loop thread.
 
 {.push raises: [].}
 
@@ -29,8 +21,7 @@ logScope:
   topics = "libp2p natservice plum"
 
 const
-  # Module-private fallbacks for PlumMapper.new()'s own default args; the public
-  # NAT timeout knobs live in natservice (DefaultDiscoveryTimeout/MappingTimeout).
+  # Private defaults for new(); the public NAT knobs live in natservice.
   DefaultDiscoverTimeout = 10.seconds
   DefaultMappingTimeout = 10.seconds
 
@@ -40,9 +31,7 @@ type
   PlumMapper* = ref object of PortMapper
     filter: ProtocolFilter
     closed: bool
-    mappings: Table[MappingKey, cint]
-      ## (externalPort, proto) -> libplum mapping id, recorded on a successful
-      ## map() so unmap()/close() can find the opaque handle to destroy.
+    mappings: Table[MappingKey, cint] ## -> libplum mapping id, to destroy later
 
 var
   plumRefCount = 0
@@ -103,8 +92,7 @@ method map*(
     externalPort = externalPort.uint16,
   )
 
-  # close() may have run during the await above and torn libplum down; drop the
-  # mapping we just created rather than recording an id for a dead handle.
+  # close() may have torn libplum down during the await; drop the new mapping.
   if self.closed:
     destroyMapping(res.id)
     return err("PlumMapper closed")

libp2p/services/nat/portmapper.nim

@@ -11,18 +11,11 @@ type
     mpTcp
     mpUdp
 
-  MappedPort* = object
-    ## Outcome of a successful mapping: the external address the NAT device
-    ## assigned. libplum returns the external host and port together with the
-    ## mapping, so there is a single `map` call rather than the old
-    ## discover-external-IP-then-map split.
+  MappedPort* = object ## External address the NAT device assigned.
     externalIp*: IpAddress
     externalPort*: Port
 
-  PortMapper* = ref object of RootObj
-    ## Abstract base for a NAT port-mapping client (libplum-backed / mock).
-    ## All operations are async: the libplum backend hands the request to the
-    ## library's internal thread and awaits its completion.
+  PortMapper* = ref object of RootObj ## Abstract async NAT port-mapping client.
 
 method map*(
     self: PortMapper, internalPort: Port, externalPort: Port, proto: MapProto

libp2p/services/natservice.nim

@@ -249,8 +249,7 @@ proc replaceTransportPort(ma: MultiAddress, port: Port): Opt[MultiAddress] =
   Opt.some(res)
 
 proc extractListenPort(ma: MultiAddress): Opt[ListenPort] =
-  # libplum maps IPv4 only (NAT-PMP/PCP/UPnP-IGD over IPv4), so drop non-IPv4
-  # listen addresses early.
+  # libplum maps IPv4 only, so drop non-IPv4 listen addresses early.
   let ta = initTAddress(ma).valueOr:
     return Opt.none(ListenPort)
   if ta.family != AddressFamily.IPv4:
@@ -285,8 +284,7 @@ type MappedEntry =
 proc mapOnePort(
     self: NATService, lp: ListenPort
 ): Future[Opt[MappedEntry]] {.async: (raises: [CancelledError]).} =
-  # libplum hands back the external address together with the mapping, so the
-  # external IP is derived here rather than in a separate discovery step.
+  # libplum returns the external address with the mapping; no separate discovery.
   let mapped = (await self.mapper.map(lp.port, lp.port, lp.proto)).valueOr:
     warn "NAT port mapping failed", port = lp.port, proto = lp.proto, err = error
     return Opt.none(MappedEntry)
@@ -356,8 +354,7 @@ proc setupMappings*(
   announced
 
 proc validatePortMapperConfig(cfg: PortMappingConfig) {.raises: [ServiceSetupError].} =
-  # libplum keeps mappings refreshed on its own internal thread, so there is no
-  # lease or refresh interval to validate — only the discovery/mapping waits.
+  # libplum refreshes mappings itself; only the discovery/mapping waits need checking.
   if cfg.discoveryTimeout <= 0.seconds:
     raise newException(
       ServiceSetupError,

tests/libp2p/services/test_natservice.nim

@@ -25,10 +25,7 @@ type
 
   MockPortMapper = ref object of PortMapper
     extIp: IpAddress
-    extPortQueue: seq[Port]
-      ## external ports handed out in order across successive map() calls; once
-      ## exhausted, map() echoes the requested port. Models an IGD assigning (or
-      ## reassigning) a different external port than requested.
+    extPortQueue: seq[Port] ## ports handed out in order; once empty, echo request
     extPortIdx: int
     mapErr: Opt[string]
     calls: seq[MockCall]
@@ -314,8 +311,7 @@ suite "NATService":
     check mock.unmappedPorts().len == 0
 
   asyncTest "non-IPv4 private addrs are skipped":
-    # libplum maps IPv4 only, so any IPv6 listenAddr (even ULA fc00::/7) must be
-    # filtered out before map().
+    # libplum maps IPv4 only, so any IPv6 listenAddr must be filtered before map().
     let mock = newMock()
     let factory = mapperFactory(mock)
 
@@ -615,8 +611,7 @@ suite "NATService (setupMappings)":
   asyncTest "IGD returning a different external port surfaces in announced":
     let
       externalIp = parseIpAddress("203.0.113.30")
-      # queued external port simulates the IGD remapping the request to a
-      # different port (e.g. because the requested one is already busy).
+      # queued external port simulates the IGD remapping to a different port.
       mapper = newMock(extIp = externalIp, extPorts = @[Port(54321)])
       factory = mapperFactory(mapper)
       cfg = upnpConfig()

tests/libp2p/services/test_plum_mapper.nim

@@ -30,8 +30,7 @@ suite "PlumMapper":
     check r.error() == "PlumMapper closed"
 
   asyncTest "unmap without a prior map returns 'no known mapping' error":
-    # libplum identifies a mapping by an opaque id recorded on a successful
-    # map(); without that record there is nothing to destroy.
+    # no recorded mapping id, so there is nothing to destroy.
     let m = PlumMapper.new().get()
     defer:
       await m.close()
@@ -41,8 +40,7 @@ suite "PlumMapper":
     check r.error() == "plum unmap: no known mapping for external port 9000"
 
   asyncTest "a second mapper shares the ref-counted libplum instance":
-    # plum_init is a global singleton; overlapping mappers must not double-init
-    # or tear libplum down while the other is still live.
+    # overlapping mappers must not double-init or tear down the shared singleton.
     let a = PlumMapper.new().get()
     let b = PlumMapper.new().get()
     await a.close()