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 ENRForkID to ENR eth2 #843

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Add ENRForkID to ENR eth2 #843

merged 1 commit into from
Apr 28, 2020

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Apr 27, 2020

Goal

TODOs work for discv 5:

  • Clients SHOULD connect to peers with fork_digest, next_fork_version, and next_fork_epoch that match local values.
  • Clients MAY connect to peers with the same fork_digest but a different next_fork_version/next_fork_epoch. Unless ENRForkID is manually updated to matching prior to the earlier next_fork_epoch of the two clients, these connecting clients will be unable to successfully interact starting at the earlier next_fork_epoch

@@ -70,4 +70,17 @@ export interface IBeaconParams {
MAX_ATTESTATIONS: number;
MAX_DEPOSITS: number;
MAX_VOLUNTARY_EXITS: number;

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 define ALL_FORKS here and load it from yaml file in lodestar-cli, is it ok? I want it to be loaded in 1 single space and be able to process it in multiple modules in config.params.ALL_FORKS. Maybe for historical hard forks we want to store it in database, just load the future ones.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine

@twoeths twoeths requested a review from a team April 27, 2020 22:40
Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@@ -70,4 +70,17 @@ export interface IBeaconParams {
MAX_ATTESTATIONS: number;
MAX_DEPOSITS: number;
MAX_VOLUNTARY_EXITS: number;

Copy link
Member

Choose a reason for hiding this comment

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

This is fine

Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

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

Curious, if we have scheduled hard forks that changes state transition, how do we handle multiple state transition dependencies. Can you even do that? 😄

@twoeths
Copy link
Contributor Author

twoeths commented Apr 28, 2020

@mpetrunic what do you mean state transition dependencies, can you tell me more about that?

@mpetrunic
Copy link
Member

@mpetrunic what do you mean state transition dependencies, can you tell me more about that?

If there is some breaking change in state transition and we have to schedule new hard fork. When you sync from genesis again you have to run with old state transition until hard fork then you need to switch to new beacon state transition I guess

@twoeths
Copy link
Contributor Author

twoeths commented Apr 28, 2020

it should work fine because the ALL_FORKS array contains all the hard fork history as well as future planned hard fork.
At epoch boundaries, it checks for any fork with previousVersion = state.fork.currentVersion and at the right epoch then it'll use the new fork for the new state.
So keeping ALL_FORKS up to date and contain all historical forks are very important in order to launch a new node.

@mpetrunic
Copy link
Member

it should work fine because the ALL_FORKS array contains all the hard fork history as well as future planned hard fork.
At epoch boundaries, it checks for any fork with previousVersion = state.fork.currentVersion and at the right epoch then it'll use the new fork for the new state.
So keeping ALL_FORKS up to date and contain all historical forks are very important in order to launch a new node.

You didn't get my point, it's not related to this PR per se. So you are syncing chain that went trough hardfork.

if fork.currentVersion === FORK_VERSION_WHEN_STATE_TRANSITION_v1
   import processBlock from "@chainsafe/beacon-state-transition@v1"
   processBlock(block)
if fork.currentVersion === FORK_VERSION_WHEN_STATE_TRANSITION_v2
   import processBlock from "@chainsafe/beacon-state-transition@v2"
   processBlock(block)

I wonder if it's even possible to do something like that

@twoeths
Copy link
Contributor Author

twoeths commented Apr 28, 2020

ah... I can't come up with something like that atm ...

@twoeths twoeths merged commit 9b94085 into master Apr 28, 2020
@twoeths twoeths deleted the tuyen/enr-eth2 branch April 28, 2020 10:42
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.

Add "eth2" key/value to ENR
3 participants