-
Notifications
You must be signed in to change notification settings - Fork 57
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
chore: Discovery in libwaku #2711
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
99 changes: 99 additions & 0 deletions
99
library/waku_thread/inter_thread_communication/requests/discovery_request.nim
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
import std/[json, sequtils] | ||
import chronos, stew/results, libp2p/multiaddress | ||
import | ||
../../../../waku/factory/waku, | ||
../../../../waku/discovery/waku_dnsdisc, | ||
../../../../waku/discovery/waku_discv5, | ||
../../../../waku/waku_core/peers, | ||
../../../alloc | ||
|
||
type DiscoveryMsgType* = enum | ||
GET_BOOTSTRAP_NODES | ||
UPDATE_DISCV5_BOOTSTRAP_NODES | ||
|
||
type DiscoveryRequest* = object | ||
operation: DiscoveryMsgType | ||
|
||
## used in GET_BOOTSTRAP_NODES | ||
enrTreeUrl: cstring | ||
nameDnsServer: cstring | ||
timeoutMs: cint | ||
|
||
## used in UPDATE_DISCV5_BOOTSTRAP_NODES | ||
nodes: cstring | ||
|
||
proc createShared( | ||
T: type DiscoveryRequest, | ||
op: DiscoveryMsgType, | ||
enrTreeUrl: cstring, | ||
nameDnsServer: cstring, | ||
timeoutMs: cint, | ||
nodes: cstring, | ||
): ptr type T = | ||
var ret = createShared(T) | ||
ret[].operation = op | ||
ret[].enrTreeUrl = enrTreeUrl.alloc() | ||
ret[].nameDnsServer = nameDnsServer.alloc() | ||
ret[].timeoutMs = timeoutMs | ||
ret[].nodes = nodes.alloc() | ||
return ret | ||
|
||
proc createRetrieveBootstrapNodesRequest*( | ||
T: type DiscoveryRequest, | ||
op: DiscoveryMsgType, | ||
enrTreeUrl: cstring, | ||
nameDnsServer: cstring, | ||
timeoutMs: cint, | ||
): ptr type T = | ||
return T.createShared(op, enrTreeUrl, nameDnsServer, timeoutMs, "") | ||
|
||
proc createUpdateBootstrapNodesRequest*( | ||
T: type DiscoveryRequest, op: DiscoveryMsgType, nodes: cstring | ||
): ptr type T = | ||
return T.createShared(op, "", "", 0, nodes) | ||
|
||
proc destroyShared(self: ptr DiscoveryRequest) = | ||
deallocShared(self[].enrTreeUrl) | ||
deallocShared(self[].nameDnsServer) | ||
deallocShared(self) | ||
|
||
proc retrieveBootstrapNodes( | ||
enrTreeUrl: string, ipDnsServer: string | ||
): Result[seq[string], string] = | ||
let dnsNameServers = @[parseIpAddress(ipDnsServer)] | ||
let discoveredPeers: seq[RemotePeerInfo] = retrieveDynamicBootstrapNodes( | ||
true, enrTreeUrl, dnsNameServers | ||
).valueOr: | ||
return err("failed discovering peers from DNS: " & $error) | ||
|
||
var multiAddresses = newSeq[string]() | ||
|
||
for discPeer in discoveredPeers: | ||
for address in discPeer.addrs: | ||
multiAddresses.add($address & "/" & $discPeer) | ||
|
||
return ok(multiAddresses) | ||
|
||
proc updateDiscv5BootstrapNodes(nodes: string, waku: ptr Waku): Result[void, string] = | ||
waku.wakuDiscv5.updateBootstrapRecords(nodes).isOkOr: | ||
return err("error in updateDiscv5BootstrapNodes: " & $error) | ||
return ok() | ||
|
||
proc process*( | ||
self: ptr DiscoveryRequest, waku: ptr Waku | ||
): Future[Result[string, string]] {.async.} = | ||
defer: | ||
destroyShared(self) | ||
|
||
case self.operation | ||
of GET_BOOTSTRAP_NODES: | ||
let nodes = retrieveBootstrapNodes($self[].enrTreeUrl, $self[].nameDnsServer).valueOr: | ||
return err($error) | ||
|
||
return ok($(%*nodes)) | ||
of UPDATE_DISCV5_BOOTSTRAP_NODES: | ||
updateDiscv5BootstrapNodes($self[].nodes, waku).isOkOr: | ||
return err($error) | ||
return ok("discovery request processed correctly") | ||
|
||
return err("discovery request not handled") |
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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 there any value in updating bootstrap nodes on runtime? aren't they only used when connecting to the network?
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.
Good point! This feature comes from what we are already offering from
go-waku
so we offer the same. I guess that was a requirement from any of the current projects that integrates us (cc @richard-ramos .)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.
To give some context (since this might be implementation-specific in go-waku)
For discv5, we use the go-ethereum implementation (which i extracted into a separate package in https://github.com/waku-org/go-discover). There are two separate instances in which discv5 requires populating the bootnodes in runtime:
1 - When starting the node, sometimes the DNS Discovery URL fails to resolve (i.e. maybe the nameserver is down). Since in Status app we shoudn't stop the login process unless it is an unrecoverable error, we do start discv5, (which will not return any peer), and keep retrying with dns discovery until the address resolves, and in that moment we setup the bootnodes.
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.
are there nodes that do not require dns discovery in the bootstrap node list?
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.
In case you asked w.r.t
wakunode2
(nwaku
), it is mandatory for the app to retrieve bootstrap nodes from DNS:nwaku/waku/factory/waku.nim
Lines 140 to 161 in 5ee4cba
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.
don't know the details then, but that sounds like an unnecessary single point of failure - normally, one would bootstrap discv5 from multiple sources (dns, hardcoded nodes, nodes from previous startups etc)
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.
My understanding is that it's not mandatory. If we don't have DNS discovery enabled
retrieveDynamicBootstrapNodes
will returnok
with an empty seq and the node will simply set up without dynamic bootstrap nodes.nwaku/waku/discovery/waku_dnsdisc.nim
Lines 130 to 131 in 5ee4cba
However, if we enable DNS discovery and it fails retrieving the nodes then the whole node setup fails. Not sure if it should be like that or if it would be better to add a warning and continue the setup using other sources as a backup (as if DNS discovery wasn't configured)
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.
Thanks for the comment @gabrielmer ! You are absolutely right with that :) With that, the
dns
source is not mandatory.On the other hand, we also have another possible source of bootstrap nodes for
discv5
, which is a config parameter:nwaku/waku/factory/external_config.nim
Lines 512 to 516 in 5ee4cba
( cc @arnetheduck )
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.
@gabrielmer - very good point!
It might sound better to only fail everything if
discv5
tries to start without any bootstrap node. Nevertheless, I think is better to leave it as it is now because that can help to rapidly identify DNS issues. If in the future we see that this represent a blocking/repetitive issue then we can reconsider the current approach :)Cheers