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

feat: Add support for both ip and hostname for peers #3432

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

Conversation

Raphexion
Copy link
Contributor

Motivation

It is convinent to be able to specify both IPs and hostnames as peers. Especially, when you are building ephemeral testnets and want to specify peers as

./snarkos start --peers "validator0:4130,client0:4130"

Where you can populate the /etc/hosts file with subnet hostnames.

Test Plan

Added unit-tests but can be tested on the commandline

Related PRs

None.

@Raphexion Raphexion force-pushed the nijo/add-support-for-hostname-peers branch from 21a4598 to eee4dc7 Compare November 8, 2024 15:11
cli/src/commands/start.rs Outdated Show resolved Hide resolved
@Raphexion Raphexion force-pushed the nijo/add-support-for-hostname-peers branch 2 times, most recently from 5c0cb10 to 126b444 Compare November 11, 2024 10:43
@Raphexion Raphexion requested a review from ljedrz November 11, 2024 10:44
cli/src/commands/start.rs Outdated Show resolved Hide resolved
cli/src/commands/start.rs Outdated Show resolved Hide resolved
cli/src/commands/start.rs Outdated Show resolved Hide resolved
cli/src/commands/start.rs Outdated Show resolved Hide resolved
It is convinent to be able to specify both IPs and hostnames as peers.
Especially, when you are building ephemeral testnets and want to specify
peers as

```
./snarkos start --peers "validator0:4130,client0:4130"
```

Where you can populate the `/etc/hosts` file with subnet hostnames.
@Raphexion Raphexion force-pushed the nijo/add-support-for-hostname-peers branch from 126b444 to 5b9c8b4 Compare November 11, 2024 10:54
@Raphexion Raphexion requested a review from ljedrz November 11, 2024 10:55
Copy link
Collaborator

@ljedrz ljedrz left a comment

Choose a reason for hiding this comment

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

LGTM 👌

let trimmed = ip_or_hostname.trim();
match trimmed.to_socket_addrs() {
Ok(mut ip_iter) => {
// A hostname might resolve to multiple IP addresses. We will use only the first one,
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid any surprises for node operators, is there any known syntax for selecting a particular index? Should we invent our own, e.g. hostname:port:1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative solution would be to enforce that only one IP is expected and allowed. So if there are more than one IP found, then we will print a warning and ignore the hostname.

The main idea behind this feature is to allow for a more flexible setup during tests. And in those cases, we can make sure that there is only one "validator-1" and only one "client-42". So I think it is a good trade-off, to enforce that only one IP is expected and found.

@vicsn
Copy link
Contributor

vicsn commented Nov 11, 2024

Though its a small and useful change, let's make sure there's buy-in and awareness from all dev orgs that the nodes now also rely on DNS, incurring a small new privacy and security risk.

Also worth looking at geth's recent addition, which apparently also picks the first ip if multiple are defined. :) ethereum/go-ethereum#18524

@Raphexion
Copy link
Contributor Author

@vicsn I agree 💯 , we should make sure that this feature is wanted and accepted 👍

And great link to geth 🙏 ☀️

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