P2P: Add QUIC support #13786

nalepae · 2024-03-21T17:13:19Z

What type of PR is this?
Feature

What does this PR do? Why is it needed?
This pull request adds the QUIC protocol support. For more information about the QUIC protocol, and how it is supported into libp2p, please checkout here.

This PR adds a new --p2p-quic-port (default: 13000) flag.

--p2p-quic-port value The QUIC port used by libp2p. (default: 13000)

This new feature is hidden behind the --enable-quic feature flag:

features OPTIONS:
   ...
   --enable-quic                             Enables connection using the QUIC protocol for peers which support it. (default: false)
   ...

P2P ports are now:

ALREADY EXISTING:
--p2p-udp-port value   The UDP port used by the discovery service discv5. (default: 12000)
--p2p-tcp-port value   The TCP port used by libp2p. (default: 13000)

NEW:
--p2p-quic-port value  The QUIC port used by libp2p. (default: 13000)

Now, port 13000 is used as both default port for libp2p TCP and QUIC(UDP).

When starting the beacon node, the following log is displayed:

INFO p2p: Started discovery v5 ENR=enr:-Mq4QMhhoVSzFFpYC3zDKE24nQndTXw8lXtReLAqd1ddTGcHEwnzBgcKeASZyjtzogf7h_Zluie8CNhBdfjNOzycCyaGAY6AcM_qh2F0dG5ldHOIAAAAAAAAAACEZXRoMpBprg6ZBQFwAP__________gmlkgnY0gmlwhF4QcmaEcXVpY4IyyIlzZWNwMjU2azGhAxaMhBX-x4-CvNk5PkOUEcuv8cCFVh9UnscPj--TtQpLiHN5bmNuZXRzAIN0Y3CCMsiDdWRwgi7g

The corresponding decoded ENR is the following:
(Note the new quic entry.)

When starting a new node, the following logs are displayed:

ALREADY EXISTING:
INFO p2p: Node started p2p server multiAddr=/ip4/172.18.0.3/tcp/13000/p2p/16Uiu2HAmEB1bM8P23R5Ptx9XU8bFMvh3hvNPqnSdkQWnsrGXzCdp

NEW:
INFO p2p: Node started p2p server multiAddr=/ip4/172.18.0.3/udp/13000/quic-v1/p2p/16Uiu2HAmEB1bM8P23R5Ptx9XU8bFMvh3hvNPqnSdkQWnsrGXzCdp

If using the --p2p-host-ip flag:

ALREADY EXISTING:
INFO p2p: Node started external p2p server multiAddr=/ip4/94.16.114.102/tcp/13000/p2p/16Uiu2HAmEB1bM8P23R5Ptx9XU8bFMvh3hvNPqnSdkQWnsrGXzCdp

NEW:
INFO p2p: Node started external p2p server multiAddr=/ip4/94.16.114.102/udp/13000/quic-v1/p2p/16Uiu2HAmEB1bM8P23R5Ptx9XU8bFMvh3hvNPqnSdkQWnsrGXzCdp

Every minute, the following log is displayed, showing the number of inbound/outbound TCP/QUIC peers.

INFO p2p: Connected peers inboundQUIC=10 inboundTCP=3 outboundQUIC=3 outboundTCP=57 total=73

Below is represented an extract of the beacon API call to <ip-address>:3500/eth/v1/beacon/genesis:

Only four items are retained in the extract:

1 TCP / Outbound
1 TCP / Inbound
1 QUIC / Outbound
1 QUIC / Inbound

Outbound / Inbound is represented in the direction field.
TCP / QUIC is represented in the last_seen_p2p_address field.

