fix(iroh): preserve ordering of IPs for nat traversal#4090
fix(iroh): preserve ordering of IPs for nat traversal#4090dignifiedquire wants to merge 2 commits intomainfrom
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/4090/docs/iroh/ Last updated: 2026-04-08T15:44:51Z |
dignifiedquire
force-pushed
the
fix-order-ips
branch
from
57edc93 to
003f874
Compare
| impl DirectAddrType { | ||
| /// Priority for NAT traversal probing. Lower = probed first. | ||
| pub(crate) fn priority(self) -> u8 { | ||
| match self { | ||
| Self::Portmapped => 0, | ||
| Self::Qad => 1, | ||
| Self::Qad4LocalPort => 2, | ||
| Self::Local => 3, | ||
| Self::Unknown => 4, | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Looking at https://github.com/n0-computer/iroh/blob/v0.35.0/iroh/src/magicsock.rs#L2690-L2824 it doesn't seem like we used to have an order to our DirectAddrs: Although we were careful to first put Portmapper, then Stun, then Stun4LocalPort, then Local addrs into our collection, this collection was a BTreeMap<SocketAddr, DirectAddrType>, and later in send_queued_call_me_maybes we stripped the DirectAddrType from the addrs before sending them over in the order they're in the BTreeMap.
The reason I'm saying all this is because I initially wanted to cross-reference the priorities we have here with what we used to have in iroh 0.35, but it turns out although it looks like we had some order to them, we actually threw away the order when we put it all in a BTreeMap, and it worked fine, too. So the apparent order we had in iroh 0.35 should be taken with a good grain of salt.
Anyways - given all of the above, the ordering seems fine.
| for i in 0..count { | ||
| let veth = format!("veth{i}"); | ||
| let peer = format!("vpeer{i}"); | ||
| let ip = format!("10.{}.{}.1/24", 200 + i, i); |
There was a problem hiding this comment.
This doesn't match the comment for this function.
The addresses this generates are
10.200.0.110.201.1.110.202.2.1
This is a weird pattern IMO. Shouldn't this just be
| let ip = format!("10.{}.{}.1/24", 200 + i, i); | |
| let ip = format!("10.200.{i}.1/24"); |
?
(And then the function's comment needs to be adjusted to say 10.200.x.1, too)
Alternatively, this can take a u16 and you split it up, so it's 10.200.x.y, but I've heard x.x.x.0 addrs are special, so perhaps the .1 at the end is a good idea.
| let mut cmd = Command::new("ip"); | ||
| cmd.args(["link", "add", &veth, "type", "veth", "peer", "name", &peer]); | ||
| device | ||
| .spawn_command_sync(cmd)? | ||
| .wait() | ||
| .context("ip link add veth")?; | ||
| let mut cmd = Command::new("ip"); | ||
| cmd.args(["addr", "add", &ip, "dev", &veth]); | ||
| device | ||
| .spawn_command_sync(cmd)? | ||
| .wait() | ||
| .context("ip addr add")?; | ||
| let mut cmd = Command::new("ip"); | ||
| cmd.args(["link", "set", &veth, "up"]); | ||
| device | ||
| .spawn_command_sync(cmd)? | ||
| .wait() | ||
| .context("ip link set up")?; |
There was a problem hiding this comment.
This scares me.
Can we do the same thing by spawning a bunch of routers in patchbay and connecting to them via interfaces? Or even better, add a bunch of interfaces to the same router or sth like that?
|
Dogfooding indicates this is doing pretty well (I found other issues w.r.t. my mobile phone hotspot, but that's likely unrelated) |
2 participants
Depends on n0-computer/noq#571