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

BOLT 1: introduce port convention for different network #968

Merged

Conversation

vincenzopalazzo
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo commented Mar 12, 2022

This is an opinionate change proposed where there are reasons to have a default behavior for different networks, but there are also good reasons to have not this behavior.

I found the PR on Bitcoin core bitcoin/bitcoin#23306 very good to have a good summary of pros and cons.

In addition, I think having a default port for different networks on lightning is a good thing to have because introducing some way to detect which network a lightning node is exposing with a particular URL composed by node_id@ip:port

We introduce a default behavior on c-lighting with the PR ElementsProject/lightning#4900

P.S: As plus we have already a default port for Bitcoin maintet.

Signed-off-by: Vincenzo Palazzo vincenzopalazzodev@gmail.com

Copy link

@jsarenik jsarenik left a comment

Choose a reason for hiding this comment

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

Have a look at my proposed changes please.

01-messaging.md Outdated Show resolved Hide resolved
01-messaging.md Outdated Show resolved Hide resolved
01-messaging.md Outdated Show resolved Hide resolved
01-messaging.md Outdated Show resolved Hide resolved
01-messaging.md Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Collaborator

Fine with me, just adding a new convention really.

@vincenzopalazzo vincenzopalazzo marked this pull request as ready for review March 16, 2022 08:30
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
Copy link
Contributor Author

Thanks for the improvement guys and the feedback, sorry if I was not present at the meeting last time.

Now the PR should be ready, and the port convention try to follow the bitcoin 0.22 convention like Mainet on port X, testnet on port 1000 + X

@vincenzopalazzo vincenzopalazzo changed the title bolt1: introduce port convention for different network BOLT 1: introduce port convention for different network Mar 16, 2022
@michaelfolkson
Copy link

michaelfolkson commented Mar 17, 2022

Signet would be 39735

Add signet too? I know a bunch of people are using the public default signet these days instead of testnet. Agree with taking out Litecoin :)

I found the PR on Bitcoin core bitcoin/bitcoin#23306 very good to have a good summary of pros and cons.

Bitcoin Core still has default ports for these different networks, it just now (after the above linked PR) supports changing the defaults without it impacting automatic connections. I don't think there's anything in there that argues against setting default ports for various networks across say Lightning implementations.

@vincenzopalazzo
Copy link
Contributor Author

Add signet too? I know a bunch of people are using the public default signet these days instead of testnet. Agree with taking out Litecoin :)

I'm not very familiar with the signet network. However, the only reason that I didn't add signet as network is that we can have two or more complete blockchains in on signet right? and in this case, the default port has no sense to me.

BTW I can have some missing on this reason.

I don't think there's anything in there that argues against setting default ports for various networks across say Lightning implementations.

Mh I don't follow all the conversation on bitcoin core, but the first introduction in the PR has good pro and cons to have a default port number. Like the following one

The folklore justification (eventually actually added as a comment to the codebase in bitcoin/bitcoin#20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in bitcoin/bitcoin#5150 (comment) e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).

And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.

@michaelfolkson
Copy link

I'm not very familiar with the signet network. However, the only reason that I didn't add signet as network is that we can have two or more complete blockchains in on signet right? and in this case, the default port has no sense to me.

There is a default signet network that Core is configured to use. Of course custom signets can be set up (and are encouraged) for testing new functionality that isn't enabled on the default signet.

And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.

Yeah I guess that is arguing for something stronger than merely supporting non-default ports. I still think you need to set a default. The alternative is somehow randomizing the port number or forcing the user to choose a port number rather than falling back to the default.

@vincenzopalazzo
Copy link
Contributor Author

There is a default signet network that Core is configured to use. Of course custom signets can be set up (and are encouraged) for testing new functionality that isn't enabled on the default signet.

Oh cool, so if there is a default signet, we can add a default port for signet! Thanks to point me out this feature.

Yeah I guess that is arguing for something stronger than merely supporting non-default ports. I still think you need to set a default. The alternative is somehow randomizing the port number or forcing the user to choose a port number rather than falling back to the default.

Yes, I agree, I think that the point of view is both correct, but lightning has some convention as in this case can be good.

Copy link

@michaelfolkson michaelfolkson left a comment

Choose a reason for hiding this comment

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

After addressing the grammatical nit I'll ACK

01-messaging.md Outdated Show resolved Hide resolved
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@michaelfolkson
Copy link

ACK 9101f33

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM ⛱

@ProofOfKeags
Copy link
Contributor

I wanted to ask about the idea of LN being chain agnostic. I don't think having a single node running mainnet and testnet in the same node will be a desired use case, but my understanding was that since we have the chain id in a lot of the peer messages, it seems the original design permits different networks to share the same port.

Do we want to limit the scope of what new networks would be added?
Do we want to specify how to tell the difference between what new networks (not specified in this PR) would warrant separate ports vs just having different chain ids?

@vincenzopalazzo
Copy link
Contributor Author

Do we want to limit the scope of what new networks would be added?
&&
Do we want to specify how to tell the difference between what new networks (not specified in this PR) would warrant separate ports vs just having different chain ids?

This PR doesn't introduce any type of scope limits, but only a standard between ports, if tomorrow's lightning network will support CatCoin we can standardize the port or you can be free to announce a new port.

You are introducing a limit if in the init message will specify MUST be chain hash of bitcoin, otherwise fails the connection

Again, this PR suggests only a standard between port numbers

vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request May 6, 2022
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 <vincenzopalazzodev@gmail.com>
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request May 6, 2022
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 <vincenzopalazzodev@gmail.com>
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request May 8, 2022
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 <vincenzopalazzodev@gmail.com>
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request May 8, 2022
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 <vincenzopalazzodev@gmail.com>
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request May 8, 2022
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 <vincenzopalazzodev@gmail.com>
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request May 19, 2022
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 <vincenzopalazzodev@gmail.com>
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request May 23, 2022
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 <vincenzopalazzodev@gmail.com>
Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐅

@rustyrussell
Copy link
Collaborator

Ack

@Roasbeef Roasbeef merged commit c74a3bb into lightning:master May 23, 2022
@vincenzopalazzo vincenzopalazzo deleted the vincenzopalazzo/default_port branch May 23, 2022 20:46
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request Jun 18, 2022
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 <vincenzopalazzodev@gmail.com>
vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this pull request Jun 25, 2022
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 <vincenzopalazzodev@gmail.com>
rustyrussell pushed a commit to ElementsProject/lightning that referenced this pull request Jun 27, 2022
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 <vincenzopalazzodev@gmail.com>
whitslack pushed a commit to whitslack/lightning that referenced this pull request Aug 30, 2022
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 <vincenzopalazzodev@gmail.com>
whitslack pushed a commit to whitslack/lightning that referenced this pull request Aug 30, 2022
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 <vincenzopalazzodev@gmail.com>
whitslack pushed a commit to whitslack/lightning that referenced this pull request Oct 8, 2022
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 <vincenzopalazzodev@gmail.com>
whitslack pushed a commit to whitslack/lightning that referenced this pull request Oct 28, 2022
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 <vincenzopalazzodev@gmail.com>
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.

7 participants