reject hole punching attempts when we don't have any public addresses

[marten-seemann]

If we don't have any public addresses, the hole punch attempt is bound to fail - there's no address that can be used to dial us.

Question: Any idea how to fix the tests? IDService.OwnObservedAddrs() always returns an empty set of addresses, as we're not really running the ID protocol (and we're only listening on localhost anyway). The obvious solution would be to mock the IDService, but it's not an interface.

[vyzo]

let's make this a named error, we use it in a few places.

[vyzo]

It doesn't have to be public.

[marten-seemann]

Let's keep them different for now. This will make debugging easier.

[vyzo]

why fmt.Errorf? There is no interpolation here.

[vyzo]

also, named error as well would be nice.

[marten-seemann]

Converted to errors.New.

[vyzo]

LGTM, but can we address the (minor) comments?

[marten-seemann]

I'm working on an update to this PR: We should wait until we actually have a public address, before we:

1. initiate hole punch attempts (direct connection attempts are ok)
2. install the hole punch protocol

The problem I'm running into is mostly how to test this.

[marten-seemann]

@vyzo I made the changes to the coordination protocol: We now only start the hole punching service once we have a public address (without a publicly dialable address, there’s no way a hole punch can succeed: the peer wouldn’t even know who to dial). I think that the code is correct as it is right now.

Even after a long time of playing around with this code, I can’t find a way to make the tests work. The problem is that

1. we only start the hole punch service once we have a public address
2. we try a direct dial if a node has a public address (note that a direct dial always succeeds in the setup, since we don't have an actual NAT implementation)

Any idea how to proceed here?

[vyzo]

can we clobber manet's private address ranges so that `IsPublicAddr` returns true?

…

On Tue, Oct 26, 2021, 16:24 Marten Seemann ***@***.***> wrote: @vyzo <https://github.com/vyzo> I made the changes to the coordination protocol: We now only start the hole punching service once we have a public address (without a publicly dialable address, there’s no way a hole punch can succeed: the peer wouldn’t even know who to dial). I *think* that the code is correct as it is right now. Even after a long time of playing around with this code, I can’t find a way to make the tests work. The problem is that 1. we only start the hole punch service once we have a public address 2. we try a direct dial if a node has a public address (note that a direct dial *always* succeeds in the setup, since we don't have an actual NAT implementation) Any idea how to proceed here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1214 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI4SR7AQD6UFC57LJOTZLUI2TZLANCNFSM5FC4L7KA> .

[marten-seemann]

can we clobber manet's private address ranges so that IsPublicAddr returns true?

We can, and that would allow us to start the holepunch service. But that would also mean that the check if any of the peer's addresses are public in

go-libp2p/p2p/protocol/holepunch/coordination.go

Line 202 in ddf96ce

if manet.IsPublicAddr(a) && !isRelayAddress(a) {

would succeed, and we wouldn't holepunch, but do a direct connect instead.

[vyzo]

it is even worse: when we enable autotelay, we install an address factory that will rewrite Addrs and end up nevwr having a public address.
We need access to observed addresses.

[marten-seemann]

I updated this PR, using the observed addresses to determine if we have a public address. Also, we can't listen for the EvtLocalAddressesUpdated event, since it's not emitted when identify gives us a new public address. Unfortunately, we don't have any event for this, so we'll have to run the check repeatedly on a timer.

[vyzo]

looks good, can we also fix the two small nits?