From 8b0b0361a088f9e1a3c14455f3b9442dc5e82276 Mon Sep 17 00:00:00 2001 From: AurelienFT Date: Thu, 24 Oct 2024 10:56:40 +0200 Subject: [PATCH 1/6] Add test node herself in reserved nodes. --- crates/services/p2p/src/p2p_service.rs | 32 ++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/crates/services/p2p/src/p2p_service.rs b/crates/services/p2p/src/p2p_service.rs index 05dd2ec1b3..f79e486491 100644 --- a/crates/services/p2p/src/p2p_service.rs +++ b/crates/services/p2p/src/p2p_service.rs @@ -883,6 +883,7 @@ mod tests { Topic, }, identity::Keypair, + multiaddr::multiaddr, swarm::{ ListenError, SwarmEvent, @@ -893,6 +894,10 @@ mod tests { use rand::Rng; use std::{ collections::HashSet, + net::{ + IpAddr, + Ipv4Addr, + }, ops::Range, sync::Arc, time::Duration, @@ -1036,6 +1041,33 @@ mod tests { stop_sender.send(()).unwrap(); } + #[tokio::test] + #[instrument] + async fn our_node_in_reserved_nodes() { + let mut p2p_config = Config::default_initialized("own_node_in_reserved_nodes"); + p2p_config.address = IpAddr::V4(Ipv4Addr::from([0, 0, 0, 0])); + p2p_config.tcp_port = 11111; + let multiaddr = multiaddr!(Ip4([127, 0, 0, 1]), Tcp(11111u16)) + .with_p2p(p2p_config.keypair.public().to_peer_id()) + .unwrap(); + p2p_config.public_address = Some(multiaddr.clone()); + p2p_config.reserved_nodes = vec![multiaddr]; + + let mut node = build_service_from_config(p2p_config).await; + loop { + tokio::select! { + event = node.next_event() => { + if let Some(FuelP2PEvent::PeerConnected(_)) = event { + panic!("The node should not connect to itself"); + } + } + _ = tokio::time::sleep(Duration::from_secs(2)) => { + break; + } + } + } + } + // We start with two nodes, node_a and node_b, bootstrapped with `bootstrap_nodes_count` other nodes. // Yet node_a and node_b are only allowed to connect to specified amount of nodes. #[tokio::test] From 6b28a081ab22a1ba89ff7ee978a7f14bb334fc95 Mon Sep 17 00:00:00 2001 From: AurelienFT Date: Tue, 29 Oct 2024 11:21:01 +0100 Subject: [PATCH 2/6] Fix timeout test --- crates/services/p2p/src/p2p_service.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/crates/services/p2p/src/p2p_service.rs b/crates/services/p2p/src/p2p_service.rs index f79e486491..a16f71dc6a 100644 --- a/crates/services/p2p/src/p2p_service.rs +++ b/crates/services/p2p/src/p2p_service.rs @@ -1054,18 +1054,16 @@ mod tests { p2p_config.reserved_nodes = vec![multiaddr]; let mut node = build_service_from_config(p2p_config).await; - loop { - tokio::select! { - event = node.next_event() => { - if let Some(FuelP2PEvent::PeerConnected(_)) = event { - panic!("The node should not connect to itself"); - } - } - _ = tokio::time::sleep(Duration::from_secs(2)) => { - break; + tokio::time::timeout(Duration::from_secs(2), async move { + loop { + let event = node.next_event().await; + if let Some(FuelP2PEvent::PeerConnected(_)) = event { + panic!("The node should not connect to itself"); } } - } + }) + .await + .expect_err("The node should not connect to itself"); } // We start with two nodes, node_a and node_b, bootstrapped with `bootstrap_nodes_count` other nodes. From b93e26a18ae1b539532a482f51ff0e28d5e42f91 Mon Sep 17 00:00:00 2001 From: AurelienFT Date: Wed, 30 Oct 2024 17:12:33 +0100 Subject: [PATCH 3/6] Add random port selection in `our_node_in_reserved_nodes` with comment about this wired methodology --- crates/services/p2p/src/p2p_service.rs | 51 +++++++++++++++++++++----- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/crates/services/p2p/src/p2p_service.rs b/crates/services/p2p/src/p2p_service.rs index a16f71dc6a..b828d8d3ce 100644 --- a/crates/services/p2p/src/p2p_service.rs +++ b/crates/services/p2p/src/p2p_service.rs @@ -897,6 +897,7 @@ mod tests { net::{ IpAddr, Ipv4Addr, + TcpListener, }, ops::Range, sync::Arc, @@ -1044,16 +1045,47 @@ mod tests { #[tokio::test] #[instrument] async fn our_node_in_reserved_nodes() { - let mut p2p_config = Config::default_initialized("own_node_in_reserved_nodes"); - p2p_config.address = IpAddr::V4(Ipv4Addr::from([0, 0, 0, 0])); - p2p_config.tcp_port = 11111; - let multiaddr = multiaddr!(Ip4([127, 0, 0, 1]), Tcp(11111u16)) - .with_p2p(p2p_config.keypair.public().to_peer_id()) + let mut retries = 10; + // We use bind to get a random port for the node to listen on. + // We use a loop because the port might be taken between the time we drop the listener and it's used by libp2p. + // We don't use `build_service_from_config` because we want to drop the tcp_listener at the last moment. + // and we don't want to `.unwrap()` if the port is taken but we prefer retry. + let mut node = loop { + let tcp_listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let random_port = tcp_listener.local_addr().unwrap().port(); + let mut p2p_config = + Config::default_initialized("own_node_in_reserved_nodes"); + p2p_config.address = IpAddr::V4(Ipv4Addr::from([0, 0, 0, 0])); + p2p_config.tcp_port = 11111; + let multiaddr = multiaddr!(Ip4([127, 0, 0, 1]), Tcp(random_port)) + .with_p2p(p2p_config.keypair.public().to_peer_id()) + .unwrap(); + p2p_config.public_address = Some(multiaddr.clone()); + // Given + p2p_config.reserved_nodes = vec![multiaddr]; + p2p_config.keypair = Keypair::generate_secp256k1(); // change keypair for each Node + let max_block_size = p2p_config.max_block_size; + let (sender, _) = + broadcast::channel(p2p_config.reserved_nodes.len().saturating_add(1)); + let mut service = FuelP2PService::new( + sender, + p2p_config, + PostcardCodec::new(max_block_size), + ) + .await .unwrap(); - p2p_config.public_address = Some(multiaddr.clone()); - p2p_config.reserved_nodes = vec![multiaddr]; - - let mut node = build_service_from_config(p2p_config).await; + drop(tcp_listener); + match service.start().await { + Ok(()) => break service, + Err(_) => { + if retries == 0 { + panic!("Failed to start the node after 10 retries"); + } + retries -= 1 + } + }; + }; + // When tokio::time::timeout(Duration::from_secs(2), async move { loop { let event = node.next_event().await; @@ -1063,6 +1095,7 @@ mod tests { } }) .await + // Then .expect_err("The node should not connect to itself"); } From f1b87833f5a620717f9b9699b5f153437fb0a81a Mon Sep 17 00:00:00 2001 From: AurelienFT Date: Wed, 30 Oct 2024 17:14:21 +0100 Subject: [PATCH 4/6] Fix outdated value used in `our_node_in_reserved_nodes` --- crates/services/p2p/src/p2p_service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/services/p2p/src/p2p_service.rs b/crates/services/p2p/src/p2p_service.rs index b828d8d3ce..a752e8e696 100644 --- a/crates/services/p2p/src/p2p_service.rs +++ b/crates/services/p2p/src/p2p_service.rs @@ -1056,7 +1056,7 @@ mod tests { let mut p2p_config = Config::default_initialized("own_node_in_reserved_nodes"); p2p_config.address = IpAddr::V4(Ipv4Addr::from([0, 0, 0, 0])); - p2p_config.tcp_port = 11111; + p2p_config.tcp_port = random_port; let multiaddr = multiaddr!(Ip4([127, 0, 0, 1]), Tcp(random_port)) .with_p2p(p2p_config.keypair.public().to_peer_id()) .unwrap(); From 6cadd115b2e8b8bf168e93648e5a6a876b5c9c21 Mon Sep 17 00:00:00 2001 From: AurelienFT Date: Thu, 31 Oct 2024 01:20:41 +0100 Subject: [PATCH 5/6] Change test connecting to our node to connecting to a node with same peerid --- crates/services/p2p/src/p2p_service.rs | 54 ++++++++------------------ 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/crates/services/p2p/src/p2p_service.rs b/crates/services/p2p/src/p2p_service.rs index a752e8e696..a110c0d9bf 100644 --- a/crates/services/p2p/src/p2p_service.rs +++ b/crates/services/p2p/src/p2p_service.rs @@ -883,7 +883,6 @@ mod tests { Topic, }, identity::Keypair, - multiaddr::multiaddr, swarm::{ ListenError, SwarmEvent, @@ -894,11 +893,6 @@ mod tests { use rand::Rng; use std::{ collections::HashSet, - net::{ - IpAddr, - Ipv4Addr, - TcpListener, - }, ops::Range, sync::Arc, time::Duration, @@ -1044,29 +1038,19 @@ mod tests { #[tokio::test] #[instrument] - async fn our_node_in_reserved_nodes() { - let mut retries = 10; - // We use bind to get a random port for the node to listen on. - // We use a loop because the port might be taken between the time we drop the listener and it's used by libp2p. - // We don't use `build_service_from_config` because we want to drop the tcp_listener at the last moment. - // and we don't want to `.unwrap()` if the port is taken but we prefer retry. - let mut node = loop { - let tcp_listener = TcpListener::bind("127.0.0.1:0").unwrap(); - let random_port = tcp_listener.local_addr().unwrap().port(); - let mut p2p_config = - Config::default_initialized("own_node_in_reserved_nodes"); - p2p_config.address = IpAddr::V4(Ipv4Addr::from([0, 0, 0, 0])); - p2p_config.tcp_port = random_port; - let multiaddr = multiaddr!(Ip4([127, 0, 0, 1]), Tcp(random_port)) - .with_p2p(p2p_config.keypair.public().to_peer_id()) - .unwrap(); - p2p_config.public_address = Some(multiaddr.clone()); + async fn dont_connect_to_node_with_same_peer_id() { + let mut p2p_config = + Config::default_initialized("dont_connect_to_node_with_same_peer_id"); + let mut node_a = build_service_from_config(p2p_config.clone()).await; + // We don't use build_service_from_config here, because we want to use the same keypair + // to have the same PeerId + let node_b = { // Given - p2p_config.reserved_nodes = vec![multiaddr]; - p2p_config.keypair = Keypair::generate_secp256k1(); // change keypair for each Node + p2p_config.reserved_nodes = vec![node_a.multiaddrs().pop().unwrap()]; let max_block_size = p2p_config.max_block_size; let (sender, _) = broadcast::channel(p2p_config.reserved_nodes.len().saturating_add(1)); + let mut service = FuelP2PService::new( sender, p2p_config, @@ -1074,29 +1058,23 @@ mod tests { ) .await .unwrap(); - drop(tcp_listener); - match service.start().await { - Ok(()) => break service, - Err(_) => { - if retries == 0 { - panic!("Failed to start the node after 10 retries"); - } - retries -= 1 - } - }; + service.start().await.unwrap(); + service }; // When - tokio::time::timeout(Duration::from_secs(2), async move { + tokio::time::timeout(Duration::from_secs(5), async move { loop { - let event = node.next_event().await; + let event = node_a.next_event().await; if let Some(FuelP2PEvent::PeerConnected(_)) = event { - panic!("The node should not connect to itself"); + panic!("Node B should not connect to Node A because they have the same PeerId"); } + assert_eq!(node_a.peer_manager().total_peers_connected(), 0); } }) .await // Then .expect_err("The node should not connect to itself"); + assert_eq!(node_b.peer_manager().total_peers_connected(), 0); } // We start with two nodes, node_a and node_b, bootstrapped with `bootstrap_nodes_count` other nodes. From de295425f831cf558efb352bfeb3eb9b168f56cf Mon Sep 17 00:00:00 2001 From: Green Baneling Date: Wed, 13 Nov 2024 17:05:37 -0500 Subject: [PATCH 6/6] Apply suggestions from code review --- crates/services/p2p/src/p2p_service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/services/p2p/src/p2p_service.rs b/crates/services/p2p/src/p2p_service.rs index 1c0e22ef1a..ec2be3f387 100644 --- a/crates/services/p2p/src/p2p_service.rs +++ b/crates/services/p2p/src/p2p_service.rs @@ -1063,7 +1063,7 @@ mod tests { // to have the same PeerId let node_b = { // Given - p2p_config.reserved_nodes = vec![node_a.multiaddrs().pop().unwrap()]; + p2p_config.reserved_nodes = node_a.multiaddrs(); let max_block_size = p2p_config.max_block_size; let (sender, _) = broadcast::channel(p2p_config.reserved_nodes.len().saturating_add(1));