signalling for hole punching #1168

marten-seemann · 2021-08-21T16:59:23Z

This is a copy of #1057, with a cleaned up commit history. I intend to continue working on this PR, and eventually close #1057.
Closes #1057.

Note: This PR targets the hole-punching branch, which is where we'll assemble all hole-punching related changes before merging into master.

p2p/protocol/holepunch/coordination.go

marten-seemann · 2021-08-31T10:59:29Z

Unfortunately, the changes in libp2p/go-libp2p-autonat#109 were not enough to prevent race conditions here, as for setting up the relay connection, we need to mark all addresses as public, and then for hole punching we need to mark these addresses as private again, while the host is running. This is inherently racy.

Stebalien

Mostly coding things, but there are some design questions around when to initiate a hole-punch and when to bail.

go.mod

Stebalien · 2021-09-03T09:59:01Z

p2p/host/basic/basic_host.go

+	if !forceDirect {
+		if h.Network().Connectedness(pi.ID) == network.Connected {
+			return nil
+		}
 	}


It looks like we need some way to check for connections with specific properties, right? Or do we just want to always dial in this case anyways.

Stebalien · 2021-09-03T10:04:08Z

p2p/host/basic/basic_host.go

@@ -208,6 +215,13 @@ func NewHost(ctx context.Context, n network.Network, opts *HostOpts) (*BasicHost
 		return nil, fmt.Errorf("failed to create Identify service: %s", err)
 	}

