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

More lenient ipv6 auto-update #266

Merged
merged 19 commits into from
Oct 14, 2024
Merged

More lenient ipv6 auto-update #266

merged 19 commits into from
Oct 14, 2024

Conversation

AgeManning
Copy link
Member

Description

On IPv4 dominated networks, local routing tables can be fully populated with ipv4 nodes limiting the number of ipv6 ping/pong responses. This can prevent the enr-auto-update from finding and updating a node's ENR to its external socket.

For security reasons, we can't just accept PONG responses from any node on the network. For this reason, we only ping peers that make it into our routing table as a number of security measures must be passed for a node to enter the table. However if the table is full, we reject peers from entering and therefore do not attempt to PING them.

This PR suggests the following mechanism:
If a peer fails to make it into the routing table, and we are lacking votes on either IPv4 or IPv6 and this peer can add a vote AND they are an outgoing peer (i.e one that we have chosen to connect to, for security reasons) then we ping them to get an extra vote for our external address.

@AgeManning
Copy link
Member Author

cc @pawanjay176

@ackintosh - What do you think of this logic? Is there an easy way to test this with the current infra?

src/service.rs Outdated Show resolved Hide resolved
@ackintosh
Copy link
Member

Currently, we accept a PONG response as a vote only if the node exists our routing table. So the PONG response is ignored even if we PING a node that failed to make it into our routing table. We should also change this condition?

discv5/src/service.rs

Lines 816 to 818 in 765abac

self.kbuckets.write().entry(&key),
kbucket::Entry::Present(_, status)
if status.is_connected() && !status.is_incoming());

@ackintosh
Copy link
Member

I think Testground doesn’t support IPv6, so we can build a test environment based on Docker Compose. It might require a little work, but I’ll give it a try.

@AgeManning
Copy link
Member Author

ah nice! Thanks for the catch!

I've modified the logic a bit to also just do this if we are in DualStack mode.

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM age, left some suggestions

src/service.rs Outdated Show resolved Hide resolved
src/service.rs Outdated Show resolved Hide resolved
src/service.rs Outdated Show resolved Hide resolved
src/service.rs Outdated Show resolved Hide resolved
src/service.rs Show resolved Hide resolved
src/service.rs Outdated Show resolved Hide resolved
src/service/test.rs Outdated Show resolved Hide resolved
src/service/test.rs Outdated Show resolved Hide resolved
src/service/test.rs Outdated Show resolved Hide resolved
src/service/test.rs Outdated Show resolved Hide resolved
AgeManning and others added 11 commits October 14, 2024 13:05
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
Co-authored-by: João Oliveira <hello@jxs.pt>
@AgeManning AgeManning merged commit 5216acf into master Oct 14, 2024
6 checks passed
@ackintosh
Copy link
Member

I've implemented a simulation (https://github.com/ackintosh/discv5-dual-stack) and tested this PR using it. The ipv6 auto-update works fine!

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