[WIP] feat: use quic multipath #3381
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3381/docs/iroh/ Last updated: 2025-08-01T09:29:38Z |
dignifiedquire
changed the title
dignifiedquire
force-pushed
the
feat-multipath
branch
3 times, most recently
from
72cb071 to
db712c0
Compare
dignifiedquire
force-pushed
the
feat-multipath
branch
2 times, most recently
from
4827e62 to
946f71c
Compare
I'm really not that happy with info or implementation. This should be improved.
The refresh interval and time when we went to pending was about the same anyway. Instead we need this info to just be up to date. This queuing mechanism was the wrong approach.
iroh-base/src/node_addr.rs
Outdated
| )] | ||
| pub struct NodeAddr { | ||
| /// The node's identifier. | ||
| #[debug("{}", node_id.fmt_short())] |
There was a problem hiding this comment.
this should be reverted before merging
There was a problem hiding this comment.
Oh, do you think so? I found it really annoying to have to look at those full public keys when we debug-log a NodeAddr. I considered using the alternative-debug format, but unfortunately tracing only has ? and % support in their macros, for Debug and Display respectively. I didn't thyink there's any strict rule about how a Debug should look? It's not like you can eval() this back into a thing like e.g. Python.
There was a problem hiding this comment.
it reduces the ability to reproduce information, that's why I am concerend about doing it by default. you can copy and paste the nodeid, relayurl and socket addrs currently, but suddenly you can't do it for the nodeid anymore
There was a problem hiding this comment.
A sigil for alternate display in tracing would be the best solution indeed. Looked again and found tokio-rs/tracing#3258 - I now pinged the tokio discord about how to move it forward (pr is open for 6 months without comments)
| any_match = true; | ||
| match sender.send(*addr, src, transmit).await { | ||
| Ok(()) => { | ||
| trace!("sent"); |
There was a problem hiding this comment.
needs delete before merge
| any_match = true; | ||
| match sender.send(url.clone(), *node_id, transmit).await { | ||
| Ok(()) => { | ||
| trace!("sent"); |
There was a problem hiding this comment.
needs delete before merge
| self.msock | ||
| .metrics | ||
| .magicsock | ||
| .send_data |
There was a problem hiding this comment.
why is send_data increased if we call mapped_addr?
| warn!(dst = ?destination, "failed to send: {err:#}"); | ||
|
|
||
| let sender = self.msock.node_map.node_state_actor(node_id); | ||
| let transmit = OwnedTransmit::from(quinn_transmit); |
There was a problem hiding this comment.
this is very painful, it means we have to allocate each packet that we send one more time 😭
There was a problem hiding this comment.
I guess this only happens on the mixed path? which seems fine then
There was a problem hiding this comment.
Yeah, the tradeoff here is that this only happens for the Intial packet of each connection. Additionally we are not copying the data itself, because that is already a Bytes instance. So yeah, this was a consious tradeoff made with this design choice.
I guess this design choice isn't documented clearly enough yet.
| /// direction as well. | ||
| pub needs_ping_back: Option<SendPing>, | ||
| } | ||
| // TODO: use this |
There was a problem hiding this comment.
lots of TODOs in this file
| /// The key is the [`quinn::Connection::stable_id`]. | ||
| connections: FxHashMap<usize, ConnectionState>, | ||
| /// Events emitted by Quinn about path changes. | ||
| #[allow(clippy::type_complexity)] |
| .unwrap_or_else(move || SecretKey::generate(&mut rng)); | ||
|
|
||
| // Override some transport config settings. | ||
| self.transport_config |
There was a problem hiding this comment.
I wonder if we should check the config and print a warning if we change them, because this does remove the ability for users to configure some of these
4 participants
Work on integrating n0-computer/quinn#28 into the iroh magic