Skip to content

Commit

Permalink
feat(iroh-net)!: Allow using a NodeId directly in connect. (#2774)
Browse files Browse the repository at this point in the history
## Description

Allow using anything can be converted to a NodeAddr when connecting.
This means that we can directly connect with NodeIds, and
connect_by_node_id is no longer needed.

Also deprecate connect_by_node_id since it is no longer needed.

## Breaking Changes

- iroh-net: Endpoint::connect now takes an `impl Into<NodeAddr>` instead
of a `NodeAddr`
- iroh-net: Endpoint::connect_by_node_id is deprecated (to be removed
next release)

## Notes & open questions

Note: Doing this means that the instrumentation of connect no longer
contains the remote, which is bad I guess. Not sure how to work around
this. One way would be to have connect and connect_impl - connect_impl
is like the current connect and can be instrumented as before. Any other
way to do this?

Note: since there is a From<NodeTicket> for NodeAddr, you can also
directly dial using a ticket, which is kinda neat.

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [x] All breaking changes documented.
  • Loading branch information
rklaehn authored Oct 2, 2024
1 parent 73ca58a commit bd5e4fa
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 20 deletions.
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`]
/// 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)))]
pub async fn connect(
&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());
// 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

0 comments on commit bd5e4fa

Please sign in to comment.