Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Improve P2P discovery #9526

Merged
merged 16 commits into from
Sep 14, 2018
Merged

Improve P2P discovery #9526

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
48cb9d4
Add `target` to Rust traces
ngotchac Sep 11, 2018
e563f98
network-devp2p: Don't remove discovery peer in main sync
ngotchac Sep 11, 2018
5e67cb5
network-p2p: Refresh discovery more often
ngotchac Sep 11, 2018
0abffce
Update Peer discovery protocol
ngotchac Sep 11, 2018
8d1cdbc
Run discovery more often when not enough nodes connected
ngotchac Sep 11, 2018
e0f61b1
Start the first discovery early
ngotchac Sep 11, 2018
973fd2c
Update fast discovery rate
ngotchac Sep 11, 2018
d586beb
Fix tests
ngotchac Sep 11, 2018
95d0b4a
Fix `ping` tests
ngotchac Sep 11, 2018
2af281f
Merge branch 'master' into ng-p2p-discovery
ngotchac Sep 13, 2018
fe230b2
Fixing remote Node address ; adding PingPong round
ngotchac Sep 13, 2018
e4d5212
Fix tests: update new +1 PingPong round
ngotchac Sep 13, 2018
c38ff7c
Merge branch 'master' into ng-p2p-discovery
ngotchac Sep 14, 2018
bbf99bd
Increase slow Discovery rate
ngotchac Sep 14, 2018
6ebdc56
Add `deprecated` to deprecated_echo_hash
ngotchac Sep 14, 2018
e98c191
Refactor `discovery_round` branching
ngotchac Sep 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 32 additions & 24 deletions util/network-devp2p/src/discovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ pub struct Discovery<'a> {
id_hash: H256,
secret: Secret,
public_endpoint: NodeEndpoint,
started_discovering: bool,
discovering: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a separate bool, I think making discovery_round an Option<u16> is cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitly, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two bools look like their doing the same thing? Could we ever be discovering == true but started_discovering == false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So started_discovery is used in order to start the first round of discovery ever. The idea is that it is set to false at the beginning, then we add some peers (bootnodes), ping them, and as soon as the pings are answered or timedout, we start the first discovery round. Then we start a new round every 10s or 30s depending on the number of peers. Without this, we would have to wait for an extra 10s for it to run for the first time.
The discovering bool is used to prevent starting a new discovery round while there is already one going.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, much clearer now. Can't really think of a better name, maybe s/started_discovering/discovery_initiated/. Or just a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds better, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two booleans look like they're doing the same thing, i.e. could we ever be in a state where discovering is true and started_discovering is false?

discovery_round: u16,
discovery_id: NodeId,
Expand All @@ -154,6 +155,7 @@ impl<'a> Discovery<'a> {
id_hash: keccak(key.public()),
secret: key.secret().clone(),
public_endpoint: public,
started_discovering: false,
discovering: false,
discovery_round: 0,
discovery_id: NodeId::new(),
Expand Down Expand Up @@ -500,7 +502,7 @@ impl<'a> Discovery<'a> {
let timestamp: u64 = rlp.val_at(2)?;
self.check_timestamp(timestamp)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should check error here and remove from in_flight_ping if there is an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will just go expired after a few 100ms, so I think it's fine as-is.


let expected_node = match self.in_flight_pings.entry(*node_id) {
let mut expected_node = match self.in_flight_pings.entry(*node_id) {
Entry::Occupied(entry) => {
let expected_node = {
let request = entry.get();
Expand All @@ -509,7 +511,7 @@ impl<'a> Discovery<'a> {
None
} else {
if request.deprecated_echo_hash == echo_hash {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know how such deprecated functionalities are managed, ideally this should disappear in a few parity version I think. It is an open question, maybe we can use things like rust deprecated tag or other to keep trace of it (and not forget it in the base code ad vita eternam)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe we want to keep it for better compatibility (not really my point of view)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure either... @5chdn maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a #[deprecated()] to deprecated_echo_hash and move on :)

I really need this for the release:)

info!(target: "discovery", "Got a Pong from old Parity version.");
trace!(target: "discovery", "Got Pong from an old parity-ethereum version.");
}
// Get the UDP port from the saved request, and
// the address from the incoming connection
Expand All @@ -529,38 +531,40 @@ impl<'a> Discovery<'a> {
expected_node
},
Entry::Vacant(_) => {
debug!(target: "discovery", "Got unexpected Pong from {:?} ; request not found", &from);
None
},
};

// If the Node couldn't be found from its ID,
// Try to find it in from the hash that was sent back
// if expected_node.is_none() {
// self.in_flight_pings.retain(|_, ping_request| {
// if ping_request.echo_hash == echo_hash || ping_request.deprecated_echo_hash == echo_hash {
// debug!(target: "discovery",
// "Found corresponding request expected={:x}@{} ; found={:x}@{}",
// node_id, from,
// ping_request.node.id, ping_request.node.endpoint.address
// );
// expected_node = Some(NodeEntry {
// id: *node_id,
// endpoint: NodeEndpoint {
// udp_port: ping_request.node.endpoint.udp_port,
// address: from.clone(),
// }
// });
// false
// } else {
// true
// }
// });
// }
if expected_node.is_none() {
let mut found = false;
self.in_flight_pings.retain(|_, ping_request| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would this be the case? Iterating through the whole in_flight_pings map on every unexpected PONG doesn't seem like a good idea to me.

If there's a good reason for this, the map might as well be keyed on the ping hash (except that it would be harder to maintain compatibility with the deprecated_echo_hash thing).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is to cater for the fact that the node answering your Ping might not have the NodeID you've been told. Another mechanism would be to Ping back any unknown received Pong ; I can try that if it sounds better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the NodeID is wrong, then the address (NodeID + Network Endpoint) is wrong, so I think it's fine to just drop the PONG.

Consider also that the sender of the PONG will likely PING back to us after receiving the PING, from which we can get the correct NodeID, so this unlikely case should resolve itself anyway.

Copy link
Contributor Author

@ngotchac ngotchac Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I'll remove it from now. Note that Geth doesn't Ping back on received Ping, it just adds it to their NodeTable .

if !found && ping_request.echo_hash == echo_hash || ping_request.deprecated_echo_hash == echo_hash {
debug!(target: "discovery",
"Found corresponding request expected={:x}@{} ; found={:x}@{}",
node_id, from,
ping_request.node.id, ping_request.node.endpoint.address
);
found = true;
expected_node = Some(NodeEntry {
id: *node_id,
endpoint: NodeEndpoint {
udp_port: ping_request.node.endpoint.udp_port,
address: from.clone(),
}
});
false
} else {
true
}
});
}

if let Some(node) = expected_node {
Ok(self.update_node(node))
} else {
debug!(target: "discovery", "Got unexpected Pong from {:?} ; request not found", &from);
Ok(None)
}
}
Expand Down Expand Up @@ -711,6 +715,10 @@ impl<'a> Discovery<'a> {

if self.discovering {
self.discover();
// Start discovering if the first pings have been sent (or timed out)
} else if self.in_flight_pings.len() == 0 && !self.started_discovering {
self.started_discovering = true;
self.start();
}
}

Expand Down