From 1c076af5ee16dce5ce942245373db427b4b96fd4 Mon Sep 17 00:00:00 2001 From: Seun LanLege Date: Wed, 12 Jun 2019 09:42:16 +0100 Subject: [PATCH] DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705) * get node IP address and udp port from Socket, if not included in PING packet * prevent bootnodes from being added to host nodes * code corrections * code corrections * code corrections * code corrections * docs * code corrections * code corrections * Apply suggestions from code review Co-Authored-By: David --- util/network-devp2p/src/discovery.rs | 29 ++++++++++++++++++++------- util/network-devp2p/src/node_table.rs | 14 +++++++++---- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/util/network-devp2p/src/discovery.rs b/util/network-devp2p/src/discovery.rs index 2777505b3..bd5cd09a6 100644 --- a/util/network-devp2p/src/discovery.rs +++ b/util/network-devp2p/src/discovery.rs @@ -169,7 +169,6 @@ pub struct Discovery<'a> { discovery_id: NodeId, discovery_nodes: HashSet, node_buckets: Vec, - // 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). @@ -258,7 +257,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); @@ -266,7 +265,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 { @@ -276,7 +275,12 @@ impl<'a> Discovery<'a> { if let Some(node) = node_to_ping { self.try_ping(node, PingReason::Default); }; - TableUpdates{added, removed: HashSet::new()} + + if node_entry.endpoint.is_valid_sync_node() { + Some(TableUpdates { added, removed: HashSet::new() }) + } else { + None + } }) } @@ -519,7 +523,18 @@ 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(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` + // 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, + udp_port: from.port() + } + }; let ping_to = NodeEndpoint::from_rlp(&rlp.at(2)?)?; let timestamp: u64 = rlp.val_at(3)?; self.check_timestamp(timestamp)?; @@ -541,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_discovery_node() { debug!(target: "discovery", "Got bad address: {:?}", entry); } else if !self.is_allowed(&entry) { debug!(target: "discovery", "Address not allowed: {:?}", entry); @@ -729,7 +744,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_discovery_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 739babfe0..0e39c43ce 100644 --- a/util/network-devp2p/src/node_table.rs +++ b/util/network-devp2p/src/node_table.rs @@ -106,10 +106,16 @@ impl NodeEndpoint { self.to_rlp(rlp); } - /// 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 && - match self.address { + /// 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. + 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() }