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

cmd/bootnode, p2p: use an alternate mapped port #26359

Merged
merged 25 commits into from
Jul 14, 2023
Merged

Conversation

dbadoy
Copy link
Contributor

@dbadoy dbadoy commented Dec 14, 2022

NAT-PMP maps an alternate port if the requested port is already in use and returns success. The current implementation define by 'failure' if the request port and the actual mapped port are different. Therefore it simply kept logging the message that 'Couldn't add port mapping' until the port was available again if the previously requested port was already used.

The goal of this PR is to have normal operation using an alternative port number even if the originally intended port is unavailable on NAT.

started from #26321 (comment), dbadoy#1

@fjl
Copy link
Contributor

fjl commented Dec 23, 2022

Sorry for the wait, I'm on leave until next year.

dbadoy and others added 7 commits July 12, 2023 02:19
* temp

* rede

* remove meaningless field

* temp comment

* set refresh time to param

* server: use channel enr.Entry

* server: rename `changedport` to `changeport`

* server: apply new nat-refresh `ethereum discovery`

* p2p/nat: add some comment

* p2p/nat: use `DefaultMapTimeout`

* p2p: fixed

* p2p/server: impr

* p2p/server: syntax change

* cmd/bootnode: add comments

* add p2p.Server changeport

* all: fix comments

* p2p/server: syntax changes

* cmd/bootnode: do not change UDPAddr Port

* p2p: remove incorrect `for` context

* p2p: add what needs to be addressed

* p2p/nat: use AddPortMapping if supported

* asynchronous port set

* remove unused variables

* change return format in `changePort`

* cmd/bootnode: consistent log

* p2p/server: rename `refreshLogger` to `newLogger`

* cmd/bootnode: fix incorrect if-else

* fix

* change log orders

* rename `natRefresh` to `natMapLoop`

* p2p: remove `changeport` in Server main loop

* server: remove duplicate code

* typo

* typo

* cmd/bootnode: consistent log message

* p2p: fix comments

test code about port change on localnode

cmd/bootnode: add log

improve test logic , add some comments

p2p/nat: add comments about not supported

p2p: remove unnecessary conversion in test

fix test failure, not setting TCP on localnode

remove note

checks mapped port first
Better to do it like this because the external IP needs to be polled as well.
@fjl fjl added this to the 1.12.1 milestone Jul 12, 2023
@fjl
Copy link
Contributor

fjl commented Jul 12, 2023

This should be ready now!

@dbadoy
Copy link
Contributor Author

dbadoy commented Jul 12, 2023

It looks really good! I made a small change.

p2p/server_nat.go Outdated Show resolved Hide resolved
p2p/server_nat_test.go Outdated Show resolved Hide resolved
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Couple questions but generally looks good.

p2p/nat/nat.go Outdated Show resolved Hide resolved
p2p/server_nat.go Outdated Show resolved Hide resolved
p2p/nat/natupnp.go Outdated Show resolved Hide resolved
@fjl fjl changed the title cmd/bootnode, p2p: use an alternate port mapped to NAT cmd/bootnode, p2p: use an alternate mapped port Jul 14, 2023
@fjl fjl merged commit 60ecf48 into ethereum:master Jul 14, 2023
1 check passed
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This changes the port mapping procedure such that, when the requested port is unavailable
an alternative port suggested by the router is used instead.

We now also repeatedly request the external IP from the router in order to catch any IP changes.

Co-authored-by: Felix Lange <fjl@twurst.com>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This pull request was closed.
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