Add fork version to topic#1624
Closed
arnetheduck wants to merge 3 commits into
Closed
Conversation
Gossipsub peers are separate from the ETH2 RPC protocol, and thus cannot rely on the application-level `Status` negotiation to establish if they're on the same network. Segregating gossipsub topics by fork version decouples RPC from gossip further and allows peers to more easily listen only to the traffic of the network they're interested in, without further negotiation.
djrtwo
reviewed
Feb 19, 2020
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
djrtwo
reviewed
Mar 6, 2020
| Topics are plain UTF-8 strings and are encoded on the wire as determined by protobuf (gossipsub messages are enveloped in protobuf messages). Topic strings have form: `/eth2/TopicName/TopicEncoding`. This defines both the type of data being sent on the topic and how the data field of the message is encoded. | ||
| Topics are plain UTF-8 strings and are encoded on the wire as determined by protobuf (gossipsub messages are enveloped in protobuf messages). Topic strings have form: `/eth2/ForkVersion/Name/Encoding`. This defines both the type of data being sent on the topic and how the data field of the message is encoded. | ||
|
|
||
| - `ForkVersion` - the hex-encoded bytes from `state.fork.current_version` of the head state of the client, as also seen in `Status.head_fork_version`. |
Contributor
There was a problem hiding this comment.
Might make sense to rephrase this in terms of fork version of the epoch of the relevant message, rather than that of the head state.
Because we might prep for and listen to multiple fork version topics of the same message type at once, it isn't strictly related to the head state.
djrtwo
reviewed
Mar 6, 2020
| Topics are plain UTF-8 strings and are encoded on the wire as determined by protobuf (gossipsub messages are enveloped in protobuf messages). Topic strings have form: `/eth2/TopicName/TopicEncoding`. This defines both the type of data being sent on the topic and how the data field of the message is encoded. | ||
| Topics are plain UTF-8 strings and are encoded on the wire as determined by protobuf (gossipsub messages are enveloped in protobuf messages). Topic strings have form: `/eth2/ForkVersion/Name/Encoding`. This defines both the type of data being sent on the topic and how the data field of the message is encoded. | ||
|
|
||
| - `ForkVersion` - the hex-encoded bytes from `state.fork.current_version` of the head state of the client, as also seen in `Status.head_fork_version`. |
Contributor
There was a problem hiding this comment.
Maybe something like:
Suggested change
| - `ForkVersion` - the hex-encoded bytes from `state.fork.current_version` of the head state of the client, as also seen in `Status.head_fork_version`. | |
| - `ForkVersion` - the hex-encoded bytes of the `fork_version` of the epoch of the related messages to be sent on the topic |
djrtwo
reviewed
Mar 6, 2020
|
|
||
| The fork version is hex-encoded using the following scheme: | ||
| ```python | ||
| ForkVersion = ''.join('{:02x}'.format(x) for x in state.fork.current_version) |
Contributor
There was a problem hiding this comment.
Suggested change
| ForkVersion = ''.join('{:02x}'.format(x) for x in state.fork.current_version) | |
| ForkVersion = ''.join('{:02x}'.format(x) for x in fork_version) |
djrtwo
reviewed
Mar 6, 2020
djrtwo
left a comment
Contributor
There was a problem hiding this comment.
Suggest making the topic fork version be specified wrt epoch of message. Otherwise, this looks good!
hwwhww
reviewed
Mar 10, 2020
| The payload is carried in the `data` field of a gossipsub message, and varies depending on the topic: | ||
|
|
||
| | Topic | Message Type | | ||
| | Name | Message Type | |
Contributor
There was a problem hiding this comment.
It seems we are still using TopicName to describe the field in the following. (Section Global topics and Attestation subnets)
Contributor
|
closing in favor of #1652 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gossipsub peers are separate from the ETH2 RPC protocol, and thus cannot
rely on the application-level
Statusnegotiation to establish ifthey're on the same network.
Segregating gossipsub topics by fork version decouples RPC from gossip
further and allows peers to more easily listen only to the traffic of
the network they're interested in, without further negotiation.
this is a work-in-progress complement to #1614 - in particular, it's an open question which fork version should be used during upgrades - most natural seems to be to use the head fork version - or more accurately perhaps, the fork version as it would be when time has advanced to where the fork has taken effect.