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

Specify Discovery v5 #48

Closed
3 of 5 tasks
fjl opened this issue Sep 21, 2018 · 13 comments
Closed
3 of 5 tasks

Specify Discovery v5 #48

fjl opened this issue Sep 21, 2018 · 13 comments

Comments

@fjl
Copy link
Collaborator

fjl commented Sep 21, 2018

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:

  • Being independent of the clock
  • Making traffic amplification prevention less weird
  • Relaying more node metadata
  • Indexing nodes by their capabilities
  • Obfuscating discovery traffic

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:

  • Writing the wire protocol spec as an EIP.
  • Specifying the upgrade procedure. We will likely announce the version through ENR but need a document that says what the key/value pair for that is and how the protocols can coexist on the same UDP port.

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:

  • Commit initial wire protocol draft
  • Commit initial requirements document draft
  • Commit topic table spec draft
  • Move ENR specification to this repo
  • Write short explainer document that ties all the specs together
@fjl
Copy link
Collaborator Author

fjl commented Sep 21, 2018

@carver
Copy link
Contributor

carver commented Oct 4, 2018

@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
Copy link
Collaborator Author

fjl commented Oct 5, 2018

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

@EBGToo
Copy link

EBGToo commented Oct 5, 2018

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
Copy link
Collaborator Author

fjl commented Oct 5, 2018

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
Copy link

EBGToo commented Oct 5, 2018

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
Copy link

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
Copy link

cleishm commented Dec 31, 2018

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
Copy link
Collaborator Author

fjl commented Jan 2, 2019

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

@cleishm
Copy link

cleishm commented Jan 2, 2019

@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
Copy link
Member

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

@fjl
Copy link
Collaborator Author

fjl commented Apr 10, 2019

@fjl
Copy link
Collaborator Author

fjl commented Apr 11, 2019

Closing this in favor of the Discovery v5 issue milestone

@fjl fjl closed this as completed Apr 11, 2019
@fjl fjl added this to the Discovery v5 milestone Apr 11, 2019
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

No branches or pull requests

6 participants