Skip to content

Conversation

AgeManning
Copy link
Member

Description

The PING mechanism for discv5 informs our peers of ENR updates as the sequence number is included with the PING. We ping every connected peer by default every 5 minutes (not all at once, each peer has a 5 minute timer).

Once we have decided that our ENR should be updated (given the PONG's from our peers have aligned on a fixed value) we have been sending out PINGs to all our peers to inform them to update our ENR. This is an optimization that allows us to quickly form connections and get our ENR into the routing table of other peers as soon as we know we are externally contactable.

We are investigating an issue, whereby a non-stable decision as to our external address can lead to ping amplifications. If we think this is likely to happen, it probably isn't worth this optimization, also sending these excessive pings is quite noisey and probably its fine to wait for the natural 5 minute ping to gradually disseminate our new ENRs to the network.

Discovery ENR propagation is a slow process anyway, so changing from rapid updates with a small risk of amplification to a slow spread of the ENR could be the more reasonable choice.

@AgeManning AgeManning requested a review from dknopik September 18, 2025 03:40
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

While this looks good to me, I wonder if there is a hybrid approach, e.g. only ping all if the last "ping all" is more than x time ago.

But that might not be worth the implementation complexity

@AgeManning
Copy link
Member Author

Yeah, because of the nature of the PING's timer. We are pinging peers all the time (because they connected at different times). So a change in our ENR will go out fast to some and slow to others just based on their ping cycles.

If we make it into earlier peer's tables, they will propagate our ENR earlier.

I'm less convinced we need this optimization, and given that we've seen an example of an amplification, even though I think #292 resolves that, its probably not worth the speed and noise to yell at everyone that we updated our ENR.

Therefore I think we merge this guy.

@AgeManning AgeManning merged commit e9e14ef into master Sep 19, 2025
6 checks passed
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.

2 participants