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

p2p-interface.md: Add quic ENR entry. #3644

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

nalepae
Copy link
Contributor

@nalepae nalepae commented Apr 2, 2024

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

enr:-MS4QEyTlybz9KMHKN7pXHOvlD8Q1muUXN7mbeUCPjSyKOEYScKDpmkaxxuE8DOC-YlCIqweCQpEw8uLG4s4z0huDAEUh2F0dG5ldHOIAAAAAAAGAACEZXRoMpBprg6ZBQFwAP__________gmlkgnY0gmlwhIjzqrKEcXVpY4IjKYlzZWNwMjU2azGhA-tFvPZaDfibme3y3o9xderqssFhY1Pnjw6AwqwreQz7iHN5bmNuZXRzAIN0Y3CCIyiDdWRwgiMo

cc @michaelsproul

image

@arnetheduck
Copy link
Contributor

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

@nalepae
Copy link
Contributor Author

nalepae commented Apr 2, 2024

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: prysmaticlabs/prysm#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.

@AgeManning
Copy link
Contributor

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.

@nalepae
Copy link
Contributor Author

nalepae commented Apr 4, 2024

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.

@AgeManning
Copy link
Contributor

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
Copy link
Contributor

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:

libp2p/rust-libp2p#2883 (comment)

@jxs
Copy link

jxs commented Apr 8, 2024

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

@wemeetagain
Copy link
Contributor

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.

@nalepae
Copy link
Contributor Author

nalepae commented May 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants