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

network: support for uPnP and PMP nat traversal #1050

Merged
merged 9 commits into from
Jan 9, 2025
Merged

Conversation

munna0908
Copy link
Contributor

This PR introduces support for new NAT traversal techniques. Issue: #905

Changes Include

  • Modified the nat CLI flag: Allowing users to select from various options for determining the public address.
The available options are:
    • any: Automatically selects any available method.
    • none: Disables NAT detection.
    • upnp: Uses UPnP (Universal Plug and Play) for NAT traversal.
    • pmp: Uses PMP (Port Mapping Protocol) for NAT traversal.
    • extip:: Specifies a custom external IP address.
  • Removed disc-ip flag: The IP address will now be automatically parsed from the provided listener address

@munna0908 munna0908 requested review from dryajov and gmega December 18, 2024 09:47
@munna0908 munna0908 self-assigned this Dec 18, 2024
@munna0908 munna0908 requested a review from benbierens December 18, 2024 10:25
Copy link
Member

@gmega gmega left a comment

Choose a reason for hiding this comment

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

Made some minor suggestions but otherwise looks good to me. As soon as the CI issue is fixed, I think we're good to go.

codex/codex.nim Outdated Show resolved Hide resolved
codex/conf.nim Outdated Show resolved Hide resolved
codex/utils/addrutils.nim Outdated Show resolved Hide resolved
tests/codex/testnat.nim Outdated Show resolved Hide resolved
@munna0908 munna0908 force-pushed the feature/nat-traversal branch from fbcf01e to b387b6a Compare January 7, 2025 10:28
@munna0908 munna0908 requested a review from gmega January 7, 2025 10:50
build.nims Outdated Show resolved Hide resolved
Copy link
Member

@gmega gmega left a comment

Choose a reason for hiding this comment

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

We're close now.

tests/codex/testnat.nim Outdated Show resolved Hide resolved
codex/utils/addrutils.nim Outdated Show resolved Hide resolved
@gmega
Copy link
Member

gmega commented Jan 7, 2025

@veaceslavdoina @benbierens Rahul is making some changes to the way NAT parameters work, we should make sure that all users of those (e.g. testnet deployers, disttests deployers) are not gonna fall apart, would be good to have your eyes here.

@munna0908 munna0908 requested a review from gmega January 7, 2025 14:04
Copy link
Member

@gmega gmega left a comment

Choose a reason for hiding this comment

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

Looks good as far as I'm concerned.

Copy link
Contributor

@dryajov dryajov left a comment

Choose a reason for hiding this comment

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

Looks good, I tested this and it seems to work fine with PMP on my network. Only request is to address the several style comments that I left, other than that, it's good to go.

else:
none(Port)

result = (ip: ip, port: port)
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting result explicitly is usually unnecessary (and in fact, can cause issues under certain condition). Nim will return whatever the result of the last evaluation of the selected codepath is.

Suggested change
result = (ip: ip, port: port)
(ip: ip, port: port)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. commit

codex/nat.nim Outdated
of true: extIp*: ValidIpAddress
of false: nat*: NatStrategy

func parseCmdArg*(T: type NatConfig, p: string): T {.raises: [ValueError].} =
Copy link
Contributor

Choose a reason for hiding this comment

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

By convention, we keep this in the conf.nim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@munna0908 munna0908 force-pushed the feature/nat-traversal branch from 2bae744 to 0934e59 Compare January 9, 2025 10:47
@gmega
Copy link
Member

gmega commented Jan 9, 2025

If you're done addressing the comments, @munna0908, feel free to merge.

@gmega gmega added this pull request to the merge queue Jan 9, 2025
Merged via the queue into master with commit 74c46b3 Jan 9, 2025
23 of 24 checks passed
@gmega gmega deleted the feature/nat-traversal branch January 9, 2025 19:20
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.

4 participants