ethereum / devp2p

Ethereum peer-to-peer networking specifications
984 stars 275 forks source link

Specify Discovery v5 #48

Closed fjl closed 5 years ago

fjl commented 5 years ago

The existing discovery protocol has been around for a long time and has it's shortcomings. Some of those are listed in the specification.

There are a few things we want in the next version of the discovery protocol:

Some of these things, specifically the 'node metadata' parts are solved by ENR. We can even fit them into the existing protocol using the ENR extension.

We also have a prototype implementation of the indexing mechanism in go-ethereum, and a pretty clear idea about the wire protocol. But nothing is set in stone yet.

What remains to be done is:

If you want to start implementing discovery v5 now, it is best to start with ENR and the v4 extension.


Remaining tasks for this tracking issue:

fjl commented 5 years ago

Implementation-tracking issues I know of are:

carver commented 5 years ago

@EBGToo asked:

I should expect current versions of Geth to continue to support v4? Between v4 and v5 the UDP packet header has changed, incompatibly. Currently, when connecting with the v4 UDP header (as packet-header = hash || signature || packet-type) I end up being thrown a errBadPrefix from the v5 code:

       /// v5
  prefix, sig, sigdata := buf[:versionPrefixSize], buf[versionPrefixSize:headSize], buf[headSize:]
  if !bytes.Equal(prefix, versionPrefix) {
      return errBadPrefix
  }

(because versionPrefix is not a hash...). I've not enabled v5 discovery on my Geth node. How is this expected to work?

fjl commented 5 years ago

Yes, we support v4 discovery in geth and will continue to do so for a long time.

EBGToo commented 5 years ago

I'm not sure how that is. I start Geth with --lightserv X --lightpeers Y and the initialization code sets the variable srv.DiscoveryV5 to true. If my mobile light client attempts to connect for discovery, using V4 with a packet header that starts with 'hash' (not 'versionPrefix') then I get instantly rejected with 'bad prefix'. So, any LESv1 or LESv2 Geth peer that I connect to for block chain data is going to reject discovery?

fjl commented 5 years ago

Can you clarify how your client gets rejected exactly? Geth full node with --lightserv ... runs both V4 and the prototype version of V5 on the same port. If a packet is deemed invalid by the V4 code, it is passed on to V5. You might be running into this problem.

EBGToo commented 5 years ago

Alas, my mistake. I saw this in the log: DEBUG[10-05|13:39:06.820] Bad packet from 127.0.0.1:49619: bad prefix and assumed that only my other bootstrap nodes were producing peers. I stripped down my client to a single bootstrap peer - I see the 'bad prefix' and also peers (as you described, via V4 apparently). Thanks for the response.

adambabik commented 5 years ago

What's the state of Discovery V5 implementation in geth, that is https://github.com/ethereum/go-ethereum/tree/master/p2p/discv5 package? Is it considered ready or subject to refactor/rewrite?

cleishm commented 5 years ago

A small request for v5 (or later) - can we add a findNodeHash field to the Neighbors packet? Currently, it is impossible to correlate a Neighbors response from a peer to the FindNode request and associated target address. This results in the need to a) avoid any concurrent FindNode requests to a peer and b) use delays to try and ensure that all Neighbors packets have been received before a new FindNode request can be sent. Even then, there is no guarantee that a Neighbors packet responding to a previous request wasn't delayed on the network and received after a new FindNode request has been sent.

Adding the hash of the FindNode request to the Neighbors response packet(s) would allow these issues to be avoided and simplify implementations.

Please let me know if there is a better place to offer this suggestion!

fjl commented 5 years ago

@cleishm I agree, we need better request/response correlation for all packets.

cleishm commented 5 years ago

@fjl: Right. I now see some work toward that in your p2p-drafts repo: https://github.com/fjl/p2p-drafts/blob/master/discv5-packets.md#neighbors-packet-0x03

What is the state of these drafts? I see some of the implementation in go-ethereum already?

FrankSzendzielarz commented 5 years ago

@cleishm Please see discv5 folder. A test implementation is underway. Things are still in flux a bit because of Eth 2.0 requirements.

fjl commented 5 years ago

WIP Go implementation is here: https://github.com/fjl/go-ethereum/tree/discover-v5/p2p/discover

fjl commented 5 years ago

Closing this in favor of the Discovery v5 issue milestone