Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configuration rework #6243

Merged

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented May 9, 2023

To make room for a future config command to dynamically set configuration variables, I had to delve into our configuration subsystem. It took far longer than I'd expected, but here is the result.

Concretely:

  1. listconfigs produces a dict, in several cases giving multiple values (!), and also has a plugins and important-plugins sub-objects. This is deprecated in favor of a configs sub-object: each value in here is a dict, including source, and type information.
  2. listconfigs now shows plugin options as you'd expect, including dynamic ones! It also hands the config-set options to restarted plugins, a long requested feature.

I apologize for the scope of this: configuration is hard and I tried not to break anything.

Fixes: #5294

Example output:

{
  "id": "pytest:listconfigs#2",
  "result": {
    "configs": {
      "lightning-dir": {
        "value_str": "/tmp/ltests-_9t7jvko/test_listconfigs_1/lightning-2/",
        "source": "cmdline"
      },
      "network": {
        "value_str": "regtest",
        "source": "cmdline"
      },
      "testnet": {
        "set": false,
        "source": "default"
      },
      "signet": {
        "set": false,
        "source": "default"
      },
      "mainnet": {
        "set": false,
        "source": "default"
      },
      "regtest": {
        "set": false,
        "source": "default"
      },
      "allow-deprecated-apis": {
        "value_bool": false,
        "source": "cmdline"
      },
      "rpc-file": {
        "value_str": "lightning-rpc",
        "source": "default"
      },
      "plugin": {
        "values_str": [],
        "sources": []
      },
      "plugin-dir": {
        "values_str": [],
        "sources": []
      },
      "clear-plugins": {
        "set": false,
        "source": "default"
      },
      "disable-plugin": {
        "values_str": [],
        "sources": []
      },
      "important-plugin": {
        "values_str": [],
        "sources": []
      },
      "always-use-proxy": {
        "value_bool": false,
        "source": "default"
      },
      "daemon": {
        "set": false,
        "source": "default"
      },
      "large-channels": {
        "set": true,
        "source": "cmdline"
      },
      "experimental-dual-fund": {
        "set": false,
        "source": "default"
      },
      "experimental-onion-messages": {
        "set": false,
        "source": "default"
      },
      "experimental-offers": {
        "set": false,
        "source": "default"
      },
      "experimental-shutdown-wrong-funding": {
        "set": false,
        "source": "default"
      },
      "experimental-peer-storage": {
        "set": false,
        "source": "default"
      },
      "experimental-quiesce": {
        "set": false,
        "source": "default"
      },
      "announce-addr-dns": {
        "value_bool": false,
        "source": "default"
      },
      "rgb": {
        "value_str": "022d22",
        "source": "default"
      },
      "alias": {
        "value_str": "SILENTARTIST-v23.05-47-g99a2a78",
        "source": "default"
      },
      "pid-file": {
        "value_str": "/tmp/ltests-_9t7jvko/test_listconfigs_1/lightning-2/lightningd-regtest.pid",
        "source": "default"
      },
      "ignore-fee-limits": {
        "value_bool": false,
        "source": "cmdline"
      },
      "watchtime-blocks": {
        "value_int": 5,
        "source": "cmdline"
      },
      "max-locktime-blocks": {
        "value_int": 2016,
        "source": "default"
      },
      "funding-confirms": {
        "value_int": 1,
        "source": "default"
      },
      "require-confirmed-inputs": {
        "value_bool": false,
        "source": "default"
      },
      "cltv-delta": {
        "value_int": 6,
        "source": "cmdline"
      },
      "cltv-final": {
        "value_int": 5,
        "source": "cmdline"
      },
      "commit-time": {
        "value_int": 10,
        "source": "default"
      },
      "fee-base": {
        "value_int": 1,
        "source": "default"
      },
      "rescan": {
        "value_int": 1,
        "source": "cmdline"
      },
      "fee-per-satoshi": {
        "value_int": 10,
        "source": "default"
      },
      "htlc-minimum-msat": {
        "value_msat": "0msat",
        "source": "default"
      },
      "htlc-maximum-msat": {
        "value_msat": "18446744073709551615msat",
        "source": "default"
      },
      "max-concurrent-htlcs": {
        "value_int": 483,
        "source": "default"
      },
      "max-dust-htlc-exposure-msat": {
        "value_msat": "50000000msat",
        "source": "default"
      },
      "min-capacity-sat": {
        "value_int": 10000,
        "source": "default"
      },
      "addr": {
        "values_str": [
          "127.0.0.1:45091"
        ],
        "sources": [
          "cmdline"
        ]
      },
      "bind-addr": {
        "values_str": [],
        "sources": []
      },
      "announce-addr": {
        "values_str": [],
        "sources": []
      },
      "announce-addr-discovered": {
        "value_str": "auto",
        "source": "default"
      },
      "announce-addr-discovered-port": {
        "value_int": 19846,
        "source": "default"
      },
      "offline": {
        "set": false,
        "source": "default"
      },
      "autolisten": {
        "value_bool": false,
        "source": "default"
      },
      "accept-htlc-tlv-type": {
        "values_int": [],
        "sources": []
      },
      "disable-dns": {
        "set": true,
        "source": "cmdline"
      },
      "encrypted-hsm": {
        "set": false,
        "source": "default"
      },
      "rpc-file-mode": {
        "value_str": "0600",
        "source": "default"
      },
      "commit-fee": {
        "value_int": 100,
        "source": "default"
      },
      "subdaemon": {
        "values_str": [],
        "sources": []
      },
      "experimental-upgrade-protocol": {
        "set": false,
        "source": "default"
      },
      "log-level": {
        "value_str": "debug",
        "source": "cmdline"
      },
      "log-timestamps": {
        "value_bool": true,
        "source": "default"
      },
      "log-prefix": {
        "value_str": "lightning1-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
        "source": "cmdline"
      },
      "log-file": {
        "values_str": [
          "-",
          "/tmp/ltests-_9t7jvko/test_listconfigs_1/lightning-2/log"
        ],
        "sources": [
          "cmdline",
          "cmdline"
        ]
      },
      "dev-no-reconnect": {
        "set": true,
        "source": "cmdline"
      },
      "dev-fail-on-subdaemon-fail": {
        "set": true,
        "source": "cmdline"
      },
      "dev-bitcoind-poll": {
        "value_int": 1,
        "source": "cmdline"
      },
      "dev-fast-gossip": {
        "set": true,
        "source": "cmdline"
      },
      "bitcoin-datadir": {
        "value_str": "/tmp/ltests-_9t7jvko/test_listconfigs_1/lightning-2/",
        "source": "cmdline",
        "plugin": "/home/rusty/devel/cvs/lightning/plugins/bcli"
      },
      "bitcoin-rpcuser": {
        "value_str": "rpcuser",
        "source": "cmdline",
        "plugin": "/home/rusty/devel/cvs/lightning/plugins/bcli"
      },
      "bitcoin-rpcpassword": {
        "value_str": "rpcpass",
        "source": "cmdline",
        "plugin": "/home/rusty/devel/cvs/lightning/plugins/bcli"
      },
      "bitcoin-rpcport": {
        "value_int": 33779,
        "source": "cmdline",
        "plugin": "/home/rusty/devel/cvs/lightning/plugins/bcli"
      },
      "disable-mpp": {
        "set": false,
        "source": "default",
        "plugin": "/home/rusty/devel/cvs/lightning/plugins/pay"
      }
    }
  }
}

