Filter-out nodes.json #7716

tomusdrw · 2018-01-29T03:01:49Z

If we reach more than 1024 nodes in the table, filter out the ones that we never attempted to connect to.

(untested yet, need to get better internet connection to test it properly)

andresilva · 2018-01-29T11:59:20Z

util/network/src/node_table.rs

+		let nodes = node_ids.into_iter()
+			.map(|id| self.nodes.get(&id).expect("self.nodes() only returns node IDs from self.nodes"))
+			.map(|node| node.clone())
+			.filter(|node| if len > MAX_NODES { node.last_attempted.is_some() } else { true })


Maybe we should change the nodes sorting so that nodes that we never connected to have a lower priority and then we .take(MAX_NODES) on the sorted list. This way we always get a node table with a predictable size.

Sounds like a good idea, we might need to tune the comparator a bit though. Might be difficult to decide what nodes are more valuable: never connected or connected once, but with a single failure.
In general I need to double check if we reset failures counter in case we successfuly connect to the node, otherwise valuable peers might be dropped to often (with time they would accumulate a lot of failures, so the preference would be to completely new nodes).

andresilva · 2018-01-29T19:10:07Z

I've updated this to remove the last_attempted and instead keep track of the number of attempts per node. The nodes are then sorted according to their failure percentage. Nodes that we never connected to have a default failure percentage of 50%.

