From fb102a70c195f49e1378711a59ad6f763fffd9b7 Mon Sep 17 00:00:00 2001 From: Seun Date: Fri, 31 May 2019 18:12:19 +0100 Subject: [PATCH 01/10] get node IP address and udp port from Socket, if not included in PING packet --- util/network-devp2p/src/discovery.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 7bf8dc62e5e..245c08a8942 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -518,7 +518,14 @@ impl<'a> Discovery<'a> { fn on_ping(&mut self, rlp: &Rlp, node_id: &NodeId, from: &SocketAddr, echo_hash: &[u8]) -> Result, Error> { trace!(target: "discovery", "Got Ping from {:?}", &from); - let ping_from = NodeEndpoint::from_rlp(&rlp.at(1)?)?; + let ping_from = if let Ok(rlp) = rlp.at(1) { + NodeEndpoint::from_rlp(&rlp)?; + } else { + NodeEndpoint { + address: from.clone(), + udp_port: from.port() + } + }; let ping_to = NodeEndpoint::from_rlp(&rlp.at(2)?)?; let timestamp: u64 = rlp.val_at(3)?; self.check_timestamp(timestamp)?; From 351b257080de39a0ca3f43b85990b2a10ecd8381 Mon Sep 17 00:00:00 2001 From: Seun Date: Tue, 4 Jun 2019 11:57:38 +0100 Subject: [PATCH 02/10] prevent bootnodes from being added to host nodes --- util/network-devp2p/src/discovery.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 245c08a8942..142aa3dc7a6 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -169,6 +169,9 @@ pub struct Discovery<'a> { discovery_nodes: HashSet, node_buckets: Vec, + // these nodes can't be used for syncing. + bootnodes: HashSet, + // Sometimes we don't want to add nodes to the NodeTable, but still want to // keep track of them to avoid excessive pinging (happens when an unknown node sends // a discovery request to us -- the node might be on a different net). @@ -200,6 +203,7 @@ impl<'a> Discovery<'a> { discovery_id: NodeId::new(), discovery_nodes: HashSet::new(), node_buckets: (0..ADDRESS_BITS).map(|_| NodeBucket::new()).collect(), + bootnodes: HashSet::new(), other_observed_nodes: LruCache::new(OBSERVED_NODES_MAX_SIZE), in_flight_pings: HashMap::new(), in_flight_find_nodes: HashMap::new(), @@ -518,11 +522,16 @@ impl<'a> Discovery<'a> { fn on_ping(&mut self, rlp: &Rlp, node_id: &NodeId, from: &SocketAddr, echo_hash: &[u8]) -> Result, Error> { trace!(target: "discovery", "Got Ping from {:?}", &from); - let ping_from = if let Ok(rlp) = rlp.at(1) { - NodeEndpoint::from_rlp(&rlp)?; + let ping_from = if let Ok(node_endpoint) = NodeEndpoint::from_rlp(&rlp.at(1)?) { + node_endpoint } else { + let mut address = from.clone(); + // address here is the node's tcp port. If we are unable to get the `NodeEndpoint` from the `ping_from` + // field then this is most likely a BootNode, set the tcp port to 0 because it can not be used for syncing. + self.bootnodes.insert(node_id.clone()); + address.set_port(0); NodeEndpoint { - address: from.clone(), + address, udp_port: from.port() } }; @@ -547,7 +556,7 @@ impl<'a> Discovery<'a> { self.send_packet(PACKET_PONG, from, &response.drain())?; let entry = NodeEntry { id: *node_id, endpoint: pong_to.clone() }; - if !entry.endpoint.is_valid() { + if !entry.endpoint.is_valid() && !self.bootnodes.contains(node_id) { debug!(target: "discovery", "Got bad address: {:?}", entry); } else if !self.is_allowed(&entry) { debug!(target: "discovery", "Address not allowed: {:?}", entry); @@ -609,8 +618,10 @@ impl<'a> Discovery<'a> { NodeValidity::Ourselves => (), } Ok(None) - } else { + } else if !self.bootnodes.contains(node_id) { Ok(self.update_node(node)) + } else { + Ok(None) } } else { debug!(target: "discovery", "Got unexpected Pong from {:?} ; request not found", &from); From 706dceb69debb98402269966b7c4724721ac36c6 Mon Sep 17 00:00:00 2001 From: Seun Date: Tue, 4 Jun 2019 14:54:28 +0100 Subject: [PATCH 03/10] code corrections --- util/network-devp2p/src/discovery.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 142aa3dc7a6..9888b3293c5 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -261,7 +261,7 @@ impl<'a> Discovery<'a> { Ok(()) => None, Err(BucketError::Ourselves) => None, Err(BucketError::NotInTheBucket{node_entry, bucket_distance}) => Some((node_entry, bucket_distance)) - }.map(|(node_entry, bucket_distance)| { + }.and_then(|(node_entry, bucket_distance)| { trace!(target: "discovery", "Adding a new node {:?} into our bucket {}", &node_entry, bucket_distance); let mut added = HashMap::with_capacity(1); @@ -279,7 +279,12 @@ impl<'a> Discovery<'a> { if let Some(node) = node_to_ping { self.try_ping(node, PingReason::Default); }; - TableUpdates{added, removed: HashSet::new()} + + if !self.bootnodes.contains(&node_entry.id) { + Some(TableUpdates { added, removed: HashSet::new() }) + } else { + None + } }) } @@ -618,10 +623,8 @@ impl<'a> Discovery<'a> { NodeValidity::Ourselves => (), } Ok(None) - } else if !self.bootnodes.contains(node_id) { - Ok(self.update_node(node)) } else { - Ok(None) + Ok(self.update_node(node)) } } else { debug!(target: "discovery", "Got unexpected Pong from {:?} ; request not found", &from); From 783a66df3bf69b52f4372fcd32efc7f538451b6b Mon Sep 17 00:00:00 2001 From: Seun Date: Tue, 4 Jun 2019 15:34:08 +0100 Subject: [PATCH 04/10] code corrections --- util/network-devp2p/src/discovery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 9888b3293c5..f94e9bdf247 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -269,7 +269,7 @@ impl<'a> Discovery<'a> { let node_to_ping = { let bucket = &mut self.node_buckets[bucket_distance]; - bucket.nodes.push_front(BucketEntry::new(node_entry)); + bucket.nodes.push_front(BucketEntry::new(node_entry.clone())); if bucket.nodes.len() > BUCKET_SIZE { select_bucket_ping(bucket.nodes.iter()) } else { From 9400dd130a15cab481a67dd753ab49132989f5e6 Mon Sep 17 00:00:00 2001 From: Seun Date: Wed, 5 Jun 2019 11:29:35 +0100 Subject: [PATCH 05/10] code corrections --- util/network-devp2p/src/discovery.rs | 18 ++++++------------ util/network-devp2p/src/node_table.rs | 4 ++++ 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index f94e9bdf247..50020f06331 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -168,10 +168,6 @@ pub struct Discovery<'a> { discovery_id: NodeId, discovery_nodes: HashSet, node_buckets: Vec, - - // these nodes can't be used for syncing. - bootnodes: HashSet, - // Sometimes we don't want to add nodes to the NodeTable, but still want to // keep track of them to avoid excessive pinging (happens when an unknown node sends // a discovery request to us -- the node might be on a different net). @@ -203,7 +199,6 @@ impl<'a> Discovery<'a> { discovery_id: NodeId::new(), discovery_nodes: HashSet::new(), node_buckets: (0..ADDRESS_BITS).map(|_| NodeBucket::new()).collect(), - bootnodes: HashSet::new(), other_observed_nodes: LruCache::new(OBSERVED_NODES_MAX_SIZE), in_flight_pings: HashMap::new(), in_flight_find_nodes: HashMap::new(), @@ -280,7 +275,7 @@ impl<'a> Discovery<'a> { self.try_ping(node, PingReason::Default); }; - if !self.bootnodes.contains(&node_entry.id) { + if node_entry.endpoint.is_valid_sync_node() { Some(TableUpdates { added, removed: HashSet::new() }) } else { None @@ -527,18 +522,17 @@ impl<'a> Discovery<'a> { fn on_ping(&mut self, rlp: &Rlp, node_id: &NodeId, from: &SocketAddr, echo_hash: &[u8]) -> Result, Error> { trace!(target: "discovery", "Got Ping from {:?}", &from); - let ping_from = if let Ok(node_endpoint) = NodeEndpoint::from_rlp(&rlp.at(1)?) { - node_endpoint + let (ping_from, bootnode) = if let Ok(node_endpoint) = NodeEndpoint::from_rlp(&rlp.at(1)?) { + (node_endpoint, false) } else { let mut address = from.clone(); // address here is the node's tcp port. If we are unable to get the `NodeEndpoint` from the `ping_from` // field then this is most likely a BootNode, set the tcp port to 0 because it can not be used for syncing. - self.bootnodes.insert(node_id.clone()); address.set_port(0); - NodeEndpoint { + (NodeEndpoint { address, udp_port: from.port() - } + }, true) }; let ping_to = NodeEndpoint::from_rlp(&rlp.at(2)?)?; let timestamp: u64 = rlp.val_at(3)?; @@ -561,7 +555,7 @@ impl<'a> Discovery<'a> { self.send_packet(PACKET_PONG, from, &response.drain())?; let entry = NodeEntry { id: *node_id, endpoint: pong_to.clone() }; - if !entry.endpoint.is_valid() && !self.bootnodes.contains(node_id) { + if !entry.endpoint.is_valid() && !bootnode { debug!(target: "discovery", "Got bad address: {:?}", entry); } else if !self.is_allowed(&entry) { debug!(target: "discovery", "Address not allowed: {:?}", entry); diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index db51890082d..70c980c7cf5 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -104,6 +104,10 @@ impl NodeEndpoint { self.to_rlp(rlp); } + pub fn is_valid_sync_node(&self) -> bool { + self.address.port() != 0 + } + /// Validates that the port is not 0 and address IP is specified pub fn is_valid(&self) -> bool { self.udp_port != 0 && self.address.port() != 0 && From 2056d6e6fee4074cfb6b79c707905e6811f00913 Mon Sep 17 00:00:00 2001 From: Seun Date: Wed, 5 Jun 2019 14:33:18 +0100 Subject: [PATCH 06/10] code corrections --- util/network-devp2p/src/discovery.rs | 4 ++-- util/network-devp2p/src/node_table.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 50020f06331..5566deb0c49 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -555,7 +555,7 @@ impl<'a> Discovery<'a> { self.send_packet(PACKET_PONG, from, &response.drain())?; let entry = NodeEntry { id: *node_id, endpoint: pong_to.clone() }; - if !entry.endpoint.is_valid() && !bootnode { + if !entry.endpoint.is_valid_sync_node() && !bootnode { debug!(target: "discovery", "Got bad address: {:?}", entry); } else if !self.is_allowed(&entry) { debug!(target: "discovery", "Address not allowed: {:?}", entry); @@ -743,7 +743,7 @@ impl<'a> Discovery<'a> { trace!(target: "discovery", "Got {} Neighbours from {:?}", results_count, &from); for r in rlp.at(0)?.iter() { let endpoint = NodeEndpoint::from_rlp(&r)?; - if !endpoint.is_valid() { + if !endpoint.is_valid_sync_node() { debug!(target: "discovery", "Bad address: {:?}", endpoint); continue; } diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index 70c980c7cf5..23c34a1bc24 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -105,12 +105,12 @@ impl NodeEndpoint { } pub fn is_valid_sync_node(&self) -> bool { - self.address.port() != 0 + self.is_valid_discovery_node() && self.address.port() != 0 } /// Validates that the port is not 0 and address IP is specified - pub fn is_valid(&self) -> bool { - self.udp_port != 0 && self.address.port() != 0 && + pub fn is_valid_discovery_node(&self) -> bool { + self.udp_port != 0 && match self.address { SocketAddr::V4(a) => !a.ip().is_unspecified(), SocketAddr::V6(a) => !a.ip().is_unspecified() From 56e9eb3c85d3c7de4ac714b2db5df76780c1420f Mon Sep 17 00:00:00 2001 From: Seun Date: Wed, 5 Jun 2019 16:42:52 +0100 Subject: [PATCH 07/10] docs --- util/network-devp2p/src/node_table.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index 23c34a1bc24..0ab2bcdcf5a 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -104,14 +104,16 @@ impl NodeEndpoint { self.to_rlp(rlp); } + /// validates that the tcp port is not 0 && is_valid_discovery_node + /// sync happens over tcp pub fn is_valid_sync_node(&self) -> bool { self.is_valid_discovery_node() && self.address.port() != 0 } - /// Validates that the port is not 0 and address IP is specified + /// Validates that the udp port is not 0 and address IP is specified + /// peer discovery happens over udp pub fn is_valid_discovery_node(&self) -> bool { - self.udp_port != 0 && - match self.address { + self.udp_port != 0 && match self.address { SocketAddr::V4(a) => !a.ip().is_unspecified(), SocketAddr::V6(a) => !a.ip().is_unspecified() } From cd43652a1bbe3ffb00092221f30fc10bc7b1bb92 Mon Sep 17 00:00:00 2001 From: Seun Date: Wed, 5 Jun 2019 17:04:33 +0100 Subject: [PATCH 08/10] code corrections --- util/network-devp2p/src/discovery.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 5566deb0c49..3e4ee164de8 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -522,17 +522,17 @@ impl<'a> Discovery<'a> { fn on_ping(&mut self, rlp: &Rlp, node_id: &NodeId, from: &SocketAddr, echo_hash: &[u8]) -> Result, Error> { trace!(target: "discovery", "Got Ping from {:?}", &from); - let (ping_from, bootnode) = if let Ok(node_endpoint) = NodeEndpoint::from_rlp(&rlp.at(1)?) { - (node_endpoint, false) + let ping_from = if let Ok(node_endpoint) = NodeEndpoint::from_rlp(&rlp.at(1)?) { + node_endpoint } else { let mut address = from.clone(); // address here is the node's tcp port. If we are unable to get the `NodeEndpoint` from the `ping_from` // field then this is most likely a BootNode, set the tcp port to 0 because it can not be used for syncing. address.set_port(0); - (NodeEndpoint { + NodeEndpoint { address, udp_port: from.port() - }, true) + } }; let ping_to = NodeEndpoint::from_rlp(&rlp.at(2)?)?; let timestamp: u64 = rlp.val_at(3)?; @@ -555,7 +555,7 @@ impl<'a> Discovery<'a> { self.send_packet(PACKET_PONG, from, &response.drain())?; let entry = NodeEntry { id: *node_id, endpoint: pong_to.clone() }; - if !entry.endpoint.is_valid_sync_node() && !bootnode { + if !entry.endpoint.is_valid_discovery_node() { debug!(target: "discovery", "Got bad address: {:?}", entry); } else if !self.is_allowed(&entry) { debug!(target: "discovery", "Address not allowed: {:?}", entry); From 2633494b188cec964b59ccdd5863168c13163577 Mon Sep 17 00:00:00 2001 From: Seun Date: Wed, 5 Jun 2019 17:12:58 +0100 Subject: [PATCH 09/10] code corrections --- util/network-devp2p/src/discovery.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 3e4ee164de8..82139850b04 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -743,7 +743,7 @@ impl<'a> Discovery<'a> { trace!(target: "discovery", "Got {} Neighbours from {:?}", results_count, &from); for r in rlp.at(0)?.iter() { let endpoint = NodeEndpoint::from_rlp(&r)?; - if !endpoint.is_valid_sync_node() { + if !endpoint.is_valid_discovery_node() { debug!(target: "discovery", "Bad address: {:?}", endpoint); continue; } From cdf80c5bcc0bcfb5c4e990f95a9b1011451a8a7f Mon Sep 17 00:00:00 2001 From: Seun LanLege Date: Wed, 12 Jun 2019 08:19:35 +0100 Subject: [PATCH 10/10] Apply suggestions from code review Co-Authored-By: David --- util/network-devp2p/src/discovery.rs | 2 +- util/network-devp2p/src/node_table.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 82139850b04..f18469e1642 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -527,7 +527,7 @@ impl<'a> Discovery<'a> { } else { let mut address = from.clone(); // address here is the node's tcp port. If we are unable to get the `NodeEndpoint` from the `ping_from` - // field then this is most likely a BootNode, set the tcp port to 0 because it can not be used for syncing. + // rlp field then this is most likely a BootNode, set the tcp port to 0 because it can not be used for syncing. address.set_port(0); NodeEndpoint { address, diff --git a/util/network-devp2p/src/node_table.rs b/util/network-devp2p/src/node_table.rs index 0ab2bcdcf5a..616a14357ab 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -104,14 +104,14 @@ impl NodeEndpoint { self.to_rlp(rlp); } - /// validates that the tcp port is not 0 && is_valid_discovery_node - /// sync happens over tcp + /// Validates that the tcp port is not 0 and that the node is a valid discovery node (i.e. `is_valid_discovery_node()` is true). + /// Sync happens over tcp. pub fn is_valid_sync_node(&self) -> bool { self.is_valid_discovery_node() && self.address.port() != 0 } - /// Validates that the udp port is not 0 and address IP is specified - /// peer discovery happens over udp + /// Validates that the udp port is not 0 and address IP is specified. + /// Peer discovery happens over udp. pub fn is_valid_discovery_node(&self) -> bool { self.udp_port != 0 && match self.address { SocketAddr::V4(a) => !a.ip().is_unspecified(),