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

Shift dual stack support to use two sockets instead of mapped addresses #160

Merged
merged 57 commits into from
May 16, 2023

Conversation

divagant-martian
Copy link
Collaborator

@divagant-martian divagant-martian commented Feb 19, 2023

Breaking changes

  • Removed the listen_socket parameter from Discv5::start().
  • Discv5ConfigBuilder::new() takes a ListenConfig for the sockets to listen on.
  • Discv5ConfigBuilder::ip_mode() has been removed. IpMode would be determined internally according to the ListenConfig parameter.
  • Discv5 and Discv5ConfigBuilder no longer implement Default since we can not determine a default value for ListenConfig.

Before

let config = Discv5ConfigBuilder::default()
    .ip_mode(discv5::IpMode::Ip4)
    .build();

let mut discv5: Discv5 = Discv5::new(enr, enr_key, config).unwrap();

let listen_socket = SocketAddr::new(Ipv4Addr::UNSPECIFIED.into(), port);

discv5.start(listen_socket).await.unwrap();

After

let listen_config = ListenConfig::Ipv4 {
    ip: Ipv4Addr::UNSPECIFIED,
    port,
};

let config = Discv5ConfigBuilder::new(listen_config).build();

let mut discv5: Discv5 = Discv5::new(enr, enr_key, config).unwrap();

discv5.start().await.unwrap();

bors bot pushed a commit to sigp/lighthouse that referenced this pull request Mar 14, 2023
## Issue Addressed
Add support for ipv6 and dual stack in lighthouse. 

## Proposed Changes
From an user perspective, now setting an ipv6 address, optionally configuring the ports should feel exactly the same as using an ipv4 address. If listening over both ipv4 and ipv6 then the user needs to:
- use the `--listen-address` two times (ipv4 and ipv6 addresses)
- `--port6` becomes then required
- `--discovery-port6` can now be used to additionally configure the ipv6 udp port

### Rough list of code changes
- Discovery:
  - Table filter and ip mode set to match the listening config. 
  - Ipv6 address, tcp port and udp port set in the ENR builder
  - Reported addresses now check which tcp port to give to libp2p
- LH Network Service:
  - Can listen over Ipv6, Ipv4, or both. This uses two sockets. Using mapped addresses is disabled from libp2p and it's the most compatible option.
- NetworkGlobals:
  - No longer stores udp port since was not used at all. Instead, stores the Ipv4 and Ipv6 TCP ports.
- NetworkConfig:
  - Update names to make it clear that previous udp and tcp ports in ENR were Ipv4
  - Add fields to configure Ipv6 udp and tcp ports in the ENR
  - Include advertised enr Ipv6 address.
  - Add type to model Listening address that's either Ipv4, Ipv6 or both. A listening address includes the ip, udp port and tcp port.
- UPnP:
  - Kept only for ipv4
- Cli flags:
  - `--listen-addresses` now can take up to two values
  - `--port` will apply to ipv4 or ipv6 if only one listening address is given. If two listening addresses are given it will apply only to Ipv4.
  - `--port6` New flag required when listening over ipv4 and ipv6 that applies exclusively to Ipv6.
  - `--discovery-port` will now apply to ipv4 and ipv6 if only one listening address is given.
  - `--discovery-port6` New flag to configure the individual udp port of ipv6 if listening over both ipv4 and ipv6.
  - `--enr-udp-port` Updated docs to specify that it only applies to ipv4. This is an old behaviour.
  - `--enr-udp6-port` Added to configure the enr udp6 field.
  - `--enr-tcp-port` Updated docs to specify that it only applies to ipv4. This is an old behaviour.
  - `--enr-tcp6-port` Added to configure the enr tcp6 field.
  - `--enr-addresses` now can take two values.
  - `--enr-match` updated behaviour.
- Common:
  - rename `unused_port` functions to specify that they are over ipv4.
  - add functions to get unused ports over ipv6.
- Testing binaries
  - Updated code to reflect network config changes and unused_port changes.

## Additional Info

TODOs:
- use two sockets in discovery. I'll get back to this and it's on sigp/discv5#160
- lcli allow listening over two sockets in generate_bootnodes_enr
- add at least one smoke flag for ipv6 (I have tested this and works for me)
- update the book
@ackintosh
Copy link
Member

ackintosh commented Apr 16, 2023

Breaking changes

  • Removed the listen_socket parameter from Discv5::start().
  • Discv5ConfigBuilder::new() takes a ListenConfig for the sockets to listen on.
  • Discv5ConfigBuilder::ip_mode() has been removed. IpMode would be determined internally according to the ListenConfig parameter.
  • Discv5 and Discv5ConfigBuilder no longer implement Default since we can not determine a default value for ListenConfig.

Before

let config = Discv5ConfigBuilder::default()
    .ip_mode(discv5::IpMode::Ip4)
    .build();

let mut discv5: Discv5 = Discv5::new(enr, enr_key, config).unwrap();

let listen_socket = SocketAddr::new(Ipv4Addr::UNSPECIFIED.into(), port);

discv5.start(listen_socket).await.unwrap();

After

