Skip to content

Gossip topics with fork digest#1652

Merged
djrtwo merged 9 commits into
devfrom
gossip-topics-with-fork-digest
Mar 11, 2020
Merged

Gossip topics with fork digest#1652
djrtwo merged 9 commits into
devfrom
gossip-topics-with-fork-digest

Conversation

@djrtwo

@djrtwo djrtwo commented Mar 10, 2020

Copy link
Copy Markdown
Contributor

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 February 17, 2020 10:03
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 March 10, 2020 20:43
@djrtwo djrtwo force-pushed the gossip-topics-with-fork-digest branch from 168a747 to 415544b Compare March 10, 2020 20:44

@protolambda protolambda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread specs/phase0/p2p-interface.md Outdated
…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 March 10, 2020 23:29

@protolambda protolambda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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.

@hwwhww hwwhww Mar 11, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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

Comment thread specs/phase0/beacon-chain.md Outdated
Comment thread specs/phase0/p2p-interface.md Outdated
Comment thread specs/phase0/p2p-interface.md
Comment thread specs/phase0/beacon-chain.md
Comment thread specs/phase0/p2p-interface.md Outdated
Comment thread specs/phase0/p2p-interface.md Outdated
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

@hwwhww hwwhww Mar 11, 2020

Copy link
Copy Markdown
Contributor

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 March 11, 2020 17:57
@djrtwo djrtwo force-pushed the gossip-topics-with-fork-digest branch from 924423c to 0881e21 Compare March 11, 2020 18:02
@djrtwo

djrtwo commented Mar 11, 2020

Copy link
Copy Markdown
Contributor Author

addressed comments @hwwhww and @arnetheduck

@hwwhww hwwhww left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some nitpicking.
LGTM 👍

Comment thread specs/phase0/beacon-chain.md Outdated
Comment thread specs/phase0/p2p-interface.md
Comment thread specs/phase0/validator.md Outdated
PR feedback

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

4 participants