Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: get NodeInfos from the MagicEndpoint #1375

Closed
wants to merge 4 commits into from
Closed

Conversation

ramfox
Copy link
Contributor

@ramfox ramfox commented Aug 18, 2023

Description

Call MagicEndpoint::node_infos() to get a Vec<NodeInfo>, or list connection information on all nodes we know about.

NodeInfo is aliased from super::magicsock::EndpointInfo.

/// The type of connection we have to the node.
#[derive(Debug, Clone, Eq, PartialEq)]
pub enum ConnectionType {
    /// Direct UDP connection
    Direct(SocketAddr),
    /// Relay connection over DERP
    Relay(u16),
    /// We have no verified connection to this peer
    None,
}

/// Details about an Endpoint
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct EndpointInfo {
    /// The id in the peer_map
    pub id: usize,
    /// The public key of the endpoint.
    pub public_key: PublicKey,
    /// Derp region, if available.
    pub derp_region: Option<u16>,
    /// List of addresses at which this node might be reachable, plus any latency information we
    /// have about that address.
    pub addrs: Vec<(SocketAddr, Option<Duration>)>,
    /// The type of connection we have to the peer, either direct or over relay.
    ///
    /// Depending on the expiry time, we may still have a direct connection to the peer
    /// but our best address for the peer may have timed out.
    /// Until we verify that connection, we assume that we may only have a relay
    /// connection to that peer.
    pub conn_type: ConnectionType,
    /// The latency of the `conn_type`.
    pub latency: Option<Duration>,
}

Notes & open questions

Please let me know if the above EndpointInfo + ConnectionType is enough for our UI and sync purposes!

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

@ramfox ramfox added this to the v0.6.0-alpha: Sync! milestone Aug 18, 2023
@ramfox ramfox self-assigned this Aug 18, 2023
@b5 b5 modified the milestones: v0.6.0-alpha: Sync!, v0.6.0-alpha2 Aug 28, 2023
@ramfox ramfox changed the base branch from sync-integration to main August 29, 2023 17:26
@ramfox ramfox changed the title WIP: get peer information from the MagicSock feat: get PeerInfos from the MagicEndpoint Aug 29, 2023
@ramfox ramfox requested review from Frando and b5 August 29, 2023 17:47
@b5
Copy link
Member

b5 commented Aug 29, 2023

We've been shifting away from Peer as a term since #137 , what do you think about NodeInfo?

@ramfox ramfox marked this pull request as ready for review August 29, 2023 20:16
@ramfox ramfox changed the title feat: get PeerInfos from the MagicEndpoint feat: get NodeInfos from the MagicEndpoint Aug 29, 2023
/// Includes the node's [`PublicKey`], potential DERP region, it's addresses with any known
/// latency, and its [`crate::magicsock::ConnectionType`], which let's us know if we are currently communicating
/// with that node over a `Direct` (UDP) or `Relay` (DERP) connection.
pub async fn node_infos(&self) -> anyhow::Result<Vec<NodeInfo>> {
Copy link
Member

@Frando Frando Aug 31, 2023

Choose a reason for hiding this comment

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

nit: maybe active_connections or so? node_infos to me sounds a bit more static than what this method does. but also not sure myself, it's fine to leave it as is.
edit: I'm just now realizing that I don't actually know - Entries are added whenever you connect to a new node. But when are entries dropped from the magicsocket's state? Never? After a timeout? Let's maybe mention this in the docs if you know the answer.

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 did some reading. We do delete endpoints, but it is currently only triggered by user action. Any time we set_network_map, we remove any connections that were made to us "in the wild".

But the only method that calls set_network_map is add_known_addrs. Which... is an interesting side effect that we haven't really surfaced. I'm not sure that we intended to have every time we add a new address to the netmap, that it should clear all connections made by other nodes to us.

However, since this happens only on user action, we are likely carrying around a bunch of "inactive" connections, so until / unless we change this, I don't think active_connections is the best name.

@b5 I think ConnectionInfo might be the right name for the EndpointInfo outside of the MagicSock.

which would make this connection_infos & I will add the above context into the function docs for both connection_infos and add_known_addrs.

@dignifiedquire
Copy link
Contributor

NodeInfo is very confusing to me, I think it should be at least sth like RemoteNodeInfo, otherwise I would assume it is the local node

@b5
Copy link
Member

b5 commented Aug 31, 2023

NodeInfo is very confusing to me, I think it should be at least sth like RemoteNodeInfo, otherwise I would assume it is the local node

Fully support this change. RemoteNodeInfo is a longer, but much clearer name

@ramfox
Copy link
Contributor Author

ramfox commented Aug 31, 2023

Out of band discussion, we settled on ConnectionInfo

@ramfox
Copy link
Contributor Author

ramfox commented Aug 31, 2023

closing this PR in favor of #1435

@ramfox ramfox closed this Aug 31, 2023
@ramfox ramfox deleted the ramfox/peer_info branch November 1, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants