feat: expose known remote addrs #3752
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3752/docs/iroh/ Last updated: 2025-12-10T12:26:50Z |
| use tracing::{debug, instrument, trace, warn}; | ||
| use url::Url; | ||
|
|
||
| use self::hooks::EndpointHooksList; |
There was a problem hiding this comment.
The import ordering was off, and when I changed a line, cargo make format picked it up again. The only actual change here is that I added the re-exports from the new remote_info module.
Frando
force-pushed
the
Frando/remote-addrs
branch
2 times, most recently
from
91b4756 to
4449690
Compare
dignifiedquire
left a comment
dignifiedquire
left a comment
There was a problem hiding this comment.
given how easy it is to implement this, and how useful this is for users, I would say, lets do this
| &self.msock.metrics | ||
| } | ||
|
|
||
| /// Returns addressing information about a recently used remote endpoint. |
There was a problem hiding this comment.
maybe mention that this is a snapshot in time, or sth, to indicate that this can have already changed by the moment it is used
Frando
force-pushed
the
Frando/remote-addrs
branch
from
93612f5 to
299d985
Compare
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
As usual I'm anxious about creating any API surface and am arguing to make it smaller 😁 😅 😰
| metrics, | ||
| } | ||
| } | ||
| pub(super) fn to_remote_addrs(&self) -> Vec<TransportAddrInfo> { |
There was a problem hiding this comment.
missing blank line? (how does rustfmt not handle this?)
|
|
||
| use crate::endpoint::Source; | ||
|
|
||
| /// Information about a remote endpoint. |
There was a problem hiding this comment.
I think this was already mentioned, but this should explain that this is a snapshot at the time it was created.
| /// The address was used, but is not currently. | ||
| Inactive { | ||
| /// Time when this address was last used. | ||
| last_used: Instant, |
There was a problem hiding this comment.
I believe this is still problematic? I was hopeing to keep postponing having to figure this out to be fair.
| last_used: Instant, | ||
| }, | ||
| /// We tried to use this address, but failed. | ||
| Unusable, |
There was a problem hiding this comment.
I am not comfortable exposing this to be fair. It forces us to keep this status around and I don't think I want to do that.
I think for now I'm at most comfortable with exposing two items on this enum:
- Active
- Inactive
No timestamps, nothing else.
| pub fn most_recent_source(&self) -> &Source { | ||
| &self.most_recent_source | ||
| } |
There was a problem hiding this comment.
I'm also not comfortable exposing this yet. Source is a number of states that I don't know we'll always want to keep and want to keep around as such. Also, there is not a single source for an addr, certainly not the way it is used currently. Which makes this messy and indicates that it's not a great thing to expose.
Source is public right now, but only because of discovery methods. I had an issue originally to figure something out about that. This was solved in 50fdda3 in a way that seemed to solve the immediate issue. But I do now think it was probably the wrong approach, given the variants on that enum. I don't think this is suitable for a 1.0 API.
| .expect("poisoned") | ||
| .get(&eid) | ||
| .and_then(|handle| handle.sender.get()) | ||
| } |
There was a problem hiding this comment.
Do you think this is wroth having? The actor would be started and shut down again. I guess that's not ideal. I'm just sad whenever there are two functions like this, one that creates-on-demand and one that returns an Option. I can live with it I guess.
Description
Adds
Endpoint::remote_infowhich returnsOption<RemoteInfo>. TheRemoteInfoexposes an iterator over all transport addrs for this remote that we know about.Fixes #3521
Breaking Changes
Notes & open questions
This basically copies the
RemotePathInfofrom the RemoteStateActor into a new structRemoteInfo. So, this an async fn over the channel to the RemoteStateActor, and allocates. As this is a rather niche API, I think that's fine.I wasn't entirely sure how much info we want to expose. Right now, the PR only includes the latest
Sourcefor the address, as I wasn't sure if we really want or need to expose the full list of sources.Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme