UX Network Fixes #6796
Open
UX Network Fixes #6796
+25
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chong-he
added a commit
to chong-he/lighthouse
that referenced
this pull request
Jan 15, 2025
|
Maybe the docs need some update as well? I have tried to update here: |
|
Nice, good catch! |
|
Nominated for inclusion in |
michaelsproul
added a commit
that referenced
this pull request
Jan 23, 2025
Squashed commit of the following: commit feac919 Author: Age Manning <Age@AgeManning.com> Date: Thu Jan 16 12:51:52 2025 +1100 Prevent duplication of NAT changes commit ee54bab Merge: 1faf0e7 b1a19a8 Author: Age Manning <Age@AgeManning.com> Date: Thu Jan 16 12:48:35 2025 +1100 Merge remote-tracking branch 'network/unstable' into ux-network-fixes commit 1faf0e7 Author: Tan Chee Keong <tanck@sigmaprime.io> Date: Wed Jan 15 11:22:41 2025 +0800 Update as per #6796 commit c742405 Author: Age Manning <Age@AgeManning.com> Date: Tue Jan 14 14:57:23 2025 +1100 Ipv6 port to default to ipv4 port when used commit 78f9c3d Author: Age Manning <Age@AgeManning.com> Date: Tue Jan 14 14:43:52 2025 +1100 Update CLI docs commit b2fc472 Author: Age Manning <Age@AgeManning.com> Date: Tue Jan 14 14:00:32 2025 +1100 Fix lint commit 2f0e4d6 Author: Age Manning <Age@AgeManning.com> Date: Mon Jan 13 17:12:15 2025 +1100 Report NAT when its not functioning commit 6a94d80 Merge: 86bf069 c9747fb Author: Age Manning <Age@AgeManning.com> Date: Mon Jan 13 16:05:33 2025 +1100 Merge remote-tracking branch 'network/unstable' into ux-network-fixes commit 86bf069 Author: Age Manning <Age@AgeManning.com> Date: Mon Jan 13 15:56:21 2025 +1100 Fix port6 CLI commit 410457a Author: Age Manning <Age@AgeManning.com> Date: Mon Jan 13 15:44:59 2025 +1100 Fix some small annoyances
jxs
approved these changes
Jan 23, 2025
Comment on lines
+989
to
+993
| let port = if let Some(port6) = maybe_port6 { | ||
| port6 | ||
| } else { | ||
| port | ||
| }; |
There was a problem hiding this comment.
nit:
Suggested change
| let port = if let Some(port6) = maybe_port6 { | |
| port6 | |
| } else { | |
| port | |
| }; | |
| let port = maybe_port6.unwrap_or(port); |
Comment on lines
+1062
to
+1066
| let port6 = if let Some(port6) = maybe_port6 { | ||
| port6 | ||
| } else { | ||
| port | ||
| }; |
There was a problem hiding this comment.
nit:
Suggested change
| let port6 = if let Some(port6) = maybe_port6 { | |
| port6 | |
| } else { | |
| port | |
| }; | |
| let port6 = maybe_port6.unwrap_or(port); |
michaelsproul
added
v6.1.0
|
Re-targeting this at |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
There were two things I came across during some recent testing, that this PR addresses.
1 - The default port for IPv6 was set to 9090, which is confusing. I've set this to match its ipv4 counterpart (i.e 9000 and 9001). This makes more sense and is easier to firewall, for those firewalls that support both versions for a single rule.
2 - Watching the NAT status of lighthouse, I notice we only set the field to 1 once the NAT is passed. We don't give it a default 0 (false). So we only see results when its successful. On peer disconnects, i've piggy-backed a loop of the connected peers to also watch and check for NAT status updates.