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

add eth2 key/value ENR to phase 0 p2p #1614

Merged
merged 5 commits into from
Mar 10, 2020
Merged

add eth2 key/value ENR to phase 0 p2p #1614

merged 5 commits into from
Mar 10, 2020

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Feb 10, 2020

[UPDATED]

  • Add genesis_validators_root to BeaconState and utilize for better signature domain separation as well as ENR/chain separation. This strengthens chain separation such that one must only be concerned about isolating fork version from a different chain when a contentious fork. Networks such as testnets and completely different chains with different genesis conditions can reuse fork versioning with no worry
  • add eth2 ENR entry that maps to 16-byte ENRForkID (current_fork_digest, next_fork_version, next_fork_epoch
  • add note on how clients should use this information to make connections.
    • One concern here is if a peer from a long-past fork version is trying to sync earlier blocks. In such a case, they would not be able to (easily) find peers.
    • On the surface this looks concerning, but in practice, this node would not be able to sync to the head and follow consensus anyway because their consensus rules are so out of date.
    • I am comfortable leaving as is, but open to further input.
  • add FAQ entry to disambiguate how fork Version is planned to be used when forking a network

@djrtwo
Copy link
Contributor Author

djrtwo commented Feb 10, 2020

cc @arnetheduck @AgeManning @nisdas

@AgeManning
Copy link
Contributor

The actual PR is generic, so looks fine to me.

  1. Seems to make sense to me. As all upgraded nodes that match a fork-version should also agree on all subsequent forks and are therefore compatible. The client maintains the canonical fork-version which should match the state after sync.

When doing status messages, which fork version should be sent (and which fork version is in the ENR), the one from the state or a client-maintained fork-version which may include next-fork, or am i missing the mechanics of this?

@protolambda
Copy link
Collaborator

From discussion earlier: how do we handle the pre-genesis case, where the root is not yet known? Default to 0x0000... and accept that too? Very stable ENRs long before genesis would be good to have. If there is one time to have lots of connections to bootnodes, and in advance to get connected and move away from the bootnode, then it is genesis.

@nisdas
Copy link
Contributor

nisdas commented Feb 16, 2020

The PR looks good to me , 1) seems to be the simplest and makes the most sense to me.

(1) is sufficient to avoid collisions unless a network does not bump their fork version on an upgrade and the previous version of the net keeps running.

This is a safe assumption, I do not know if there would ever be a situation where there is a network upgrade without updating the fork version.

From discussion earlier: how do we handle the pre-genesis case, where the root is not yet known? Default to 0x0000... and accept that too? Very stable ENRs long before genesis would be good to have. If there is one time to have lots of connections to bootnodes, and in advance to get connected and move away from the bootnode, then it is genesis.

For pre-genesis it would make sense to default with 0x0000... or something like

hash(0x000000...)[:4]

either one is fine with me

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 4, 2020

Made updates re: our conversation today. It isnt exactly what we discussed, but as I worked through, it seemed clean. I'm not yet set on this. Open to feedback

cc @arnetheduck @AgeManning @nisdas @protolambda

add genesis_validators_root for domain/chain separation
* `next_fork_epoch` is the epoch at which the next fork is planned and the `current_fork_version` will be updated. If no future fork is planned, set `next_fork_epoch = FAR_FUTURE_EPOCH` to signal this fact

Clients SHOULD connect to peers with `current_fork_digest`, `next_fork_version`, and `next_fork_epoch` that match local values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add a note that clients SHOULD connect to peers that have previous fork digests as their current fork digests ? Ex: Peers syncing from genesis/past epochs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been back and forth on this. I think the answer is no in the general case.

Having a different fork digest means that the current slot wrt wall time, you disagree on fundamental consensus rules (even if it is just the fork version). From the perspective of the ENR, I can't tell if the client wrt sync is in the past or up to the current slot. If they are in the past, I could presumable serve them some blocks to get them up to a certain point, but then we will fork. In such a case, it is better for them to find peers that are actually on their chain (fork digest) and sync all the way to the head rather than me connecting to them to see if I can help them part of the way.

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Some nitpicks, looks good.

specs/phase0/beacon-chain.md Outdated Show resolved Hide resolved
specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
specs/phase0/p2p-interface.md Outdated Show resolved Hide resolved
@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 10, 2020

Feedback addressed @protolambda

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

LGTM

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

4 participants