Config changes #3268

rustyrussell · 2019-11-18T09:51:05Z

Files now moved into bitcoin/testnet/regtest/etc subdir.
Config file still parsed in old location, but also network/config is parsed
Default for new installs in bitcoin, not testnet!

jb55 · 2019-11-18T16:55:50Z

this breaks my current system clightning system service with: network: not permitted in configuration files. since I had network=bitcoin in my config file, so now I will need to add it to the systemd service line. Not too big a deal but just giving a heads up.

jb55 · 2019-11-18T19:00:03Z

Another thing that is a bit confusing is lightning-cli now shows this message when clightning isn't running:
lightning-cli: WARNING: default network changing in 2020: please set network=testnet in config!

rustyrussell · 2019-11-18T23:09:29Z

Oops, this is a bug. Network should only be disallowed in the /config file. Should still be valid on the root config.

rustyrussell · 2019-11-18T23:55:17Z

Thanks @jb55 ... and this time with tests!

BTW, you can restore with mv -i ~/.lightning/bitcoin/* ~/.lightning && rmdir ~/.lightning/bitcoin if you want to test again :)

jb55 · 2019-11-19T00:19:09Z

hmm I was able to test it and the network=bitcoin in my config file worked without doing your mv and rmdir command, but then again my folder is ~/.lightning-bitcoin which I explicitly set as an argument to lightningd. I also explicitly set --conf

cdecker

This might be a very painful upgrade, but honestly I don't see how we could make it less painful. Some general comments below:

We have no recursion depth, so circular includes can result in endless
loops
Starting a node without a --network parameter may end up moving testnet
files into the bitcoin directory and treating them like mainnet.

The latter is really what bothers me, we are moving things around without really knowing what network the config is for, but I also don't see how we could avoid that without reading the DB chain and then comparing with the chainparams.

cdecker · 2019-11-21T16:54:55Z

lightningd/options.c

+		if (all_args[i] == NULL)
+			continue;
+
+		if (!strstarts(all_args[i], "--")) {


This makes me think we should make the all_args entries into a struct that carries a type with it. Something like this seems better for future extensibility:

enum ConfigLineType { CONFIG_COMMENT, CONFIG_ARGUMENT, CONFIG_INCLUDE, }; struct ConfigEntry { ConfigLineType type; char *line; char *filename; size_t pos; };

It can also be used to reconstruct the config from the in-memory representation and provide nicer error messages.

niftynei

I think this could help people keep testnet/mainnet nodes running at the same time, so overall I'm a fan.

The includes capability is particularly nice.

As @cdecker mentioned, if we want to be super safe for the transition/upgrade path we could check the database for the chainhash. Not sure what our risk profile is though :)

niftynei · 2019-11-21T19:46:17Z

common/configdir.c

+void parse_include(const char *filename, bool must_exist, bool early)
+{
+	char *contents, **lines;
+	char **all_args; /*For each line: either argument string or NULL*/


might also be a path to load for an includes

niftynei · 2019-11-21T19:48:17Z

common/configdir.c

+	all_args = tal_arr_label(options_ctx, char *, tal_count(lines) - 1,
+				 TAL_LABEL(options_array_notleak, ""));
+
+	for (i = 0; i < tal_count(lines) - 1; i++) {


does this skip the last line because the tal_strsplit above leaves the last line blank?

It NULL-terminates the array, yes.

niftynei · 2019-11-21T20:25:37Z

common/configdir.c

+		config_basedir = base;
+
+	/* If it doesn't exist, it's not testnet. */
+	if (stat(base, &st) != 0) {


i think you might mean to be testing config_basedir here?

niftynei · 2019-11-21T20:27:36Z

common/configdir.c

+	}
+
+	/* Does it have a bitcoin/ subdir and no testnet/ subdir? */
+	if (stat(path_join(base, config_basedir, "bitcoin"), &st) == 0


ok i'll admit i'm a bit confused here. at L255 we might have set config_basedir to be base; in that case we'd be joining the same path twice.

First arg is context: we're not relying on tmpctx here, so I'm abusing base as a per-function ctx. Will comment :)

niftynei · 2019-11-21T20:34:18Z

lightningd/options.c

@@ -972,7 +974,7 @@ void handle_early_opts(struct lightningd *ld, int argc, char *argv[])
 				 ld->config_netdir);

 	/* Set default PID file name to be per-network */
