Skip to content
This repository has been archived by the owner on Jun 12, 2023. It is now read-only.

Disable nat on seeds and validators #1559

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jadeallenx
Copy link
Contributor

By default NAT is enabled. For seed nodes and validators this should not be necessary. The docker validator configuration still leaves NAT enabled though. (Should it be disabled there?)

@jadeallenx
Copy link
Contributor Author

Checking on how this affects NAT mapping

@kellybyrd
Copy link
Contributor

kellybyrd commented May 21, 2022

Does this turn off the public address discovery for a validator? If so, a few comments:

  • For Docker validators, the Docker command line in the deployment guide docs uses Docker's bridge networking which does NAT from the host to the container. I know some validator operators that run host networking, but it's likely not the bulk of them.

  • Several popular VPS providers (AWS, GCP, likely others) do a sort of 1:1 NAT between the VM and the outside world. In these cases, running Docker or directly on the host, I worry this PR will cause those validators to not discover their real public IPs to gossip.

Long term, it might be better for the validator fleet to force operators to be explicit about their networking config with this setting and the NAT_... environment variables, but short term, this would surprise many operators when they couldn't connect back to the p2p networking on restart.

Note that I can't test either bullet point above, I'm not affected by either of them right now.

@madninja
Copy link
Member

I agree with kellybrd here.. I think we should use this as an opportunity to remove NAT and declare a single set of variables for binding listen ipports and public ip with a port for libp2p and a port for grpc. Or something.. it'll need some thought as to all that needs to be captured and explicitly declared by validator operators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants