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: Require validators to provide CLI param --public-addresses for starting nodes #5266

Open
lexnv opened this issue Aug 6, 2024 · 5 comments

Comments

@lexnv
Copy link
Contributor

lexnv commented Aug 6, 2024

Requiring validators to provide public addresses ensures the authority can be discovered sooner.

This should happen in one of the following releases, after the release that includes:

@lexnv lexnv added this to Networking Aug 6, 2024
@bkchr
Copy link
Member

bkchr commented Aug 12, 2024

Why? I still don't get this. You should just be able to query the local interfaces to find out the public addr? (Assuming they are not behind a NAT, which we don't support any way for validators)

@nicolasochem
Copy link

in a datacenter environment, it's pretty common to be behind a load balancer, in which case the public ip is never discoverable from a local interface.

but libp2p has various mechanisms to find the "real" ip so I also think it should not be mandatory to force-set it.

@bkchr
Copy link
Member

bkchr commented Sep 28, 2024

in a datacenter environment, it's pretty common to be behind a load balancer, in which case the public ip is never discoverable from a local interface.

What kind of load balancer? I mean you can only have 1 validator running, so what is it load balancing?

@lexnv
Copy link
Contributor Author

lexnv commented Oct 1, 2024

My initial thought was that we may have validators running under a NAT. Similar to the comment above, a node operator might start a node on their machine and will probably be behind a firewall.
Some authority DHT records provided addresses that were unreachable last year when I experimented with collecting records.

I think we can change the behavior of the authority discovery to:

  • publish [public addresses (first) ++ local interface if global (second) ++ network discovered via Identify protocol]
  • emit this warning if no public addresses are provided and no global ips are discovered from the local interfaces

I double checked the listen addresses reported by litep2p running in the cloud, they will look similar to:

/ip6/[valid-ipv6-addr/tcp/3333/ws/p2p/...
/ip6/::1/tcp/30333/ws/p2p/...
/ip4/[valid-ip-v4]/tcp/30333/ws/p2p/...
/ip4/127.0.0.1/tcp/30333/...

From these details, it looks like we can find at least 2 global addresses, so in most of the cases we'll never emit the warning.

I'll come with a follow-up to the initial PR and only warn if we cannot find global ips in our listen addresses: #5240

Thanks for the feedback, let me know if that sounds like a plan 🙏

@bkchr
Copy link
Member

bkchr commented Oct 8, 2024

My initial thought was that we may have validators running under a NAT.

At least for Polkadot we would not support this right now. AFAIK we don't support NAT hole punching and this would be problematic for collators who need a direct connection to the validators.

  • publish [public addresses (first) ++ local interface if global (second) ++ network discovered via Identify protocol]

Yeah that sounds reasonable. I think I had proposed somewhere already.

  • emit this warning if no public addresses are provided and no global ips are discovered from the local interfaces

Yeah also a good point.

github-merge-queue bot pushed a commit that referenced this issue Nov 5, 2024
#6298)

This PR's main goal is to add public listen addresses to the DHT
authorities records. This change improves the discoverability of
validators that did not provide the `--public-addresses` flag.

This PR populates the authority DHT records with public listen addresses
if any.

The change effectively ensures that addresses are added to the DHT
record in following order:
 1. Public addresses provided by CLI `--public-addresses`
 2. Maximum of 4 public (global) listen addresses (if any)
3. Any external addresses discovered from the network (ie from
`/identify` protocol)


While at it, this PR adds the following constraints on the number of
addresses:
- Total number of addresses cached is bounded at 16 (increased from 10).
- A maximum number of 32 addresses are published to DHT records
(previously unbounded).
  - A maximum of 4 global listen addresses are utilized.
      
This PR also removes the following warning:
`WARNING: No public address specified, validator node may not be
reachable.`

### Next Steps
- [ ] deploy and monitor in versi network

Closes: #6280
Part of: #5266

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Bastian Köcher <git@kchr.de>
github-actions bot pushed a commit that referenced this issue Nov 5, 2024
#6298)

This PR's main goal is to add public listen addresses to the DHT
authorities records. This change improves the discoverability of
validators that did not provide the `--public-addresses` flag.

This PR populates the authority DHT records with public listen addresses
if any.

The change effectively ensures that addresses are added to the DHT
record in following order:
 1. Public addresses provided by CLI `--public-addresses`
 2. Maximum of 4 public (global) listen addresses (if any)
3. Any external addresses discovered from the network (ie from
`/identify` protocol)

While at it, this PR adds the following constraints on the number of
addresses:
- Total number of addresses cached is bounded at 16 (increased from 10).
- A maximum number of 32 addresses are published to DHT records
(previously unbounded).
  - A maximum of 4 global listen addresses are utilized.

This PR also removes the following warning:
`WARNING: No public address specified, validator node may not be
reachable.`

### Next Steps
- [ ] deploy and monitor in versi network

Closes: #6280
Part of: #5266

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Bastian Köcher <git@kchr.de>
(cherry picked from commit 762f824)
lexnv added a commit that referenced this issue Nov 15, 2024
#6298)

This PR's main goal is to add public listen addresses to the DHT
authorities records. This change improves the discoverability of
validators that did not provide the `--public-addresses` flag.

This PR populates the authority DHT records with public listen addresses
if any.

The change effectively ensures that addresses are added to the DHT
record in following order:
 1. Public addresses provided by CLI `--public-addresses`
 2. Maximum of 4 public (global) listen addresses (if any)
3. Any external addresses discovered from the network (ie from
`/identify` protocol)


While at it, this PR adds the following constraints on the number of
addresses:
- Total number of addresses cached is bounded at 16 (increased from 10).
- A maximum number of 32 addresses are published to DHT records
(previously unbounded).
  - A maximum of 4 global listen addresses are utilized.
      
This PR also removes the following warning:
`WARNING: No public address specified, validator node may not be
reachable.`

### Next Steps
- [ ] deploy and monitor in versi network

Closes: #6280
Part of: #5266

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

3 participants