Service's Configuration has optional fields that shouldn't be optional

[cecton]

Following the discussion on: https://github.com/paritytech/substrate/pull/4692/files/bea809d4c14a2ede953227ac885e3b3f9771c548#r372049016

There are 2 conflicting things we would like to have:

• Running a node requires a Configuration and some of the fields are not optional but they are defined as Option
• We should be able to define defaults to the Configuration before the command line get parsed. Therefore some of the fields need to be Option

The idea is to be able to make a Configuration before it being modified by the commandline arguments. For that I need defaults and chain_spec, like some other fields, are Option even though they are required for running a node.

It's not ideal. I think it would be best to have some kind of intermediate object, one that has the Options that can be converted to one that doesn't have and it would be fail-able at that point.

[TimvanScherpenzeel]

I was going to make a new ticket but I think my issue is possible related to this ticket and known.

It is currently not possible to disable the grafana data source server by setting the grafana_port to None as suggested in:

substrate/client/service/src/config.rs

Lines 75 to 76 in 26e37ff

	/// Grafana data source http port. `None` if disabled.
	pub grafana_port: Option<SocketAddr>,

This appears to be because of:

substrate/client/cli/src/lib.rs

Lines 637 to 642 in 26e37ff

	if config.grafana_port.is_none() {
	let grafana_interface: &str = if cli.grafana_external { "0.0.0.0" } else { "127.0.0.1" };
	config.grafana_port = Some(
	parse_address(&format!("{}:{}", grafana_interface, 9955), cli.grafana_port)?
	);
	}

The grafana data source server should not be a strict requirement to run a node right?

Same goes for rpc_http and rpc_ws (where setting None doesn't actually disable them):

substrate/client/cli/src/lib.rs

Lines 628 to 635 in 26e37ff

	if config.rpc_http.is_none() {
	let rpc_interface: &str = interface_str(cli.rpc_external, cli.unsafe_rpc_external, cli.validator)?;
	config.rpc_http = Some(parse_address(&format!("{}:{}", rpc_interface, 9933), cli.rpc_port)?);
	}
	if config.rpc_ws.is_none() {
	let ws_interface: &str = interface_str(cli.ws_external, cli.unsafe_ws_external, cli.validator)?;
	config.rpc_ws = Some(parse_address(&format!("{}:{}", ws_interface, 9944), cli.ws_port)?);
	}

[cecton]

Is it new? It doesn't look related to my change, the code was there before:

substrate/client/cli/src/lib.rs

Lines 1072 to 1076 in 1ed1cc5

	config.rpc_http = Some(parse_address(&format!("{}:{}", rpc_interface, 9933), cli.rpc_port)?);
	config.rpc_ws = Some(parse_address(&format!("{}:{}", ws_interface, 9944), cli.ws_port)?);
	config.grafana_port = Some(
	parse_address(&format!("{}:{}", grafana_interface, 9955), cli.grafana_port)?
	);

[TimvanScherpenzeel]

It is not directly related to your latest changes I believe but I thought it might relate to this ticket. If it does not I’ll gladly make a new ticket specifically for the issue that I’m facing.

…

Op 30 jan. 2020 om 19:01 heeft Cecile Tonglet ***@***.***> het volgende geschreven:  Is it new? It doesn't look related to my change, the code was there before: https://github.com/paritytech/substrate/blob/1ed1cc542110aaaee9cf0de87a7c256a1de407c6/client/cli/src/lib.rs#L1072-L1076 — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[cecton]

No no I think you're right, it's definitely related to this ticket. I was just afraid I broke something.

The comment really is an indication that it was meant to be an option but clearly it's never None when running a node.

[cecton]

@TimvanScherpenzeel actually I'm an idiot. The main reason for this refactoring was to allow more flexibility so you can customize at different places how you run the node. So you CAN actually put those values to None if you want.

In the most common case, you would use the "run" function to run the node:

substrate/bin/node-template/src/command.rs

Lines 39 to 46 in 26e37ff

	None => sc_cli::run(
	config,
	opt.run,
	service::new_light,
	service::new_full,
	chain_spec::load_spec,
	&version,
	)

But what this function does is just initializing and running the node:

substrate/client/cli/src/lib.rs

Lines 245 to 249 in 26e37ff

	{
	init(&mut config, spec_factory, &run_cmd.shared_params, version)?;
	run_cmd.run(config, new_light, new_full, version)
	}

So what you can do is actually initialized yourself, modify the configuration and then run the node:

	sc_cli::init(&mut config, spec_factory, &run_cmd.shared_params, version)?;

	config.grafana_port = None;

	run_cmd.run(config, new_light, new_full, version)

[cecton]

I got a brilliant idea on how to solve this ticket and the implementation is almost exactly like this crate: https://crates.io/crates/optional_struct but also kinda the opposite:

• instead of derive it would be a proc macros: so we can have the optional struct in sc_cli and not in sc_service (because it doesn't make sense to have it on sc_service)
• instead of [struct].apply_options([optional_struct]) it would be a [optional_struct].into() -> [struct]
• instead of defining all the default values of each field in one function, define all the default values of each field in a dedicated function (fn default_[field]()). If one function is not declared: fail to compile (this will force the developer who adds one field to write down the default value)
• the From (into) implementation would be generated automatically and will use default_[field]() for every field that is None

With this implementation we would have:

• sc_service::Configuration not have Option on fields that are required
• the compiler will enforce that every field added to sc_service::Configuration will have a default value defined in a function in sc_cli (so sc_cli would fail to compile if we add a field that is not optional and doesn't have a default value)
• it would avoid mistakes like here where the value in config is forced without checking if the option is already defined in the config

[cecton]

Though this whole thing doesn't make #4842 irrelevant: #4842 adds:

• tests
• better clarity (separated modules)
• the API is less error prone and prevents issues like this one where the wrong structopt is used to populate the config

[cecton]

@bkchr can you check this private repo here and let me know what you think of it?

This is very similar to optional_struct but it does enforce defaults at compile time:

Example build output where a default field is missing its default function:

error[E0599]: no function or associated item named `default_field_2` found for type `Test` in the current scope
  --> examples/fail_to_compile.rs:6:1
   |
6  |   optional_struct!(struct Test {
   |   -                ----------- function or associated item `default_field_2` not found for this
   |  _|
   | |
7  | |     field_1: u32,
8  | |     field_2: usize,
9  | |     field_opt: Option<String>,
10 | | }
11 | | );
   | |  ^
   | |  |
   | |  function or associated item not found in `Test`
   | |__help: there is a method with a similar name: `default_field_1`
   |    in this macro invocation

[cecton]

Mentioning #5204 which shows that the API is still not good.

Note 1: probably enforcing the fields to be required would prevent this situation where database and config_dir are not set.

Note 2: optional_struct won't help for this case.