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(anvil): add ability to listen on multiple IP addresses #5222

Merged
merged 13 commits into from
Jul 11, 2023

Conversation

0xZerohero
Copy link
Contributor

@0xZerohero 0xZerohero commented Jun 26, 2023

Motivation

In complex setups, there is sometimes a need to listen on multiple internal network addresses or both IPv4 and IPv6 simultaneously.

Resolves #4941

Solution

I updated the NodeHandle to store Vecs of server handles and addresses.
The following commands work the same way they worked before:
anvil - makes it listen on 127.0.0.1:8545
env ANVIL_IP_ADDR="1.1.1.1" anvil - makes it listen on 1.1.1.1:8545
anvil --host 1.1.1.1 - makes it listen on 1.1.1.1:8545
etc.
but now you can also add additional hosts if needed
anvil --host :: --host 127.0.0.1 --host 1.1.1.1 - makes it listen on [::]:8545, 127.0.0.1:8545, 1.1.1.1:8545
env ANVIL_IP_ADDR="::1,1.1.1.1" anvil - makes it listen on [::1]:8545, 1.1.1.1:8545

The limitations in the current implementation are:

  • all hosts will listen on the same port. This can be fixed in a clean way only by introducing a breaking change

TBD:

  • The "host" option can be in format "ip:port" - which provides the most flexibility. However this change is not backward compatible with the current ANVIL_IP_ADDR format.
  • I noticed an odd arrangement: the "host" command option is located in the "Server options" section, while the "port" option is placed within the "EVM options" section. In my opinion, it would be more logical to have the "port" option in the "Server options" section.
  • NodeHandle::socket_address now returns the first listening address, making sure all other methods that depend on it play nicely. Based on my observations, these methods are primarily used for testing, but in any case, they should work. So I reckon this should be good to go, right?

I'm opening a Draft Pull Request to get your feedback on this :)

@Evalir Evalir requested review from Evalir and mattsse June 26, 2023 21:58
anvil/src/cmd.rs Show resolved Hide resolved
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

Will review and QA later to ensure this is not a breaking change

@0xZerohero
Copy link
Contributor Author

0xZerohero commented Jun 27, 2023

- you can't add more than one host when using the environment variable ANVIL_IP_ADDR
This was actually easier to fix than I thought. Description is updated.

@0xZerohero 0xZerohero changed the title [WIP] feat(anvil): add ability to listen on multiple IP addresses feat(anvil): add ability to listen on multiple IP addresses Jul 5, 2023
@0xZerohero
Copy link
Contributor Author

0xZerohero commented Jul 5, 2023

Is there anything else that needs to be done here ?
@Evalir

@Evalir
Copy link
Member

Evalir commented Jul 5, 2023

Will review soon! sorry for the wait

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I can see how this could be useful,

supportive, since this only adds functionality and doesn't break anything

anvil/src/cmd.rs Outdated Show resolved Hide resolved
anvil/src/lib.rs Outdated Show resolved Hide resolved
anvil/src/lib.rs Show resolved Hide resolved
@Evalir Evalir requested review from Evalir and mattsse July 7, 2023 20:35
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Evalir Evalir 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 now!

@mattsse mattsse merged commit 8c8ab75 into foundry-rs:master Jul 11, 2023
20 checks passed
@llllvvuu
Copy link
Contributor

llllvvuu commented Jul 12, 2023

Nice! Minor point since my issue is already solved (becomes even more minor if Node.js 20+ keeps Happy Eyeballs on by default - but they may revert 🥴), but one way to make localhost "just work" could be to introduce (if it's not too breaking?):

- default_value = "127.0.0.1",
+ default_value = "::1,127.0.0.1",
- host: vec![IpAddr::V4(Ipv4Addr::LOCALHOST)],
+ host: vec![IpAddr::V4(Ipv4Addr::LOCALHOST), IpAddr::V6(Ipv6Addr::LOCALHOST)],
- self.host = if host.is_empty() { vec![IpAddr::V4(Ipv4Addr::LOCALHOST)] } else { host };
+ self.host = if host.is_empty() { vec![IpAddr::V4(Ipv4Addr::LOCALHOST), IpAddr::V6(Ipv6Addr::LOCALHOST)] } else { host };

Or support hostnames and default to localhost (could be even more breaking?):

; evcxr
>> use std::net::ToSocketAddrs;
>> let addrs_iter = "localhost:8545".to_socket_addrs().unwrap();
>> addrs_iter
IntoIter([[::1]:8545, 127.0.0.1:8545])

For reference, geth serves on localhost by default (although this is still wonky in Go).

the tl;dr of the rabbit hole is that:

  • Most of the non-Rust world resolves localhost and serves on the first matching interface. It would have been a fair assumption a few years ago that localhost resolves to only one interface.
  • Much of the Rust world doesn't bother resolving localhost and just uses 127.0.0.1 (except for Iron, which does resolve localhost to the first interface). It would have been a fair assumption a few years ago that these are the same thing.
  • None of 127.0.0.1, ::1, or localhost are guaranteed to be substitutable for each other. Mixing conventions causes mildly confusing issues.

Given how all over the place the networking standards/conventions are (to the point where even language stdlibs don't have it together), perhaps the sanest thing may be to ignore all of my above ideas and just put something in the docs to the effect of "anvil serves on only 127.0.0.1 by default, don't try to connect to it via localhost" and call it a day. hardhat switched to 127.0.0.1 a while back so we'd be in good company.

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.

anvil: listen on ipv6
4 participants