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

Improve network address discovery / NAT setup #323

Merged
merged 4 commits into from
Mar 2, 2021
Merged

Conversation

kdeme
Copy link
Contributor

@kdeme kdeme commented Jan 18, 2021

Testing out logic for now. Moving also the setupNat (now setupAddress) inside nim-eth, as the same or similar code is used for nimbus-eth2, waku v1, waku v2, nimbus-eth1 and dcli.

Needs further polish on error passing, log messages, etc.

eth/net/nat.nim Outdated Show resolved Hide resolved
@kdeme kdeme force-pushed the setup-address branch 2 times, most recently from 4e5f77d to eed4a1e Compare February 16, 2021 15:36
@kdeme
Copy link
Contributor Author

kdeme commented Feb 16, 2021

Finished up the logic. Could use another pass for a bigger refactor and changes, but it would depend on some other features that one might want to add so I kept it to a minimum for now and did not go there in this PR.

Some of those items that could be added/changed, but probably better in another PR:

  • Allow for tcp or udp port map only (e.g. when running only a discovery service, don't port map TCP)
  • Allow for multiple port maps (e.g. Waku bridge has such use case, were a Waku v1 and Waku v2 node needs to be run) (or multiple addresses to be set up basically)
  • Allow for custom port maps e.g. 9000:1234 (not sure of there is a real use case for this)
  • Depending on whether we want to stick with that --nat:... option or rather have several independent options, the code could be reorganized and slightly simplified probably. Could also just start with splitting config parsing and actual logic.
  • Additional calls for IPv6 version when we want to support that.

@kdeme kdeme merged commit 0700ec7 into master Mar 2, 2021
@kdeme kdeme deleted the setup-address branch March 2, 2021 16:13
return (some(ip), some(tcpPort), some(udpPort))
except ValueError:
error "Not a valid IP address", address = nat[6..^1]
quit QuitFailure
Copy link
Member

Choose a reason for hiding this comment

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

since this is a function in a library, we really shouldn't be quitting..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point. Thought about that actually and was also one of the reasons why I don't quit on for example https://github.com/status-im/nim-eth/pull/323/files#diff-161c148740a4e155d7afca5fd40f23461ba6581e2949c694ffd89aca776b0d0dR293 .
I guess I could already split the parsing of the options with the actual logic, and keep the first in nimbus-eth2. Was planning on doing that together with possible cli option changes (if we decided on that). Either that or returning a result, but that is a bit silly unless I provide the currently logged error messages in (and possibly an enum) and remove the logs there. Not really needed imo.

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