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

rpc: use the standard port derivation in connect command #5242

Merged

Conversation

vincenzopalazzo
Copy link
Collaborator

adding the port derivation when the port is not specified in the connect command.

Complete implementation of BOLT1 port derivation proposal lightning/bolts#968

Let's see if some tests in CI will be unhappy

Signed-off-by: Vincenzo Palazzo [email protected]

@vincenzopalazzo vincenzopalazzo changed the title rpc: use the standart port derivation in connect command rpc: use the standard port derivation in connect command May 6, 2022
@vincenzopalazzo vincenzopalazzo force-pushed the macros/dafault_port branch 2 times, most recently from b2bb0cb to c60287b Compare May 8, 2022 14:03
@vincenzopalazzo vincenzopalazzo force-pushed the macros/dafault_port branch 2 times, most recently from de82bf1 to 4911862 Compare May 11, 2022 20:21
@rustyrussell
Copy link
Contributor

Second commit fixes up mistakes in the first: please clean that up!

Results look good though!

@vincenzopalazzo
Copy link
Collaborator Author

vincenzopalazzo commented May 19, 2022

Second commit fixes up mistakes in the first: please clean that up!

Ops! sorry!

However, in this PR there are a few things that are missing related to the default port in the regtest! I will add this by the end of the week

Looks like already done :) seems that I clean up my memory!

@rustyrussell
Copy link
Contributor

Here's a little more documentation:

diff --git a/doc/lightning-connect.7.md b/doc/lightning-connect.7.md
index c2e9851f7..ebdc3d8c0 100644
--- a/doc/lightning-connect.7.md
+++ b/doc/lightning-connect.7.md
@@ -18,7 +18,11 @@ be of the form *id@host* or *id@host:port*. In this case, the *host* and
 
 *host* is the peer's hostname or IP address.
 
-If not specified, the *port* defaults to 9735.
+If not specified, the *port* depends on the current network:
+- **bitcoin** mainnet: 9735.
+- bitcoin **testnet**: 19735.
+- bitcoin **signet**: 39735.
+- bitcoin **regtest**: 19846.
 
 If *host* is not specified (or doesn't work), the connection will be attempted to an IP
 belonging to *id* obtained through gossip with other already connected
diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md
index 5fc07bea9..07388d262 100644
--- a/doc/lightningd-config.5.md
+++ b/doc/lightningd-config.5.md
@@ -370,7 +370,8 @@ network.
 Note that for simple setups, the implicit *autolisten* option does the
 right thing: for the mainnet (bitcoin) network it will try to bind to
 port 9735 on IPv4 and IPv6, and will announce it to peers if it seems
-like a public address.
+like a public address (and other default ports for other networks,
+as described below).
 
 Core Lightning also support IPv4/6 address discovery behind NAT routers.
 If your node detects an new public address, it will update its announcement.

@vincenzopalazzo
Copy link
Collaborator Author

trivial rebase and docs updated

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.

You also need to get rid of "DEFAULT_PORT" throughout the code (many are in tests: the can have a local definition no problems!).

But this shows some places you missed!

bitcoin/chainparams.h Outdated Show resolved Hide resolved
Complete implementation of BOLT1 port derivation proposal lightning/bolts#968

Changelog-Added: rpc: use the standard port derivation in connect command when the port is not specified.

Signed-off-by: Vincenzo Palazzo <[email protected]>
This doesn't make sense, but for now we just keep it

Signed-off-by: Vincenzo Palazzo <[email protected]>
Suggested-by: @rustyrussell 

Signed-off-by: Vincenzo Palazzo <[email protected]>

# Title: 

Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo vincenzopalazzo force-pushed the macros/dafault_port branch 2 times, most recently from 5adb0f3 to 2aa523d Compare June 25, 2022 17:51
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.

Looks good, one overzealous change found!

devtools/gossipwith.c Outdated Show resolved Hide resolved
@vincenzopalazzo vincenzopalazzo force-pushed the macros/dafault_port branch 2 times, most recently from 7b1d127 to 6eeb74c Compare June 26, 2022 23:39
@rustyrussell
Copy link
Contributor

I fixed the last commit (I was wrong: gossipwith did not set chainparams, which already caused problems, since gossipwith -h would crash!).

[Also fixes crash with -h, since chainparams was not set! --RR]
@vincenzopalazzo
Copy link
Collaborator Author

Thanks! Looks like also that the tests related to the network looks good!

@rustyrussell
Copy link
Contributor

Ack 90b6cbc

@rustyrussell rustyrussell merged commit 2754e30 into ElementsProject:master Jun 27, 2022
@vincenzopalazzo vincenzopalazzo deleted the macros/dafault_port branch June 27, 2022 20:58
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.

3 participants