Honor --max-peers if --min-peers is not specified (#8087)

* Change interpretation min and max peers

* Only min specified -> Set min to that value and max to default
* Only max specified -> Set min and max to that value
* Both specified -> Set min the smallest value and max to the largest value

* simplify logic, new ParseError & add tests

* simplify code according to the review comments

* address review comments

* more fine-grained tests

parity/cli/mod.rs

@@ -409,11 +409,11 @@ usage! {
 			"--port=[PORT]",
 			"Override the port on which the node should listen.",
 
-			ARG arg_min_peers: (u16) = 25u16, or |c: &Config| c.network.as_ref()?.min_peers.clone(),
+			ARG arg_min_peers: (Option<u16>) = None, or |c: &Config| c.network.as_ref()?.min_peers.clone(),
 			"--min-peers=[NUM]",
 			"Try to maintain at least NUM peers.",
 
-			ARG arg_max_peers: (u16) = 50u16, or |c: &Config| c.network.as_ref()?.max_peers.clone(),
+			ARG arg_max_peers: (Option<u16>) = None, or |c: &Config| c.network.as_ref()?.max_peers.clone(),
 			"--max-peers=[NUM]",
 			"Allow up to NUM peers.",
 
@@ -1504,8 +1504,8 @@ mod tests {
 			// -- Networking Options
 			flag_no_warp: false,
 			arg_port: 30303u16,
-			arg_min_peers: 25u16,
+			arg_min_peers: Some(25u16),
-			arg_max_peers: 50u16,
+			arg_max_peers: Some(50u16),
 			arg_max_pending_peers: 64u16,
 			arg_snapshot_peers: 0u16,
 			arg_allow_ips: "all".into(),
@@ -1896,4 +1896,18 @@ mod tests {
 			stratum: None,
 		});
 	}
+
+	#[test]
+	fn should_not_accept_min_peers_bigger_than_max_peers() {
+		match Args::parse(&["parity", "--max-peers=39", "--min-peers=40"]) {
+			Err(ArgsError::PeerConfiguration) => (),
+			_ => assert_eq!(false, true),
+		}
+	}
+
+	#[test]
+	fn should_accept_max_peers_equal_or_bigger_than_min_peers() {
+		Args::parse(&["parity", "--max-peers=40", "--min-peers=40"]).unwrap();
+		Args::parse(&["parity", "--max-peers=100", "--min-peers=40"]).unwrap();
+	}
 }

parity/cli/usage.rs

@@ -161,6 +161,7 @@ macro_rules! usage {
 			Clap(ClapError),
 			Decode(toml::de::Error),
 			Config(String, io::Error),
+			PeerConfiguration,
 		}
 
 		impl ArgsError {
@@ -177,6 +178,10 @@ macro_rules! usage {
 						println_stderr!("{}", e);
 						process::exit(2)
 					},
+					ArgsError::PeerConfiguration => {
+						println_stderr!("You have supplied `min_peers` > `max_peers`");
+						process::exit(2)
+					}
 				}
 			}
 		}
