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

p2p/nat: add stun protocol #31064

Merged
merged 15 commits into from
Jan 24, 2025
Merged

p2p/nat: add stun protocol #31064

merged 15 commits into from
Jan 24, 2025

Conversation

fearlessfe
Copy link
Contributor

@fjl here is the new pr to replace the old one, which Allow edits by maintainers. I rebase the branch from the master.

fixes #30881

@@ -760,7 +760,7 @@ var (
}
NATFlag = &cli.StringFlag{
Name: "nat",
Usage: "NAT port mapping mechanism (any|none|upnp|pmp|pmp:<IP>|extip:<IP>)",
Usage: "NAT port mapping mechanism (any|none|upnp|pmp|pmp:<IP>|extip:<IP>|stun:default|stun:<IP:PORT>)",
Copy link
Member

Choose a reason for hiding this comment

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

We can omit the default, e.g. stun|stun:<IP:PORT>

p2p/nat/nat.go Outdated
@@ -59,13 +59,16 @@ type Interface interface {
// "upnp" uses the Universal Plug and Play protocol
// "pmp" uses NAT-PMP with an auto-detected gateway address
// "pmp:192.168.0.1" uses NAT-PMP with the given gateway address
// "stun:default" uses stun protocol with default stun server
Copy link
Member

Choose a reason for hiding this comment

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

We can use "stun" to represent using stun protocol with default server

@@ -0,0 +1,94 @@
// Copyright 2024 The go-ethereum Authors
Copy link
Member

Choose a reason for hiding this comment

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

2025

p2p/nat/nat_stun.go Outdated Show resolved Hide resolved
return fmt.Sprintf("STUN(%s)", s.server)
}

func (stun) SupportsMapping() bool {
Copy link
Member

Choose a reason for hiding this comment

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

This function is not needed

responseError = event.Error
return
}
if err := mappedAddr.GetFrom(event.Message); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err := mappedAddr.GetFrom(event.Message); err != nil {
responseError = mappedAddr.GetFrom(event.Message)

@fjl
Copy link
Contributor

fjl commented Jan 23, 2025

I have pushed my changes:

  • added a more comprehensive list of STUN servers. The list contains only IP addresses because we don't want the node to make any DNS requests for these servers. There is also a script for updating the servers.
  • three servers will be selected at random from the list. the previous approach always used the servers in order, while remembering which ones didn't work. I think it's better to use random server and not remember. The code in p2p.Server will occasionally retry the ExternalIP request, so if it doesn't work the first time, it may work next time.
  • concurrent requests are removed. usually the request will be quick. But even if it's not super quick, I still think it's OK. The ExternalIP request runs on its own goroutine anyway.

@fjl fjl added this to the 1.15.0 milestone Jan 23, 2025
@@ -0,0 +1,24 @@
package nat
Copy link
Member

Choose a reason for hiding this comment

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

pls add the license

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the license

rjl493456442
rjl493456442 previously approved these changes Jan 24, 2025
Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

one nitpick, otherwise lgtm

@fjl fjl merged commit 75526bb into ethereum:master Jan 24, 2025
1 of 2 checks passed
tokeyg pushed a commit to tokeyg/go-ethereum that referenced this pull request Feb 6, 2025
This implements a basic mechanism to query the node's external IP using
a STUN server. There is a built-in list of public STUN servers for convenience.
The new detection mechanism must be selected explicitly using `--nat=stun` 
and is not enabled by default in Geth.

Fixes ethereum#30881

---------

Co-authored-by: Felix Lange <fjl@twurst.com>
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.

Add stun protocol for p2p/nat
3 participants