-	ld->pidfile = tal_fmt(ld, "lightningd-%s.pid",
+	ld->pidfile = tal_fmt(ld, "../lightningd-%s.pid",


this is going to change where 'custom set' lightning-directory PID files are located.

No, because they get parsed next.

However, this "../" is wrong (network/ may be a symlink): this code is left over from before I gave in and made basedir and netdir explicit. Will fix:

/* Set default PID file name to be per-network (in base dir) */ ld->pidfile = path_join(ld, ld->config_basedir, tal_fmt(tmpctx, "lightningd-%s.pid", chainparams->network_name));

niftynei · 2019-11-21T20:35:58Z

tests/test_misc.py

+                          '--lightning-dir={}'.format(l1.daemon.opts.get("lightning-dir"))],
+                         stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    assert out.returncode == 1
+    assert "lightning-dir: not permitted in implicit configuration files" in out.stderr.decode('utf-8')


maybe add test for PID location?

rustyrussell · 2019-11-21T23:33:57Z

This might be a very painful upgrade, but honestly I don't see how we could make it less painful. Some general comments below:
* We have no recursion depth, so circular includes can result in endless
  loops

Yes, this I can fix quite easily.

* Starting a node without a `--network` parameter may end up moving `testnet`
  files into the `bitcoin` directory and treating them like mainnet.

That should not happen (unless they specify --allow-deprecated-apis=false): if nothing is specified, they get testnet on existing installs. I will add a test for this, however.

The latter is really what bothers me, we are moving things around without really knowing what network the config is for, but I also don't see how we could avoid that without reading the DB chain and then comparing with the chainparams.

rustyrussell · 2019-11-21T23:35:40Z

As @cdecker mentioned, if we want to be super safe for the transition/upgrade path we could check the database for the chainhash. Not sure what our risk profile is though :)

Doing so is amazingly ugly, since we need to plumb though a lot of code to get the db open early. So I chose the simpler approach of leaving existing installs as testnet by default.

niftynei

ACK 46e6b38

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: Config: configuration files now support `include`.

…annel_start With coming changes, this will segfault if we access it when param code is trying to get usage from functions. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightning-cli is going to need to know what network we're on, so it will need to parse the config files. Move the code which does the initial bootstrap parsing into common, as well as the config file parsing core. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This lets you have a default, but also a network-specific config. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-changed: Options: `config` and <network>/`config` read by default.

…twork> subdir Changelog-changed: .lightningd plugins and files moved into <network>/ subdir Changelog-changed: WARNING: If you don't have a config file, you now may need to specify the network to lightning-cli Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We're going to want this for changing the default network. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Changelog-changed: Default network (new installs) is now bitcoin, not testnet. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

1. "conf" can't be specified in a configuration file. 2. "lightning-dir" can't be specified in a configuration file unless the file was explicitly set with --conf=. 3. "network" options can't be set in a per-network configuration file. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

They're already qualified with network name, and there's little point moving them; it might even be dangerous if multiple are running. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Otherwise we don't print out the upgrading messages when we move things! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Don't spend too much effort on it, but this is better than running out of memory and crashing. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Do the same thing '--help' does with them; append `...`. Valgrind noticed that we weren't NUL-terminarting if answer was over 78 characters. Changelog-Fixed: JSONRPC: listconfigs appends '...' to truncated config options.

rustyrussell · 2019-11-23T01:47:58Z

Trivial rebase to fold fixes.

darosior

ACK ee79dee

I noticed a nit while testing though: Creating configuration [...] Moving [...] ... would be shown two times when upgrading testnet old directories, but not regtest ones.

rustyrussell · 2019-11-23T22:42:22Z

Yes, I can remove doubling in a separate patch, is a side effect of when we finally set up the log. Previously it was only for broken messages, which usually meant we exited before log setup anyway...

rustyrussell added this to the 0.7.4 milestone Nov 18, 2019

rustyrussell requested a review from cdecker as a code owner November 18, 2019 09:51

rustyrussell added the config label Nov 18, 2019

rustyrussell force-pushed the guilt/config-changes branch from 0b891dd to b16fdf6 Compare November 18, 2019 23:55

