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.

Sign up for GitHub

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

Jump to bottom

explicit use of GENESIS_SLOT in fork choice #1510

Merged
merged 2 commits into from
Dec 10, 2019
Merged

explicit use of GENESIS_SLOT in fork choice #1510

merged 2 commits into from
Dec 10, 2019

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Dec 10, 2019

resolves #1502

  • Make use of GENESIS_SLOT explicit when calculating slot for fork choice
  • use get_current_slot(store) when assessing if can include new attestation in on_attestation
  • Note: the issue above recommends changing time to slot in Store. We did not take this approach because the granularity of time may very well be necessary in future forks if we, for example, change the SECONDS_PER_SLOT and need to do non-trivial slot calculations

@djrtwo djrtwo requested a review from protolambda December 10, 2019 02:18
@djrtwo djrtwo mentioned this pull request Dec 10, 2019
Merged
@mkalinin
Copy link
Contributor

We might want to use get_current_slot(store) in a few other places:

@djrtwo
Copy link
Contributor Author

djrtwo commented Dec 10, 2019

yep, thanks @mkalinin

@terencechain terencechain mentioned this pull request Dec 10, 2019
Closed
protolambda
protolambda approved these changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

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

👍 for the explicit get_slots_since_genesis :)

@djrtwo djrtwo merged commit 4636b3e into v09x Dec 10, 2019
@djrtwo djrtwo deleted the genesis-slot-time branch December 10, 2019 20:41
@protolambda protolambda mentioned this pull request Dec 10, 2019
Closed
@terencechain terencechain mentioned this pull request Dec 12, 2019
Closed
11 tasks
@wemeetagain wemeetagain mentioned this pull request Jan 21, 2020
Closed
11 tasks
@ColinSchwarz ColinSchwarz mentioned this pull request Feb 3, 2020
Closed
@ralexstokes ralexstokes mentioned this pull request Feb 26, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@protolambda protolambda protolambda approved these changes

Assignees
No one assigned
Labels
scope:fork-choice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@djrtwo @mkalinin @protolambda @hwwhww