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

Gossip topics with fork digest #1652

Merged
merged 9 commits into from Mar 11, 2020
Merged

Gossip topics with fork digest #1652

merged 9 commits into from Mar 11, 2020

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Mar 10, 2020

Supersedes @arnetheduck's #1624 to instead use the ForkDigest (introduced in #1614) for gossip topic versioning.

Note, a 4-byte digest is used for domain separation in p2p (ENR, status, gossip topics). This should be sufficient for practical separation on this layer and induces low overhead in the ENR and status messages. In the signature domain, we use 28-bytes of the digest (along with 4-byte domain-type) for a cryptographically secure domain separation of signatures.

Also moves some helpers into state transition spec to increase code use between p2p and state transition spec

arnetheduck and others added 4 commits Feb 17, 2020
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.
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
@djrtwo djrtwo requested review from hwwhww and protolambda Mar 10, 2020
@djrtwo djrtwo force-pushed the gossip-topics-with-fork-digest branch from 168a747 to 415544b Compare Mar 10, 2020
Copy link
Collaborator

@protolambda protolambda left a comment

We should clarify what happens to the subnets backbone during a hardfork, nodes should randomly maintain a few future-fork topics as well.

specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
…ssip topic

Co-Authored-By: Diederik Loerakker <proto@protolambda.com>
@djrtwo djrtwo force-pushed the gossip-topics-with-fork-digest branch from ecc3060 to baee673 Compare Mar 10, 2020
Copy link
Collaborator

@protolambda protolambda left a comment

LGTM, good to see subnets have a double backbone during hardforks 👍

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/ForkDigest/Name/Encoding`. This defines both the type of data being sent on the topic and how the data field of the message is encoded.

- `ForkDigest` - the lowercase hex-encoded (no "0x" prefix) bytes of `ForkDigest(compute_fork_data_root(current_fork_version, genesis_validators_root)[:4])` where
- `current_fork_version` is the fork version of the epoch of the message to be sent on the topic
Copy link
Contributor

@arnetheduck arnetheduck Mar 11, 2020

Choose a reason for hiding this comment

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

What about slashings though? they span 2 epochs, potentially. We need to send to both, or have everyone stay on multiple slashing topics for as long as slashing is possible.

We can also leave it unspecified, noting that only one fork version exists for now, and that the hard fork will decide the upgrade procedure.

Copy link
Contributor

@hwwhww hwwhww Mar 11, 2020

Choose a reason for hiding this comment

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

@arnetheduck I asked about what will happen if slashing messages formats are different cross forks: #1475
It seems the status-quo is to believe in the honest majority.

Copy link
Contributor Author

@djrtwo djrtwo Mar 11, 2020

Choose a reason for hiding this comment

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

We can make a table of messages and related field that dictates the epoch/gossipchannel.

As for cross-fork slashings, it is a more fundamental problem than just which channel to send it on. To catch slashings across a fork boundary in which the data-type changed would require specific data structures to handle mismatched types and separate functions for verifying validity of each. As discussed in #1475, I'm happy to sweep that under the rug in early phases and address it if/when we need to.

When we upgrade to Phase 1, we'll make it clear that fraud proofs across the boundary are not accepted and that this channel upgrade path only supports message types from the respective fork

specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
specs/phase0/beacon-chain.md Show resolved Hide resolved
specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
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/ForkDigest/Name/Encoding`. This defines both the type of data being sent on the topic and how the data field of the message is encoded.

- `ForkDigest` - the lowercase hex-encoded (no "0x" prefix) bytes of `ForkDigest(compute_fork_data_root(current_fork_version, genesis_validators_root)[:4])` where
- `current_fork_version` is the fork version of the epoch of the message to be sent on the topic
Copy link
Contributor

@hwwhww hwwhww Mar 11, 2020

Choose a reason for hiding this comment

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

@arnetheduck I asked about what will happen if slashing messages formats are different cross forks: #1475
It seems the status-quo is to believe in the honest majority.

@djrtwo djrtwo force-pushed the gossip-topics-with-fork-digest branch 2 times, most recently from def1ad5 to 924423c Compare Mar 11, 2020
@djrtwo djrtwo force-pushed the gossip-topics-with-fork-digest branch from 924423c to 0881e21 Compare Mar 11, 2020
@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 11, 2020

addressed comments @hwwhww and @arnetheduck

hwwhww
hwwhww approved these changes Mar 11, 2020
Copy link
Contributor

@hwwhww hwwhww left a comment

Some nitpicking.
LGTM 👍

specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase0/p2p-interface.md Show resolved Hide resolved
specs/phase0/validator.md Outdated Show resolved Hide resolved
PR feedback

Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
@djrtwo djrtwo merged commit 81dc71c into dev Mar 11, 2020
10 checks passed
@djrtwo djrtwo deleted the gossip-topics-with-fork-digest branch Mar 11, 2020
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.

None yet

4 participants