-
Notifications
You must be signed in to change notification settings - Fork 879
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
feat: Node discovery via DNS #7129
feat: Node discovery via DNS #7129
Conversation
-- Use besu-dns-discovery which is a fork of Tuweni dns discovery Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…nsensys_dns_discovery Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
I'm curious as to why this was ported to a separate Consensys lib and not folded directly into Besu. Are there other projects that use this library? Is it because it's written on Kotlin? It's only 5 production files and 2 test files if wiring in a kotlin compiler step isn't desired (surely less effort than maintaining an ongoing deployment). I't also makes it more difficult to alter the classes if features such as streaming the nodes instead of querying them all in one go is desired. |
@shemnon Hi Danno, the initial thought was just to port it as it is to make further changes and include it as a library, however, you've raised valid points, I concur that folding these classes (the dns-discovery) into besu should not be too difficult. I am not aware of any other projects using the Tuweni dns-discovery module. @macfarla any further thoughts? |
yeah fair point @shemnon if it's easier long term I'm not opposed to folding it into Besu. Initial thinking was we could still revert to tuweni libs again if they got updated but that also may not eventuate. |
As a bit more context, I'm looking at folding the tuweni-bytes libraries back into besu (undoing #215 in effect). My main reason is a tighter optmiization loop when speeding up the EVM as well as being able to purpose-build some of the bytes implementation for speed. Coordinating these changes across repos creates significant friction. |
-- migrated from Tuweni Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…rticle Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
// Adapted from https://github.com/tmio/tuweni and licensed under Apache 2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this sufficient attribution as far as the apache license is concerned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is. Although we may have to update our NOTICE file with Tuweni's NOTICE file. Our NOTICE file is empty though. Also, we didn't do a verbatim copy of the code, we adapted and converted it into Java language from Kotlin language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we add all contributors info to the notice going forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we are "reproducing" or "including" code from an Apache license project that contains a NOTICE file, it ought to be included in our NOTICE file as well. I am not sure whether we had to include NOTICE attributions from Vertx and other libraries that are included in binary form. @non-fungible-nelson FYI for your input as well.
ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/dns/MockDNSServer.java
Outdated
Show resolved
Hide resolved
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/dns/DNSDaemon.java
Outdated
Show resolved
Hide resolved
ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/dns/DNSEntry.java
Outdated
Show resolved
Hide resolved
...reum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/dns/DnsPortSplitTest.java
Outdated
Show resolved
Hide resolved
ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/dns/MockDNSServer.java
Outdated
Show resolved
Hide resolved
ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/dns/DNSDaemonTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
…very Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
NOTICE
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
…very Signed-off-by: Usman Saleem <[email protected]>
…very Signed-off-by: Usman Saleem <[email protected]>
PR description
Fold Tuweni dns discovery module into Besu p2p discovery module. The Tuweni dns-discovery was Kotlin code using kotlin co-routines. It has been translated (and adapted) in Java. Utilized vertx worker verticle to launch dns daemon in a separate thread outside event loop. Rest of the method protocol is exactly same at the moment.
The screenshot shows mainnet peer connection for four nodes which were configured with SNAP and CHECKPOINT.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests