Add Discovery V5 alongside V4 #3489
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also cleanup peer pool and EthereumNode async start and termination. Async raises in Discovery V4 also refactored. There is no algorithm changes, only modify error handling of both receiver proc. So when discovery V4 cannot handle incoming message, there is discovery V5 waiting to handle it. If both of them error, the error will be logged with two error message.
kdeme
reviewed
Jul 21, 2025
Comment on lines
+126
to
+160
| proc open*( | ||
| proto: Eth1Discovery, enableDiscV4: bool, enableDiscV5: bool | ||
| ) {.raises: [TransportOsError].} = | ||
| # TODO: allow binding to both IPv4 and IPv6 | ||
|
|
||
| if not (enableDiscV4 or enableDiscV5): | ||
| return | ||
|
|
||
| privateAccess(DiscV4) | ||
| privateAccess(DiscV5) | ||
|
|
||
| info "Starting discovery node", | ||
| node = proto.discv4.localNode, | ||
| bindAddress = proto.discv4.address, | ||
| discV4 = enableDiscV4, | ||
| discV5 = enableDiscV5 | ||
|
|
||
| if enableDiscV4 and not enableDiscV5: | ||
| proto.discv4.open() | ||
| proto.discv5 = nil | ||
| return | ||
|
|
||
| if enableDiscV5 and not enableDiscV4: | ||
| proto.discv5.open() | ||
| proto.discv4 = nil | ||
| proto.discv5.seedTable() | ||
| return | ||
|
|
||
| # Both DiscV4 and DiscV5 share the same transport | ||
| # Unhandled data from DiscV4 will be handled by DiscV5 | ||
| let ta = initTAddress(proto.discv4.bindIp, proto.discv4.bindPort) | ||
| proto.discv4.transp = newDatagramTransport(processClient, udata = proto, local = ta) | ||
| proto.discv5.transp = proto.discv4.transp | ||
| proto.discv5.seedTable() | ||
|
|
There was a problem hiding this comment.
This isn't exactly the cleanest solution as it rather hacks around the existing APIs and requires some duplication also.
But I assume it works and allows for quick way of using both. However if both are required in the "long" term future still, we should revise this definitely.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also cleanup peer pool and EthereumNode async start and termination. Async raises in Discovery V4 also refactored.
There is no algorithm changes, only modify error handling of both receiver proc. So when discovery V4 cannot handle incoming message, there is discovery V5 waiting to handle it.
If both of them error, the error will be logged with two error message.
And this is test result from hive, some test cases are failing. But other clients also have no perfect score:
fix #2799