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

nat: add HasNAT method for checking NAT environments #2346

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
nat: add HasNAT method for checking NAT environments
  • Loading branch information
sukunrt committed Jun 9, 2023
commit bed6dcaae5cc081a35c8cba4d1cb0ca3474e50b6
5 changes: 2 additions & 3 deletions p2p/host/basic/basic_host.go
Original file line number Diff line number Diff line change
@@ -843,11 +843,10 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {

finalAddrs = network.DedupAddrs(finalAddrs)

// natmgr is nil if we do not use nat option;
if h.natmgr != nil {
// use nat mappings if we have them
if h.natmgr != nil && h.natmgr.HasNAT() {
// We have successfully mapped ports on our NAT. Use those
// instead of observed addresses (mostly).

// Next, apply this mapping to our addresses.
for _, listen := range listenAddrs {
extMaddr := h.natmgr.GetMapping(listen)
7 changes: 7 additions & 0 deletions p2p/host/basic/natmgr.go
Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@ import (
// and tries to obtain port mappings for those.
type NATManager interface {
GetMapping(ma.Multiaddr) ma.Multiaddr
HasNAT() bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should call this HasNAT because this checks if we can communicate with a NAT device. If we have a NAT, but are unable to communicate with it (e.g. via upnp) then, this would return false.

Maybe call it HasDiscoveredNAT instead?

io.Closer
}

@@ -86,6 +87,12 @@ func (nmgr *natManager) Close() error {
return nil
}

func (nmgr *natManager) HasNAT() bool {
nmgr.natMx.RLock()
defer nmgr.natMx.RUnlock()
return nmgr.nat != nil
}

func (nmgr *natManager) background(ctx context.Context) {
defer nmgr.refCount.Done()