@@ -312,6 +317,13 @@ macro_rules! usage {
 			pub fn parse<S: AsRef<str>>(command: &[S]) -> Result<Self, ArgsError> {
 				let raw_args = RawArgs::parse(command)?;
 
+				if let (Some(max_peers), Some(min_peers)) = (raw_args.arg_max_peers, raw_args.arg_min_peers) {
+					// Invalid configuration pattern `mix_peers` > `max_peers`
+					if min_peers > max_peers {
+						return Err(ArgsError::PeerConfiguration);
+					}
+				}
+
 				// Skip loading config file if no_config flag is specified
 				if raw_args.flag_no_config {
 					return Ok(raw_args.into_args(Config::default()));

parity/configuration.rs

@@ -14,12 +14,12 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
+use std::cmp::{max, min};
 use std::time::Duration;
 use std::io::Read;
 use std::net::SocketAddr;
 use std::path::{Path, PathBuf};
 use std::collections::BTreeMap;
-use std::cmp::max;
 use std::str::FromStr;
 use cli::{Args, ArgsError};
 use hash::keccak;
@@ -55,6 +55,9 @@ use account::{AccountCmd, NewAccount, ListAccounts, ImportAccounts, ImportFromGe
 use snapshot::{self, SnapshotCommand};
 use network::{IpFilter};
 
+const DEFAULT_MAX_PEERS: u16 = 50;
+const DEFAULT_MIN_PEERS: u16 = 25;
+
 #[derive(Debug, PartialEq)]
 pub enum Cmd {
 	Run(RunCmd),
@@ -469,8 +472,9 @@ impl Configuration {
 	}
 
 	fn max_peers(&self) -> u32 {
-		let peers = self.args.arg_max_peers as u32;
+		self.args.arg_max_peers
-		max(self.min_peers(), peers)
+			.or(max(self.args.arg_min_peers, Some(DEFAULT_MAX_PEERS)))
+			.unwrap_or(DEFAULT_MAX_PEERS) as u32
 	}
 
 	fn ip_filter(&self) -> Result<IpFilter, String> {
@@ -481,7 +485,9 @@ impl Configuration {
 	}
 
 	fn min_peers(&self) -> u32 {
-		self.args.arg_peers.unwrap_or(self.args.arg_min_peers) as u32
+		self.args.arg_min_peers
+			.or(min(self.args.arg_max_peers, Some(DEFAULT_MIN_PEERS)))
+			.unwrap_or(DEFAULT_MIN_PEERS) as u32
 	}
 
 	fn max_pending_peers(&self) -> u32 {
@@ -1970,4 +1976,56 @@ mod tests {
 		assert_eq!(std.directories().cache, dir::helpers::replace_home_and_local(&base_path, &local_path, ::dir::CACHE_PATH));
 		assert_eq!(base.directories().cache, "/test/cache");
 	}
+
+	#[test]
+	fn should_respect_only_max_peers_and_default() {
+		let args = vec!["parity", "--max-peers=50"];
+		let conf = Configuration::parse(&args, None).unwrap();
+		match conf.into_command().unwrap().cmd {
+			Cmd::Run(c) => {
+				assert_eq!(c.net_conf.min_peers, 25);
+				assert_eq!(c.net_conf.max_peers, 50);
+			},
+			_ => panic!("Should be Cmd::Run"),
+		}
+	}
+
+	#[test]
+	fn should_respect_only_max_peers_less_than_default() {
+		let args = vec!["parity", "--max-peers=5"];
+		let conf = Configuration::parse(&args, None).unwrap();
+		match conf.into_command().unwrap().cmd {
+			Cmd::Run(c) => {
+				assert_eq!(c.net_conf.min_peers, 5);
+				assert_eq!(c.net_conf.max_peers, 5);
+			},
+			_ => panic!("Should be Cmd::Run"),
+		}
+	}
+
+	#[test]
+	fn should_respect_only_min_peers_and_default() {
+		let args = vec!["parity", "--min-peers=5"];
+		let conf = Configuration::parse(&args, None).unwrap();
+		match conf.into_command().unwrap().cmd {
+			Cmd::Run(c) => {
+				assert_eq!(c.net_conf.min_peers, 5);
+				assert_eq!(c.net_conf.max_peers, 50);
+			},
+			_ => panic!("Should be Cmd::Run"),
+		}
+	}
+
+	#[test]
+	fn should_respect_only_min_peers_and_greater_than_default() {
+		let args = vec!["parity", "--min-peers=500"];
+		let conf = Configuration::parse(&args, None).unwrap();
+		match conf.into_command().unwrap().cmd {
+			Cmd::Run(c) => {
+				assert_eq!(c.net_conf.min_peers, 500);
+				assert_eq!(c.net_conf.max_peers, 500);
+			},
+			_ => panic!("Should be Cmd::Run"),
+		}
+	}
 }