feat: introduce rtt_hints for opening paths #278
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/quinn/pr/278/docs/iroh_quinn/ Last updated: 2025-12-17T16:38:43Z |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
==========================================
- Coverage 76.69% 76.65% -0.04%
==========================================
Files 83 83
Lines 23241 23271 +30
==========================================
+ Hits 17824 17838 +14
- Misses 5417 5433 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let pto = match rtt_hint { | ||
| Some(rtt_hint) => rtt_hint, | ||
| None => { | ||
| // Fallback to the pessimistic calculation | ||
| // TODO: we could consider using known PTOs in this case | ||
| self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base() | ||
| } | ||
| }; |
There was a problem hiding this comment.
| let pto = match rtt_hint { | |
| Some(rtt_hint) => rtt_hint, | |
| None => { | |
| // Fallback to the pessimistic calculation | |
| // TODO: we could consider using known PTOs in this case | |
| self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base() | |
| } | |
| }; | |
| if let Some(rtt_hint) = rtt_hint { | |
| data.rtt = RttEstimator::new(rtt_hint); | |
| } | |
| let pto = self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base(); |
That seems more correct to me...
We don't want to only change the PathOpen timer, we also want to change the time at which we resend path challenges, and change when we consider packets lost, etc.
Also, the PTO would actually be a different value in this case, because we make assumptions about variance, and add the max ACK delay. It surprises me that all of that would be skipped in your version.
| pub fn initiate_nat_traversal_round<F>( | ||
| &mut self, | ||
| now: Instant, | ||
| ) -> Result<Vec<SocketAddr>, iroh_hp::Error> { | ||
| rtt_hints: F, | ||
| ) -> Result<Vec<SocketAddr>, iroh_hp::Error> | ||
| where | ||
| F: Fn(&SocketAddr) -> Option<Duration>, | ||
| { |
There was a problem hiding this comment.
This gives you a lot of control in theory. But I'd have preferred to keep it simple with only a single RTT hint for all probes. This RTT would then be set to the worst RTT you expect to get. E.g. when we haven't hole-punched an IP path yet, then this should be set to the relay path's RTT, and if we have some IP paths already, then this should be set to the worst IP path's RTT.
There was a problem hiding this comment.
a single value seems bad imho, making it a lot less useful, as we might be using multiple transports between different probes
flub
added
holepunching
multipath
No description provided.