Set default port according to network #4900

jsarenik · 2021-11-01T20:19:41Z

The idea is to have different default ports for different networks.

Current default port is 9735 for everything. Let's use it for
the mainnet and reuse the difference added to the default port
from rpc_port values in bitcoin/chainstate.c.

Testnet would be 19735 (adding rpc_port - 8332 = 10000).

Signet would be 39735 (adding rpc_port - 8332 = 30000).

Regtest would be 19846 (adding rpc_port - 8332 = 10111).

With Vincenzo's pair-programming help.

Changelog-Changed: If the port is unspecified, the default port is chosen according to used network similarly to Bitcoin Core.

jsarenik · 2021-11-02T07:03:22Z

Overall idea is that command-line specified port always takes precedence, but if it is not set then the port lightningd will listen to depends on the network.

From Bitcoin Core -help we see this:

  -port=<port>
       Listen for connections on <port>. Nodes not using the default ports
       (default: 8333, testnet: 18333, signet: 38333, regtest: 18444)
       are unlikely to get incoming connections. Not relevant for I2P
       (see doc/i2p.md).

And that means the differences to the default mainnet port are what the new variable port_add contains.

UPDATE: Simplified using an already-defined rpc_port instead of adding port_add.

jsarenik · 2021-11-02T07:24:03Z

@rustyrussell I need help. Shell-scripting mindset seems to be of little use here :)

UPDATE: Got help from @vincenzopalazzo

jsarenik · 2021-11-03T12:58:37Z

Vincenzo says this change may need some change in the specs.

vincenzopalazzo · 2021-11-03T13:24:17Z

UPDATE: Got help from @vincenzopalazzo

Just suggested how to fix the segmentation fault :) nothing else

Vincenzzo says this change may need some change in the specs.

I like the idea to have some standards also on different ports, like testnet, request but (maybe not for liquid or different network like litecoin?)

vincenzopalazzo

LGTM.

I like your proposal, I have only a question on how to derive the lightning port from the bitcoin port standard, and also add a check to avoid the overriding of a custom port.

vincenzopalazzo · 2021-11-03T13:32:37Z

bitcoin/chainparams.c

@@ -34,6 +34,7 @@ const struct chainparams networks[] = {
 				      0x5a, 0x08, 0x9c, 0x68, 0xd6, 0x19, 0x00,


I really like to have this type of offset for the network port, and I think that using this technique is pretty good. However, I things in this case we are mixing the bitcoin config with only one propriety related to c-lightning.

However, we can derive your offset from the following formula lightning_port = defautl_lightning_port + (bitcoin_network_port - bitcoin_mainet_port). As an example we can have the following example:

lightning_port = 9735 + (18332 - 8332) where we will have that the lightning port is 19735.

What do you think?

Sure. This is much cleaner than my pre-processed constants (10000, etc.).

So do you recommend to do port_add = 18332 - 8332 instead of port_add = 10000?

Not sure how to implement it, the mine was only an idea to derive the standard network port of lightning from what we have defined in the bitcoins. However, we need to manage also the corner case here, and maybe the case is too much.

I need to think about that

Check now. bbc94ac

What's the corner case? I tested it on mainnet and signet. And also with a port 12345 specified on the command line. Works well.

And even better, see b525259.

And now with a comment: 17a66f4

Enough. Will wait for feed-back now.

See where one test was fixed along with the rpc_port for regtest network.

lightningd/lightningd.c

jsarenik · 2021-11-03T19:21:06Z

@cdecker please have a look and comment.

jsarenik · 2021-11-04T08:00:05Z

This may be a breaking change for anyone who used the default port 9735 for testnet or signet and set the firewall to let it in. If this commit is merged the default port will be different and the firewall rule obsolete (in case of other networks — i.e. non-mainnet).

jsarenik · 2021-11-05T10:01:42Z

This is going to be bigger than few-lines-patch :)

