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

Filter Discovered peers for ipv6 support #6498

Merged
merged 10 commits into from
Jan 31, 2024

Conversation

garyschulte
Copy link
Contributor

PR description

updates PeerDiscoveryAgent to

  • use existing NetworkUtility to filter for any/none peers
  • fall back to source address if ipv6 is not supported (like in a docker container)

Fixed Issue(s)

fixes #6475

Copy link

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@garyschulte garyschulte force-pushed the bugfix/discovery-ipv6-errors branch from b9d08c7 to ebf915b Compare January 31, 2024 01:48
@macfarla macfarla requested a review from pinges January 31, 2024 02:18
@garyschulte garyschulte force-pushed the bugfix/discovery-ipv6-errors branch from d9d5aab to 260d6b4 Compare January 31, 2024 02:51
Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

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

Left two comments, but LGTM

.orElseGet(sourceEndpoint::getHost);
.orElseGet(
() -> {
LOG.trace(
Copy link
Contributor

Choose a reason for hiding this comment

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

LOG.atTrace would be nice ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since this is already in a lambda, the extra log lambda isn't necessary :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wait, yeah. you are right! I originally had this logging every time, but it was noisy and moved it to trace

@@ -173,4 +196,20 @@ private static boolean isPortAvailableForUdp(final int port) {
public static boolean isPortAvailable(final int port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've had a look at the isPortAvailableFor[Tcp|Udp] and I thought that we might want to close the ServerSocket if we successfully binded to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently SO_REUSEADDR makes it moot, but I don't see why we wouldn't unbind. It isn't in the scope of this PR, but I can put up a subsequent one.

…ltering, add ipv6 check/fallback

Signed-off-by: garyschulte <[email protected]>
Signed-off-by: garyschulte <[email protected]>
Signed-off-by: garyschulte <[email protected]>
Signed-off-by: garyschulte <[email protected]>
Signed-off-by: garyschulte <[email protected]>
@garyschulte garyschulte force-pushed the bugfix/discovery-ipv6-errors branch from 260d6b4 to 7dc8bc7 Compare January 31, 2024 03:51
@garyschulte garyschulte enabled auto-merge (squash) January 31, 2024 03:51
Signed-off-by: garyschulte <[email protected]>
@garyschulte garyschulte force-pushed the bugfix/discovery-ipv6-errors branch from 1757144 to 59da07a Compare January 31, 2024 03:58
@garyschulte garyschulte disabled auto-merge January 31, 2024 04:07
@garyschulte garyschulte enabled auto-merge (squash) January 31, 2024 04:07
@garyschulte garyschulte merged commit 79245bb into hyperledger:main Jan 31, 2024
18 checks passed
Gabriel-Trintinalia pushed a commit to Gabriel-Trintinalia/besu that referenced this pull request Feb 1, 2024
* use existing NetworkUtility for PeerDiscoveryAgent pingpacket data filtering, add ipv6 check/fallback
* log at debug when we override pingpacket from
* use java native address parsing rather than lookup by host

Signed-off-by: garyschulte <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
@garyschulte garyschulte deleted the bugfix/discovery-ipv6-errors branch April 3, 2024 19:25
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.

IPv6 peer discovery errors
2 participants