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(iroh-net)!: Allow using a NodeId directly in connect. #2774

Merged
merged 6 commits into from
Oct 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions iroh-net/examples/dht_discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,7 @@ async fn chat_client(args: Args) -> anyhow::Result<()> {
.bind()
.await?;
println!("We are {} and connecting to {}", node_id, remote_node_id);
let connection = endpoint
.connect_by_node_id(remote_node_id, CHAT_ALPN)
.await?;
let connection = endpoint.connect(remote_node_id, CHAT_ALPN).await?;
println!("connected to {}", remote_node_id);
let (mut writer, mut reader) = connection.open_bi().await?;
let _copy_to_stdout =
Expand Down
2 changes: 1 addition & 1 deletion iroh-net/src/dialer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl Dialer {
let res = tokio::select! {
biased;
_ = cancel.cancelled() => Err(anyhow!("Cancelled")),
res = endpoint.connect_by_node_id(node_id, alpn) => res
res = endpoint.connect(node_id, alpn) => res
};
(node_id, res)
});
Expand Down
19 changes: 10 additions & 9 deletions iroh-net/src/discovery.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
//! Node address discovery.
//!
//! To connect to an iroh-net node a [`NodeAddr`] is needed, which needs to contain either a
//! [`RelayUrl`] or one or more *direct addresses*. However it is often more desirable to
//! be able to connect with only the [`NodeId`], as [`Endpoint::connect_by_node_id`] does.
//! To connect to an iroh-net node a [`NodeAddr`] is needed, which may contain a
//! [`RelayUrl`] or one or more *direct addresses* in addition to the [`NodeId`].
//!
//! For connecting by [`NodeId`] to work however, the endpoint has to get the addressing
//! information by other means. This can be done by manually calling
//! [`Endpoint::add_node_addr`], but that still requires knowing the other addressing
//! information.
//! Since there is a conversion from [`NodeId`] to [`NodeAddr`], you can also use
//! connect directly with a [`NodeId`].
//!
//! For this to work however, the endpoint has to get the addressing information by
//! other means. This can be done by manually calling [`Endpoint::add_node_addr`],
//! but that still requires knowing the other addressing information.
//!
//! Node discovery is an automated system for an [`Endpoint`] to retrieve this addressing
//! information. Each iroh-net node will automatically publish their own addressing
Expand Down Expand Up @@ -761,7 +762,7 @@ mod test_dns_pkarr {
.await?;

// we connect only by node id!
let res = ep2.connect(ep1.node_id().into(), TEST_ALPN).await;
let res = ep2.connect(ep1.node_id(), TEST_ALPN).await;
assert!(res.is_ok(), "connection established");
Ok(())
}
Expand All @@ -782,7 +783,7 @@ mod test_dns_pkarr {
.await?;

// we connect only by node id!
let res = ep2.connect(ep1.node_id().into(), TEST_ALPN).await;
let res = ep2.connect(ep1.node_id(), TEST_ALPN).await;
assert!(res.is_ok(), "connection established");
Ok(())
}
Expand Down
24 changes: 18 additions & 6 deletions iroh-net/src/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,10 +432,12 @@ impl Endpoint {

/// Connects to a remote [`Endpoint`].
///
/// A [`NodeAddr`] is required. It must contain the [`NodeId`] to dial and may also
/// contain a [`RelayUrl`] and direct addresses. If direct addresses are provided, they
/// will be used to try and establish a direct connection without involving a relay
/// server.
/// A value that can be converted into a [`NodeAddr`] is required. This can be either a
/// [`NodeAddr`], a [`NodeId`] or a [`iroh_base::ticket::NodeTicket`].
///
/// The [`NodeAddr`] must contain the [`NodeId`] to dial and may also contain a [`RelayUrl`]
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use dial in any of the API names of iroh-net::Endpoint so I prefer not to use that in documentation either as that is probably confusing to users. I usually stick to "connect", "establish a connection" or something like that.

(yeah, there's the somewhat weird dialer module which is a bit of an odd duck...)

/// and direct addresses. If direct addresses are provided, they will be used to try and
/// establish a direct connection without involving a relay server.
///
/// If neither a [`RelayUrl`] or direct addresses are configured in the [`NodeAddr`] it
/// may still be possible a connection can be established. This depends on other calls
Expand All @@ -450,8 +452,14 @@ impl Endpoint {
/// The `alpn`, or application-level protocol identifier, is also required. The remote
/// endpoint must support this `alpn`, otherwise the connection attempt will fail with
/// an error.
#[instrument(skip_all, fields(me = %self.node_id().fmt_short(), remote = %node_addr.node_id.fmt_short(), alpn = ?String::from_utf8_lossy(alpn)))]
pub async fn connect(&self, node_addr: NodeAddr, alpn: &[u8]) -> Result<quinn::Connection> {
#[instrument(skip_all, fields(me = %self.node_id().fmt_short(), alpn = ?String::from_utf8_lossy(alpn)))]
rklaehn marked this conversation as resolved.
Show resolved Hide resolved
pub async fn connect(
rklaehn marked this conversation as resolved.
Show resolved Hide resolved
&self,
node_addr: impl Into<NodeAddr>,
alpn: &[u8],
) -> Result<quinn::Connection> {
let node_addr = node_addr.into();
tracing::Span::current().record("remote", node_addr.node_id.fmt_short());
Copy link
Contributor

Choose a reason for hiding this comment

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

that's cool! TIL

// Connecting to ourselves is not supported.
if node_addr.node_id == self.node_id() {
bail!(
Expand Down Expand Up @@ -502,6 +510,10 @@ impl Endpoint {
/// information being provided by either the discovery service or using
/// [`Endpoint::add_node_addr`]. See [`Endpoint::connect`] for the details of how it
/// uses the discovery service to establish a connection to a remote node.
#[deprecated(
since = "0.27.0",
note = "Please use `connect` directly with a NodeId. This fn will be removed in 0.28.0."
)]
pub async fn connect_by_node_id(
&self,
node_id: NodeId,
Expand Down
2 changes: 1 addition & 1 deletion iroh/examples/custom-protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ impl BlobSearch {
// Establish a connection to our node.
// We use the default node discovery in iroh, so we can connect by node id without
// providing further information.
let conn = self.endpoint.connect_by_node_id(node_id, ALPN).await?;
let conn = self.endpoint.connect(node_id, ALPN).await?;

// Open a bi-directional in our connection.
let (mut send, mut recv) = conn.open_bi().await?;
Expand Down
Loading