refactor(nat): reimplement NAT Traversal#110
Conversation
markspanbroek
force-pushed
the
pmp-gateway
branch
from
d7d5acc to
f3108c9
Compare
markspanbroek
force-pushed
the
refactor-nat
branch
from
c408d6b to
01e0133
Compare
- explicit start and stop of port mapping renewals - updates DHT when external port numbers change - map to any external port if specified port doesn't work - use taskpool instead of threadvars
1 thread does not work apparently
Announce port is configured as port 0, so we should take the actual port from libp2p, not the port from the config
reason: the node now updates the DHT with external addresses only, so for integration testing we set an external ip address of 127.0.0.1 manually
markspanbroek
force-pushed
the
refactor-nat
branch
from
01e0133 to
b44d6a9
Compare
dryajov
left a comment
dryajov
left a comment
There was a problem hiding this comment.
Overall, this is a great rewrite, just a few nitpicks, the biggest issue is the thread cancelation handling path.
| var mapped: ?!MultiAddress | ||
| trace "spawning port mapping background task", address | ||
| traversal.taskpool.spawn mapTask(portmapping, address, signal, addr mapped) | ||
| if error =? catchAsync(await signal.wait()).errorOption: |
There was a problem hiding this comment.
This doesn't handle cross-thread cancellations correctly on it's own. Cancelling without waiting for the thread to finish, will lead to all sorts of issue with writing to potentially released memory, etc... Look at how we're handling this in erasure and the kvstore, use a .join to wrap the signal future and avoid cancelling it, wait for the tread to finish, and the propagate the cancellation.
| maintenance = | ||
| BlockMaintainer.new(repoStore, interval = config.overlayMaintenanceInterval) | ||
|
|
||
| natTraversal = !NatTraversal.new(config.nat, config.natRenewal, tp) |
There was a problem hiding this comment.
nat should be best effort, we should not crash the node on nat failure to start.
| s.archivistNode.discovery.updateDhtRecord(discoveryAddrs) | ||
| let announceAddresses = s.archivistNode.switch.peerInfo.addrs | ||
| let discoveryPort = s.config.discoveryPort | ||
| let discoveryAddress = MultiAddress.init(IPv4_any(), udpProtocol, discoveryPort) |
There was a problem hiding this comment.
why are we hardcoding the address family here? This should be derived from the mapped address, not hardcoded. Also, wouldn't 0.0.0.0, mess with the nat:none path, as it is right now?
| of NatStrategy.ExternalIp: | ||
| mapping.mapExternalIp(address) | ||
| of NatStrategy.None: | ||
| mapping.mapRoutingTable(address) |
There was a problem hiding this comment.
I think this unconditionally maps private addresses to public routes, even with nat:none? I would put this behind a explicit flag, and reject otherwise. I would not be very happy to find out that I just got exposed to the public internet, when I'm explicitely binding to a lan address and setting nat to none.
| traversal.renewing = traversal.renew() | ||
| debug "started nat traversal" | ||
|
|
||
| proc stop*(traversal: NatTraversal) {.async: (raises: []).} = |
There was a problem hiding this comment.
are we dropping the mappings on stop/shutdown? otherwise they'll just leave stale mapping entries.
| import pkg/questionable/results | ||
|
|
||
| proc discoverGateways*(upnp: Miniupnp, timeout: Duration): ?!void = | ||
| upnp.discoverDelay = timeout.seconds.cint |
There was a problem hiding this comment.
this takes ms, you're passing seconds?
2 participants
Depends on #102