{
  "data": [
...
    {
      "peer_id": "16Uiu2HAkxVmsQQtNCvgADvdEFZUw1hq6rVBxZ3hb4bNt7jDYJAgp",
      "enr": "enr:-Ly4QM65O2LrfDEEmk-eH08SbTgD4c-TqJduC4-Q2CqE1UwpDpDewwwUz0oRV7lwC6_NlEWiOMaQoHNJJOPt98X8lhsWh2F0dG5ldHOIAAAAAAAAgAGEZXRoMpBprg6ZBQFwAP__________gmlkgnY0gmlwhMIhKIyJc2VjcDI1NmsxoQItoAsu0kuhTEY5ewhZLhu83v0zSnoin7O_p4U6_ULO4YhzeW5jbmV0cwSDdGNwgiMsg3VkcIIjLA",
      "last_seen_p2p_address": "/ip4/194.33.40.140/tcp/9004",
      "state": "connected",
      "direction": "outbound"
    },
...
    {
      "peer_id": "16Uiu2HAm3GPSjdiaAkCbr1qafWAojzepodknQCoZfFbR5k5b92LL",
      "enr": "",
      "last_seen_p2p_address": "/ip4/168.119.10.116/tcp/16741",
      "state": "connected",
      "direction": "inbound"
    },
...
    {
      "peer_id": "16Uiu2HAmFiyzWcJedxXkeca5btmqPD7MYKsuUmKkd1RzaVfZX5KH",
      "enr": "enr:-MW4QGIXCYuk2GRAkumwtfHj_NhEYZRB0W_qO19jgmZMK7vqX9zPEANC05iLtu5dB22asvlJCg8hKSDMvqKCv580ds6ByYdhdHRuZXRziAAAAAAAABgAhGV0aDKQaa4OmQUBcAD__________4JpZIJ2NIJpcIRKMkPehHF1aWOCIymJc2VjcDI1NmsxoQMtmPqbx98SSU8z8FXOpdD1hesW3rvogahgJpn9jUGjpIhzeW5jbmV0cwCDdGNwgiMog3VkcIIjKA",
      "last_seen_p2p_address": "/ip4/74.50.67.222/udp/9001/quic-v1",
      "state": "connected",
      "direction": "outbound"
    },
...
    {
      "peer_id": "16Uiu2HAkyiEJirC3Efo7NcAnHhVqZVREDWGiCRMUtE6FN36ARC2V",
      "enr": "",
      "last_seen_p2p_address": "/ip4/80.249.120.20/udp/9001/quic-v1",
      "state": "connected",
      "direction": "inbound"
    },
...
  ]
}

Decoded ENR of the TCP/Outbound peer is represented as:
(No quic entry in present the ENR.)

Decoded ENR of the QUIC/Outbound peer is represented as:
(The quic entry is present in the ENR.)

Note:
When an outbound peer supports both the TCP and the QUIC protocols, then Prysm favors the QUIC protocol.

nisdas · 2024-04-03T07:32:51Z

cmd/flags.go

 		Value: 12000,
 	}
