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

feat: config additions #5136

Merged

Conversation

m-schmoock
Copy link
Collaborator

This adds config and commanline switches for HTLC min/max values and announcement of auto discovered IPs

rustyrussell
rustyrussell approved these changes Mar 28, 2022
@@ -1117,6 +1137,9 @@ static void register_opts(struct lightningd *ld)
opt_register_arg("--announce-addr", opt_add_announce_addr, NULL,
ld,
"Set an IP address (v4 or v6) or .onion v3 to announce, but not listen on");
opt_register_noarg("--disable-ip-discovery", opt_set_invbool,
&ld->config.announce_discovered_ip,
"Turn off address announcement of detected public IP");
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note: I usually prefer positive options, not negative, with the default being !ld->always_use_proxy.

In theory always-use-proxy could be a normal SOCKS proxy, and for all we know you've set up a reverse tunnel. But that's a really weird case (and you can just advertize an explicit IP for that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, if I make it a positive bool option, then ip announcement can't be enabled by default, otherwise the switch would do nothing. Or do I get somthing wrong? What I could do is rename the internal code variables also to disable_ip_discovery which defaults to false and use an opt_set_bool instead of opt_set_invbool. Do you meant that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed the internal variable also to disable_ip_discovery defaulting to false. This is now a switch, not an bool option. The switch effectively turns of ip discovery anouncements by using opt_set_bool instead of opt_set_invbool.

Can you re-check?

doc/lightningd-config.5.md Outdated Show resolved Hide resolved
doc/lightningd-config.5.md Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

You need to add the names to the doc/schemas/listconfig.schema.json file too...

This adds config and commandline options for htlc_min_msat, htlc_max_msat and
announce_discovered_ip. The default is 0msat for htlc_min_msat, unlimited for
htlc_max_msat and enabled for announce_discovered_ip.

The announce_discovered_ip gets the disable commandline switch --disable-ip-discovery

Changelog-added: Config options for htlc_min_msat, htlc_max_msat and announce_discovered_ip.
@m-schmoock m-schmoock force-pushed the feat/config_additions branch 2 times, most recently from 7a745fd to 5996ca7 Compare March 30, 2022 21:26
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 0f25b7b

@rustyrussell rustyrussell merged commit 0799328 into ElementsProject:master Apr 4, 2022
@rustyrussell rustyrussell mentioned this pull request Apr 4, 2022
@m-schmoock m-schmoock deleted the feat/config_additions branch April 7, 2022 20:25
@heyambob
Copy link

heyambob commented Apr 8, 2022

disable-ip-discovery=true

I put this in the config, and it says

lightningd: /clight/config line 41: disable-ip-discovery: doesn't allow an argument

But passing it on the command line is okay.

@m-schmoock
Copy link
Collaborator Author

m-schmoock commented Apr 8, 2022

That's because it's a switch and not an argument. Try without =true or use startup parameter --disable-ip-discovery. :)
Re-checked: The config is clear about this, just as for the other switches like disable-dns that are also without argument.

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

Successfully merging this pull request may close these issues.

4 participants