ethereum / devp2p

Ethereum peer-to-peer networking specifications
971 stars 273 forks source link

discv5: indicate v4 as the current version #216

Closed ryandotsmith closed 1 year ago

fjl commented 1 year ago

This change is not correct. Both Discovery v4 and v5 are actively used. The current protocol version of discv5 is v5.1.

ryandotsmith commented 1 year ago

This change is not correct. Both Discovery v4 and v5 are actively used. The current protocol version of discv5 is v5.1.

Is Geth using v5?

fjl commented 1 year ago

Geth uses Discvoery v5 in light client mode (but that mode does not work since the merge).

ryandotsmith commented 1 year ago

Geth uses Discvoery v5 in light client mode (but that mode does not work since the merge).

I see. That has been my experience.

I am motivated to make this change (or something similar) from my own experience of attempting to implement the discovery process. I started by reading the v5 docs. Which led me down the road of implementing the v5 protocol. It was only until I began testing my implementation that I realized Geth uses v4. So I then implemented the v4 protocol. I would have not had to go through this confusion hard there been clear guidance in the v5 docs.

fjl commented 1 year ago

I'm sorry to hear that. All I can say is, both protocol versions are in use, and therefore I will not say that v4 is canonical in the v5 spec.

ryandotsmith commented 1 year ago

Well, both versions are not "in use" since Geth doesn't use v5. If you want to work with the execution layer network as it is deployed today you need to use v4.

My point here is that somewhere in this repo, it should be explicitly mentioned that you will need to use v4 and that v5 is not widely deployed. If you have a better idea for where to lay out that expectation, that is fine. But I think it needs to exist.

ajsutton commented 1 year ago

discv5 is in use by the consensus layer and is indeed very widely deployed.

ryandotsmith commented 1 year ago

discv5 is in use by the consensus layer and is indeed very widely deployed.

True. But if you want to find a geth node and interact with the execution layer, v5 will not work and v4 must be used.

Obviously this is a subtle detail, but again, to anyone who is wanting to develop within the execution layer, a clear sentence like "to interact with geth / execution layer you must use v4 and not v5" would be the correct and helpful thing to say.

ajsutton commented 1 year ago

But the discv5 spec shouldn't include that detail. If you want to interact with some software you should check what protocols that software uses, not expect other specifications to tell you.

ryandotsmith commented 1 year ago

Sure. I see how my current suggested change isn't the best. I'm open to coming up with a different solution.

I think it would be great for there to be some snippet in this repository that says something like:

"The consensus layer uses discv5 and the execution layer uses discv4"

ryandotsmith commented 1 year ago

I should add that my current suggested change was motivated by what I saw in the v4 docs. I quite literally copy/pasted my suggested change from the v4 doc. @ajsutton to your point, perhaps this should be removed?

image