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

docs: update networking page #6289

Merged
merged 7 commits into from
Jan 13, 2024
Merged

Conversation

nflaig
Copy link
Member

@nflaig nflaig commented Jan 12, 2024

Motivation

There were several users already that asked about how to retrieve their ENR, hence i think it makes sense to explicitly mention this in our docs.

Also simplified ports a little bit, I think it's better to only talk about protecting incoming ports as users should not be concerned about outgoing ports, there is no reason to block any of those and if people do, they probably have a good reason for it and know what they are doing.

There is also no point in listing Prysm ports, those were only about outbound ports and there are plenty of setups that change the default 9000 p2p port to something else, e.g. rocketpool uses 9001. Listing them all does not make sense.

Description

  • minor rephrase / typo fix
  • mention how to retrieve ENR from Lodestar and decode it
  • simplified firewall port instructions

@nflaig nflaig requested a review from a team as a code owner January 12, 2024 16:07
@nflaig nflaig force-pushed the nflaig/update-networking-md branch from d2670de to a0591e7 Compare January 12, 2024 16:11
@nflaig nflaig force-pushed the nflaig/update-networking-md branch from a0591e7 to 9fe07d8 Compare January 12, 2024 16:20

This comment was marked as off-topic.

This comment was marked as off-topic.

Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

Great idea to add this! Just a couple small comments

docs/pages/beacon-management/networking.md Outdated Show resolved Hide resolved
Comment on lines -76 to -77
- 13000/TCP - Prysm P2P communication port
- 12000/UDP - Prysm P2P communication port
Copy link
Member

Choose a reason for hiding this comment

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

These should not be removed or lodestar will have issue p2p communicating with Prysm. I get that these are Lodestar docs but we communicate with Prysm

https://docs.prylabs.network/docs/prysm-usage/p2p-host-ip#configure-your-firewall

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps they go in an "optional" section with a note so people know the issue exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

lodestar will have issue p2p communicating with Prysm

This only concerns inbound ports, not outbound. I already mentioned that in the PR description, if you start speculating on what ports other nodes in the network are running on, you are fighting a losing battle.

Just an example, on dappnode Lodestar p2p port is configured to use 9512, same goes for other clients, they don't use default ports.

Maybe we should highlight that it is important to not block any outbound ports, but I felt like this is unnecessary as nobody does this.

Copy link
Member

Choose a reason for hiding this comment

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

OK makes sense. Maybe we can add a note in another PR that these are the defaults and that there are some extenuating circumstances that do not fall under these rules

Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed in the prysm docs you linked above that they state to allow outbound traffic from any port
image
This might be something to consider but from my experience this is not an issue as there is no guide that I know that instructions users to block outbound traffic.

docs/pages/beacon-management/networking.md Outdated Show resolved Hide resolved
docs/pages/beacon-management/networking.md Show resolved Hide resolved
nflaig and others added 3 commits January 12, 2024 19:49
Co-authored-by: Matthew Keil <me@matthewkeil.com>
Copy link
Member

@matthewkeil matthewkeil left a comment

Choose a reason for hiding this comment

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

LGTM!!! 🚀

@matthewkeil matthewkeil enabled auto-merge (squash) January 13, 2024 05:44
@matthewkeil matthewkeil merged commit 098d35a into unstable Jan 13, 2024
14 of 15 checks passed
@matthewkeil matthewkeil deleted the nflaig/update-networking-md branch January 13, 2024 05:50
ensi321 pushed a commit to ensi321/lodestar that referenced this pull request Jan 22, 2024
* docs: update networking page

* Update wordlist

* Update port description

* must instead of should

* Apply feedback

Co-authored-by: Matthew Keil <me@matthewkeil.com>

* Why on startup, who cares

* Add log exmaple

---------

Co-authored-by: Matthew Keil <me@matthewkeil.com>
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.15.0 🎉

jeluard pushed a commit to jeluard/lodestar that referenced this pull request Jan 30, 2024
* docs: update networking page

* Update wordlist

* Update port description

* must instead of should

* Apply feedback

Co-authored-by: Matthew Keil <me@matthewkeil.com>

* Why on startup, who cares

* Add log exmaple

---------

Co-authored-by: Matthew Keil <me@matthewkeil.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.

None yet

3 participants