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.

Sign up for GitHub

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

Jump to bottom

ipdiscovery: adds autobool config switch #5841

Merged
merged 7 commits into from
Jan 25, 2023
Merged

ipdiscovery: adds autobool config switch #5841

merged 7 commits into from
Jan 25, 2023

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Dec 20, 2022 •
edited
Loading

This adds an 'AUTOBOOL' config switch that explicitly enables the IP discovery feature by announce-addr-discovered, as users may also want to have IP discovery plus TOR just as fallback to increase overall connectivity.

Currently its still a bit tricky to use IP discovery as one need to disable any usable addresses i.e. by binding just to a specific interface and configure without TOR. This will enable 'default' behavior: when there are no usable addresses try ip discovery. Without this trick, when a node finds just a single (a likely changing IP address), it will announce this forever and IP discovery is disabled and never rechecks addresses bound to initially.

If unset, the default/auto behavior stays the same, meaning only do IP discovery when there are no usable addresses.

NOTE: there is a simpler version of this PR in #5843 that does the same but without the autobool, thus defaulting to disabled IP discovery.

@m-schmoock m-schmoock force-pushed the ipdiscovery/autobool branch 2 times, most recently from 8452aa1 to 7a7adaf Compare December 21, 2022 12:40
@m-schmoock m-schmoock changed the title ipdiscovery: adds explicit config switch ipdiscovery: adds autobool explicit config switch Dec 21, 2022
@m-schmoock m-schmoock changed the title ipdiscovery: adds autobool explicit config switch ipdiscovery: adds explicit autobool config switch Dec 21, 2022
@m-schmoock m-schmoock mentioned this pull request Dec 21, 2022
Closed
@m-schmoock m-schmoock changed the title ipdiscovery: adds explicit autobool config switch ipdiscovery: adds autobool config switch Dec 21, 2022
@m-schmoock m-schmoock requested a review from cdecker December 22, 2022 07:53
cdecker
cdecker reviewed Jan 2, 2023
View reviewed changes
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Very nice change, I like the autobool behavior, though in order places we simply set AUTO = NULL so we can distinguish optional values from their default value. Checking #5843 as an alternative next.

doc/FAQ.md Outdated Show resolved Hide resolved
gossipd/gossip_generation.c Show resolved Hide resolved
gossipd/gossipd_wire.csv Show resolved Hide resolved
@m-schmoock m-schmoock force-pushed the ipdiscovery/autobool branch 2 times, most recently from d0cab87 to 8061db1 Compare January 3, 2023 12:11
@m-schmoock
Copy link
Collaborator Author

I rephased the doc changes so its more clear.

@m-schmoock m-schmoock added this to the v23.02 milestone Jan 3, 2023
@m-schmoock m-schmoock force-pushed the ipdiscovery/autobool branch from 8061db1 to cc3929c Compare January 9, 2023 17:11
rustyrussell
rustyrussell approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

I like this approach, esp with the fixes at the end, but I think the name of the option is wrong, and the autobool should be in common/ or something, not ccan/

ccan/ccan/opt/helpers.c Outdated Show resolved Hide resolved
ccan/ccan/opt/opt.h Outdated Show resolved Hide resolved
doc/lightning-listconfigs.7.md Outdated Show resolved Hide resolved
gossipd/gossip_generation.c Outdated Show resolved Hide resolved
gossipd/gossipd.c Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
@m-schmoock m-schmoock force-pushed the ipdiscovery/autobool branch 2 times, most recently from 7c01005 to d340cba Compare January 18, 2023 21:52
@m-schmoock
Copy link
Collaborator Author

@rustyrussell Thanks. Addressed your input....

@m-schmoock m-schmoock force-pushed the ipdiscovery/autobool branch 4 times, most recently from 0a4b834 to 54e887f Compare January 23, 2023 20:37
@m-schmoock
ci: adds git fetch before doing schema checks
9ba1469 
This fixes the CI errors when doing `make check-source`
steps 'schema-added-check' and 'schema-removed-check'.
These errors prevented CI from performing these steps correctly:

```
fatal: ambiguous argument 'main': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
```

I changed it so that CI does a `git fetch origin` at first and do
the `git diff` against 'origin/master' (which then exist).
Also fixed a bug in the script that was missing $$master in the same line.

Also I added that the script shows the actual diff before failing,
so the user quickly sees whats wrong.
@m-schmoock m-schmoock force-pushed the ipdiscovery/autobool branch 2 times, most recently from 824252b to 89a35dd Compare January 23, 2023 20:57
@m-schmoock
opts: adds the autobool on/off/auto feature
35b162e
@m-schmoock
opts: announce-addr-discovered on/off/auto switch
d12143c 
This adds the option to explicitly enable ip-discovery, which maybe
helpful for example when a user wants TOR announced along with
discovered IPs to improve connectivity and have TOR just as a fallback.

Changelog-Added: Adds config switch 'announce-addr-discovered': on/off/auto
@m-schmoock
doc: announce-addr-discovered config switch
6bc4f66
@m-schmoock
pytest: fix and adapt test_remote_addr_disabled
e3e1eaf
@m-schmoock
opts: deprecate --disable-ip-discovery switch
008e046 
This switch was not doing anything useful anymore.
We deprecate it anyways to notify the user about the new switch.

Changelog-Deprecated: The old --disable-ip-discovery config switch
@m-schmoock
cli: getinfo output to regard --ip-discovery
e57527a
@m-schmoock m-schmoock force-pushed the ipdiscovery/autobool branch from 89a35dd to e57527a Compare January 24, 2023 10:04
@m-schmoock m-schmoock mentioned this pull request Jan 24, 2023
Closed
@m-schmoock
Copy link
Collaborator Author

@ALL
Just a final question to the parameter name, is --announce-addr-discovered a good name? We will have to align this with #5842 (where we set the port to be used for announcement of discovered addresses), so this would mean we will have to have something like --announce-addr-discovered-port.

Or should we go back to --ip-discovery and --ip-discovery-port ?

@cdecker
Copy link
Member

cdecker commented Jan 24, 2023

No preference on my side, I like the explicit nature of the first couple of names, and the brevity of the latter ones, so count me as undecided :-)

endothermicdev
endothermicdev reviewed Jan 25, 2023
View reviewed changes
Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

I like announce-addr-discovered but don't have strong feelings on it. Looks good to me apart from the typo.

doc/lightning-listconfigs.7.md Show resolved Hide resolved
@m-schmoock m-schmoock mentioned this pull request Jan 25, 2023
Merged
@endothermicdev
Copy link
Collaborator

ACK e57527a

@m-schmoock m-schmoock merged commit 34cfc93 into ElementsProject:master Jan 25, 2023
@m-schmoock m-schmoock deleted the ipdiscovery/autobool branch January 25, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@endothermicdev endothermicdev endothermicdev left review comments

@cdecker cdecker cdecker left review comments

@rustyrussell rustyrussell rustyrussell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v23.02
Development

Successfully merging this pull request may close these issues.
4 participants
@m-schmoock @cdecker @endothermicdev @rustyrussell