-	// P2PTCPPort defines the port to be used by libp2p.
+	// P2PQUICPort defines the QUIC port to be used by libp2p.
+	P2PQUICPort = &cli.IntFlag{


Along with this flag, is it possible to also add a feature flag enable-quic ? We do not want QUIC to immediately be the default as if there are issues with other Lighthouse nodes and their QUIC implementation it could lead to issues for node operators.

Fixed in c69efdb.

nisdas · 2024-04-03T07:35:22Z

beacon-chain/p2p/peers/status.go

@@ -449,6 +450,32 @@ func (p *Status) InboundConnected() []peer.ID {
 	return peers
 }

+// InboundConnectedTCP returns the current batch of inbound peers that are connected using TCP.
+func (p *Status) InboundConnectedTCP() []peer.ID {


Instead of having separate methods for different protocols, how about:

func (p *Status) InboundConnected(protocol string) []peer.ID

that way, you don't have to duplicate the implementation.

Fixed in f42455b.

nisdas · 2024-04-03T07:35:45Z

beacon-chain/p2p/peers/status.go

@@ -475,7 +502,33 @@ func (p *Status) OutboundConnected() []peer.ID {
 	return peers
 }

-// Active returns the peers that are connecting or connected.
+// OutboundConnected returns the current batch of outbound peers that are connected using TCP.
+func (p *Status) OutboundConnectedTCP() []peer.ID {


Fixed in f42455b.

nisdas · 2024-04-04T04:12:02Z

testing/endtoend/components/beacon_node.go

@@ -257,6 +257,7 @@ func (node *BeaconNode) Start(ctx context.Context) error {
 		fmt.Sprintf("--%s=%s", flags.ExecutionJWTSecretFlag.Name, jwtPath),
 		fmt.Sprintf("--%s=%d", flags.MinSyncPeers.Name, 1),
 		fmt.Sprintf("--%s=%d", cmdshared.P2PUDPPort.Name, e2e.TestParams.Ports.PrysmBeaconNodeUDPPort+index),
+		fmt.Sprintf("--%s=%d", cmdshared.P2PQUICPort.Name, e2e.TestParams.Ports.PrysmBeaconNodeQUICPort+index),


We should also add the enable-quic flag here, so that every prysm node is running with it enabled in e2e

Fixed in 1f0d84d.

nisdas · 2024-04-04T04:12:38Z

config/features/flags.go

@@ -229,6 +234,7 @@ var BeaconChainFlags = append(deprecatedBeaconFlags, append(deprecatedFlags, []c
 	DisableRegistrationCache,
 	EnableLightClient,
 	BlobSaveFsync,
+	EnableQUIC,


can you add this to our dev flags too ? So that any user running with --devis also running this

Fixed in 1f0d84d.

nisdas · 2024-04-04T04:28:14Z

beacon-chain/p2p/discovery.go

+// If the node has a both a QUIC and a TCP port set in their ENR, then
+// the multiaddr corresponding to the QUIC port is added first, followed
+// by the multiaddr corresponding to the TCP port.
+func convertToMultiAddrs(node *enode.Node) ([]ma.Multiaddr, error) {


why was the name changed here ? its easy to get this mixed up with convertToMultiAddr . The previous name emphasized the singular nature

Nvm, I do understand why singular was removed but its easy to confuse it with convertToMultiAddr , another name would be better

Fixed in a0c0326.

nisdas · 2024-04-04T04:46:48Z

beacon-chain/p2p/discovery.go

+		return nil, nil, errors.Wrapf(err, "could not convert to peer info: %v", multiAddrs)
+	}
+
+	if len(infos) > 1 {


This seems wrong, what happens if we have 2 multiaddrs returned from convertToMultiAddrs . This error would always be triggered.

peer.AddrInfosFromP2pAddrs groups input maddrs by node.

If multiaddrs contains 5 multiaddrs, but belonging to only 2 nodes, (for example 3 multiaddrs for one node and 2 multiaddrs for the other node), then len(infos) == 2 and:

len(infos[0].Addrs) == 3

len(infos[1].Addrs) == 2

For information, this is the code of AddrInfosFromP2PAddrs:

// AddrInfosFromP2pAddrs converts a set of Multiaddrs to a set of AddrInfos. func AddrInfosFromP2pAddrs(maddrs ...ma.Multiaddr) ([]AddrInfo, error) { m := make(map[ID][]ma.Multiaddr) for _, maddr := range maddrs { transport, id := SplitAddr(maddr) if id == "" { return nil, ErrInvalidAddr } if transport == nil { if _, ok := m[id]; !ok { m[id] = nil } } else { m[id] = append(m[id], transport) } } ais := make([]AddrInfo, 0, len(m)) for id, maddrs := range m { ais = append(ais, AddrInfo{ID: id, Addrs: maddrs}) } return ais, nil }

In our case, all the multiaddrs come from the same node, so we expect at most 1 item in infos.

Ok in that case, if we do get a value not equal to 1, we should simply return an error. Otherwise the caller will assume the address info is valid and proceed to use it

I also added a check in func (s *Service) filterPeer(node *enode.Node) bool.

In this function, even if the only way to have peerData == nil is to have at the same time multiAddrs == nil, it's better to check peerData nilness explicitely.

Fixed in e9d80e2.

nisdas · 2024-04-04T04:47:10Z

beacon-chain/p2p/discovery.go

+		return nil, nil, errors.Errorf("infos contains %v elements, expected not more than 1", len(infos))
+	}
+
+	if len(infos) == 0 {


if we get no info, then we should return an error imo.

the caller will expect the addr info not to be nil and will end up panicking here

See comment about if len(infos) > 1.

Fixed in e9d80e2.

nisdas

Great Job ! Looks great to me

* (Unrelated) DoppelGanger: Improve message. * `beacon-blocks-by-range`: Add `--network` option. * `ensurePeerConnections`: Remove capital letter in error message. * `MultiAddressBuilder{WithID}`: Refactor. * `buildOptions`: Improve log. * `NewService`: Bubbles up errors. * `tcp` ==> `libp2ptcp` * `multiAddressBuilderWithID`: Add the ability to build QUIC multiaddr * `p2p Start`: Fix error message. * `p2p`: Add QUIC support. * Status: Implement `{Inbound,Outbound}Connected{TCP,QUIC}`. * Logging: Display the number of TCP/QUIC connected peers. * P2P: Implement `{Inbound,Outbound}ConnectedWithProtocol`. * Hide QUIC protocol behind the `--enable-quic` feature flag. * `e2e`: Add `--enable-quic` flag. * Add `--enable-quic` in `devModeFlag`. * `convertToMultiAddrs` ==> `retrieveMultiAddrsFromNode`. * `convertToAddrInfo`: Ensure `len(infos) == 1`.

nalepae force-pushed the quic branch 11 times, most recently from 02cdb59 to c3ccf8d Compare April 2, 2024 09:36

nalepae marked this pull request as ready for review April 2, 2024 11:09

nalepae requested a review from a team as a code owner April 2, 2024 11:09

nalepae requested review from rauljordan, nisdas and james-prysm April 2, 2024 11:09

nalepae mentioned this pull request Apr 2, 2024

p2p-interface.md: Add quic ENR entry. ethereum/consensus-specs#3644

Merged

nisdas reviewed Apr 3, 2024

View reviewed changes

nalepae added 12 commits April 3, 2024 10:34

(Unrelated) DoppelGanger: Improve message.

933ad98

beacon-blocks-by-range: Add --network option.

628633c

ensurePeerConnections: Remove capital letter in error message.

9a365f9

MultiAddressBuilder{WithID}: Refactor.

90b5cea

buildOptions: Improve log.

8d4eaa2

NewService: Bubbles up errors.

841f3d7

tcp ==> libp2ptcp

feb2b6f

multiAddressBuilderWithID: Add the ability to build QUIC multiaddr

b4bd51a

p2p Start: Fix error message.

924db98

p2p: Add QUIC support.

b4ae4fb

Status: Implement {Inbound,Outbound}Connected{TCP,QUIC}.

236579d

Logging: Display the number of TCP/QUIC connected peers.

ea9dcc8

nalepae added 2 commits April 3, 2024 11:37

P2P: Implement {Inbound,Outbound}ConnectedWithProtocol.

f42455b

Hide QUIC protocol behind the --enable-quic feature flag.

c69efdb

nalepae force-pushed the quic branch from c3ccf8d to c69efdb Compare April 3, 2024 11:27

nisdas reviewed Apr 4, 2024

View reviewed changes

nalepae added 4 commits April 4, 2024 10:38

e2e: Add --enable-quic flag.

1f0d84d

Add --enable-quic in devModeFlag.

a26fb88

convertToMultiAddrs ==> retrieveMultiAddrsFromNode.

a0c0326

convertToAddrInfo: Ensure len(infos) == 1.

e9d80e2

nisdas approved these changes Apr 4, 2024

View reviewed changes

nisdas enabled auto-merge April 4, 2024 09:33

nisdas added this pull request to the merge queue Apr 4, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 4, 2024

nalepae added this pull request to the merge queue Apr 4, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 4, 2024

nisdas added this pull request to the merge queue Apr 4, 2024

Merged via the queue into develop with commit be1bfcc Apr 4, 2024
16 of 17 checks passed

nisdas deleted the quic branch April 4, 2024 12:51

nisdas mentioned this pull request Apr 12, 2024

Revert Peer Log Changes #13872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P2P: Add QUIC support #13786

P2P: Add QUIC support #13786

nalepae commented Mar 21, 2024 •

edited

Loading

nisdas Apr 3, 2024

nalepae Apr 3, 2024

nisdas Apr 3, 2024

nalepae Apr 3, 2024

nisdas Apr 3, 2024

nalepae Apr 3, 2024

nisdas Apr 4, 2024

nalepae Apr 4, 2024

nisdas Apr 4, 2024

nalepae Apr 4, 2024

nisdas Apr 4, 2024

nisdas Apr 4, 2024

nalepae Apr 4, 2024

nisdas Apr 4, 2024

nalepae Apr 4, 2024 •

edited

Loading

nisdas Apr 4, 2024

nalepae Apr 4, 2024

nalepae Apr 4, 2024

nisdas Apr 4, 2024

nisdas Apr 4, 2024

nalepae Apr 4, 2024

nalepae Apr 4, 2024

nisdas left a comment

P2P: Add QUIC support #13786

P2P: Add QUIC support #13786

Conversation

nalepae commented Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalepae Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas left a comment

Choose a reason for hiding this comment

nalepae commented Mar 21, 2024 •

edited

Loading

nalepae Apr 4, 2024 •

edited

Loading