@SimonVrouwe
Copy link
Collaborator

If you're not in a hurry, then I would love to delve into this and test it (give me 10days).

One question/idea about listconfigs: Would it be possible to have the descriptions (pulled from the schemas, or from a plugin) somehow added to the result ?

@SimonVrouwe
Copy link
Collaborator

SimonVrouwe commented May 21, 2023

to dynamically set configuration variables

Does it mean we can switch from --regtest to --mainnet without a restart ? That sounds horrible and impossible.

listconfigs now shows plugin options as you'd expect

Not sure what you mean, but when I run it with --allow-deprecated-apis=false then it shows all plugin options flat inside the configs object and the only ordering being alphabetical without any ordering, was this the intention? edit

IMHO being able to see to which plugin an option belongs to makes everything more understandable. Maybe the configs object can have a plugin array again (containing path and options array ) ?

@SimonVrouwe
Copy link
Collaborator

And finally a little off-topic, it would be nice if users can see more option's default values, it makes everything less obscure. Many build-in plugins based on libplugin don't supply default values and for example to find the funder-* options values you have to look into the source.

@rustyrussell
Copy link
Contributor Author

rustyrussell commented May 23, 2023

If you're not in a hurry, then I would love to delve into this and test it (give me 10days).

One question/idea about listconfigs: Would it be possible to have the descriptions (pulled from the schemas, or from a plugin) somehow added to the result ?

