Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't hole punch if either peer is behind a Symmetric NAT #1046

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Feb 3, 2021

For #1039.

If either peer is behind a Symmetric NAT, there's no point in wasting a hole punch and bandwidth on Relay servers.

TODO

  • Unit Tests.

@aarshkshah1992
Copy link
Contributor Author

Based on an offline discussion with @willscott

The fact that we dial a peer if it has any public addresses at all before giving up on Hole Punching due to NAT types will take care of Full Cone NATs.

It will break for address restricted Cones. So, we need to look at how our current code performs in the wild (with some stats around failures rates) to decide if we need to remove the restriction this PR introduces.

@aarshkshah1992 aarshkshah1992 changed the title Event Driven Hole Punching Don't hole punch if either peer is behind a Symmetric NAT Feb 4, 2021
@godcong
Copy link
Contributor

godcong commented Feb 5, 2021

but after my experiment, some operators can make reverse connections through the opened ports.
I just don’t know how many operators support it now

@aarshkshah1992
Copy link
Contributor Author

@godcong Yeah, that's a FULL Cone NAT. This PR will dial public addresses before giving up because of the NAT type.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of nits, but overall looks good.

Comment on lines 46 to 48
UDPNATDeviceTypeKey = "UdpNATDeviceType"
// TCPNATDeviceTypeKey is the key with which we will persist a peer's TCP NAT Device Type to the peerstore.
TCPNATDeviceTypeKey = "TcpNATDeviceType"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funky capitalization in the strings; it's UDP not Udp and same for TCP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 15 to 17
UNKNOWN = 100;
CONE = 200;
SYMMETRIC = 300;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0,1,2...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


optional NATDeviceType tcpNATDeviceType = 10;
optional NATDeviceType udpNATDeviceType = 11;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few more comments.


for {
select {
case _, ok := <-sub.Out():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we look at the actual event? why are we ignoring it?

Copy link
Contributor

@vyzo vyzo Feb 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, where do we set the support meta-variables into the peerstore?
Shouldn't we be doing it here? Or is it handled elsewhere?

I think we need a comment if this is correct as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vyzo

  1. Our local vars are set in the peerstore in basic_host.go during initialisation of the Host & updated in the obs addr manager.

  2. Remote vars are set in the peerstore by Identify.

Both changes are included in this PR. I've added a comment.


return mes
}

func toPbNATDeviceTyp(typ network.NATDeviceType) pb.Identify_NATDeviceType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in method name; shouldn't this be Type?
Also, typ is a very funky variable name, better call it just t.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

vyzo
vyzo previously requested changes Feb 18, 2021
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the handler is a BUG.

if hs.peerSupportsHolePunching(hs.host.ID(), hs.host.Addrs()) {
hs.host.SetStreamHandler(protocol, hs.handleNewStream)
} else {
hs.host.RemoveStreamHandler(protocol)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, this is a BUG -- we lose the ability to dial back connect to a public node that has dialed us.

@vyzo vyzo dismissed their stale review February 18, 2021 08:46

oh wait, nvm we just direct dial back when we see the relayed connection without coordination. Sorry about that.

@aarshkshah1992 aarshkshah1992 changed the base branch from feat/simopen to feat/hole-punching-signalling February 19, 2021 09:47
@aarshkshah1992 aarshkshah1992 changed the base branch from feat/hole-punching-signalling to feat/simopen February 19, 2021 09:56
@aarshkshah1992 aarshkshah1992 changed the base branch from feat/simopen to feat/hole-punching-signalling February 19, 2021 10:05
Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vyzo

Rebased and addressed review.


for {
select {
case _, ok := <-sub.Out():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vyzo

  1. Our local vars are set in the peerstore in basic_host.go during initialisation of the Host & updated in the obs addr manager.

  2. Remote vars are set in the peerstore by Identify.

Both changes are included in this PR. I've added a comment.


return mes
}

func toPbNATDeviceTyp(typ network.NATDeviceType) pb.Identify_NATDeviceType {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -200,6 +207,21 @@ func (oas *ObservedAddrManager) filter(observedAddrs []*observedAddr) []ma.Multi
}
}

// For certain use cases such as hole punching, it's better to advertise even unactivated observed addresses rather than none at all
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vyzo This needs your attention. I discussed this with @Stebalien and he was okay with it.

@BigLep BigLep marked this pull request as draft October 24, 2021 04:42
@BigLep
Copy link
Contributor

BigLep commented Oct 24, 2021

@marten-seemann @vyzo @aarshkshah1992 : I'm putting this back to draft for now. Feel free to close if that's the better option.

@vyzo
Copy link
Contributor

vyzo commented Oct 24, 2021

I actually think we ahould close it, i personally find it dangerous.

@introspection3
Copy link

@aarshkshah1992 @BigLep @vyzo @marten-seemann
"Don't hole punch if either peer is behind a Symmetric NAT" is an evil proposal, as
Symmetric can connect Full Cone,IP restrict.
Symmetric can also connect Port restrict use
Use the birthday paradox

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants