Listen for p2p connections on IPv6 (besides IPv4)

[dagonharett]

This simple PR makes grin listen for p2p connections on both IPv6 and IPv4 as default. It also adds a summary of all the available options into the config file comments.

It's 2020 and IPv6 is now deployed throughout the world. We are now even getting to the point where some machines only have IPv6 connection. This happens already with several cloud providers that offer IPv6-only VPSes.

Seems wise to turn on IPv6 by default now. Otherwise, grin nodes running in IPv6-only environments will not be able to reach any peers. Also, as detailed on #461, IPv6 brings in other advantages like avoiding the need for NAT.

Fortunately grin already supports IPv6 out-of-the-box. By setting the listening address to ::, the underlying Rust libraries set up a socket that listens both on IPv4 and IPv6.

I tested this on several Linux machines (baremetal and VPS) and they successfully connected together with both IPv4 and IPv6. Would be nice if someone could test on Windows and macOS. Anyway, this should be more than safe now. We would have to go far into the past to find versions of these operating systems without an IPv6 stack.

[hashmap]

Thanks for your contribution! Can we add couple tests for the default config, eg test ipv4 and ipv6 clients?

[quentinlesceller]

Thank you for your contribution @dagonharett, going to run some tests on macOS/Windows. In any case that's a simple yet very useful PR :)

[WhoNeedszZz]

I had a feeling enabling IPv6 with Rust would be that simple. As mentioned above, some unit tests testing each interface would be appreciated.

[antiochp]

We are seeing some ipv4-mapped ipv6 addresses on the network currently and circumstantially we have noticed that peer behavior feels different on public nodes.
We are investigating and it may not be related - but I'd like to hold off merging this until we are confident one way or the other.

[dagonharett]

Thank you all for the feedback! And sorry for the delay on my reply.
@quentinlesceller did the tests on Windows and macOS turned out ok?

As I've never programmed in Rust, I am pretty oblivious to where and how I should to create the unit tests. I would appreciate if someone experienced could step in and take this task.

Anyway, I took a look into p2p/tests and it seems that I could re-use peer_handshake(). Instead of the hardcoded 127.0.0.1 I could have it accept the server and client addresses as arguments, and then alternate between 127.0.0.1 and ::1.
Seems sensible? If so, where should I put the new tests, perhaps p2p/tests/ip_version.rs? Should I keep the refactored peer_handshake() at peer_handshake.rs or move it somewhere else?

[quentinlesceller]

Hi @dagonharett, I haven't tried again since we changed several things on the node side.

I think something like peer_handshake() in another file like you mentioned would be great ( p2p/tests/ip_version.rs)!

[dagonharett]

I've just pushed a commit that adds the IPv6 test (IPv4 was already there, I just had to do some minor refactoring). I opted to add the test into p2p/tests/peer_handshake.rs as that keeps all the code together. After all both are handshake tests, just with different IP versions.
Comments are welcomed.

Unfortunately, the IPv6 test fails on macos in a way that I cannot reproduce on Linux: Store(LmdbErr(Error::Code(17, 'File exists')))
Does macos somehow interpret ::1 as a file path? Help from someone running macos would be highly appreciated.
Besides Linux, it also seems to run fine on Windows.

(I did a rebase to make sure no conflicting code had been introduced. This forced me to do a force push. Perhaps not the wisest decision.)

[hashmap]

I tested on OSX, it fails. The problem is not related to your change, you run the same code twice, it creates a p2p server with data folder .grin, it seems OSX is too slow to remove it after the first run. In integration tests we clean up manually calling https://github.com/mimblewimble/grin-wallet/blob/master/integration/tests/framework.rs#L38 in your case I'd recommend to generate a unique name using hash of the first argument or smth.

[dagonharett]

@hashmap Thanks a lot for testing and for the suggestion. I've now replaced the static .grin data folder for a unique hash. This made all tests run successfully on all platforms.

After this I added a new test, peer_handshake_mixed, which starts a server at :: and a client at 127.0.0.1:5000. This should work, as per my testing between different machines, :: makes the server accept both IPv4 and IPv6 connections. Indeed, that's the case on the test, the connection is established and the handshake is exchanged. Unfortunately the test fails because the server detects the client connection as coming from an IPv6:

DEBUG grin_p2p::store - save_peer: PeerAddr(V6([::1]:5000)) marked Healthy

Seems like things work a bit different when using the local host. Any idea on how to force the client to IPv4? Or should I just remove this test?

[hashmap]

I'm not sure it's specific to localhost and having this test is a good idea. The problem is here - https://github.com/mimblewimble/grin/blob/master/p2p/src/handshake.rs#L255 - we get an IPv4 addr as advertised but then we take the address of the existing connection (which is IPv6) and add the port from the advertised address. We should keep this behavior but take into account the protocol version, so we should take port and the protocol version from advertised and socket address from the existing connection. Does it make sense?

