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?
There was a problem hiding this comment.
@Frando has added something in latest patchbay asfaik
There was a problem hiding this comment.
This doesn't look that bad tbh. I mean any type of bad addr/or interface injection should do the job.
|
Dogfooding indicates this is doing pretty well (I found other issues w.r.t. my mobile phone hotspot, but that's likely unrelated) |
|
@Arqu FYI there was discussion about this issue in discord: https://discord.com/channels/949724860232392765/1491399258526580807/1491457127812960386 TLDR: We've fixed the underlying issue with having "too many interfaces" in another way that is simpler: n0-computer/noq#575 |
|
@matheus23 @Frando this tests should be ported though to make sure this actually works if we drop this PR, have you done that already? |
|
Hasn't happened yet, AFAIK. |
Thanks for the context! |
3 participants
Depends on n0-computer/noq#571