chore: discv5 re-org setup #1815

SionoiS · 2023-06-21T14:56:39Z

Description

Setting the code for the move of discv5 out of node. Note that some changes might look weird without context, feel free to ask.

Changes

keys, netconfig and ENR are created when the app initialize
modified node builder

tracking #1812

Ivansete-status

Thanks for the PR! However, I see that are a few points that I'd adapt a bit more, IMHO.

I hope that helps :)

Ivansete-status · 2023-06-22T08:32:52Z

apps/wakunode2/app.nim

@@ -94,8 +97,137 @@ func version*(app: App): string =

 ## Initialisation

+proc networkConfiguration(conf: WakuNodeConf): NetConfigResult =


I think is better to have these two WakuNodeConf-procs in https://github.com/waku-org/nwaku/blob/master/apps/wakunode2/config.nim

Not entirely sure I agree. These procs are concerned with translating the already-initialised config to interpreted config types that are specific to the app. In contrast, config.nim is only interested in parsing and initialising external config and is already complex enough as is without more app constructs. I'd be happy if we move these two functions to a new network_config.nim module to keep app.nim cleaner, but happy to go with majority opinion here. :)

Lets rename config.nim to ext-config.nim then? or user-config.nim

Actually, the best place I think is the next file, where the NetConfig is declared:
https://github.com/waku-org/nwaku/blob/master/waku/v2/node/config.nim

Ivansete-status · 2023-06-22T08:52:43Z

apps/wakunode2/app.nim

    rng: ref HmacDrbgContext
+    nodeKey: crypto.PrivateKey


IMO, the App shouldn't know anything about nodekey. I believe we need to keep the App as simple and generic as possible. The only one interested in the nodekey is the node instance, which is already part of the App.

The same applies to netConf. Both the nodeKey and netConf are derived/created from the information in conf. And I think it should be the WakuNodeConf (config.nim module) the one in charge of translating the information from the outer world (the user params) into the wakunode space.

I agree with the principle but both discv5 and node require keys for setup that why it's higher in the call chain.

Would calling it appkey helps :P

We indeed have an appKey if it's used to setup both the discovery and the node. The appKey is then used to initialise the (logically distinct) keys for the node and discv5 with the same value.

Ok understood :)

However, given that discv5 should act as a separate service, a good place to start it would be in the next proc:

nwaku/apps/wakunode2/app.nim

Line 763 in a44d4bf

proc setupMonitoringAndExternalInterfaces*(app: var App): AppResult[void] =

The key could be generated there from the 'key' info in the config.

Ivansete-status · 2023-06-22T08:57:22Z

apps/wakunode2/app.nim

