-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add QUIC Address Discovery to iroh #3049
base: main
Are you sure you want to change the base?
Conversation
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3049/docs/iroh/ Last updated: 2025-01-17T21:00:32Z |
2a51de3
to
d18f3d1
Compare
The downside of this approach is that you need to lock the What if we made a separate mapping outside of the So we use a new subnet in our existing IPv6 Unique Local Addr space. This subnet works just like the existing subnet otherwise. But is used to map real IP addresses to other fake ones. This allows the code that needs to check whether something is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way of connecting the quinn::Endpoint
into the MagicSock
is nice!
1e24436
to
4f9db39
Compare
59b2744
to
ee67d96
Compare
8b69c62
to
96aa2a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So my biggest question is about why IpMappedAddr maps to a SocketAddr rather than an Ipv6Addr. But then I also have that question about NodeIdMappedAddr which I don't think should be solved in this PR. And since the IpMappedAddr works as is, I'm happy to defer that as well if you'd prefer.
Otherwise this looks really good, though I guess we agreed that the IpMappedAddr would move into net-report for now.
iroh-base/src/ip_mapped_addrs.rs
Outdated
return Err(IpMappedAddrError(String::from("not mapped addr"))); | ||
} | ||
let octets = addr.ip().octets(); | ||
if octets[6..8] != IpMappedAddr::ADDR_SUBNET { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to check that the entire prefix matches, [0..8]
, no? Just the subnet is not enough.
With some luck you can compute the full prefix in a const fn, but I'm not sure if that's possible. You can always just write a full const PREFIX: [u8; 8] = [0xfd, 21, 7, 10, 81, 11, 0, 1];
next to the existing constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted this. I just checked each section of the slice, similar to how we fill the slice when we generate the addrs. Can you please look at this again and let me know if it's acceptable?
iroh/src/magicsock.rs
Outdated
SocketAddr::V6(addr) => { | ||
if addr.port() != MAPPED_ADDR_PORT { | ||
anyhow::bail!("not a mapped addr"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I have the same comments here as on the IpMappedAddr. But I do appreciate that this already had a dummy port in it. So let's not change that. I'll see if I can remove it reasonably in a followup PR and so I'm happy to leave this as-is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made the change already and here was my logic for why it was okay to do:
We never in the code checked the port before, that was something I added with the try_from
, so I'm comfortable taking it "away". Now we have a method NodeIdMappedAddr::socket_addr
that returns the addr w/ the MAPPED_ADDR_PORT
for when we need to use the socket address.
39b2cca
to
7177bf0
Compare
`MagicSock::spawn` creates a `quinn::Endpoint`. `MagicSock` is now an `AsyncUdpSocket`, and can be passed into the `quinn::Endpoint`. `magicsock::Handle` now owns the `quinn::Endpoint`, and `iroh::Endpoint` interacts with the `quinn::Endpoint` through `Handle::endpoint()`. This allows us to pass the `quinn::Endpoint` to the `magicsock::Actor` for use in QAD, without any circular dependencies.
also fix weird rebase errors
…o mis-handling dns resolutions
Also, `NodeIdMappedAddr` and `IpMappedAddr` `try_from` is implemented for `Ipv6Addr`
refactors `NodeIdMappedAddr` to contain a `Ipv6Addr` rather than a `SocketAddr`
2b99483
to
2c94b82
Compare
Description
There were two main things to "solve" on the iroh side:
The first was accomplished by figuring out how to make
MagicSock::spawn
be the location that builds thequinn::Endpoint
. This is what was changed:AsyncUdpSocket
onMagicSock
itself, rather thanmagicsock::Handle
magicsock::Handle
now contains thequinn::Endpoint
server_config
as aMagicSock::Option
MagicSock::spawn
:MagicSock
we now build thequinn::Endpoint
usingArc<MagicSock>
as theAsyncUdpSocket
quinn::Endpoint
and passed it to themagicsock::Actor
(which is independent ofMagicSock
)quinn::Endpoint
to themagicsock::Handle
, which is the actual struct that we use to interact with the magicsocketiroh::Endpoint
now interacts with thequinn::Endpoint
usingmsock.endpoint()
in all placesThe second was accomplished by keeping a list of special "QAD addresses" on theNodeMap
(NodeMap::qad_addrs
), and using specific methods toadd_qad_addrs
,get_qad_addrs_for_send
andget_qad_addrs_for_recv
to deal with the fickle way that thequinn::Endpoint
expects SocketAddrs to behave when it dials and when it receives packets:before we do a net-report, we first attempt to resolve theRelayUrl
s in theRelayMap
to get theSocketAddr
s that we expect we will dial when we do a QAD probeNodeMap
~on the "send" side of theAsyncUdpSocket
, after we do our normal checks for QuicMappedAddrs, we then check to see if the QuicMappedAddr is actually just a normal socket addr that we expect to use for QAD. If so, we just send the packets using theget_qad_addr_for_send
addresson the "recv" side of theAsyncUdpSocket
, after we check to see if the recv'd address can be mapped to a QuicMappedAddr & therefore can be received using our normal process, we then check to see if the address is one that we expect for QAD. If so, we make sure to associate the packet with theget_qad_addr_for_recv
address and pass it alongThe most "unreliable" bits of this are due to dns. Before running a net report, we now have to do dns discovery for all the RelayUrls. I've capped this at 300 ms but it means that until we cache the dns responses then we have this delay before starting net-report. I've also done a bit of a cheat: when we initially start themagicsock::Actor
, I've added a call toresolve_qad_addrs
that has a timeout of 10 ms, just to send out the dns packets and hopefully get a jump on caching the responses.The second was accomplished by creating a new
IpMappedAddrs
struct that keeps track ofIpMappedAddrs
to the actualSocketAddr
that it represents. This is akin to how we deal withQuicMappedAddr
(which is now calledNodeIdMappedAddr
.netreport
now takes an optionalIpMappedAddrs
, and when it resolves aRelayUrl
, it adds the resolved IP addr to theIpMappedAddrs
, and uses the mapped address when sending on QUIC. Iniroh
, we check to see if the destination or source address is anIpMappedAddr
or aNodeIdMappedAddr
. If so, it maps the addresses correctly before returning the transmit to the endpoint or sending the transmit over UDP.Most interesting bit
So...if someone adds an IP address to
IpMappedAddrs
, we can allow folks to use the Endpoint as a sort of normal quinn::Endpoint, bypassing our special iroh holepunching sauce.depends on #3032
Change checklist