I also noticed that my nodes.json file is corrupt most of the times because of the unclean shutdown (#7695) and is basically ignored.

andresilva · 2018-01-30T01:27:03Z

Here's what my nodes.json looks like (https://gist.github.com/andresilva/78e8caeb021378206cd43863840af519). After I restart the node it seems to pick up peers a bit faster in the beginning but it may just be my impression. 🙃

❯ ./target/release/parity --min-peers 100
2018-01-30 01:22:51 UTC Starting Parity/v1.10.0-unstable-6dd69196d-20180129/x86_64-macos/rustc1.24.0-beta.8
...
2018-01-30 01:23:04 UTC Imported #4997033 1888…31df (179 txs, 7.49 Mgas, 636.76 ms, 27.11 KiB) + another 1 block(s) containing 76 tx(s)
2018-01-30 01:23:20 UTC Imported #4997033 d609…65b8 (166 txs, 6.66 Mgas, 363.01 ms, 25.29 KiB)
2018-01-30 01:23:28 UTC   36/100 peers   219 KiB chain 100 MiB db 0 bytes queue 31 KiB sync  RPC:  0 conn,  0 req/s,   0 µs

tomusdrw

Andre's changes look good to me.

tomusdrw · 2018-01-30T16:04:07Z

util/network/src/lib.rs


 #[cfg(test)]
 extern crate tempdir;

+


Superflous.

tomusdrw · 2018-01-30T16:07:50Z

util/network/src/node_table.rs

+		if self.attempts == 0 {
+			DEFAULT_FAILURE_PERCENTAGE
+		} else {
+			((self.failures as f64 / self.attempts as f64 * 100.0 / 5.0).round() * 5.0) as usize


Do we really need to fallback to floats? What about:

self.failures * 10_000 / self.attempts

(then we get a number within 0 .. 10_000 representing percents with two decimal places)

True, tbh I don't really care about the decimal places so it can just be self.failures * 100 / self.attempts. I'm using floats for rounding into buckets of 5% but integer math should do.

tomusdrw · 2018-01-30T16:10:24Z

util/network/src/node_table.rs

+			.collect();
+		refs.sort_by(|a, b| {
+			let mut ord = a.failure_percentage().cmp(&b.failure_percentage());
+			if ord == Ordering::Equal {


a.failure_percentage().cmp(&b.failure_percentage()) .then_with(|| a.failures.cmp(&b.failures)) .then_with(|| b.attempts.cmp(&a.attempts)

tomusdrw · 2018-01-30T16:11:09Z

util/network/src/node_table.rs

 	pub failures: u32,
-	pub last_attempted: Option<Tm>,


Interesting, I didn't notice it's not used at all.

tomusdrw · 2018-01-30T17:08:27Z

@arkpar Could you have a look if changes here make sense to you?

andresilva · 2018-01-30T17:44:32Z

Does this completely fix #7697 (and other duplicate issues)?

arkpar · 2018-01-30T19:14:06Z

LGTM

* Filter-out nodes.json * network: sort node table nodes by failure ratio * network: fix node table tests * network: fit node failure percentage into buckets of 5% * network: consider number of attempts in sorting of node table * network: fix node table grumbles

* Filter-out nodes.json (#7716) * Filter-out nodes.json * network: sort node table nodes by failure ratio * network: fix node table tests * network: fit node failure percentage into buckets of 5% * network: consider number of attempts in sorting of node table * network: fix node table grumbles * Fix client not being dropped on shutdown (#7695) * parity: wait for client to drop on shutdown * parity: fix grumbles in shutdown wait * parity: increase shutdown timeouts * Wrap --help output to 120 characters (#7626) * Update Clap dependency and remove workarounds * WIP * Remove line breaks in help messages for now * Multiple values can only be separated by commas (closes #7428) * Grumbles; refactor repeating code; add constant * Use a single Wrapper rather than allocate a new one for each call * Wrap --help to 120 characters rather than 100 characters

nueverest · 2018-02-01T23:23:44Z

Deleting nodes.json fixed the issue for me.

Filter-out nodes.json

6dd6919

tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. B0-patch labels Jan 29, 2018

5chdn added the P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Jan 29, 2018

5chdn added this to the 1.10 milestone Jan 29, 2018

andresilva reviewed Jan 29, 2018

View reviewed changes

andresilva force-pushed the td-nodes branch from cd32f91 to 2711085 Compare January 29, 2018 18:59

network: sort node table nodes by failure ratio

f9f3410

andresilva force-pushed the td-nodes branch from 2711085 to f9f3410 Compare January 29, 2018 19:29

andresilva added 3 commits January 29, 2018 22:23

network: fix node table tests

95848ad

network: fit node failure percentage into buckets of 5%

dede52f

network: consider number of attempts in sorting of node table

de5c75e

tomusdrw commented Jan 30, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 30, 2018

tomusdrw changed the title ~~[WIP] Filter-out nodes.json~~ Filter-out nodes.json Jan 30, 2018

tomusdrw requested a review from arkpar January 30, 2018 17:08

network: fix node table grumbles

11d5d15

andresilva force-pushed the td-nodes branch from 952803e to 11d5d15 Compare January 30, 2018 17:08

5chdn approved these changes Jan 31, 2018

View reviewed changes

5chdn merged commit f5c68c6 into master Jan 31, 2018

5chdn deleted the td-nodes branch January 31, 2018 08:50

5chdn mentioned this pull request Jan 31, 2018

Parity connecting to very few peers #7697

Closed

andresilva mentioned this pull request Jan 31, 2018

[Beta] Backports #7756

Merged

andresilva mentioned this pull request Jan 31, 2018

[Stable] Backports #7758

Merged

5chdn mentioned this pull request Jan 31, 2018

Failures parameter in nodes.json blocks connection to some peers #7761

Closed

5chdn mentioned this pull request Mar 23, 2018

Can't keep peer count above min #7531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter-out nodes.json #7716

Filter-out nodes.json #7716

tomusdrw commented Jan 29, 2018

andresilva Jan 29, 2018

tomusdrw Jan 29, 2018

andresilva commented Jan 29, 2018 •

edited

andresilva commented Jan 30, 2018

tomusdrw left a comment

tomusdrw Jan 30, 2018

tomusdrw Jan 30, 2018

andresilva Jan 30, 2018

tomusdrw Jan 30, 2018

tomusdrw Jan 30, 2018

tomusdrw commented Jan 30, 2018

andresilva commented Jan 30, 2018

arkpar commented Jan 30, 2018

nueverest commented Feb 1, 2018

Filter-out nodes.json #7716

Filter-out nodes.json #7716

Conversation

tomusdrw commented Jan 29, 2018

andresilva Jan 29, 2018

Choose a reason for hiding this comment

tomusdrw Jan 29, 2018

Choose a reason for hiding this comment

andresilva commented Jan 29, 2018 • edited

andresilva commented Jan 30, 2018

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Jan 30, 2018

Choose a reason for hiding this comment

tomusdrw Jan 30, 2018

Choose a reason for hiding this comment

andresilva Jan 30, 2018

Choose a reason for hiding this comment

tomusdrw Jan 30, 2018

Choose a reason for hiding this comment

tomusdrw Jan 30, 2018

Choose a reason for hiding this comment

tomusdrw commented Jan 30, 2018

andresilva commented Jan 30, 2018

arkpar commented Jan 30, 2018

nueverest commented Feb 1, 2018

andresilva commented Jan 29, 2018 •

edited