lightningd should not crash if setconfig fails to write new config file

[whitslack]

Issue and Steps to Reproduce

On my system, the user that runs lightningd does not have permission to write to the directory containing the config file. I tried to use setconfig to change a dynamic config setting, and CLN straight-up crashed. Not nice at all since it'll now take ~15 minutes for my node to start again! (That's another issue that needs fixing.)

$ lightning-cli setconfig min-capacity-sat 975000
lightning-cli: reading response: socket closed

From the log:

2024-12-21T18:03:24.571Z DEBUG   lightningd: setconfig!
2024-12-21T18:03:24.572Z INFO    lightningd: setconfig: commented out line 184 of /etc/lightning/lightningd.conf (min-capacity-sat=1000000)
2024-12-21T18:03:24.572Z **BROKEN** lightningd: Creating /etc/lightning/lightningd.conf.setconfig.RKZ2Uf: Permission denied
2024-12-21T18:03:24.869Z **BROKEN** lightningd: FATAL SIGNAL 6 (version 24.08.2-gentoo-r0)
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: common/daemon.c:75 (crashdump) 0x5582f31798c2
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x7f696f4aaf0f
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x7f696f50507b
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x7f696f4aae71
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x7f696f48f4f1
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/log.c:728 (fatal_vfmt) 0x5582f3121af1
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/log.c:738 (fatal) 0x5582f3121b8d
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/configs.c:496 (configfile_replace_var) 0x5582f31507cc
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/configs.c:526 (configvar_save) 0x5582f31507cc
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/configs.c:565 (setconfig_success) 0x5582f31507cc
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/configs.c:548 (setconfig_success) 0x5582f31507cc
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/configs.c:631 (json_setconfig) 0x5582f3150968
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/jsonrpc.c:796 (command_exec) 0x5582f311c4be
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/plugin_hook.c:250 (plugin_hook_call_) 0x5582f31489b8
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/jsonrpc.c:1027 (plugin_hook_call_rpc_command) 0x5582f311db82
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/jsonrpc.c:1145 (parse_request) 0x5582f311db82
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/jsonrpc.c:1251 (read_json) 0x5582f311db82
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:60 (next_plan) 0x5582f31d5e36
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:422 (do_plan) 0x5582f31d5e36
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: ccan/ccan/io/io.c:439 (io_ready) 0x5582f31d5e36
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: ccan/ccan/io/poll.c:455 (io_loop) 0x5582f31d7f7b
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/io_loop_with_timers.c:22 (io_loop_with_timers) 0x5582f311bebe
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: lightningd/lightningd.c:1470 (main) 0x5582f30f316b
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x7f696f490f59
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x7f696f491014
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0x5582f30f5140
2024-12-21T18:03:24.869Z **BROKEN** lightningd: backtrace: (null):0 ((null)) 0xffffffffffffffff

For what it's worth, to a sysadmin it is very unexpected and unwelcome for a daemon to modify its own config files merely as a consequence of modifying a runtime parameter at runtime. I'm sure you thought you were being helpful by having setconfig rewrite the config file, but automatic persistence of config changes is actually contrary to the typical admin workflow of: apply config change, test outcome, commit to persistent storage if acceptable. I intentionally made my config file unwritable by my lightning user explicity to sidestep this undesired behavior, so imagine my shock when my whole node crashed just because setconfig couldn't provide a "convenience" to me.

getinfo output

This is with CLN 24.08.2. Apologies if this has already been fixed in 24.11.

[rustyrussell]

This is actually what most people want, though. They don't want their changes to vanish mysteriously on restart. Nor do we want config options silently overridden elsewhere because it was set using setconfig some time in the past.

Nor do we want partial failure, where we don't fully set it. Yet we have an atomicity issue, which is why we crash here.

I think the answers are:

1. Test for writability before activation.
2. setconfig option to non-persistently change config.
3. Prefer to add dynamic changes to a file ending in ".dynamic" to encourage grouping. Create and include this ton new install by default.
4. Add an option to change persistence to off by default.

You on Signal BTW?

[whitslack]

vanish mysteriously on restart

There's nothing mysterious. The config file specifies how the runtime variables are to be initialized at startup. Changing those runtime variables at runtime is orthogonal to changing the config file that provides their initial values. I think what you're trying to argue is that the variables conceptually outlast any one particular session, more like the "preferences" of a graphical desktop application that are persisted at the same time as they are applied. That's possibly an okay argument, but then you shouldn't allow those preferences to be configured outside of the running application. How do you change the default paper size for Microsoft Word documents outside of a running instance of Microsoft Word? You aren't meant to. There is no user-accessible config file for Word; you use the preferences dialog box inside the application to configure it. So, what you're doing with Core Lightning is mixing metaphors: you're saying, here's this user-editable file where the user is meant to specify how they want the application to behave, but then, by the way, the application is also going to manage that same file. Now you have two head chefs in the same kitchen, and they will forever be stepping on each other's toes.

I don't see anything wrong with simply not writing the config change to disk if the file can't be written due to permissions but otherwise carrying on normally, perhaps logging a warning that the config file couldn't be written. That was certainly my expectation of what would happen.

You on Signal BTW?

I use Signal, but I don't see the relevance here.