Signalling for Hole Punching by aarshkshah1992 · Pull Request #1057 · libp2p/go-libp2p

aarshkshah1992 · 2021-02-19T04:55:02Z

@vyzo
Replaces #711 to make it easier to review.

Based on https://github.com/libp2p/specs/pull/173/files.

TODO

Tests
Deps.

vyzo · 2021-02-19T08:47:48Z

+	if cfg.EnableHolePunching && !cfg.Relay {
+		return nil, errors.New("cannot enable hole punching; relay is not enabled")
+	}
+


I think we should enable this by default; but maybe that's for later, after we've done phase 1 testing.

So this check gets in the way for testing with limited relays, as we will have the v1 relay disabled in this case.

vyzo · 2021-02-19T08:55:03Z

+const (
+	protocol         = "/libp2p/holepunch/1.0.0"
+	maxMsgSize       = 4 * 1024 // 4K
+	holePunchTimeout = 2 * time.Minute


this also needs to be dialed down me thinks.

Keeping this to 1 minute for now. Can always fix after the alpha testing.

aarshkshah1992

@vyzo Addressed feedback and the PR is now open against a Staging branch.

aarshkshah1992 · 2021-02-19T09:33:27Z

+const (
+	protocol         = "/libp2p/holepunch/1.0.0"
+	maxMsgSize       = 4 * 1024 // 4K
+	holePunchTimeout = 2 * time.Minute


Keeping this to 1 minute for now. Can always fix after the alpha testing.

…signalling

aarshkshah1992 · 2021-02-19T10:20:02Z

@Stebalien

If you can find the time, it would be great to have your review on this one and it's not a big PR.

aarshkshah1992 · 2021-02-19T10:20:28Z

@vyzo Looks like we are ready to go. Not merging though till I finish writing some solid unit tests.

vyzo

An important detail re: the implementation of QUIC hole punching with the use of the SimultaneousConnect option.

I am working on that, but the code here will need to change.

vyzo · 2021-02-19T10:55:53Z

+}
+
+func (hs *HolePunchService) holePunchConnectWithBackoff(pi peer.AddrInfo) {
+	forceDirectConnCtx := network.WithForceDirectDial(hs.ctx, "hole-punching")


we also need WithSimultaneousConnect

aarshkshah1992 · 2021-02-19T11:00:28Z

@vyzo I'll block on this PR till you have the QUIC changes merged and we can then revisit this.

vyzo · 2021-02-19T11:03:58Z

they don't need to merge, just have an open pr for testing, ETA shortly.

vyzo · 2021-02-20T12:15:44Z

@aarshkshah1992 we are ready with the QUIC pr; let's test to see whether it works, and then we can merge that.

marten-seemann · 2021-02-22T11:15:00Z

+
+// Close closes the Hole Punch Service.
+func (hs *HolePunchService) Close() error {
+	hs.closeSync.Do(func() {


The cancel func can be called multiple times. I believe that allows you to get rid of this Once here.

vyzo · 2021-02-22T15:25:31Z

+	msg := new(pb.HolePunch)
+	msg.Type = pb.HolePunch_CONNECT.Enum()
+	msg.ObsAddrs = addrsToBytes(hs.ids.OwnObservedAddrs())
+	if err := w.WriteMsg(msg); err != nil {
+		s.Reset()
+		log.Errorf("failed to send hole punch CONNECT, err: %s", err)
+		return
+	}
+	tstart := time.Now()


the start of the RTT measurement has to move before writing the message I think.

vyzo · 2021-02-22T15:26:09Z

+	if err := rd.ReadMsg(msg); err != nil {
+		s.Reset()
+		log.Errorf("failed to read connect message from remote peer, err: %s", err)
+		return
+	}
+	if msg.GetType() != pb.HolePunch_CONNECT {
+		s.Reset()
+		log.Debugf("expectd HolePunch_CONNECT message, got %s", msg.GetType())
+		return
+	}
+	obsRemote := addrsFromBytes(msg.ObsAddrs)
+	rtt := time.Since(tstart)


and the end of the RTT measurement has to move right after reading the message.

marten-seemann · 2021-07-05T23:06:11Z

Note: we'll have to update the QUIC Hole Punching coordination logic if / when libp2p/go-libp2p-quic-transport#210 is merged.

mxinden · 2021-08-12T10:23:02Z

Question on retry procedure

I wonder whether I am understanding the retry procedure implemented in this pull request correctly. Is the following a good summary?

Both peers exchange the CONNECT and SYNC messages as described in relay/DCUtR: Add Direct Connection Upgrade through Relay protocol specs#173.
Both peers, A on receiving the SYNC, B on RTT/2, try to connect to the other.
On failure a peer retries to connect up to 5 times with a wait of 2 * time.Second between each attempt.

In case the above is correct, I wonder why the two peers do not go through the CONNECT and SYNC procedure for each retry. Doing an additional CONNECT and SYNC for each retry would prevent a flawed RTT measurement on the first attempt to badly influence all following retry attempts.

@vyzo @aarshkshah1992 can you help me out here?

vyzo · 2021-08-12T11:28:02Z

I think it might make sense to implement it the way you suggest, with another sync round to compute a the rtt anew.

…

On Thu, Aug 12, 2021, 13:23 Max Inden ***@***.***> wrote: *Question on retry procedure* I wonder whether I am understanding the retry procedure implemented in this pull request correctly. Is the following a good summary? 1. Both peers exchange the CONNECT and SYNC messages as described in libp2p/specs#173 <libp2p/specs#173>. 2. Both peers, A on receiving the SYNC, B on RTT/2, try to connect to the other. 3. On failure a peer retries to connect up to 5 times with a wait of 2 * time.Second between each attempt. In case the above is correct, I wonder why the two peers do not go through the CONNECT and SYNC procedure for each retry. Doing an additional CONNECT and SYNC for each retry would prevent a flawed RTT measurement on the first attempt to badly influence all following retry attempts. @vyzo <https://github.com/vyzo> @aarshkshah1992 <https://github.com/aarshkshah1992> can you help me out here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1057 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SUXPCN43KHEIRTWXV3T4OOJFANCNFSM4X3WENDQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

marten-seemann · 2021-09-08T16:51:35Z

Closing, since #1168 has been merged (into the feature branch at least).

hole punching protocol

3296c58

aarshkshah1992 requested a review from vyzo February 19, 2021 04:55

This was referenced Feb 19, 2021

Hole Punching Protocol #711

Closed

Project Flare(decentralised Hole Punching) Phase1 Meta Issue #1039

Closed

aarshkshah1992 commented Feb 19, 2021

View reviewed changes

Comment thread p2p/protocol/holepunch/coordination.go

vyzo reviewed Feb 19, 2021

View reviewed changes

address review

b85f227

aarshkshah1992 commented Feb 19, 2021

View reviewed changes

aarshkshah1992 changed the base branch from master to staging/hole-punching-phase1 February 19, 2021 09:46

vyzo reviewed Feb 19, 2021

View reviewed changes

Comment thread p2p/protocol/holepunch/coordination.go Outdated

Comment thread p2p/protocol/holepunch/coordination.go Outdated

Comment thread p2p/protocol/holepunch/pb/holepunch.proto Outdated

aarshkshah1992 added 2 commits February 19, 2021 15:39

Merge remote-tracking branch 'origin/master' into feat/hole-punching-…

fe9288c

…signalling

fix nits

08178ad

aarshkshah1992 requested a review from Stebalien February 19, 2021 10:19

vyzo reviewed Feb 19, 2021

View reviewed changes

init correctly

d1b3f07

vyzo mentioned this pull request Feb 20, 2021

hole punching support libp2p/go-libp2p-quic-transport#194

Merged

aarshkshah1992 added 5 commits February 22, 2021 09:48

withSimulConnect

326afcc

update swarm

30c5b0b

log direct conns even on error

2d3c67c

log connection ID

8325659

correct index

aedab04

marten-seemann reviewed Feb 22, 2021

View reviewed changes

Comment thread p2p/protocol/holepunch/coordination.go Outdated

vyzo reviewed Feb 22, 2021

View reviewed changes

Comment thread p2p/protocol/holepunch/coordination.go Outdated

Comment thread p2p/protocol/holepunch/coordination.go Outdated

change dial timeout

b4dad3e

vyzo reviewed Feb 22, 2021

View reviewed changes

vyzo and others added 8 commits February 23, 2021 09:43

remove check for relay configuration when enabling hole punching

c80a8ec

deduplicate hole punching attempts

93ab8e6

tests WIP

072db05

move rtt measurement to the right spots, add some logging

6ebc4f1

testing phase 1

cc02626

merge

f24dc44

tests complete

6f0f02d

use transient connections in identify streams

630f73b

vyzo mentioned this pull request Feb 25, 2021

use transient connections in identify streams #1061

Merged

vyzo added 6 commits February 25, 2021 10:55

clean up error handling

d3ae1ac

disable broken test

81ffb18

support options for holepunching service

e9f9c12

add tracing support for hole punching service

3c329b8

include both peer IDs in tracer events

fdcd9b1

fix log arguments

4fec3e0

This was referenced Apr 6, 2021

Project Flare aka. decentralised hole punching protocol/web3-dev-team#21

Open

NAT traversal tracking issue libp2p/specs#312

Open

mxinden mentioned this pull request May 25, 2021

NAT traversal libp2p/rust-libp2p#2052

Closed

14 tasks

marten-seemann mentioned this pull request Jun 30, 2021

Project Flare / Hole Punching tracking issue #1122

Closed

godcong mentioned this pull request Aug 12, 2021

The port is opened using the upnp protocol, but it is not used #1142

Closed

mxinden mentioned this pull request Aug 17, 2021

relay/DCUtR: Add Direct Connection Upgrade through Relay protocol libp2p/specs#173

Merged

marten-seemann mentioned this pull request Aug 21, 2021

signalling for hole punching #1168

Merged

marten-seemann closed this Sep 8, 2021

Conversation

aarshkshah1992 commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aarshkshah1992 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aarshkshah1992 commented Feb 19, 2021

Uh oh!

aarshkshah1992 commented Feb 19, 2021

Uh oh!

vyzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarshkshah1992 commented Feb 19, 2021

Uh oh!

vyzo commented Feb 19, 2021

Uh oh!

vyzo commented Feb 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marten-seemann commented Jul 5, 2021

Uh oh!

mxinden commented Aug 12, 2021

Uh oh!

vyzo commented Aug 12, 2021 via email

Uh oh!

marten-seemann commented Sep 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

aarshkshah1992 commented Feb 19, 2021 •

edited

Loading

aarshkshah1992 left a comment •

edited

Loading