rustyrussell force-pushed the guilt/config-changes branch 4 times, most recently from d2546ce to 9188e0d Compare November 21, 2019 05:53

cdecker reviewed Nov 21, 2019

View reviewed changes

niftynei reviewed Nov 21, 2019

View reviewed changes

rustyrussell requested a review from cdecker November 22, 2019 00:35

niftynei approved these changes Nov 22, 2019

View reviewed changes

rustyrussell force-pushed the guilt/config-changes branch 3 times, most recently from 1337d51 to 8eb7eee Compare November 23, 2019 00:23

rustyrussell added 8 commits November 23, 2019 12:14

config: Add include directive support.

e92ca27

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Added: Config: configuration files now support `include`.

lightningd: don't use chainparams before param() call in json_fund_ch…

7fa7071

…annel_start With coming changes, this will segfault if we access it when param code is trying to get usage from functions. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

config: Read both top-level and network-subdir config files.

4550228

This lets you have a default, but also a network-specific config. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-changed: Options: `config` and <network>/`config` read by default.

lightningd: automatically move files for existing deployments.

f9c7938

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common: parse --allow-deprecated-apis extremely early.

e4272e1

We're going to want this for changing the default network. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common: change default network from testnet to mainnet for new installs.

fee3f50

Changelog-changed: Default network (new installs) is now bitcoin, not testnet. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell added 7 commits November 23, 2019 12:16

lightningd: keep pid files in top-level config dir.

1010594

They're already qualified with network name, and there's little point moving them; it might even be dangerous if multiple are running. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

pytest: test for config parsing errors.

f387cd4

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

log: print UNUSUAL messages before log initialization too.

c525240

Otherwise we don't print out the upgrading messages when we move things! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

pytest: upgrade tests.

de6ad3e

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

config: limit depth on includes.

cd37319

Don't spend too much effort on it, but this is better than running out of memory and crashing. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: fix handling of truncated config options.

ee79dee

Do the same thing '--help' does with them; append `...`. Valgrind noticed that we weren't NUL-terminarting if answer was over 78 characters. Changelog-Fixed: JSONRPC: listconfigs appends '...' to truncated config options.

rustyrussell force-pushed the guilt/config-changes branch from 8eb7eee to e04199d Compare November 23, 2019 01:47

rustyrussell force-pushed the guilt/config-changes branch from e04199d to ee79dee Compare November 23, 2019 01:48

darosior approved these changes Nov 23, 2019

View reviewed changes

rustyrussell merged commit 14997f6 into ElementsProject:master Nov 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config changes #3268

Config changes #3268

rustyrussell commented Nov 18, 2019

jb55 commented Nov 18, 2019

jb55 commented Nov 18, 2019

rustyrussell commented Nov 18, 2019

rustyrussell commented Nov 18, 2019 •

edited

Loading

jb55 commented Nov 19, 2019 •

edited

Loading

cdecker left a comment

cdecker Nov 21, 2019

niftynei left a comment

niftynei Nov 21, 2019

niftynei Nov 21, 2019

rustyrussell Nov 21, 2019

niftynei Nov 21, 2019

niftynei Nov 21, 2019

rustyrussell Nov 21, 2019

niftynei Nov 21, 2019

rustyrussell Nov 21, 2019

niftynei Nov 21, 2019

rustyrussell commented Nov 21, 2019

rustyrussell commented Nov 21, 2019

niftynei left a comment

rustyrussell commented Nov 23, 2019

darosior left a comment

rustyrussell commented Nov 23, 2019

Config changes #3268

Config changes #3268

Conversation

rustyrussell commented Nov 18, 2019

jb55 commented Nov 18, 2019

jb55 commented Nov 18, 2019

rustyrussell commented Nov 18, 2019

rustyrussell commented Nov 18, 2019 • edited Loading

jb55 commented Nov 19, 2019 • edited Loading

cdecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftynei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell commented Nov 21, 2019

rustyrussell commented Nov 21, 2019

niftynei left a comment

Choose a reason for hiding this comment

rustyrussell commented Nov 23, 2019

darosior left a comment

Choose a reason for hiding this comment

rustyrussell commented Nov 23, 2019

rustyrussell commented Nov 18, 2019 •

edited

Loading

jb55 commented Nov 19, 2019 •

edited

Loading