Skip to content

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

Merged
djrtwo merged 5 commits into
devfrom
eth2-enr
Mar 10, 2020
Merged

add eth2 key/value ENR to phase 0 p2p#1614
djrtwo merged 5 commits into
devfrom
eth2-enr

Conversation

@djrtwo

@djrtwo djrtwo commented Feb 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

[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

djrtwo commented Feb 10, 2020

Copy link
Copy Markdown
Contributor Author

cc @arnetheduck @AgeManning @nisdas

@AgeManning

AgeManning commented Feb 11, 2020

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

protolambda commented Feb 11, 2020

Copy link
Copy Markdown
Contributor

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

nisdas commented Feb 16, 2020

Copy link
Copy Markdown
Contributor

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

@arnetheduck arnetheduck mentioned this pull request Feb 17, 2020
Closed
@djrtwo

djrtwo commented Mar 4, 2020

Copy link
Copy Markdown
Contributor Author

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

@djrtwo djrtwo mentioned this pull request Mar 5, 2020
Merged
@djrtwo
Merge pull request #1638 from ethereum/domain-separation-by-root
b119f5f 
add genesis_validators_root for domain/chain separation
@djrtwo djrtwo requested a review from protolambda March 9, 2020 16:55
@djrtwo djrtwo mentioned this pull request Mar 9, 2020
Merged
nisdas
nisdas reviewed Mar 10, 2020
View reviewed changes
Comment thread specs/phase0/p2p-interface.md
* `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.

@nisdas nisdas Mar 10, 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.

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

@djrtwo djrtwo Mar 10, 2020

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.

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.

protolambda
protolambda reviewed Mar 10, 2020
View reviewed changes

@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.

Some nitpicks, looks good.

Comment thread specs/phase0/beacon-chain.md Outdated
Comment thread specs/phase0/p2p-interface.md Outdated
Comment thread specs/phase0/p2p-interface.md Outdated
@djrtwo

djrtwo commented Mar 10, 2020

Copy link
Copy Markdown
Contributor Author

Feedback addressed @protolambda

protolambda
protolambda approved these changes Mar 10, 2020
View reviewed changes

@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

@djrtwo djrtwo merged commit bd5231a into dev Mar 10, 2020
@djrtwo djrtwo deleted the eth2-enr branch March 10, 2020 19:02
@djrtwo djrtwo mentioned this pull request Mar 10, 2020
Merged
This was referenced Mar 17, 2020
Closed
Merged
This was referenced Mar 24, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@nisdas nisdas nisdas left review comments

@protolambda protolambda protolambda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

networking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@djrtwo @AgeManning @protolambda @nisdas