$ git grep -w 9735
CHANGELOG.md: - options: Allow the Tor inbound service port differ from 9735 ([3155](https://github.com/ElementsProject/lightning/pull/3155))
Dockerfile:ENV LIGHTNINGD_PORT=9735
Dockerfile:EXPOSE 9735 9835
README.md:(and optional `<port>`, if not 9735) and has the node ID `<node_id>`:
common/test/run-ip_port_parsing.c:	assert(parse_wireaddr("[::1]:9735", &addr, 500, false, NULL));
common/test/run-ip_port_parsing.c:	assert(addr.port == 9735);
common/test/run-ip_port_parsing.c:	assert(streq(ip, "[::1]:9735"));
common/test/run-wireaddr.c:	assert(parse_wireaddr("127.0.0.1:9735", &expect->u.wireaddr, 0, NULL, &err));
common/wireaddr.h: * The default TCP port is 9735. This corresponds to hexadecimal
common/wireaddr.h:#define DEFAULT_PORT 9735
contrib/linuxarm32v7.Dockerfile:ENV LIGHTNINGD_PORT=9735
contrib/linuxarm32v7.Dockerfile:EXPOSE 9735 9835
contrib/linuxarm64v8.Dockerfile:ENV LIGHTNINGD_PORT=9735
contrib/linuxarm64v8.Dockerfile:EXPOSE 9735 9835
contrib/pyln-proto/examples/listen.py:listener.bind(('0.0.0.0', 9735))
contrib/pyln-proto/pyln/proto/wire.py:def connect(local_privkey, node_id, host: str, port: int = 9735,
contrib/pyln-spec/bolt1/pyln/spec/bolt1/text.py:The default TCP port is 9735. This corresponds to hexadecimal `0x2607`: the Unicode code point for LIGHTNING.<sup>[1](#reference-1)</sup>
contrib/pyln-testing/pyln/testing/utils.py:    def __init__(self, lightning_dir, bitcoindproxy, port=9735, random_hsm=False, node_id=0):
doc/PLUGINS.md:    "addr": "34.239.230.56:9735",
doc/TOR.md:bind-addr=127.0.0.1:9735
doc/TOR.md:2.  `bind-addr` informs C-Lightning to bind itself to port 9735.
doc/TOR.md:    9735 is the normal Lightning Network port, so this setting may already be present.
doc/TOR.md:bind-addr=127.0.0.1:9735
doc/TOR.md:9735 port.
doc/TOR.md:Generally `--bind-addr=127.0.0.1:9735` should work fine.
doc/TOR.md:persistent .onion address will be 9735, but you can change this by
doc/TOR.md:HiddenServicePort 1234 127.0.0.1:9735
doc/TOR.md:line is `HiddenServicePort 1234 127.0.0.1:9735` the .onion address will be reachable at
doc/TOR.md:.onion address through the 9735 port.
doc/TOR.md:be able to connect to it through the 9735 port.
doc/lightning-connect.7.md:If not specified, the *port* defaults to 9735.
hsmd/libhsmd.c:	/* Clearly, we should use 9735, the unicode point for lightning! */
hsmd/libhsmd.c:				  BIP32_INITIAL_HARDENED_CHILD|9735,

lightningd-config.5.md already addressed, see the diff and comment
lightningd-listnodes.7.md - no changes neded, right?
lightningd-listconfigs.7.md - no changes neded, right?

jsarenik · 2021-12-02T15:17:44Z

@cdecker the FreeBSD test ends with following:

/usr/bin/ssh -t freebsd
Pseudo-terminal will not be allocated because stdin is not a terminal.
ssh: connect to host localhost port 2222: Connection refused
Error: The process '/usr/bin/ssh' failed with exit code 255

cdecker · 2021-12-02T15:59:42Z

Yep, that's a flake, given the VM has some timing issues. Kicking CI 👍

jsarenik · 2021-12-02T21:01:53Z

@cdecker please kick it once again. Just one Normal Test timed-out now.

UPDATE: That was actually the Elements test because with Elements the default port would be different than with regtest. The test was changed accordingly (to accept any port now .... which can be from 0000 to FFFF).

vincenzopalazzo

The other CI failure looks unrelated, but I'm not sure

tests/test_gossip.py

vincenzopalazzo

LGTM overall but I have a small missing in the test_gossip.py

jsarenik · 2021-12-05T16:52:29Z

The last CI run failed on this: https://github.com/ElementsProject/lightning/runs/4421365349?check_suite_focus=true. Re-running.

The idea is to have different default ports for different networks. Current default port is `9735` for everything. Let's use it for the mainnet and reuse the difference added to the default port from `rpc_port` values in `bitcoin/chainstate.c`. Testnet would be `19735` (adding rpc_port - 8332 = `10000`). Signet would be `39735` (adding rpc_port - 8332 = `30000`). Regtest would be `19846` (adding rpc_port - 8332 = `10111`). With Vincenzo's kind pair-programming help over tmate. Two other commits were squashed into this one so that bisecting never ends up in half-baked state: 1. chainparams: Fix regtest default rpc_port bitcoind -help says this: -rpcport=<port> Listen for JSON-RPC connections on <port> (default: 8332, testnet: 18332, signet: 38332, regtest: 18443) 2. test_gossip: Default port for regtest hex: 2607 is now .... (could be 4d86 but Elements uses another port) dec: 9735 is now any port (could be 19846 ^^ but now is for any port) The lines which were binding to default port were removed as the default port is different on each network. NOTE: Remember not to modify gossip_store tests which loads everything raw including the checksums. Changelog-Changed: If the port is unspecified, the default port is chosen according to used network similarly to Bitcoin Core.

Also nit: s/possible/possibly suggested by michaelfolkson.

jsarenik · 2021-12-05T18:08:04Z

It seems to me that "Flaky Tests" should be taken out of the regular CI runs and run separately on regular basis with opt-in reporting (for whoever likes to receive Flaky Test failures). This way it would not block or confuse anyone and still whoever wants the results will get them and may use them for what they are worth.

rustyrussell · 2021-12-06T05:58:42Z

It seems to me that "Flaky Tests" should be taken out of the regular CI runs and run separately on regular basis with opt-in reporting (for whoever likes to receive Flaky Test failures). This way it would not block or confuse anyone and still whoever wants the results will get them and may use them for what they are worth.

No, flaky tests should be fixed. But it's often an ongoing battle, to detect whether it's a test flake, or a real intermittent issue. And it also doesn't happen on my test boxes :(

rustyrussell · 2021-12-06T06:38:04Z

I will apply this, as it's been sitting here too long, and restore the default-port-is-correct test as a separate commit.

Ack 51ce5bb

Completed.

michaelfolkson · 2022-02-26T20:16:57Z

Vincenzo says this change may need some change in the specs.

Just a reminder that this needs a change to BOLT 1 (maybe worth opening a spec PR and/or discussing in the spec meeting @vincenzopalazzo @jsarenik?)

vincenzopalazzo · 2022-02-28T09:44:45Z

Just a reminder that this needs a change to BOLT 1 (maybe worth opening a spec PR and/or discussing in the spec meeting @vincenzopalazzo @jsarenik?)

This is something that I think for a while, but I don't have time to know how bitcoin core remove the default port bitcoin/bitcoin#23306 I will check this week,

Thanks for pinging on this point!

vincenzopalazzo · 2022-03-12T18:44:56Z

Opened a draft on the Spec lightning/bolts#968 I will review the text in the next few days to avoid different typos e improve the English form.

jsarenik changed the title ~~WIP: Use default ports for chosen network~~ WIP: Set default ports according to network Nov 2, 2021

jsarenik changed the title ~~WIP: Set default ports according to network~~ WIP: Set default port according to network Nov 2, 2021

jsarenik force-pushed the jsn/default_ports branch 3 times, most recently from a563cb6 to 8605695 Compare November 3, 2021 10:39

jsarenik changed the title ~~WIP: Set default port according to network~~ Set default port according to network Nov 3, 2021

jsarenik force-pushed the jsn/default_ports branch 2 times, most recently from 2e02ee7 to 6ba8215 Compare November 3, 2021 11:48

vincenzopalazzo requested changes Nov 3, 2021

View reviewed changes

jsarenik force-pushed the jsn/default_ports branch from 6ba8215 to 01efd37 Compare November 3, 2021 16:36

jsarenik force-pushed the jsn/default_ports branch 3 times, most recently from b525259 to 17a66f4 Compare November 4, 2021 07:43

jsarenik requested a review from cdecker as a code owner November 4, 2021 10:46

jsarenik force-pushed the jsn/default_ports branch 6 times, most recently from 5a81a9e to 9c4bdb0 Compare November 5, 2021 08:23

jsarenik force-pushed the jsn/default_ports branch 3 times, most recently from 85d2737 to aaf1dbf Compare November 5, 2021 11:14

jsarenik force-pushed the jsn/default_ports branch 3 times, most recently from efe8194 to 2949bfe Compare December 3, 2021 07:29

vincenzopalazzo requested changes Dec 3, 2021

View reviewed changes

tests/test_gossip.py Outdated Show resolved Hide resolved

jsarenik force-pushed the jsn/default_ports branch 4 times, most recently from 2c86386 to 26a319e Compare December 3, 2021 14:00

vincenzopalazzo mentioned this pull request Dec 4, 2021

ci: Fix FreeBSD workflow #4954

Merged

vincenzopalazzo reviewed Dec 4, 2021

View reviewed changes

tests/test_gossip.py Show resolved Hide resolved

vincenzopalazzo reviewed Dec 4, 2021

View reviewed changes

jsarenik force-pushed the jsn/default_ports branch from 26a319e to 3a80abb Compare December 5, 2021 08:05

jsarenik force-pushed the jsn/default_ports branch from 3a80abb to 5738b63 Compare December 5, 2021 16:52

jsarenik added 2 commits December 5, 2021 18:55

doc: lightningd-config.5.md - default ports

51ce5bb

Also nit: s/possible/possibly suggested by michaelfolkson.

jsarenik force-pushed the jsn/default_ports branch from 5738b63 to 51ce5bb Compare December 5, 2021 17:55

rustyrussell merged commit 4274080 into ElementsProject:master Dec 6, 2021

rustyrussell mentioned this pull request Dec 6, 2021

pytest: restore gossip advertizement tests for default port. #4958

Merged

vincenzopalazzo mentioned this pull request Mar 12, 2022

BOLT 1: introduce port convention for different network lightning/bolts#968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default port according to network #4900

Set default port according to network #4900

jsarenik commented Nov 1, 2021 •

edited

Loading

jsarenik commented Nov 2, 2021 •

edited

Loading

jsarenik commented Nov 2, 2021 •

edited

Loading

jsarenik commented Nov 3, 2021 •

edited

Loading

vincenzopalazzo commented Nov 3, 2021

vincenzopalazzo left a comment

vincenzopalazzo Nov 3, 2021

jsarenik Nov 3, 2021

jsarenik Nov 3, 2021

vincenzopalazzo Nov 3, 2021

jsarenik Nov 4, 2021

jsarenik Nov 4, 2021

jsarenik Nov 4, 2021

jsarenik Nov 4, 2021

jsarenik Nov 4, 2021

jsarenik commented Nov 3, 2021

jsarenik commented Nov 4, 2021 •

edited

Loading

jsarenik commented Nov 5, 2021 •

edited

Loading

jsarenik commented Dec 2, 2021

cdecker commented Dec 2, 2021

jsarenik commented Dec 2, 2021 •

edited

Loading

vincenzopalazzo left a comment

vincenzopalazzo left a comment

jsarenik commented Dec 5, 2021

jsarenik commented Dec 5, 2021

rustyrussell commented Dec 6, 2021

rustyrussell commented Dec 6, 2021 •

edited

Loading

michaelfolkson commented Feb 26, 2022

vincenzopalazzo commented Feb 28, 2022

vincenzopalazzo commented Mar 12, 2022

		@@ -34,6 +34,7 @@ const struct chainparams networks[] = {
		0x5a, 0x08, 0x9c, 0x68, 0xd6, 0x19, 0x00,

Set default port according to network #4900

Set default port according to network #4900

Conversation

jsarenik commented Nov 1, 2021 • edited Loading

jsarenik commented Nov 2, 2021 • edited Loading

jsarenik commented Nov 2, 2021 • edited Loading

jsarenik commented Nov 3, 2021 • edited Loading

vincenzopalazzo commented Nov 3, 2021

vincenzopalazzo 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

jsarenik commented Nov 3, 2021

jsarenik commented Nov 4, 2021 • edited Loading

jsarenik commented Nov 5, 2021 • edited Loading

jsarenik commented Dec 2, 2021

cdecker commented Dec 2, 2021

jsarenik commented Dec 2, 2021 • edited Loading

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

jsarenik commented Dec 5, 2021

jsarenik commented Dec 5, 2021

rustyrussell commented Dec 6, 2021

rustyrussell commented Dec 6, 2021 • edited Loading

michaelfolkson commented Feb 26, 2022

vincenzopalazzo commented Feb 28, 2022

vincenzopalazzo commented Mar 12, 2022

jsarenik commented Nov 1, 2021 •

edited

Loading

jsarenik commented Nov 2, 2021 •

edited

Loading

jsarenik commented Nov 2, 2021 •

edited

Loading

jsarenik commented Nov 3, 2021 •

edited

Loading

jsarenik commented Nov 4, 2021 •

edited

Loading

jsarenik commented Nov 5, 2021 •

edited

Loading

jsarenik commented Dec 2, 2021 •

edited

Loading

rustyrussell commented Dec 6, 2021 •

edited

Loading