refactor(iroh): remove the addr arg from start (#1830)

## Description Removes the add from the start args as discussed internally. Reason for this is that it's currently not used. In reality, the port was not used. If we do add this back, most likely we want to also use the address to allow ipv6 listening nodes. In the spirit of removing code we opt for letting the code reflect how it's actually used in the moment ## Notes & open questions na ## Change checklist - [ ] Self-review. - [ ] Documentation updates if relevant. - [ ] Tests if relevant. --------- Co-authored-by: Asmir Avdicevic <asmir.avdicevic64@gmail.com>
n0-computer · Nov 24, 2023 · e03de38 · e03de38
1 parent 54acfcb
commit e03de38
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 84 deletions.
diff --git a/iroh/src/commands/start.rs b/iroh/src/commands/start.rs
@@ -43,11 +43,6 @@ pub enum RunType {
 
 #[derive(Args, Debug, Clone)]
 pub struct StartArgs {
-    /// Listening address to bind to.
-    ///
-    /// Only used with `start` or `--start`
-    #[clap(long, short, global = true, default_value_t = SocketAddr::from(iroh::node::DEFAULT_BIND_ADDR))]
-    addr: SocketAddr,
     /// Use a token to authenticate requests for data.
     ///
     /// Pass "random" to generate a random token, or base32-encoded bytes to use as a token
@@ -208,7 +203,6 @@ impl StartArgs {
             .derp_mode(derp_mode)
             .custom_auth_handler(Arc::new(StaticTokenAuthHandler::new(token)))
             .peers_data_path(peers_data_path)
-            .bind_addr(self.addr)
             .runtime(rt)
             .rpc_endpoint(rpc_endpoint)
             .secret_key(secret_key)

diff --git a/iroh/src/node.rs b/iroh/src/node.rs
@@ -8,7 +8,7 @@
 use std::fmt::Debug;
 use std::future::Future;
 use std::io;
-use std::net::{Ipv4Addr, SocketAddr};
+use std::net::SocketAddr;
 use std::path::PathBuf;
 use std::pin::Pin;
 use std::sync::{Arc, Mutex};
@@ -76,7 +76,7 @@ const HEALTH_POLL_WAIT: Duration = Duration::from_secs(1);
 
 /// Default bind address for the node.
 /// 11204 is "iroh" in leetspeak <https://simple.wikipedia.org/wiki/Leet>
-pub const DEFAULT_BIND_ADDR: (Ipv4Addr, u16) = (Ipv4Addr::LOCALHOST, 11204);
+pub const DEFAULT_BIND_PORT: u16 = 11204;
 
 /// How long we wait at most for some endpoints to be discovered.
 const ENDPOINT_WAIT: Duration = Duration::from_secs(5);
@@ -123,7 +123,7 @@ where
     S: DocStore,
     E: ServiceEndpoint<ProviderService>,
 {
-    bind_addr: SocketAddr,
+    bind_port: u16,
     secret_key: SecretKey,
     rpc_endpoint: E,
     db: D,
@@ -169,7 +169,7 @@ impl<D: Map, S: DocStore> Builder<D, S> {
     /// Creates a new builder for [`Node`] using the given database.
     fn with_db_and_store(db: D, docs: S) -> Self {
         Self {
-            bind_addr: DEFAULT_BIND_ADDR.into(),
+            bind_port: DEFAULT_BIND_PORT,
             secret_key: SecretKey::generate(),
             db,
             keylog: false,
@@ -197,7 +197,7 @@ where
     ) -> Builder<D, S, E2> {
         // we can't use ..self here because the return type is different
         Builder {
-            bind_addr: self.bind_addr,
+            bind_port: self.bind_port,
             secret_key: self.secret_key,
             db: self.db,
             keylog: self.keylog,
@@ -241,11 +241,11 @@ where
         }
     }
 
-    /// Binds the node service to a different socket.
+    /// Binds the node service to a different port.
     ///
     /// By default it binds to `127.0.0.1:11204`.
-    pub fn bind_addr(mut self, addr: SocketAddr) -> Self {
-        self.bind_addr = addr;
+    pub fn bind_port(mut self, port: u16) -> Self {
+        self.bind_port = port;
         self
     }
 
@@ -318,7 +318,7 @@ where
             Some(path) => endpoint.peers_data_path(path),
             None => endpoint,
         };
-        let endpoint = endpoint.bind(self.bind_addr.port()).await?;
+        let endpoint = endpoint.bind(self.bind_port).await?;
         trace!("created quinn endpoint");
 
         let (cb_sender, cb_receiver) = mpsc::channel(8);
@@ -1835,7 +1835,6 @@ impl RequestAuthorizationHandler for StaticTokenAuthHandler {
 mod tests {
     use anyhow::bail;
     use futures::StreamExt;
-    use std::net::Ipv4Addr;
     use std::path::Path;
 
     use crate::rpc_protocol::WrapOption;
@@ -1857,7 +1856,7 @@ mod tests {
         let doc_store = iroh_sync::store::memory::Store::default();
         let hash = hashes["test"].into();
         let node = Node::builder(db, doc_store)
-            .bind_addr((Ipv4Addr::UNSPECIFIED, 0).into())
+            .bind_port(0)
             .runtime(&rt)
             .spawn()
             .await
@@ -1877,7 +1876,7 @@ mod tests {
         let db = iroh_bytes::store::mem::Store::new(rt);
         let doc_store = iroh_sync::store::memory::Store::default();
         let node = Node::builder(db, doc_store)
-            .bind_addr((Ipv4Addr::UNSPECIFIED, 0).into())
+            .bind_port(0)
             .runtime(&test_runtime())
             .spawn()
             .await?;
@@ -1902,7 +1901,7 @@ mod tests {
         let db = iroh_bytes::store::mem::Store::new(rt);
         let doc_store = iroh_sync::store::memory::Store::default();
         let node = Node::builder(db, doc_store)
-            .bind_addr((Ipv4Addr::UNSPECIFIED, 0).into())
+            .bind_port(0)
             .runtime(&test_runtime())
             .spawn()
             .await?;

diff --git a/iroh/tests/cli.rs b/iroh/tests/cli.rs
@@ -19,7 +19,6 @@ use regex::Regex;
 use testdir::testdir;
 use walkdir::WalkDir;
 
-const ADDR: &str = "127.0.0.1:0";
 const RPC_PORT: &str = "4999";
 
 fn make_rand_file(size: usize, path: &Path) -> Result<Hash> {
@@ -267,13 +266,7 @@ fn cli_provide_tree_resume() -> Result<()> {
         make_rand_file(5000, &file3)?;
     }
     // leave the provider running for the entire test
-    let provider = make_provider_in(
-        &src_iroh_data_dir,
-        Input::Path(src.clone()),
-        false,
-        None,
-        None,
-    )?;
+    let provider = make_provider_in(&src_iroh_data_dir, Input::Path(src.clone()), false, None)?;
     let count = count_input_files(&src);
     let ticket = match_provide_output(&provider, count, BlobOrCollection::Collection)?;
     {
@@ -381,8 +374,6 @@ fn cli_provide_persistence() -> anyhow::Result<()> {
             iroh_bin(),
             [
                 "start",
-                "--addr",
-                ADDR,
                 "--rpc-port",
                 "0",
                 "--add",
@@ -443,13 +434,7 @@ fn cli_provide_addresses() -> Result<()> {
     make_rand_file(1000, &path)?;
 
     let iroh_data_dir = dir.join("iroh-data-dir");
-    let mut provider = make_provider_in(
-        &iroh_data_dir,
-        Input::Path(path),
-        true,
-        Some("127.0.0.1:4333"),
-        Some(RPC_PORT),
-    )?;
+    let mut provider = make_provider_in(&iroh_data_dir, Input::Path(path), true, Some(RPC_PORT))?;
     // wait for the provider to start
     let _ticket = match_provide_output(&mut provider, 1, BlobOrCollection::Collection)?;
 
@@ -610,16 +595,9 @@ fn make_provider_in(
     iroh_data_dir: &Path,
     input: Input,
     wrap: bool,
-    addr: Option<&str>,
     rpc_port: Option<&str>,
 ) -> Result<ReaderHandle> {
-    let mut args = vec![
-        "start",
-        "--addr",
-        addr.unwrap_or(ADDR),
-        "--rpc-port",
-        rpc_port.unwrap_or("0"),
-    ];
+    let mut args = vec!["start", "--rpc-port", rpc_port.unwrap_or("0")];
     if wrap {
         args.push("--wrap");
     }
@@ -710,7 +688,7 @@ fn test_provide_get_loop(input: Input, output: Output) -> Result<()> {
 
     let dir = testdir!();
     let iroh_data_dir = dir.join("iroh-data-dir");
-    let mut provider = make_provider_in(&iroh_data_dir, input.clone(), wrap, None, None)?;
+    let mut provider = make_provider_in(&iroh_data_dir, input.clone(), wrap, None)?;
 
     // test provide output & scrape the ticket from stderr
     let ticket = match_provide_output(&mut provider, num_blobs, input.is_blob_or_collection())?;
@@ -776,7 +754,7 @@ fn test_provide_get_loop_single(input: Input, output: Output, hash: Hash) -> Res
     let dir = testdir!();
     let iroh_data_dir = dir.join("iroh-data-dir");
 
-    let mut provider = make_provider_in(&iroh_data_dir, input.clone(), true, None, None)?;
+    let mut provider = make_provider_in(&iroh_data_dir, input.clone(), true, None)?;
 
     // test provide output & get all in one ticket from stderr
     let ticket = match_provide_output(&mut provider, num_blobs, BlobOrCollection::Collection)?;

diff --git a/iroh/tests/provide.rs b/iroh/tests/provide.rs
@@ -1,6 +1,6 @@
 use std::{
     collections::BTreeMap,
-    net::{Ipv4Addr, Ipv6Addr, SocketAddr},
+    net::SocketAddr,
     ops::Range,
     path::PathBuf,
     sync::Arc,
@@ -40,12 +40,9 @@ fn test_runtime() -> runtime::Handle {
     runtime::Handle::from_current(1).unwrap()
 }
 
-fn test_node<D: Store>(
-    db: D,
-    addr: SocketAddr,
-) -> Builder<D, store::memory::Store, DummyServerEndpoint> {
+fn test_node<D: Store>(db: D) -> Builder<D, store::memory::Store, DummyServerEndpoint> {
     let store = iroh_sync::store::memory::Store::default();
-    Node::builder(db, store).bind_addr(addr)
+    Node::builder(db, store).bind_port(0)
 }
 
 #[tokio::test]
@@ -147,7 +144,6 @@ fn get_options(node_id: NodeId, addrs: Vec<SocketAddr>) -> iroh::dial::Options {
 #[tokio::test(flavor = "multi_thread")]
 async fn multiple_clients() -> Result<()> {
     let content = b"hello world!";
-    let addr = "127.0.0.1:0".parse().unwrap();
 
     let mut db = iroh_bytes::store::readonly_mem::Store::default();
     let expect_hash = db.insert(content.as_slice());
@@ -161,7 +157,7 @@ async fn multiple_clients() -> Result<()> {
     )?;
     let hash = db.insert_many(collection.to_blobs()).unwrap();
     let rt = test_runtime();
-    let node = test_node(db, addr).runtime(&rt).spawn().await?;
+    let node = test_node(db).runtime(&rt).spawn().await?;
 
     let mut tasks = Vec::new();
     for _i in 0..3 {
@@ -250,8 +246,7 @@ where
     // sort expects by name to match the canonical order of blobs
     expects.sort_by(|a, b| a.0.cmp(&b.0));
 
-    let addr = "127.0.0.1:0".parse().unwrap();
-    let node = test_node(mdb.clone(), addr).runtime(rt).spawn().await?;
+    let node = test_node(mdb.clone()).runtime(rt).spawn().await?;
 
     let (events_sender, mut events_recv) = mpsc::unbounded_channel();
 
@@ -356,8 +351,7 @@ async fn test_server_close() {
     )
     .unwrap();
     let hash = db.insert_many(collection.to_blobs()).unwrap();
-    let addr = "127.0.0.1:0".parse().unwrap();
-    let mut node = test_node(db, addr).runtime(&rt).spawn().await.unwrap();
+    let mut node = test_node(db).runtime(&rt).spawn().await.unwrap();
     let node_addr = node.local_endpoint_addresses().await.unwrap();
     let peer_id = node.node_id();
 
@@ -429,8 +423,7 @@ async fn test_ipv6() {
     let rt = test_runtime();
 
     let (db, hash) = create_test_db([("test", b"hello")]);
-    let addr = (Ipv6Addr::UNSPECIFIED, 0).into();
-    let node = match test_node(db, addr).runtime(&rt).spawn().await {
+    let node = match test_node(db).runtime(&rt).spawn().await {
         Ok(provider) => provider,
         Err(_) => {
             // We assume the problem here is IPv6 on this host.  If the problem is
@@ -458,8 +451,7 @@ async fn test_not_found() {
 
     let db = iroh_bytes::store::readonly_mem::Store::default();
     let hash = blake3::hash(b"hello").into();
-    let addr = (Ipv6Addr::UNSPECIFIED, 0).into();
-    let node = match test_node(db, addr).runtime(&rt).spawn().await {
+    let node = match test_node(db).runtime(&rt).spawn().await {
         Ok(provider) => provider,
         Err(_) => {
             // We assume the problem here is IPv6 on this host.  If the problem is
@@ -502,8 +494,7 @@ async fn test_chunk_not_found_1() {
     let data = (0..1024 * 64).map(|i| i as u8).collect::<Vec<_>>();
     let hash = blake3::hash(&data).into();
     let _entry = db.get_or_create_partial(hash, data.len() as u64).unwrap();
-    let addr = (Ipv6Addr::UNSPECIFIED, 0).into();
-    let node = match test_node(db, addr).runtime(&rt).spawn().await {
+    let node = match test_node(db).runtime(&rt).spawn().await {
         Ok(provider) => provider,
         Err(_) => {
             // We assume the problem here is IPv6 on this host.  If the problem is
@@ -541,8 +532,7 @@ async fn test_run_ticket() {
     let rt = test_runtime();
     let (db, hash) = create_test_db([("test", b"hello")]);
     let token = Some(RequestToken::generate());
-    let addr = (Ipv4Addr::UNSPECIFIED, 0).into();
-    let node = test_node(db, addr)
+    let node = test_node(db)
         .custom_auth_handler(Arc::new(StaticTokenAuthHandler::new(token.clone())))
         .runtime(&rt)
         .spawn()
@@ -615,8 +605,7 @@ async fn run_collection_get_request(
 async fn test_run_fsm() {
     let rt = test_runtime();
     let (db, hash) = create_test_db([("a", b"hello"), ("b", b"world")]);
-    let addr = (Ipv4Addr::UNSPECIFIED, 0).into();
-    let node = test_node(db, addr).runtime(&rt).spawn().await.unwrap();
+    let node = test_node(db).runtime(&rt).spawn().await.unwrap();
     let addrs = node.local_endpoint_addresses().await.unwrap();
     let peer_id = node.node_id();
     tokio::time::timeout(Duration::from_secs(10), async move {
@@ -665,8 +654,7 @@ async fn test_size_request_blob() {
     let last_chunk = last_chunk(&expected);
     let (db, hashes) = iroh_bytes::store::readonly_mem::Store::new([("test", &expected)]);
     let hash = Hash::from(*hashes.values().next().unwrap());
-    let addr = "127.0.0.1:0".parse().unwrap();
-    let node = test_node(db, addr).runtime(&rt).spawn().await.unwrap();
+    let node = test_node(db).runtime(&rt).spawn().await.unwrap();
     let addrs = node.local_endpoint_addresses().await.unwrap();
     let peer_id = node.node_id();
     tokio::time::timeout(Duration::from_secs(10), async move {
@@ -693,12 +681,7 @@ async fn test_collection_stat() {
     let child1 = make_test_data(123456);
     let child2 = make_test_data(345678);
     let (db, hash) = create_test_db([("a", &child1), ("b", &child2)]);
-    let addr = "127.0.0.1:0".parse().unwrap();
-    let node = test_node(db.clone(), addr)
-        .runtime(&rt)
-        .spawn()
-        .await
-        .unwrap();
+    let node = test_node(db.clone()).runtime(&rt).spawn().await.unwrap();
     let addrs = node.local_endpoint_addresses().await.unwrap();
     let peer_id = node.node_id();
     tokio::time::timeout(Duration::from_secs(10), async move {
@@ -759,8 +742,7 @@ async fn test_token_passthrough() -> Result<()> {
     let rt = test_runtime();
     let expected = b"hello".to_vec();
     let (db, hash) = create_test_db([("test", expected.clone())]);
-    let addr = SocketAddr::from((Ipv4Addr::LOCALHOST, 0));
-    let node = test_node(db, addr)
+    let node = test_node(db)
         .custom_auth_handler(Arc::new(CustomAuthHandler))
         .runtime(&rt)
         .spawn()

diff --git a/iroh/tests/sync.rs b/iroh/tests/sync.rs
@@ -1,6 +1,5 @@
 use std::{
     future::Future,
-    net::SocketAddr,
     sync::Arc,
     time::{Duration, Instant},
 };
@@ -37,7 +36,6 @@ fn test_runtime() -> runtime::Handle {
 
 fn test_node(
     rt: runtime::Handle,
-    addr: SocketAddr,
     secret_key: SecretKey,
 ) -> Builder<iroh_bytes::store::mem::Store, store::memory::Store, DummyServerEndpoint> {
     let db = iroh_bytes::store::mem::Store::new(rt.clone());
@@ -46,7 +44,6 @@ fn test_node(
         .secret_key(secret_key)
         .derp_mode(DerpMode::Disabled)
         .runtime(&rt)
-        .bind_addr(addr)
 }
 
 // The function is not `async fn` so that we can take a `&mut` borrow on the `rng` without
@@ -59,7 +56,7 @@ fn spawn_node(
 ) -> impl Future<Output = anyhow::Result<Node<iroh_bytes::store::mem::Store>>> + 'static {
     let secret_key = SecretKey::generate_with_rng(rng);
     async move {
-        let node = test_node(rt, "127.0.0.1:0".parse()?, secret_key);
+        let node = test_node(rt, secret_key);
         let node = node.spawn().await?;
         info!(?i, me = %node.node_id().fmt_short(), "node spawned");
         Ok(node)
@@ -738,11 +735,9 @@ async fn doc_delete() -> Result<()> {
     let rt = test_runtime();
     let db = iroh_bytes::store::mem::Store::new(rt.clone());
     let store = iroh_sync::store::memory::Store::default();
-    let addr = "127.0.0.1:0".parse().unwrap();
     let node = Node::builder(db, store)
         .gc_policy(iroh::node::GcPolicy::Interval(Duration::from_millis(100)))
         .runtime(&rt)
-        .bind_addr(addr)
         .spawn()
         .await?;
     let client = node.client();