besu icon indicating copy to clipboard operation
besu copied to clipboard

Updates to DNS discovery

Open diega opened this issue 2 years ago • 5 comments

Signed-off-by: Diego López León [email protected]

PR description

This PR adds a new implementation for EIP-14159 replacing DNSDaemon from Tuweni with a new one, vert.x based. This new implementation provides the following benefits:

  • Random tree traversal
  • Batched discovery
  • Periodic traversal continues from last state
  • Non blocking implementation, based on vert.x DNS client

Fixed Issue(s)

Fixes #1707

Notes

This is sent as a draft to gather feedback on this implementation. How often should we check the DNS for new peers? How many leaves should be attempted to connect per branch? Should this parameters need to be parameterized through the CLI? Based on the willingness to merge this proposal I would finish the specification by adding support to linked trees

Documentation

  • [x] I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

diega avatar Jun 27 '22 19:06 diega

@diega is something wrong with the DNSDaemon from Tuweni? I didn't see feedback or anything on Tuweni, any issues worth reporting?

atoulme avatar Jul 19 '22 01:07 atoulme

Can we engage on this @diega ? Have you done any work to test the implementation and outcomes?

non-fungible-nelson avatar Jul 19 '22 20:07 non-fungible-nelson

Thanks @atoulme for taking the time of looking at it. There is nothing wrong with Tuweni's implementation, but it currently doesn't have any dependency on vert.x and I wanted to take the chance of this requirement to make use of vert.x DNS client to better integrate its execution within the existing event loop (instead of using a java.util.Timer) . Having that said, I coded it in a pretty abstract way so if you think this would be suitable to be part of Tuweni I would be happy to port it to Kotlin and send it as a PR.

diega avatar Jul 20 '22 19:07 diega

@matt-nelson-csi absolutely. Beside the unit testing part of this PR, I ran it using --bootnodes= --discovery-dns-url=enrtree://AKA3AM6LPBYEUDMVNU3BSVQJ5AD45Y7YPOHJLEF6W26QOE4VTUDPE@all.mainnet.ethdisco.net, which allowed me to tweak some constants like how many records per round try to connect (I chose 100, but we can change that based on some other experience). It would be difficult to establish a comparison between both implementations regarding peers. Eventually both would attempt to connect to the same set of boot nodes but this implementation does it random and incrementally. A next step would be to only request new nodes from the DNS tree only when we are bellow certain number of peers instead of doing it every 60 seconds.

diega avatar Jul 20 '22 19:07 diega

@diega sounds good. FWIW a PR to Tuweni would be awesome, but if this PR here and now does the job, no problem. Interesting on the use of Vert.x, that makes sense. I think dnsjava has been less mature in ways, I never did try the DNSClient offered by Vert.x.

atoulme avatar Jul 20 '22 21:07 atoulme

Hi @diega any reasons we ended up closing this PR instead of moving forward to make it to main?

gfukushima avatar May 18 '23 05:05 gfukushima

@gfukushima I don't really remember, but if you find this useful we can reopen it and try to complete the spec

diega avatar May 18 '23 05:05 diega