Skip to content

Conversation

@dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Apr 28, 2025

A first step towards the work of #3276

Breaking changes

  • iroh::watcher is now its own crate n0-watcher
  • iroh::endpoint::Endpoint::node_addr now returns impl Watcher<Value = Option<NodeAddr>>
  • iroh::endpoint::Endpoint::home_relay now returns impl Watcher<Value = Vec<RelayUrl>>
  • iroh::endpoint::Endpoint::bound_sockets now returns Vec<SocketAddr>
  • iroh-quinn is updated to 0.14.0

Depends on

@n0bot n0bot bot added this to iroh Apr 28, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Apr 28, 2025
@Frando
Copy link
Member

Frando commented May 5, 2025

(Note: my "merged main into refactor-transports" commit was a pushed-the-button-by-accident, didn't actually want to do that at that point in time)

@github-actions
Copy link

github-actions bot commented May 8, 2025

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3279/docs/iroh/

Last updated: 2025-06-11T13:34:43Z

Cargo.lock Outdated
checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb"
dependencies = [
"windows-sys 0.59.0",
"windows-sys 0.48.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea, this changes back in my other PR, cargo seems to resolve this differently in some cases 🤷

.expect("disconnected")
.into_iter()
.filter_map(|addr| SocketAddr::try_from(addr).ok())
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this could be written without allocating to a Vec with a bit more gymnastics... Anyway, I don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +402 to +403
for sender in &self.ip {
if sender.is_valid_send_addr(addr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is like super-nitpicky. But I noticed every time when I review that that I have to double-check these lines to figure out what is going on. In a way something like for sender in self.ip.iter().filter(|ip| ip.is_valid_send_addr(addr)) would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually find that much harder to read, because the for loop gets split over multiple lines


match destination {
#[cfg(wasm_browser)]
Addr::Ip(..) => return Err(io::Error::other("IP is unsupported in browser")),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if ErrorKind::AddrNotAvailable would be suitable for these errors. Though making it an Other probably makes it clearer where this is coming from, so perhaps this is better anyway.

metrics: Arc<MagicsockMetrics>,
) -> Self {
// Currently gets updated on manual rebind
// TODO: update when UdpSocket under the hood rebinds automatically
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how important is this todo? probably fairly important for ios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's important todo eventually, but the world doesn't end if it doesn't make it in this PR

pub(crate) fn new(config: RelayActorConfig) -> Self {
let (relay_datagram_send_tx, relay_datagram_send_rx) = mpsc::channel(256);

let (relay_datagram_recv_tx, relay_datagram_recv_rx) = mpsc::channel(512);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you're probably copying these values... but how did we end up with the recv queue being twice as long as the send queue? Anyway, something to clean up some other time.

}
};

buf_out[..dm.buf.len()].copy_from_slice(&dm.buf);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long, long in the future I can now see us polling the TCP socket from the relay server directly during this poll, allowing us to avoid this copy. Especially once we don't have any non-quic messages left since then only very few times we'll need to send a non-QUIC message over the relay protocol that should not be passed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one can dream

@dignifiedquire dignifiedquire enabled auto-merge June 11, 2025 13:33
@dignifiedquire dignifiedquire added this pull request to the merge queue Jun 11, 2025
Merged via the queue into main with commit d915bfd Jun 11, 2025
29 of 30 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in iroh Jun 11, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jun 24, 2025
## Description 

- Use more `Watchable` in  more places
- Drops STUN, ICMP and hairpin probes, they are not actually needed in
our world
- restructure report generation
- Keep QAD connections open, instead of recreating them on every report
- Removes `STUN` functionality from the relay server

**Fixes**

- #3315


## Breaking Changes

- removes `iroh_relay::protos::stun::StunError`
- removes `iroh_relay::server::testing::stun_config`
- removes `iroh_relay::protos::stun`
- removes `iroh_relay::quic::QuicClient::get_addr_and_latency`
- removes `DEFAULT_STUN_PORT`
 


## Depends on 

- [x] #3279 
- [x] n0-computer/net-tools#27

---------

Co-authored-by: Kasey <[email protected]>
@ramfox ramfox deleted the refactor-transports branch June 25, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

5 participants