[ Oops, I realized you are looking at the old PR. I hated this one, so I've reworked the format... Fixing tests now! ]

Well, since the format is now a dict, it's possible. It's a bit weird though, since it's not really dynamic.

@rustyrussell
Copy link
Contributor Author

to dynamically set configuration variables

Does it mean we can switch from --regtest to --mainnet without a restart ? That sounds horrible and impossible.

Yeah, we won't be doing that 🤣

It would be on selected options, which would explicitly opt-in.

listconfigs now shows plugin options as you'd expect

Not sure what you mean, but when I run it with --allow-deprecated-apis=false then it shows all plugin options flat inside the configs object and the only ordering being alphabetical without any ordering, was this the intention? edit

IMHO being able to see to which plugin an option belongs to makes everything more understandable. Maybe the configs object can have a plugin array again (containing path and options array ) ?

That was redundant with plugins list, however I like the idea of showing what plugin the options is associated with. Now we have a dict per option, this is easy to add.

Testing now...

@rustyrussell
Copy link
Contributor Author

Pushed updated, rebased version, and included more details in PR description!

@rustyrussell rustyrussell force-pushed the guilt/config-command branch 9 times, most recently from 580b4a3 to 13df5f3 Compare May 27, 2023 04:41
@SimonVrouwe
Copy link
Collaborator

Ok looks good, descriptions can easily be added now when desired (I think so).

For convenience I did it on this branch, see the last two commits.

Perhaps default values can also added to the descriptions, which should be easy for plugin options.
For build-in options this is a bit harder because those defaults are lost when a user configures them and some default values are transient, i.e. they depend on the network choice.

@rustyrussell
Copy link
Contributor Author

The problem now is that our descriptions are in several places: the man pages, and the code. We should settle on one place though.

The man page is more chatty, which is useful, but probably overkill for the code? Perhaps we should get one line from the code, and paste in the rest of the chatter for the man page. Anyway, I think this is a follow-on from this PR, which I'd like to merge soon.

@rustyrussell rustyrussell mentioned this pull request May 30, 2023
@rustyrussell
Copy link
Contributor Author

Trivial rebase to make --dev-allowdustreserve dev-only in its own commit, rather than a driveby...

Currently it fails if a field is missing, but sometimes that's OK.  So
allow a fieldname ending in `?` to mean "skip over if it's missing".

Signed-off-by: Rusty Russell <[email protected]>
We leave the code in contrib/pyln-client/pyln/client/lightning.py to handle
msat null fields for now, though, for a bit more compatibility.

Signed-off-by: Rusty Russell <[email protected]>
The new listconfigs fields will be objects, and this messes us up!

Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
This adds:
1. ability to search for an option by name.
2. allowance to set our own bits when registering options.
3. show callbacks which can say "don't show", and variable length.

Signed-off-by: Rusty Russell <[email protected]>
Now we can look up the name using opt_find_long, we can do that first,
simplifying this code.

Signed-off-by: Rusty Russell <[email protected]>
…l casing.

Note that this actually changes listconfigs output for three msat
fields, which were not changed with the great msat merge.  Since
listconfigs isn't actually used by grpc, and the values are always a
little vague, I simply changed this.

Changelog-Fixed: JSON-RPC: `listconfigs` `htlc-minimum-msat`, `htlc-maximum-msat` and `max-dust-htlc-exposure-msat` fields are now numbers, not strings.
Signed-off-by: Rusty Russell <[email protected]>
These are gathered from the config files and the commandline, but the
process is rather complex!  We want to remember where the options came
from in future (for a `setconfig` command), and also generalize
and simplify handling.

Signed-off-by: Rusty Russell <[email protected]>
… times.

This will be used for listconfig, which knows these should be presented
as arrays, not single values.

Signed-off-by: Rusty Russell <[email protected]>
Otherwise, we should rename it?

Signed-off-by: Rusty Russell <[email protected]>
This is a nod towards moving to runtime-developer-mode, and also
quite clear; we currently have all these options prefixed with dev,
but this flags makes handling explicit.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor Author

Rebased on master to resolve simple conflicts.

Now we wire in the code which gathers configvars and parses from there;
lightningd keeps the array of configuration variables for future use.

Note that lightning-cli also needs to read the config, but it has its
own options (including short ones!) and doesn't want to use this
configvar mechanism, so we have a different API for that now.

Signed-off-by: Rusty Russell <[email protected]>
Developers, rejoice (we already have --testnet, --signet and --mainnet!).

Changelog-Added: Config: `--regtest` option as alias for `--network=regtest`
Signed-off-by: Rusty Russell <[email protected]>
It's an obscure command, but this we won't see old plugin commands in
listconfigs (once it uses configvars).

Signed-off-by: Rusty Russell <[email protected]>
Clearly, listconfigs shouldn't list these.

Also, hoist the opt_hidden check since it's independent of whether
there's an arg or not.

Signed-off-by: Rusty Russell <[email protected]>
We have hacky code to show some listconfigs values as literals; instead
explicitly encode the types.

Signed-off-by: Rusty Russell <[email protected]>
It literally contained \" to avoid it being interpreted as a literal;
now we have OPT_SHOWINT we no longer need this hack.

It's obscure, so I'm not bothering with a deprecation cycle.

Changelog-Fixed: JSON-RPC: `listconfigs` `rpc-file-mode` no longer has gratuitous quotes (e.g. "0600" not "\"0600\"").
Signed-off-by: Rusty Russell <[email protected]>
listconfigs is convenient, but it doesn't handle multi-options well: it
outputs an object with duplicate fields in this case (e.g. log-file), nor
is it extensible to show more than raw values.

However, listconfigs doesn't do what other list commands do (use a
sub-object "configs") so we can put the new values under that.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: JSON-RPC: `listconfigs` now has `configs` subobject with more information about each config option.
Changelog-Deprecated: Plugins: `default` no longer accepted on `flag` type parameters (it was silently ignored, so just don't set it).
Signed-off-by: Rusty Russell <[email protected]>
We use multi-specifiable options elsewhere, this is just another.
Otherwise you can't add, you can only set them all.

Changelog-Added: Config: `accept-htlc-tlv-type` (replaces awkward-to-use `accept-htlc-tlv-types`)
Changelog-Deprecated: Config: `accept-htlc-tlv-types` (use `accept-htlc-tlv-type` multiple times)
Signed-off-by: Rusty Russell <[email protected]>
This integrates them with configvars properly: they almost "just work"
in listconfigs now, and we don't put them in a special sub-object
under their plugin.

Unfortunately, this means `listconfigs` now has a loose schema: any
plugin can add something to it.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: reloaded plugins get passed any vars from configuration files.
Changelog-Deprecated: Config: boolean plugin options set to `1` or `0` (use `true` and `false` like non-plugin options).
Use the configs object, as the others are about to be deprecated.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Deprecated: JSON-RPC: `listconfigs` direct fields, use `configs` sub-object and `set`, `value_bool`, `value_str`, `value_int`, or `value_msat` fields.
Signed-off-by: Rusty Russell <[email protected]>
I chose the full path name, not just the basename.

Suggested-by: @SimonVrouwe
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor Author

Rebase to fix trivial error for liquid tests (assuming network=regtest in msg)

Comment on lines +343 to +345
opt_register_early_noarg("--regtest",
opt_set_specific_network, "regtest",
"Alias for --network=regtest");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I the only one who dislikes aliased options? --network=regtest == --regtest 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm split. I hate the redundancy, but love the brevity...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic plugins are not passed lightningd config options?
3 participants