consensus-specs icon indicating copy to clipboard operation
consensus-specs copied to clipboard

`p2p-interface.md`: Add `quic` ENR entry.

Open nalepae opened this issue 1 year ago • 11 comments

This ENR entry seems already used by Lighthouse BN. Exemple found on the Holesky network:

enr:-MS4QEyTlybz9KMHKN7pXHOvlD8Q1muUXN7mbeUCPjSyKOEYScKDpmkaxxuE8DOC-YlCIqweCQpEw8uLG4s4z0huDAEUh2F0dG5ldHOIAAAAAAAGAACEZXRoMpBprg6ZBQFwAP__________gmlkgnY0gmlwhIjzqrKEcXVpY4IjKYlzZWNwMjU2azGhA-tFvPZaDfibme3y3o9xderqssFhY1Pnjw6AwqwreQz7iHN5bmNuZXRzAIN0Y3CCIyiDdWRwgiMo

cc @michaelsproul

image

nalepae avatar Apr 02 '24 11:04 nalepae

libp2p has several quic implementations of varying quality - which one is this? if anything, we should be using the standards-compliant one, ie quic-v1: https://docs.libp2p.io/concepts/transports/quic/#distinguishing-multiple-quic-versions-in-libp2p

but this has not yet gone through a standardisation / testing process on the ethereum side (ie afaik this is an LH experiment) - I'm not sure adding an entry here is the right end to start that process

arnetheduck avatar Apr 02 '24 12:04 arnetheduck

As far as I know, LH uses quic-v1 (multiaddr looking like /ip4/192.0.2.0/udp/65432/quic-v1/). I agree a quic-v1 entry would be more idiomatic than quic.

To add a little bit of context to this present pull request, I'm implementing the QUIC (V1) support in Prysm: https://github.com/prysmaticlabs/prysm/pull/13786

With this linked PR, Prysm BN is able to connect to other Prysm BN and LH BN with both QUIC (V1) and TCP. QUIC (V1) is favored if both are available. As far as I know, only LH, and with the linked PR, Prysm, do support QUIC (V1).

The linked PR also adds the quic ENR entry in Prysm BN, as LH does. Because this entry is not currently standardised, I think it is a good time to start such a process.

nalepae avatar Apr 02 '24 12:04 nalepae

Yep. I added this field in lighthouse.

It was originally a test amongst all Lighthouse peers because other clients didn't support quic. We wanted to test to see how well the transport works.

We didn't bother standardising it, as it is likely involved in a larger conversation around which kind of quic and who wants to support it etc as @arnetheduck has suggested.

However if others want to test along with us, I have no objections to adding it to the specs. I'm personally fine with the quic key, provided we also standardise the exact version we are using. Also happy to change to quic-v1 if others desire.

AgeManning avatar Apr 03 '24 00:04 AgeManning

It was originally a test amongst all Lighthouse peers because other clients didn't support quic. We wanted to test to see how well the transport works.

