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

chore(docs): amend port guidance to enable QUIC support #5029

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

zilayo
Copy link
Contributor

@zilayo zilayo commented Dec 28, 2023

Issue Addressed

Following release v4.5.0 Lighthouse will attempt to make QUIC connections.

This isn't immediately obvious unless reading through the beacon-node CLI options and/or release notes as it isn't mentioned in the advanced networking section. The guidance in FAQs will also lead to port conflict errors due to still suggesting port 9001 (default QUIC port) for setting up a secondary node:

Use the flag ```--port <PORT>``` in the beacon node. This flag can be useful when you are running two beacon nodes at the same time. You can leave one beacon node as the default port 9000, and configure the second beacon node to listen on, e.g., ```--port 9001```.

The Docker instructions also don't forward port 9001, so there is likely a large number of nodes running without QUIC which will hinder the ongoing monitoring for latency/bandwidth improvements:

```bash
docker run -p 9000:9000/tcp -p 9000:9000/udp -p 127.0.0.1:5052:5052 -v $HOME/.lighthouse:/root/.lighthouse sigp/lighthouse lighthouse --network mainnet beacon --http --http-address 0.0.0.0
```

Proposed Changes

https://github.com/zilayo/lighthouse/blob/8c13142449930b9114618545a7f11c4140c465bc/book/src/faq.md?plain=1#L103
https://github.com/zilayo/lighthouse/blob/8c13142449930b9114618545a7f11c4140c465bc/book/src/faq.md?plain=1#L429
https://github.com/zilayo/lighthouse/blob/8c13142449930b9114618545a7f11c4140c465bc/book/src/faq.md?plain=1#L103

Additional Info

Please note - there are some formatting diffs due to the settings in https://github.com/sigp/lighthouse/blob/stable/.editorconfig (particularly a lot in faq.md).

I would suggest a new workflow job is added for book push and/or PRs to format all .md files with prettier using the .editorconfig settings to avoid unnecessary diffs in future PRs.

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2023

CLA assistant check
All committers have signed the CLA.

@chong-he chong-he added ready-for-review The code is ready for review docs Documentation labels Jan 3, 2024
Copy link
Member

@chong-he chong-he left a comment

Choose a reason for hiding this comment

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

Thanks for the update, just a minor suggestion here.

Looping @AgeManning or @jmcph4 to have a look at the QUIC networking modifications, particularly there is a plan to change the default QUIC port to move away from port + 1 to a big number port to avoid using conflicting ports.

book/src/faq.md Outdated

Use the flag `--port <PORT>` in the beacon node. This flag can be useful when you are running two beacon nodes at the same time. You can leave one beacon node as the default port 9000, and configure the second beacon node to listen on, e.g., `--port 9100`.
Since V4.5.0, Lighthouse supports QUIC and by default will use the value of `--port` + 1 to listen via UDP (default `9001`).
This can be configured by using the flag `--quic-port`.
Copy link
Member

Choose a reason for hiding this comment

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

A suggestion here to link back to the Advanced Networking page for more information.

Suggested change
This can be configured by using the flag `--quic-port`.
This can be configured by using the flag `--quic-port`. Refer to [Advanced Networking](./advanced_networking.md#nat-traversal-port-forwarding) for more information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to the most recent commit

This can be configured by using the flag `--quic-port`. Refer to [Advanced Networking](./advanced_networking.md#nat-traversal-port-forwarding) for more information.

@AgeManning
Copy link
Member

Yeah. I think the default port of 9001 may have been a bad choice.

We probably will need to make the shift to something better in a new release

Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

I did a quick skim. This looks good to me.

Thanks for the addition :)


The `stability` is:

* `-unstable` for the `unstable` branch
* empty for a tagged release or the `stable` branch
- `-unstable` for the `unstable` branch
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how our editorconfig is causing these changes. As far as I can tell we haven't said "prefer - to *". Similarly with indentation, why are the bodies of the bullet points getting indented?

Do you mind letting us know which editor you're using and which markdown linter it is using? I agree with the idea of linting the book markdown so it's consistent, but would prefer to do that in a separate PR and keep the changes in this PR to a minimum (you could git cherry-pick -p to select just the relevant lines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be due to vscode doing format on save with prettier whenever a configuration file is present (by default prettier will look for either a .prettierrc file, or otherwise an .editorconfig file).

Would have been avoided if I didn't have a global installation of prettier / auto format on save.

Prettier uses - instead of * for unordered lists (introduced in prettier/prettier#4440) and the indentation seems to be an ongoing issue (prettier/prettier#5019).

I've rebased onto the most recent merged unstable commit and applied only the relevant changes to this PR. Definitely makes the diff a lot cleaner!

@dapplion dapplion added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 29, 2024
@zilayo
Copy link
Contributor Author

zilayo commented Feb 4, 2024

@zilayo please undo all unrelated formating changes

@dapplion Most formatting related changes have been removed, however there's multiple trailing whitespaces deletions in faq.md due to the .editorconfig settings.

[*]
indent_style=space
indent_size=4
end_of_line=lf
charset=utf-8
trim_trailing_whitespace=true

I think it's best to keep these deletions in the PR? Otherwise anytime someone updates faq.md it'll keep reintroducing the whitespace deletions.

Have also squashed everything into a single commit.

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Copy looks good to me now, thanks!

@chong-he chong-he added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 6, 2024
@AgeManning
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Feb 8, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at e470596

mergify bot added a commit that referenced this pull request Feb 8, 2024
@mergify mergify bot merged commit e470596 into sigp:unstable Feb 8, 2024
28 of 30 checks passed
danielramirezch pushed a commit to danielramirezch/lighthouse that referenced this pull request Feb 14, 2024
* chore(docs): amend port guidance to enable QUIC support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants