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

Emit event for User's NAT Type i.e. Hard NAT or Easy NAT #1042

Merged
merged 14 commits into from
Feb 19, 2021

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Jan 27, 2021

For #1039.

We should emit an event informing the user of the user's NAT type for better user experience for hole punching/testing Hole punching in the wild.

TODO

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.

I am a little sceptical about the activated address heuristic; if we have one, then yes we are obviously behind an easy NAT.
But if we don't we might still be behind a penetrable NAT, especially with UDP.

@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Jan 27, 2021

@vyzo

We don't determine the NAT to be hard simply if we don't have an activated address. We determine the NAT to be hard when four different peers report four different observed addresses on outbound connections. I think this should be good enough to conclude that the NAT is bad, no ?

We can augment the code to report NAT types separately for UDP & non-UDP connections if that's what you mean.

@vyzo
Copy link
Contributor

vyzo commented Jan 27, 2021

Yeah, we should probably do that.

p2p/protocol/identify/obsaddr.go Show resolved Hide resolved
oas.emitUDPTypeUnlocked(allObserved)
}

func (oas *ObservedAddrManager) emitTCPNATTypeUnlocked(addrs []*observedAddr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this and the subsequent emitUDPNATTypeUnlocked are doing basically the same thing. can you factor out a common method that takes an additional parameter (ma.P_TCP vs ma.P_UDP)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done.

@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Feb 1, 2021

@willscott I've addressed both your comments. Have also tested this on my machine for both TCP & QUIC and it looks good.
Thanks !

Copy link
Contributor

@willscott willscott left a comment

Choose a reason for hiding this comment

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

one minor nit, and you need to get the dependencies merged and go.mod pulled up to versions before merging, but this seems ready.

//
// Please see the documentation on the enumerations for `network.NATDeviceType` for more details about these NAT Device types
// and how they relate to NAT traversal via Hole Punching.
func (oas *ObservedAddrManager) emitNATTypeUnlocked() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this method named Unlocked? maybe this is observeAllNATTypes and the next one is observeSpecificNATType? the naming of the two feels not quite representative of what they do.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah what's with the name? Is there a locked variant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have fixed this.

@aarshkshah1992
Copy link
Contributor Author

cc @vyzo

//
// Please see the documentation on the enumerations for `network.NATDeviceType` for more details about these NAT Device types
// and how they relate to NAT traversal via Hole Punching.
func (oas *ObservedAddrManager) emitNATTypeUnlocked() {
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah what's with the name? Is there a locked variant?


// returns true along with the new NAT device type if the NAT device type for the given protocol has changed.
// returns false otherwise.
func (oas *ObservedAddrManager) emitNATType(addrs []*observedAddr, protoCode int, transportProto event.NATTransportProtocol,
Copy link
Contributor

Choose a reason for hiding this comment

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

This could range from a little to very expensive.
How often do we call it?
Can we skip it altogether if we are not NATted?

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. What makes this expensive ?
  2. We call it every-time we have a new observed address but this function will return as soon as it sees any activated address.

The problem with skipping it altogether if we are not NATT'd is that AutoNAT dosen't take into account TCP vs UDP whereas we want to emit separate events for both.

Do you mean to say that we should emit this event for both TCP & UDP ONLY if reachability is private ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imagine a big public peer with thousands of connections, this could become problematic.

And yes, this event is only relevant if we have private reachability and can/should be skipped altogether if we are public.

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.

@aarshkshah1992
Copy link
Contributor Author

@vyzo

  1. Addressed your review.
  2. Rebased & updated deps.

Please can you take a look ?

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.

One bug and a bunch of stylistic issues...

Comment on lines 262 to 267
case evt, ok := <-subChan:
if !ok {
subChan = nil
}
ev := evt.(event.EvtLocalReachabilityChanged)
oas.reachability = ev.Reachability
Copy link
Contributor

Choose a reason for hiding this comment

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

this will panic when the channel gets closed, as evt will be nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops :/ Fixed.

Comment on lines 470 to 472
for k := range oas.addrs {
allObserved = append(allObserved, oas.addrs[k]...)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Index again when you mean to iterate.... It's more idiomatic to write this as

for _, addrs := range oas.addrs {
  allObserved = append(allObserved, addrs...)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

More pertinent however, do we really need this copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, it's probably a micro-optimization to fix the copy. Have fixed the looping .

Copy link
Contributor

Choose a reason for hiding this comment

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

no you haven't, it's still there.

Comment on lines 493 to 494
for i := range addrs {
oa := addrs[i]
Copy link
Contributor

Choose a reason for hiding this comment

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

and here again:

for _, oa := range addrs {
...
}

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.

// An observed address on an outbound connection that has ONLY been seen by one peer
if now.Sub(oa.lastSeen) <= oas.ttl && oa.numInbound == 0 && len(oa.seenBy) == 1 {
cnt++
for s, _ := range oa.seenBy {
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need the , _.

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 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

Have made the changes.

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.

needs a go mod tidy; also, fix dat loop!

go.mod Outdated
Comment on lines 7 to 8
github.com/google/gopacket v1.1.18 // indirect
github.com/google/uuid v1.1.2 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

what's that crap popping up? Do a go mod tidy.

Comment on lines 470 to 472
for k := range oas.addrs {
allObserved = append(allObserved, oas.addrs[k]...)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no you haven't, it's still there.

@aarshkshah1992
Copy link
Contributor Author

@vyzo This is ready.

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.

alright, let's merge this.

@aarshkshah1992 aarshkshah1992 merged commit 69916ed into master Feb 19, 2021
@aarshkshah1992 aarshkshah1992 deleted the feat/nat-type branch February 19, 2021 09:44
@Stebalien Stebalien mentioned this pull request May 11, 2021
27 tasks
@aschmahmann aschmahmann mentioned this pull request May 14, 2021
71 tasks
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.

3 participants