Nice! When this PR is merged and the --enable-quic feature flag is used, then Prysm BN is able to other peers via QUIC (as long as the quic entry is in the peer's ENR and the peer does support quic-v1 protocol.)

This PR has been successfully tested with multiple inbound/outbound (LH) peers.

About the ENR entry itself, I would vote for quic-v1.

Pros: The currently used TCP multiaddr has this shape: /ip4/94.16.114.102/tcp/13000/p2p/16Uiu2HAkv4gec8oWzbk89naQ7dymAM7QYDvy6ZnzuC7XXChULiQt

  • The substring /tcp/ is included into the multiaddr.
  • tcp entry is used in the ENR.

Using quic-v1 ENR entry would achieve the same result: The currently used QUIC multiaddr has this shape: /ip4/94.16.114.102/udp/13000/quic-v1/p2p/16Uiu2HAkv4gec8oWzbk89naQ7dymAM7QYDvy6ZnzuC7XXChULiQt

  • The substring /quic-v1/ is included into the multiaddr.
  • Using quic-v1 entry in the ENR instead of quic will be more consistent.

Also, using the quic-v1 entry indicates that RFC 9000 is used and not draft-29. (See Distinguishing multiple QUIC versions in libp2p

Cons:

  • Using quic-v1 ENR entry instead of quic adds some extra work for LH.

nalepae avatar Apr 04 '24 10:04 nalepae

Sure. I'm happy to switch to the quic-v1 ENR field. Logic seems reasonable to me.

I guess up until the next hard fork we will also support the quic field and then deprecate it after the next fork.

AgeManning avatar Apr 07 '24 23:04 AgeManning

For reference @jxs linked me some research done last year into the different implementations and ultimately why we went with Quinn. Might be useful for others:

https://github.com/libp2p/rust-libp2p/issues/2883#issuecomment-1547189526

AgeManning avatar Apr 07 '24 23:04 AgeManning

It was originally a test amongst all Lighthouse peers because other clients didn't support quic. We wanted to test to see how well the transport works.

Nice! When this PR is merged and the --enable-quic feature flag is used, then Prysm BN is able to other peers via QUIC (as long as the quic entry is in the peer's ENR and the peer does support quic-v1 protocol.)

This PR has been successfully tested with multiple inbound/outbound (LH) peers.

About the ENR entry itself, I would vote for quic-v1.

Pros: The currently used TCP multiaddr has this shape: /ip4/94.16.114.102/tcp/13000/p2p/16Uiu2HAkv4gec8oWzbk89naQ7dymAM7QYDvy6ZnzuC7XXChULiQt

  • The substring /tcp/ is included into the multiaddr.
  • tcp entry is used in the ENR.

Using quic-v1 ENR entry would achieve the same result: The currently used QUIC multiaddr has this shape: /ip4/94.16.114.102/udp/13000/quic-v1/p2p/16Uiu2HAkv4gec8oWzbk89naQ7dymAM7QYDvy6ZnzuC7XXChULiQt

  • The substring /quic-v1/ is included into the multiaddr.
  • Using quic-v1 entry in the ENR instead of quic will be more consistent.

Also, using the quic-v1 entry indicates that RFC 9000 is used and not draft-29. (See Distinguishing multiple QUIC versions in libp2p

Cons:

  • Using quic-v1 ENR entry instead of quic adds some extra work for LH.

there's also the issue with the ipv6 fields, what could be the ipv6 quic ENR field? Both quic6-v1 or quic-v1-6 seem odd, OTOH tcp6 already doesn't have equivalence with its multiaddress counterpart

jxs avatar Apr 08 '24 12:04 jxs

RE the field name, one thing to keep in mind is that ENRs are limited by the spec to 300 bytes when encoded. So making longer field names isn't free. It eats into space that may be used by future fields.

Using quic-v1 instead of quic uses an additional 1% of the total available size.

As far as mapping the field name to the multiaddr prefix, I don't think this needs to be preserved. As mentioned above, we already don't have equivalence there. And keep in mind that the multiaddr string prefixes are just human-readable representations of a varint code. We have different needs here (field names are serialized directly, limited size) so we likely shouldn't be choosing field names based on entirely on human-readability.

My opinion would be to use a smaller field name, all else being the same. The meaning of the field will be handled by standards, this PR or an EIP, so ambiguity shouldn't be a problem for conforming implementations.

wemeetagain avatar Apr 18 '24 15:04 wemeetagain

RE the field name, one thing to keep in mind is that ENRs are limited by the spec to 300 bytes when encoded. So making longer field names isn't free. It eats into space that may be used by future fields.

Not related to this topic, but EIP-7594 introduces a new custody_subnet_count ENR field, which is the longest we have so far:

image

According to your comment, it may be interesting to rename it.

nalepae avatar May 03 '24 08:05 nalepae

Let's stick with quic . We can reduce it to qui if needed.

nalepae avatar Jun 26 '24 12:06 nalepae

I agree with quic. I think it's short enough already. I also agree that we don't need to make it consistent with libp2p multiaddr.

Another point I want to make is that I think Discv5 and ENR are considered independent from libp2p so I think it makes sense that we don't need to make them consistent.

ppopth avatar Jun 27 '24 12:06 ppopth

@hwwhww @jtraglia I think this is ready to be merged.

ppopth avatar Dec 02 '24 03:12 ppopth