chore: Discovery in libwaku

[Ivansete-status]

Description

Finalizing the addition of discovery features to libwaku.

Changes

• Extension of libwaku with discovery capabilities.
• Extension of cwaku_example so that we can check how the new exposed functions work.
• Enhancement in node_lifecycle_request.nim for better error control. That helps to determine when a wrong JSON config parameter is passed to the library.

Issue

closes #2455

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2711-rln-v1

Built from 45eedb6

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2711-rln-v2

Built from 45eedb6

[gabrielmer]

LGTM, thank you! :))

[gabrielmer]

is there any value in updating bootstrap nodes on runtime? aren't they only used when connecting to the network?

[Ivansete-status]

is there any value in updating bootstrap nodes on runtime? aren't they only used when connecting to the network?

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 .)

[richard-ramos]

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.

2. One thing we noticed from the go-ethereum implementation is that if you get disconnected for a while, the routing tables from discv5 remove all the nodes including the bootnodes. So, when that happens, we do add the bootnodes back to discv5. Perhaps this is something that does not happen in nwaku's discv5?

[arnetheduck]

dns discovery

are there nodes that do not require dns discovery in the bootstrap node list?

[Ivansete-status]

dns discovery

are there nodes that do not require dns discovery in the bootstrap node list?

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

	let dynamicBootstrapNodesRes = waku_dnsdisc.retrieveDynamicBootstrapNodes(
	confCopy.dnsDiscovery, confCopy.dnsDiscoveryUrl, confCopy.dnsDiscoveryNameServers
	)
	if dynamicBootstrapNodesRes.isErr():
	error "Retrieving dynamic bootstrap nodes failed",
	error = dynamicBootstrapNodesRes.error
	return err(
	"Retrieving dynamic bootstrap nodes failed: " & dynamicBootstrapNodesRes.error
	)
	let nodeRes = setupNode(confCopy, some(rng))
	if nodeRes.isErr():
	error "Failed setting up node", error = nodeRes.error
	return err("Failed setting up node: " & nodeRes.error)
	var waku = Waku(
	version: git_version,
	conf: confCopy,
	rng: rng,
	key: confCopy.nodekey.get(),
	node: nodeRes.get(),
	dynamicBootstrapNodes: dynamicBootstrapNodesRes.get(),

[arnetheduck]

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)

[gabrielmer]

My understanding is that it's not mandatory. If we don't have DNS discovery enabled retrieveDynamicBootstrapNodes will return ok 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

	debug "No method for retrieving dynamic bootstrap nodes specified."
	ok(newSeq[RemotePeerInfo]()) # Return an empty seq by default

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)

[Ivansete-status]

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

	discv5BootstrapNodes* {.
	desc:
	"Text-encoded ENR for bootstrap node. Used when connecting to the network. Argument may be repeated.",
	name: "discv5-bootstrap-node"
	.}: seq[string]

( cc @arnetheduck )

[Ivansete-status]

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)

@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

[SionoiS]

LGTM Thank you!