let listen_config = ListenConfig::Ipv4 {
    ip: Ipv4Addr::UNSPECIFIED,
    port,
};

let config = Discv5ConfigBuilder::new(listen_config).build();

let mut discv5: Discv5 = Discv5::new(enr, enr_key, config).unwrap();

discv5.start().await.unwrap();

@ackintosh ackintosh mentioned this pull request Apr 26, 2023
@divagant-martian divagant-martian marked this pull request as ready for review May 4, 2023 22:00
@divagant-martian divagant-martian changed the title [WIP] two sockets for ipv6 Shift dual stack support to use two sockets instead of mapped addresses May 4, 2023
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

This looks good to me.

There were a few things I noticed, but they seem fine to me.

As we now support ipv6, we technically shouldn't be trying to send any messages to ipv6 nodes if we are listening only on an ipv4 socket. Pretty sure this is enforced when we check if the ENR is reachable based on the ip mode.

When we send a packet we check to see if we have an open socket of the correct type of the address. If we are not, we just emit a trace error, which would be hard to find. I would suggest raising errors of this form (where we try to send an ipv6 packet but fail because we only have an ipv4 socket, or we have no socket at all). The reason I dont think this is necessary, as I think its impossible for us to do so.

It might be worth having some debug asserts or something if anyone else is worried about this. These can be put in the socket/send.rs when we try sending the packets.

One other minor suggestion could be to add some user facing helping functions around ListenConfig which is now used by all users. Maybe we could implement a Default or like an ipv4_listen() function or something. Its just ergonomics tho, and building the variant as is done in the examples seems fine to me.

Otherwise, this all looks good to me.

Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Ready for some merging and some thorough testing?

src/socket/mod.rs Show resolved Hide resolved
src/socket/mod.rs Show resolved Hide resolved
src/socket/mod.rs Show resolved Hide resolved
src/socket/mod.rs Show resolved Hide resolved
divagant-martian and others added 4 commits May 16, 2023 14:18
Co-authored-by: Age Manning <Age@AgeManning.com>
Co-authored-by: Age Manning <Age@AgeManning.com>
Co-authored-by: Age Manning <Age@AgeManning.com>
Co-authored-by: Age Manning <Age@AgeManning.com>
@divagant-martian divagant-martian merged commit 269b7f0 into sigp:master May 16, 2023
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed
Add support for ipv6 and dual stack in lighthouse. 

## Proposed Changes
From an user perspective, now setting an ipv6 address, optionally configuring the ports should feel exactly the same as using an ipv4 address. If listening over both ipv4 and ipv6 then the user needs to:
- use the `--listen-address` two times (ipv4 and ipv6 addresses)
- `--port6` becomes then required
- `--discovery-port6` can now be used to additionally configure the ipv6 udp port

### Rough list of code changes
- Discovery:
  - Table filter and ip mode set to match the listening config. 
  - Ipv6 address, tcp port and udp port set in the ENR builder
  - Reported addresses now check which tcp port to give to libp2p
- LH Network Service:
  - Can listen over Ipv6, Ipv4, or both. This uses two sockets. Using mapped addresses is disabled from libp2p and it's the most compatible option.
- NetworkGlobals:
  - No longer stores udp port since was not used at all. Instead, stores the Ipv4 and Ipv6 TCP ports.
- NetworkConfig:
  - Update names to make it clear that previous udp and tcp ports in ENR were Ipv4
  - Add fields to configure Ipv6 udp and tcp ports in the ENR
  - Include advertised enr Ipv6 address.
  - Add type to model Listening address that's either Ipv4, Ipv6 or both. A listening address includes the ip, udp port and tcp port.
- UPnP:
  - Kept only for ipv4
- Cli flags:
  - `--listen-addresses` now can take up to two values
  - `--port` will apply to ipv4 or ipv6 if only one listening address is given. If two listening addresses are given it will apply only to Ipv4.
  - `--port6` New flag required when listening over ipv4 and ipv6 that applies exclusively to Ipv6.
  - `--discovery-port` will now apply to ipv4 and ipv6 if only one listening address is given.
  - `--discovery-port6` New flag to configure the individual udp port of ipv6 if listening over both ipv4 and ipv6.
  - `--enr-udp-port` Updated docs to specify that it only applies to ipv4. This is an old behaviour.
  - `--enr-udp6-port` Added to configure the enr udp6 field.
  - `--enr-tcp-port` Updated docs to specify that it only applies to ipv4. This is an old behaviour.
  - `--enr-tcp6-port` Added to configure the enr tcp6 field.
  - `--enr-addresses` now can take two values.
  - `--enr-match` updated behaviour.
- Common:
  - rename `unused_port` functions to specify that they are over ipv4.
  - add functions to get unused ports over ipv6.
- Testing binaries
  - Updated code to reflect network config changes and unused_port changes.

## Additional Info

TODOs:
- use two sockets in discovery. I'll get back to this and it's on sigp/discv5#160
- lcli allow listening over two sockets in generate_bootnodes_enr
- add at least one smoke flag for ipv6 (I have tested this and works for me)
- update the book
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