feat: Enable Hole Punching service #1766

diegomrsantos · 2023-05-29T12:43:07Z

Description:

Enable the Hole Punching Service.

alrevuelta

does a private node with this code get inbound connections out of the box as it is in the waku network?

alrevuelta · 2023-05-29T13:09:46Z

waku/v2/node/waku_node.nim

@@ -186,7 +197,8 @@ proc new*(T: type WakuNode,
    sendSignedPeerRecord = sendSignedPeerRecord,
    agentString = agentString,
    peerStoreCapacity = peerStoreCapacity,
-    services = @[Service(getAutonatService(rng))],
+    services = @[Service(hpservice)],


just to make sure i got it right: autonatservice is included indirectly in hpservice? so no need for services=[hpservice, autonatservice]?

wondering what would happen if i try:
services=[hpservice, autonatservice]

Yes, autonatservice is added at line 184. Regarding your second question, it'd create two autonatservice, I didn't test it tbh, but it shouldn't be done.

I see the hpservice as a wrapper around autonatservice, so I think we don't need to add it twice.

https://github.com/status-im/nim-libp2p/blob/74c402ed9db4652a455c00c8d1713b222e3ef3d5/libp2p/services/hpservice.nim#L110

alrevuelta · 2023-05-29T13:12:31Z

waku/common/utils/nat.nim

@@ -54,6 +54,9 @@ proc setupNat*(natConf, clientId: string,
        endpoint.udpPort = some(extUdpPort)

  else: # NatNone
+    if natConf == "none":


isnt this redundant? its the else case of if strategy != NatNone: so it will always be none?

Yes, but before it executed the code below:

if not natConf.startsWith("extip:"): return err("not a valid NAT mechanism: " & $natConf)

Shall we just return an error in this case (else: # NatNone)?

I have the impression that the next is always run:

return err("not a valid NAT mechanism: " & $natConf)

alrevuelta · 2023-05-29T13:12:51Z

waku/v2/node/waku_node.nim

@@ -42,6 +46,8 @@ import
  ./peer_manager,
  ./waku_switch

+export chronos


is this needed?

@Menduist do you have more info about this issue? Is there a PR related to that?

Can't remember, what happens without this line?

I can't remember the error now, but it is related to generic procs. I think adding the export is the only way to fix it. I believe in this case it is caused by the new proc.

Right, if you use a package inside a generic proc, you need to export it
Not a bug, just a design quirk of nim

alrevuelta · 2023-05-29T13:14:46Z

waku/v2/node/waku_node.nim

+  let relayClient = RelayClient.new()
+  let autoRelayService = AutoRelayService.new(1, relayClient, nil, rng)
+  let autonatService = getAutonatService(rng)
+  let hpservice = HPService.new(autonatService, autoRelayService)


wondering if we should have hole punching behind a flag or always on by default. specially now at the begining that its new. opinion? @jm-clius

Maybe a flag that is true by default

Ivansete-status · 2023-06-02T12:11:36Z

Thanks so much indeed for submitting this PR @diegomrsantos !
May kindly elaborate a bit more on its description please? For example, the next sections might help:

summary.
deeper details about what this PR addresses.
issue. If exists, a link to the issue that needs to be sorted out.

You'll need to set an appropriate PR's title.

And, in order to fix the conflicts I would make this in your working branch:

```
 git fetch origin
```
```
 git rebase origin/master
```
fix the possible conflicts :)

diegomrsantos · 2023-06-02T12:32:17Z

@Ivansete-status Sure, I'll do it. Sorry, should have created it as a draft.

Ivansete-status

It looks great indeed!
I've added a little nitpicky comment.

How could we test it?

Ivansete-status · 2023-09-13T08:46:19Z

waku/node/waku_switch.nim

      .withAutonat()

+    if relay != nil:


Little nitpick:

Suggested change

if relay != nil:

if not isNil(relay):

Ivansete-status · 2023-09-13T09:12:39Z

waku/v2/node/waku_node.nim

@@ -186,7 +197,8 @@ proc new*(T: type WakuNode,
    sendSignedPeerRecord = sendSignedPeerRecord,
    agentString = agentString,
    peerStoreCapacity = peerStoreCapacity,
-    services = @[Service(getAutonatService(rng))],
+    services = @[Service(hpservice)],


I see the hpservice as a wrapper around autonatservice, so I think we don't need to add it twice.

https://github.com/status-im/nim-libp2p/blob/74c402ed9db4652a455c00c8d1713b222e3ef3d5/libp2p/services/hpservice.nim#L110

diegomrsantos · 2023-09-19T13:55:23Z

@Ivansete-status @jm-clius I'm putting this work on hold for now. The Hole Punching Service requires that a private peer's relay address be advertised to peers interested in connecting to it. We are going to use the Rendezvous protocol for that, but the current nwaku version is just mounting the protocol. Advertising the peer's signed record, requesting other peers ones, and using it to discover new peers need to be implemented. It seems those things are already in place in go-waku. Please let me know when those have been implemented in nwaku and fully tested with go.

Ivansete-status · 2023-09-21T08:49:34Z

@Ivansete-status @jm-clius I'm putting this work on hold...

Thanks for the comment @diegomrsantos! Is there anything that we should do to help this feature progress?
Kind regards

diegomrsantos · 2023-09-21T16:59:52Z

@Ivansete-status If I get it right, circuit-relay addresses from private nodes aren't discoverable through discv5, because they are too big. For that reason, nwaku nodes need to use Rendezvous to advertise such addresses when they are private. Likewise, they also need to ask Rendezvous Points for such node's addresses.

Ivansete-status · 2023-09-22T08:33:12Z

Thanks for the answer @diegomrsantos!
Then, in order to make this Hole Punching service PR progress we first need to achieve the next items right?

nwaku should use Rendezvous to advertise "circuit-relay addresses" when they are private.
nwaku should ask Rendezvous Points for "circuit-relay addresses".

diegomrsantos · 2023-09-22T12:24:53Z

That's correct. Regarding the first point, nwaku could use Rendezvous to advertise its addresses all the time, not only when it's private. This would probably make the implementation easier, but I'm not sure about the interaction with the current discv5 usage. This can be decided by the team, the only requirement for Hole Punching is that circuit-relay addresses of private nodes are advertised, discoverable, and dialed by other nodes.

diegomrsantos · 2023-09-22T12:47:45Z

I have added the advertising part (in all cases, not only when the node is private) here f64d12b. It can be used as inspiration, I'm not sure I did it properly.

It requires some fixes that are being done here vacp2p/nim-libp2p#951.

alrevuelta reviewed May 29, 2023

View reviewed changes

diegomrsantos marked this pull request as draft June 2, 2023 12:29

diegomrsantos force-pushed the master branch from 73083f8 to 19fc616 Compare September 12, 2023 16:07

diegomrsantos added 2 commits September 12, 2023 18:08

Add HP service

5e84569

Add HP service

f02ca75

diegomrsantos force-pushed the master branch from 19fc616 to f02ca75 Compare September 12, 2023 16:08

Ivansete-status changed the title ~~Enable Hole Punching service~~ feat: Enable Hole Punching service Sep 13, 2023

Ivansete-status reviewed Sep 13, 2023

View reviewed changes

Use DiscoveryManager

f64d12b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable Hole Punching service #1766

feat: Enable Hole Punching service #1766

diegomrsantos commented May 29, 2023

alrevuelta left a comment

alrevuelta May 29, 2023

diegomrsantos May 29, 2023

Ivansete-status Sep 13, 2023

alrevuelta May 29, 2023

diegomrsantos May 31, 2023

Ivansete-status Jun 2, 2023

alrevuelta May 29, 2023

diegomrsantos May 31, 2023 •

edited

Loading

Menduist Jun 2, 2023

diegomrsantos Jun 2, 2023

Menduist Jun 2, 2023

alrevuelta May 29, 2023

Menduist Jun 2, 2023

Ivansete-status commented Jun 2, 2023 •

edited

Loading

diegomrsantos commented Jun 2, 2023

Ivansete-status left a comment

Ivansete-status Sep 13, 2023

Ivansete-status Sep 13, 2023

diegomrsantos commented Sep 19, 2023 •

edited

Loading

Ivansete-status commented Sep 21, 2023

diegomrsantos commented Sep 21, 2023

Ivansete-status commented Sep 22, 2023

diegomrsantos commented Sep 22, 2023

diegomrsantos commented Sep 22, 2023 •

edited

Loading

feat: Enable Hole Punching service #1766

Are you sure you want to change the base?

feat: Enable Hole Punching service #1766

Conversation

diegomrsantos commented May 29, 2023

alrevuelta 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomrsantos May 31, 2023 • edited Loading

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

Choose a reason for hiding this comment

Ivansete-status commented Jun 2, 2023 • edited Loading

diegomrsantos commented Jun 2, 2023

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

diegomrsantos commented Sep 19, 2023 • edited Loading

Ivansete-status commented Sep 21, 2023

diegomrsantos commented Sep 21, 2023

Ivansete-status commented Sep 22, 2023

diegomrsantos commented Sep 22, 2023

diegomrsantos commented Sep 22, 2023 • edited Loading

diegomrsantos May 31, 2023 •

edited

Loading

Ivansete-status commented Jun 2, 2023 •

edited

Loading

diegomrsantos commented Sep 19, 2023 •

edited

Loading

diegomrsantos commented Sep 22, 2023 •

edited

Loading