@@ -467,7 +531,7 @@ proc initNode(conf: WakuNodeConf,

 proc setupWakuNode*(app: var App): AppResult[void] =
  ## Waku node
-  let initNodeRes = initNode(app.conf, app.rng, app.peerStore, app.dynamicBootstrapNodes)
+  let initNodeRes = initNode(app.conf, app.netConf, app.rng, app.nodeKey, app.record, app.peerStore, app.dynamicBootstrapNodes)


This comment doesn't add nothing new from what I already mentioned :)
However, I believe we could generate the netConf, nodeKey and record inside the initNode with the information provided by the app.conf param.

I can't do that because discv5 will not be in initNode but in setupWakuNode <- could be renamed to Waku app?

BTW the term app and node are not super defined IMO. The binary is a Waku node but internally it's called app and has a node inside it's confusing.

Okay got it! What if we try to do something similar that is done with the rest server?
I mean, start the discv5 service in

nwaku/apps/wakunode2/app.nim

Line 772 in a44d4bf

let startRestServerRes = startRestServer(app, app.conf.restAddress, Port(app.conf.restPort + app.conf.portsShift), app.conf)

I don't have strong opinion about renaming the setupWakuNode. Both cases can be correct :)

SionoiS · 2023-06-22T11:38:52Z

For more context check out d19d192

jm-clius

Generally happy with this direction. Agree that we can start clarifying terms between "app" and "node" (e.g. appKey). I know this is WIP to prepare for next step in refactoring, so feel free to address suggestions in this or next PR.

jm-clius · 2023-06-22T13:27:43Z

apps/wakunode2/app.nim

    rng: ref HmacDrbgContext
+    nodeKey: crypto.PrivateKey


We indeed have an appKey if it's used to setup both the discovery and the node. The appKey is then used to initialise the (logically distinct) keys for the node and discv5 with the same value.

jm-clius · 2023-06-22T13:38:16Z

apps/wakunode2/app.nim

@@ -94,8 +97,137 @@ func version*(app: App): string =

 ## Initialisation

+proc networkConfiguration(conf: WakuNodeConf): NetConfigResult =


Not entirely sure I agree. These procs are concerned with translating the already-initialised config to interpreted config types that are specific to the app. In contrast, config.nim is only interested in parsing and initialising external config and is already complex enough as is without more app constructs. I'd be happy if we move these two functions to a new network_config.nim module to keep app.nim cleaner, but happy to go with majority opinion here. :)

jm-clius · 2023-06-22T13:42:40Z

apps/wakunode2/app.nim

  var node: WakuNode

  let pStorage = if peerStore.isNone(): nil
                 else: peerStore.get()

-  let rng = crypto.newRng()


Mmm. Wait, was this rng generation redundant before as it's also an argument for this function?

Yes that's what I concluded.

SionoiS · 2023-06-22T15:09:57Z

~~Github is not showing latest commit for this branch here, weird...~~

~~https://github.com/waku-org/nwaku/tree/chore-discv5-reorg 4 commits but only 3 here?~~

Ivansete-status

Hey! I approve now so that I don't block the upcoming PR. I think it will get much clearer by then :)
Thanks for this!

SionoiS marked this pull request as ready for review June 21, 2023 17:10

SionoiS requested review from alrevuelta, jm-clius and Ivansete-status June 21, 2023 17:11

Ivansete-status requested changes Jun 22, 2023

View reviewed changes

jm-clius approved these changes Jun 22, 2023

View reviewed changes

SionoiS added 4 commits June 22, 2023 11:19

Reorg Setup

43d5fcd

Nicer App Init

6533dd5

Moved some code

16e6275

Renamed config files

81d2d56

SionoiS force-pushed the chore-discv5-reorg branch from 15b521e to 81d2d56 Compare June 22, 2023 15:21

SionoiS requested a review from Ivansete-status June 22, 2023 15:27

Ivansete-status approved these changes Jun 22, 2023

View reviewed changes

SionoiS merged commit 44f9d8d into master Jun 22, 2023

SionoiS deleted the chore-discv5-reorg branch June 22, 2023 20:58

SionoiS mentioned this pull request Jun 28, 2023

chore: Discv5 reorganisation #1812

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: discv5 re-org setup #1815

chore: discv5 re-org setup #1815

SionoiS commented Jun 21, 2023 •

edited

Loading

Ivansete-status left a comment

Ivansete-status Jun 22, 2023

SionoiS Jun 22, 2023

jm-clius Jun 22, 2023

SionoiS Jun 22, 2023 •

edited

Loading

Ivansete-status Jun 22, 2023

Ivansete-status Jun 22, 2023

SionoiS Jun 22, 2023 •

edited

Loading

jm-clius Jun 22, 2023

Ivansete-status Jun 22, 2023 •

edited

Loading

Ivansete-status Jun 22, 2023

SionoiS Jun 22, 2023 •

edited

Loading

Ivansete-status Jun 22, 2023 •

edited

Loading

SionoiS commented Jun 22, 2023

jm-clius left a comment

jm-clius Jun 22, 2023

jm-clius Jun 22, 2023

jm-clius Jun 22, 2023

SionoiS Jun 22, 2023

SionoiS commented Jun 22, 2023 •

edited

Loading

Ivansete-status left a comment

		@@ -94,8 +97,137 @@ func version*(app: App): string =

		## Initialisation

		proc networkConfiguration(conf: WakuNodeConf): NetConfigResult =

chore: discv5 re-org setup #1815

chore: discv5 re-org setup #1815

Conversation

SionoiS commented Jun 21, 2023 • edited Loading

Description

Changes

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivansete-status Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Ivansete-status Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

SionoiS commented Jun 22, 2023

jm-clius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS commented Jun 22, 2023 • edited Loading

Ivansete-status left a comment

Choose a reason for hiding this comment

SionoiS commented Jun 21, 2023 •

edited

Loading

SionoiS Jun 22, 2023 •

edited

Loading

SionoiS Jun 22, 2023 •

edited

Loading

Ivansete-status Jun 22, 2023 •

edited

Loading

SionoiS Jun 22, 2023 •

edited

Loading

Ivansete-status Jun 22, 2023 •

edited

Loading

SionoiS commented Jun 22, 2023 •

edited

Loading