refactor(iroh-net): Proper abstraction around best_addr
#1675
Conversation
Frando
changed the title
best_addrbest_addr
Frando
force-pushed
the
refactor-best-addr
branch
from
85e41d4
to
14e2665
Compare
Frando
force-pushed
the
refactor-best-addr
branch
from
14e2665
to
ea48b12
Compare
Frando
changed the title
best_addrbest_addr
There was a problem hiding this comment.
lgtm but should get review from @divagant-martian before merging
| trust_for = ?trust_best_addr_until.duration_since(Instant::now()), | ||
| "new best_addr (candidate address with most recent pong)" | ||
| self.best_addr.insert( | ||
| pong.from.as_socket_addr(), |
There was a problem hiding this comment.
this matches the previous code but I think this is wrong. The docs for best_addr state that it's
/// Best non-DERP path.
as well as the docs of the new function
/// (...) with the candidate udp addr (..)
but as_socket_addr maps derp addresses to fake socket addresses. In reality as_socket_addr is a bit of a footgun. Fixing this goes beyond a simple refactor so if you think this should go in another pr I'd agree with that
There was a problem hiding this comment.
Ah, true that SendAddr::as_socket_addr maps Derp addresses to fake addresses. However, in this place I think it is correct (even though not enforced on the type level), because we only use pongs and thus SendAddrs out of Endpoint::direct_addr_state, which should only ever contain pongs for direct (UDP) addresses. So I do think that this is correct here, unless I'm missing something.
I already thought of abstracing the direct addr state in a similar manner to this PR.
In any case - if the behavior is the same as it is on main, let's leave this to a followup and verify there that no fake Derp addresses can ever end up in the best_addr.
There was a problem hiding this comment.
ah yes, I fixed that in fact. Probably the reason why it's still there is because we store a pong for the derper (in the derp_region) as well
| @@ -0,0 +1,202 @@ | |||
| //! The [`BestAddr`] is the currently active best address for UDP sends. | |||
There was a problem hiding this comment.
this file is not pub but it would be good to document its contents for us
iroh-net/src/magicsock/endpoint.rs
Outdated
| @@ -153,8 +150,9 @@ impl Endpoint { | |||
|
|
|||
| /// Returns info about this endpoint | |||
| pub fn info(&self) -> EndpointInfo { | |||
| let (conn_type, latency) = if self.is_best_addr_valid(Instant::now()) { | |||
| let addr_info = self.best_addr.as_ref().expect("checked"); | |||
| // TODO: I think we should return direct here for outdated. | |||
There was a problem hiding this comment.
to me it makes sense what you say. When it's outdated we return both this address and the derp and metrics are only updated when the address is cleared. So even there, we are counting an outdated conn as a a direct one
There was a problem hiding this comment.
I changed it so that we return Direct if best_addr is set but expired.
Actually the more proper fix might be to return something like ConnectionType::Both because if the best_addr is set but expired, we send over both Derp and the Udp address. But not sure how important this is to report to the outside.
There was a problem hiding this comment.
In the most recent commit I changed this to return ConnectionType::Mixed in that case. Also put it all into a single match statement for clarity.
Description
This refactors how we use and calculate
best_addr. It should not have any functional changes, only refactoring to make things more robust for the future.Notes & open questions
Change checklist