+	if opts.EnableHolePunching {


Future: Dependency injection would make this a lot less invasive.

p2p/protocol/holepunch/tracer.go

p2p/protocol/holepunch/pb/holepunch.proto

p2p/protocol/holepunch/coordination.go

Stebalien · 2021-09-03T14:25:32Z

p2p/protocol/holepunch/coordination.go

+
+	// Hole punch if it's an inbound proxy connection.
+	// If we already have a direct connection with the remote peer, this will be a no-op.
+	if dir == network.DirInbound && isRelayAddress(v.RemoteMultiaddr()) {


We should probably bail early if we're known to be dialable. This would largely mitigate a lot of DoS concerns.

Also, is there some discussion on picking a side to initiate? On one hand, having the receiver initiate gives them a chance to try a "normal" dial. On the other hand, it means the receiver may do a lot of work on behalf of a potentially malicious party.

Also, is there some discussion on picking a side to initiate? On one hand, having the receiver initiate gives them a chance to try a "normal" dial. On the other hand, it means the receiver may do a lot of work on behalf of a potentially malicious party.

The node behind the NAT is the one dialing. The reason for that is that there's a good change that the peer is not behind a NAT, and we can just dial him directly. Only if the peer is not dialable, we have to fall back to hole punching.

We should probably bail early if we're known to be dialable. This would largely mitigate a lot of DoS concerns.

Then the remote address wouldn't be a relay address, would it?

To clarify, we're looking at the remote address being a relay address, AND the connection being an inbound connection.
If we know to be publicly dialable, we shouldn't be behind a relay in the first place.

The node behind the NAT is the one dialing. The reason for that is that there's a good change that the peer is not behind a NAT, and we can just dial him directly. Only if the peer is not dialable, we have to fall back to hole punching.

Makes sense.

If we know to be publicly dialable, we shouldn't be behind a relay in the first place.

That doesn't stop someone from making a relayed connection. We'll accept those regardless (there are some valid use-cases for this, e.g., no common transport).

To know if we're publicly dialable, we need to check our "nat" status (subscribe to the reachability event).

That makes sense. Let's do that in a separate PR (this PR is targeting a feature-branch anyway).

p2p/protocol/holepunch/coordination.go

Stebalien · 2021-09-08T09:37:47Z

p2p/protocol/holepunch/coordination.go

+// Close closes the Hole Punch Service.
+func (hs *Service) Close() error {
+	hs.closeMx.Lock()
+	hs.closed = true


It would be nice to make this idempotent (not critical, but it's something I generally expect from close methods). I.e., if already closed, skip to the Wait call.

Really, everything here should already be idempotent, but the tracer's close method may not be.

This is idempotent, Wait will return immediately if the counter is already 0. Otherwise, both (concurrent) calls to Close will wait for the Wait to return, which technically is the more correct way to do this, if you want to interpret "Close returned" as "this service is now properly closed".

I agree. My point is that the tracer's close function could get called multiple times, and it may not be expecting that.

Of course, it should just "deal" with it, so we're probably fine.

Stebalien · 2021-09-08T09:46:39Z

p2p/protocol/holepunch/coordination.go

+	hpCtx := network.WithUseTransient(hs.ctx, "hole-punch")
+	sCtx := network.WithNoDial(hpCtx, "hole-punch")


super-nit: store this context and reuse it.

marten-seemann commented Aug 21, 2021

View reviewed changes

p2p/protocol/holepunch/coordination.go Outdated Show resolved Hide resolved

marten-seemann commented Aug 21, 2021

View reviewed changes

p2p/protocol/holepunch/coordination.go Show resolved Hide resolved

marten-seemann commented Aug 21, 2021

View reviewed changes

p2p/protocol/holepunch/coordination.go Outdated Show resolved Hide resolved

marten-seemann force-pushed the hole-punching-signaling branch from 75ac61e to 5cf36a6 Compare August 24, 2021 14:43

mxinden reviewed Aug 24, 2021

View reviewed changes

p2p/protocol/holepunch/coordination.go Outdated Show resolved Hide resolved

BigLep mentioned this pull request Aug 24, 2021

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

Closed

23 tasks

marten-seemann force-pushed the hole-punching-signaling branch 3 times, most recently from 7cc8b06 to 667bf78 Compare August 29, 2021 11:01

marten-seemann marked this pull request as ready for review August 29, 2021 11:04

marten-seemann requested review from Stebalien, vyzo and aarshkshah1992 August 29, 2021 11:04

marten-seemann mentioned this pull request Aug 29, 2021

remove context from constructor, implement a proper Close method libp2p/go-libp2p-autonat#109

Merged

marten-seemann force-pushed the hole-punching-signaling branch 2 times, most recently from 25c5b0a to 997a2a0 Compare August 30, 2021 09:51

marten-seemann changed the base branch from master to hole-punching August 30, 2021 09:51

marten-seemann force-pushed the hole-punching branch from 19c507a to b0ee082 Compare August 30, 2021 09:53

marten-seemann force-pushed the hole-punching-signaling branch from 997a2a0 to cac4a42 Compare August 30, 2021 09:53

marten-seemann force-pushed the hole-punching branch from b0ee082 to 2d07a0d Compare August 30, 2021 09:57

marten-seemann force-pushed the hole-punching-signaling branch 4 times, most recently from ebe2411 to 69573b9 Compare August 31, 2021 10:25

Stebalien reviewed Sep 3, 2021

View reviewed changes

marten-seemann force-pushed the hole-punching-signaling branch 2 times, most recently from f4e9663 to ce0878c Compare September 3, 2021 16:24

aarshkshah1992 and others added 2 commits September 4, 2021 12:17

hole punching protocol

3e4c436

implement the new retry logic (opening a new stream per hole punch)

0cea0ab

marten-seemann added 6 commits September 4, 2021 12:17

rename the holepunch.HolePunchService to holepunch.Service

8f7f08e

remove stream handler when closing

a19072e

remove misleading comment from protobuf, add link to spec

4903d38

remove unnecessary netNotifiee.HolePunchService

49bc8ef

simplify receiving from the timer channel

4023d84

don't defer the closing of the hole punching stream

18045c9

marten-seemann force-pushed the hole-punching-signaling branch from f7d9920 to 18045c9 Compare September 4, 2021 11:18

marten-seemann added 5 commits September 4, 2021 13:05

only accept hole punch streams from relayed peers

43f5080

refactor error logging

380a2c4

fix incorrect counting of incoming hole punch attempts

df6924a

rename HolePunch to DirectConnect

1be1555

fix usage of the wait group to close the hole punch service

93eebc0

marten-seemann requested a review from Stebalien September 4, 2021 16:28

Stebalien reviewed Sep 7, 2021

View reviewed changes

p2p/protocol/holepunch/coordination.go Outdated Show resolved Hide resolved

p2p/protocol/holepunch/coordination.go Outdated Show resolved Hide resolved

marten-seemann added 3 commits September 7, 2021 17:23

use a gc to the garbage collect the holepunch tracer

03d3a5d

check directionality of connection when accepting the hole punch stream

e1edbca

add comment why we only call Connect once

2605890

marten-seemann requested a review from Stebalien September 7, 2021 17:49

marten-seemann force-pushed the hole-punching-signaling branch 2 times, most recently from 54861f8 to 0ed5f82 Compare September 7, 2021 18:10

move deduplication logic to the holepunch service

84900aa

marten-seemann force-pushed the hole-punching-signaling branch from 0ed5f82 to 84900aa Compare September 7, 2021 18:16

Stebalien approved these changes Sep 8, 2021

View reviewed changes

marten-seemann merged commit 8206fcd into hole-punching Sep 8, 2021

marten-seemann mentioned this pull request Sep 8, 2021

Signalling for Hole Punching #1057

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signalling for hole punching #1168

signalling for hole punching #1168

marten-seemann commented Aug 21, 2021 •

edited

Loading

marten-seemann commented Aug 31, 2021

Stebalien left a comment

Stebalien Sep 3, 2021

Stebalien Sep 3, 2021

Stebalien Sep 3, 2021

marten-seemann Sep 4, 2021

marten-seemann Sep 4, 2021

marten-seemann Sep 4, 2021

Stebalien Sep 7, 2021

marten-seemann Sep 7, 2021

Stebalien Sep 8, 2021

marten-seemann Sep 8, 2021 •

edited

Loading

Stebalien Sep 8, 2021

Stebalien Sep 8, 2021

		hpCtx := network.WithUseTransient(hs.ctx, "hole-punch")
		sCtx := network.WithNoDial(hpCtx, "hole-punch")

signalling for hole punching #1168

signalling for hole punching #1168

Conversation

marten-seemann commented Aug 21, 2021 • edited Loading

marten-seemann commented Aug 31, 2021

Stebalien 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann commented Aug 21, 2021 •

edited

Loading

marten-seemann Sep 8, 2021 •

edited

Loading