[quentinlesceller]

Reading again these lines of code. There is two parts in the code where we call this function:

1. https://github.com/mimblewimble/grin/blob/master/p2p/src/handshake.rs#L190
2. https://github.com/mimblewimble/grin/blob/master/p2p/src/handshake.rs#L208

In both case the advertised argument is the PeerAddr from the Hand.

At which point we take this PeerAddr, "extract" the port and try to get the SocketAddr from the TcpStream. If we can resolve it we build a new PeerAddr using the previous SocketAddr with the ip() function which returns either a IpAddr:V4 or IpAddr:V6. If we cannot get the SocketAddr from the TcpStream we simply return the advertised PeerAddr.

Three questions here:

1. @hashmap you are saying that the PeerAddr of the Hand is an IPV4 mapped IPV6?
2. Why not use directly https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.set_port instead of recreating a SocketAddr?
3. Why is this function even needed?

[hashmap]

1. Yeah, in this case the incoming connection from the peer is V6, but the peers listens on V4 address, so it advertises V4 address, Hand is correct. However, we build a new addr on top of the incoming connection, ignoring advertised except the port value.
2. Because advertised and incoming connection we have may refer to different addresses and we want to take IP of the later, but we didn't take into account V6 case
3. I can only guess - to prevent a malicious peer from advertising a wrong address, eg to ddos this address or for any other malicious reasons. Or perhaps smth NAT-related is applicable too, not sure

[quentinlesceller]

Thanks for the answer. Yeah I understand that if we listen on ipv4 then this will be an ipv4 mapped ivp6.

Regarding 2 I was talking about doing something like that (untested code and doesn't change the logic at all...):

/// Resolve the correct peer_addr based on the connection and the advertised port.
fn resolve_peer_addr(advertised: PeerAddr, conn: &TcpStream) -> PeerAddr {
	let port = advertised.0.port();
	if let Ok(mut addr) = conn.peer_addr() {
		PeerAddr(addr.set_port(port));
	} else {
		advertised
	}
}

[hashmap]

Wouldn’t it alter the existing incoming connection? Even if not the intention is less clear now.

[dagonharett]

we get an IPv4 addr as advertised but then we take the address of the existing connection (which is IPv6)

Bingo!

Advertised: 127.0.0.1:5000.
Connection: [::1]:46386.

Thanks for taking a look at it.

We should keep this behavior but take into account the protocol version, so we should take port and the protocol version from advertised and socket address from the existing connection. Does it make sense?

@hashmap Makes sense. But poses a problem: how to translate the IP address? IPv6 to IPv4 translation would only work for localhost and IPv4 mapped IPv6. We would have to assume (and check) that we are in one of those scenarios. Actually, I cannot think of any other (non-malicious) scenario where the connection is IPv6 and advertised is IPv4.

The code excerpt from @quentinlesceller above will not work due to the lack of this IP address translation. I run a test with a slightly modified version, that avoids modifying the incoming connection, as @hashmap suggested:

fn resolve_peer_addr(advertised: PeerAddr, conn: &TcpStream) -> PeerAddr {
    let port = advertised.0.port();
    if let Ok(addr) = conn.peer_addr() {
        let mut new_addr = addr.clone();
        new_addr.set_port(port);
        PeerAddr(new_addr)
    } else {
        advertised
    }
}

The peer_handshake_mixed test still fails, because this saves an IPv6 address:

DEBUG grin_p2p::store - save_peer: PeerAddr(V6([::1]:5000)) marked Healthy

We would like it to save an IPv4 address (127.0.0.1:5000).

Should I add some checks into resolve_peer_addr to make sure the connection is coming from either localhost or an IPv4 mapped IPv6, and then translate it to IPv4?

[quentinlesceller]

Hi @dagonharett not sure if you are still around but I still think this would be very interesting to have this in Grin. I'll have a look again on your PR and see why tests are not passing.

[quentinlesceller]

I took the liberty to merge the current master into your branch @dagonharett in order to debug the problem. Hope that's fine 😄

[dagonharett]

Hi @quentinlesceller sorry for the delay on my reply. Yes, I am still around and willing to get this done.
Thanks for merging the current master. I'll give it another look later this week.
If nothing substantial was modified we will still face the same problem detailed in my previous comment: mismatch between IP version of Advertised and Connection when connection is coming from localhost or an IPv4 mapped IPv6.

[yeastplume]

Closed and marked as 'abandoned'. This does not mean the PR is unworthy. This means that merging it would require further work that nobody appears prepared to do at this time, or the original author has been unresponsive for a significant amount of time. Anyone who want to continue work on this functionality is free to re-open this at any time.