Discv5 POC integration #748
Conversation
Jenkins BuildsClick to see older builds (29)
|
| @@ -53,6 +57,15 @@ proc initAddress(T: type MultiAddress, str: string): T {.raises: [Defect, ValueE | |||
| raise newException(ValueError, | |||
| "Invalid bootstrap node multi-address") | |||
|
|
|||
| func getTransportProtocol(typedR: TypedRecord): Option[IpTransportProtocol] = | |||
| if typedR.tcp6.isSome or typedR.tcp.isSome: | |||
There was a problem hiding this comment.
can we make this as a separate function and call it here, as for new transport we need to add the functionality in getTransportProtocol rather than modifying it.
| if typedR.tcp6.isSome or typedR.tcp.isSome: | ||
| return some(IpTransportProtocol.tcpProtocol) | ||
|
|
||
| if typedR.udp6.isSome or typedR.udp.isSome: |
| let ip = ipv4(typedR.ip.get) | ||
| addrs.add MultiAddress.init(ip, tcpProtocol, Port typedR.tcp.get) | ||
|
|
||
| if typedR.ip6.isSome: |
There was a problem hiding this comment.
it seems we are reusing the code for tcp and udp .
There was a problem hiding this comment.
True, except that we read different transport addresses for each, of course. It's not pretty, but in this instance I prefer having it typed out as readable as possible, rather than having, say, a template where the construction of the addresses can become a bit obscure. WDYT?
There was a problem hiding this comment.
It is tradeoff, and i get what you mean, my reasoning was that if we start supporting more transport then we have to add in this section again with redundant code, however for only 2 transports it is readable in current format.
|
UPDATE: not sure why CI is failing for Github CI (it's passing on Jenkins for all three platforms). It may be related to subtle differences in |
|
|
||
| var discoveredPeers: seq[RemotePeerInfo] | ||
|
|
||
| for node in discoveredNodes: |
There was a problem hiding this comment.
queryRandom does not have a size guard , do we accept all the discovered nodes anyway. A limit over the discovered nodes might be useful for certain topologies.
There was a problem hiding this comment.
Quite right! I think we do not yet have a clear idea how quickly our own "discovered" peer store will grow and we may have to implement stricter limits at some point.
In practice, note that we have the following limits already:
- for libp2p connections, the
maxConnectionssetting - the discv5 routing table, and query results like this one, are limited by k-buckets and bucket size. You can see how the underlying query is limited here
| # Discovery v5 API # | ||
| #################### | ||
|
|
||
| proc findRandomPeers*(wakuDiscv5: WakuDiscoveryV5): Future[Result[seq[RemotePeerInfo], cstring]] {.async.} = |
There was a problem hiding this comment.
As a suggestion, I'd define a type for Result[seq[RemotePeerInfo], cstring]
There was a problem hiding this comment.
Generally agree, but for a Result being used only once I think we may have some unnecessary indirection? For example, this module elsewhere returns Result[enr.Record, cstring] which is not reused either, but would then also require its own type for consistency.
There was a problem hiding this comment.
As it is wrapped inside a Future type, I thought it is prettier to be defined as a separate type, though not necessary.
|
|
||
| if newPeers.len > 0: | ||
| debug "Connecting to newly discovered peers", count=newPeers.len() | ||
| await connectToNodes(node, newPeers) |
There was a problem hiding this comment.
Any limit on the number of connected Peers?
There was a problem hiding this comment.
The underlying libp2p max connections which is now configurable. Though we may indeed want to implement something more sophisticated for our own peer connection management (e.g. prioritising peers we discovered ourselves above incoming connections, etc.)
|
Merging this PR in order to get some traction on the release from a stable codebase. Happy to address any further comments in subsequent PRs, if there are any. :) |
This PR relates to vacp2p/research#75
It adds POC integration of
discv5innim-waku.Background
Discovery v5 provides the first "true" ambient peer discovery mechanism for
nim-waku. In combination with DNS discovery this providesnim-wakuclients the option to bootstrap connection to the network and gradually discover other peers.Clients may choose not to enable
discv5for many reasons, including:We try to mitigate the latter by providing external API calls to start and stop
discv5. The needle-in-a-haystack problem is an ongoing investigation, together with capability discovery.For now Discovery v5 is disabled by default.
Why is this POC?
It shows that discovery v5 works for
nim-waku. What still needs to be proven/tested is:discv5is only enabled for short periods of time (using thestartandstopAPI calls)Next steps for
discv5innim-wakuNext steps for peer discovery in
nim-wakunim-libp2p, with a lot to gain from it)Also see discussion re desktop requirements