feat(discv5): added find random nodes with predicate

[LNSD]

Description

Waku discovery5 implementation does random walks through the DHT and reports the discovered nodes. To support Waku relay static shards discovery, we want to announce in the ENR the shards the node is subscribed to and keep only the discovered nodes that match specific criteria (a.k.a. predicate).

Changes

• Simplified and refactored some parts of the WakuDiscv5 implementation (e.g., moved from exceptions to Results)
• Added a predicate argument to the "find random peers" method to filter out records that do not match the predicate.
• Reworked the Waku Discv5 test suites.

[alrevuelta]

isnt this to much to log one line for every peer? perhaps we can trace it, but debug is too much imho.

i wouldnt remove the "Discovered peers" as its quite relevant to see if we are finding new peers. Im fine with moving it to debug if info is too verbose, but wouldnt remove it.

[LNSD]

isnt this to much to log one line for every peer? perhaps we can trace it, but debug is too much imho.

Maybe. We can just log the new peers in debug level.

i wouldnt remove the "Discovered peers" as its quite relevant to see if we are finding new peers. Im fine with moving it to debug if info is too verbose, but wouldnt remove it.

It is not about verbosity. info level should never be used in libraries. This goes against standard practices and, as a consequence, the expectations of the library users. The standard is that a library must not emit logs unless the debug level is enabled.

Typically, info, warn, and error are application-specific log levels. If one library uses the error level clearly shows that there is an API design flaw. We have Results and Exceptions to convey these "erroneous states" up in the call chain.

Based on this reasoning: waku/v2/waku_node is a library module. It should not use info, warn, or error logs. If used, those are code smells and should be converted to either Result/Defect or pushed one layer up to the application: wakunode2/app.nim#App.

---

Note that this is not just my opinion; it is based on facts and good practices and aligns with a discussion about logging that happened in the Nimbus server (for example).

[LNSD]

isnt this to much to log one line for every peer? perhaps we can trace it, but debug is too much imho.

Maybe. We can just log the new peers in debug level.

Changed here: https://github.com/waku-org/nwaku/compare/975924dd1b6c1e7c67aa49a6ef7a14448571af5d..b972dd88a66f7afa97410b133e9825bec817dd18

[alrevuelta]

not sure I follow.

This log has been removed, which I think is important.

info "Discovered peers", discovered=discoveredPeers.len, new=newSeen

If you dont want to use info, fine, use debug.

and regarding the log you have added, having debug "peer discovered" (which is logged for every discovered peer every discovery hearbeat) is going to add too many logs.

not a blocker though.

[jm-clius]

LGTM, thanks!

[jm-clius]

This pred is not set yet anywhere. In other words, the remaining work here is to add the static topic configuration and construct a predicate that checks ENRs against that. LMK if you planned to do this or if we should schedule it our side (no expectations, of course, just don't want to interfere if you have more PRs lined up :) )

[LNSD]

Yes, I did not integrate it into the app. I think that before adding the predicate (e.g., for filtering static sharding or filtering by node capabilities), some questions should be answered:

• How will we pass the "static shard" topics to the "nwaku infra nodes"?
• Shall we do it via --topics config option?
• If we identify that one of those topics is a "static shard" topic and the discv5 discovery loop is enabled, shall we filter the discovered records?

Depending on these questions, some tasks should be done:

• Move the Discv5 discovery loop to thewakunode2 application (from waku_node module).
• Define how we are going to pass the "static shard" topics to the "nwaku infra nodes".
• Parse the "static shard" topics provided into NsPubsubTopics.
• Use these topics to create a predicate to filter the discovered topics.

---

For the record:

My leave precipitated this PR, but I am not fully convinced about passing the predicate via an argument in the findRandomPeers procedure. I think that we should create a "discovery service" that should be within the App and should be responsible for finding (and filtering) the peers and feeding them to the peer store.

[jm-clius]

@LNSD, I apologise for not having seen this comment earlier. At some point I must have added it to a TODO and then lost track :)

For now I think the best idea is to use the --topics config option with the app then identifying this as a static shard configuration and (somehow) passing in the predicate for discv5 filtering. This indeed points to the fact that at least certain types of stand-beside discovery should happen as a service separate to the node. For now, depending on capacity, we may do something simpler first, but I agree that this is the direction we should be taking.

That said, you've helped us a lot so happy to take ownership to get this and the subsequent features merged. To make it a bit easier for us to rebase (which may help fix some unrelated tests) I've merged your changes to a base repo branch in #1763. If you agree that we take over from here (thanks for getting this done!), we can close this PR without merging and continue the feature from there?

[vpavlin]

LGTM

[vpavlin]

Seems like macos test is failing with

2023-05-02T15:07:34.3574730Z     /Users/runner/work/nwaku/nwaku/tests/v2/test_waku_discv5.nim(145, 48): Check failed: node2.wakuDiscv5.protocol.nodesDiscovered == 2
2023-05-02T15:07:34.3575280Z     node2.wakuDiscv5.protocol.nodesDiscovered was 1

Is it due to "missing" sleep?:)

[LNSD]

Seems like macos test is failing with

2023-05-02T15:07:34.3574730Z     /Users/runner/work/nwaku/nwaku/tests/v2/test_waku_discv5.nim(145, 48): Check failed: node2.wakuDiscv5.protocol.nodesDiscovered == 2
2023-05-02T15:07:34.3575280Z     node2.wakuDiscv5.protocol.nodesDiscovered was 1

Is it due to "missing" sleep?:)

Hehehe 😆

No, I already increased the "sleep time," and the issue still happens. I have reduced the test checks to the bare minimum, just "sanity checks". It should be sufficient.

[jm-clius]

Closing as duplicated and merged elsewhere.