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

Make it clear that the default port for ETH2 is 9000 udp/tcp #2421

Merged
merged 4 commits into from
Mar 22, 2021

Conversation

actuallymentor
Copy link
Contributor

Hello team!

The only place where the default port is explicitly mentioned is in a troubleshooting section which is suboptimal for people trying to find the right ports for firewall configuration.

Please confirm that 9000 on both udp/tcp is the default, right now I am only going on that troubleshooting section.

@actuallymentor
Copy link
Contributor Author

Update, the port is indeed 9000, see network.nim.

Thanks for checking @jclapis.

@kdeme
Copy link
Contributor

kdeme commented Mar 17, 2021

Thank you for pointing out that it is not clear what the default used port is.
While documentation can help here somewhat, the best UX solution is to simply have this added in the --help print.
There is an open issue to take care of this consistently for all defaults: status-im/nim-confutils#19

@@ -44,8 +44,8 @@ The following options are available:
addresses.
--listen-address Listening address for the Ethereum LibP2P and Discovery v5
traffic.
--tcp-port Listening TCP port for Ethereum LibP2P traffic.
--udp-port Listening UDP port for node discovery.
--tcp-port Override default listening TCP port 9000 TCP port for Ethereum LibP2P traffic.
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is a dump of the --help command, I wouldn't really alter the documentation here.

Copy link
Member

Choose a reason for hiding this comment

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

@deme we can actually change the help docs in conf.nim manually for the time being, until proper support is added in confutils

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that was what I was implying on, rather change it there for now then just in these docs here.

@actuallymentor
Copy link
Contributor Author

Thank you for pointing out that it is not clear what the default used port is.
While documentation can help here somewhat, the best UX solution is to simply have this added in the --help print.
There is an open issue to take care of this consistently for all defaults: status-im/nim-confutils#19

I see. Do I understand from that conversation there is already a PR for it or should I do that?

@arnetheduck
Copy link
Member

Thank you for pointing out that it is not clear what the default used port is.
While documentation can help here somewhat, the best UX solution is to simply have this added in the --help print.
There is an open issue to take care of this consistently for all defaults: status-im/nim-confutils#19

I see. Do I understand from that conversation there is already a PR for it or should I do that?

if you can update the help text in conf.nim here as well, that's great - there's no PR presently:

desc: "Listening TCP port for Ethereum LibP2P traffic"

@actuallymentor
Copy link
Contributor Author

@arnetheduck I just edited the descriptions in conf.nim and added the same text to the docs. Let me know if this suffices!

@arnetheduck arnetheduck changed the base branch from stable to unstable March 18, 2021 11:22
@arnetheduck arnetheduck merged commit 895fe4b into status-im:unstable Mar 22, 2021
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