feat(discv5): advertise custom multiaddresses

[rymnc]

Should fix #1475, as a follow up to #1509. Makes discv5 broadcast custom-set multiaddrs

[LNSD]

@rymnc I do not agree with moving discv5 to the waku_node module.

I think that all the discovery mechanisms should be managed and orchestrated by the peer manager. As a consequence, these mechanisms should be:

• Either the wakunode2 app should register the event handlers to the discovery services and handle them by the peer manager instance.
• Or dependency of the peer manager and passed into the peer manager constructor.

[rymnc]

@rymnc I do not agree with moving discv5 to the waku_node module.

I think that all the discovery mechanisms should be managed and orchestrated by the peer manager. As a consequence, these mechanisms should be:

• Either the wakunode2 app should register the event handlers to the discovery services and handle them by the peer manager instance.
• Or dependency of the peer manager and passed into the peer manager constructor.

I am not sure if the PeerManager was intended to have this functionality, i.e discovery. It is currently used to manage dialing, adding and removing peers. But yes, I agree with your point.

Maybe @alrevuelta can weigh in here, was this something you intended for the PeerManager to do?

[LNSD]

I am not sure if the PeerManager was intended to have this functionality, i.e discovery. It is currently used to manage dialing, adding and removing peers.

Note the following from my comment:

Either the wakunode2 app should register the event handlers to the discovery services and handle them by the peer manager instance.

This would be something like:

discoveryMechanism.onPeerDiscovered do (peer: PeerId):
  peerManager.addPeer(peer)

[alrevuelta]

thanks! some comments

[status-im-auto]

Jenkins Builds

Click to see older builds (5)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	185a10a	#1	2023-01-26 23:03:12	~15 min	macos	📄log
❌	bffcca7	#2	2023-01-27 22:54:52	~7 min	macos	📄log
✔️	b060cf2	#3	2023-01-30 23:02:36	~15 min	macos	📦bin
✔️	f69810d	#4	2023-01-31 23:01:03	~13 min	macos	📦bin
✔️	9c0e294	#5	2023-02-01 23:01:23	~14 min	macos	📦bin

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	29a3f11	#6	2023-02-02 15:24:21	~13 min	macos	📄log
✔️	29a3f11	#7	2023-02-02 15:50:49	~13 min	macos	📦bin
✔️	032b3a3	#8	2023-02-06 15:17:20	~29 min	macos	📦bin

[jm-clius]

Thanks, this is more or less the direction I had in mind too. I've added some comments re keeping the config vs initialisation a bit cleaner and consolidating the two different ENRs into one.

[LNSD]

As I said in my initial comment, I see a bad idea in moving the discovery mechanisms inside the waku_node module.

Some comments second this idea: moving the code inside the node module means you have to thread down the configuration. But it also implies that the WakuNode type has more than one responsibility (besides of orchestrating the different protocols and configuring the switch it also generates the local ENR, configures the discv5 service and holds the discv5 state). So... you cannot test the WakuNode without the discovery instantiated.

In summary, this couples the node code with the discovery code.

---

If everybody agrees, I won't block the PR. But I think we should go in the opposite direction: splitting the content of the big modules instead of merging them into big modules.

[rymnc]

Moving this PR to draft to decouple disc and WakuNode initialization.

[jm-clius]

Agree with @LNSD here. The issue is currently our configuration and initialisation is very convoluted.

• WakuDiscv5 requires some addresses only available in WakuNode initialisation for its own initialisation
• WakuNode requires an initialised WakuDiscv5 to be constructed with a consolidated ENR

Perhaps an increment to make it a bit better is extracting those parts of initialisation to do with constructing addresses and making sure that that's available to both WakuDiscv5 and WakuNode during initialisation? That way WakuDiscv5 initialisation can be decoupled and performed before WakuNode initialisation.

For this, the constructor arguments for WakuDiscv5 may also have to change so that it can be initialised from config.

I know from previously looking at this that there are some intricacies and gotchas, so I'm okay with incrementing here in reasonable steps to something cleaner and decoupled.

[alrevuelta]

thanks for addressing the comments, resolved them all.

Maybe @alrevuelta can weigh in here, was this something you intended for the PeerManager to do?

missed this comment. I think we can keep the peer manager and discovery separated. Peer manager should be agnostic on how the peers are discovered/added to the peer manager.

[rymnc]

@LNSD, I've decoupled the discv5 and waku_node setup. Added a deprecated flag to the original Wakunode.new() proc in favor of the WakuNodeAddrMeta variant (because there are a lot of usages of Wakunode.new()).

[LNSD]

Please, check my comments :)

[LNSD]

LGTM ✅

[jm-clius]

Thanks! I think this is a great step forward to also clean up the confusing address configuration and initialisation. It may be worth adding a unit test suite for other aspects of NetConfig (e.g. populating sensible announcedAddresses) in future PRs.