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

initial epoch options #504

Closed
djrtwo opened this issue Jan 28, 2019 · 14 comments
Closed

initial epoch options #504

djrtwo opened this issue Jan 28, 2019 · 14 comments
Labels
general:enhancement New feature or request

Comments

@djrtwo
Copy link
Contributor

djrtwo commented Jan 28, 2019

Issue

We have a number of pretty ugly workarounds for avoiding underflows in slot/epoch arithmetic in early slots/epochs. Most (all?) of these could be entirely avoided if we just have the genesis_slot start far greater than 0.

Proposed solution

Use chainstart_time which is logged in ChainStart to seed the genesis_slot/epoch instead of a pre-defined GENESIS_SLOT.

  • pass chainstart_time into get_initial_beacon_state
  • define genesis_epoch as slot_to_epoch(chainstart_time // EPOCH_LENGTH) + 1
  • define genesis_slot as get_epoch_start_slot(genesis_epoch)
  • remove GENESIS_EPOCH and GENESIS_SLOT constants
  • clean up the ugly underflow workarounds
@djrtwo
Copy link
Contributor Author

djrtwo commented Jan 28, 2019

note, we put a quick fix in via #505 but might consider this solution for the next release

@JustinDrake
Copy link
Collaborator

If we set genesis_slot to chainstart_time would it be natural to:

  1. have chainstart_time be a multiple of EPOCH_LENGTH
  2. increment slot by SLOT_DURATION at every slot (as opposed to + 1).

Not a fan of change 2).

@djrtwo
Copy link
Contributor Author

djrtwo commented Jan 28, 2019

defining genesis_epoch as the chainstart_time_epoch + 1 and then defining genesis_slot as the start of that next epoch, ensures that the slots are a multiple of epoch length. I think this is sufficient.

@JustinDrake
Copy link
Collaborator

Oh wait, 86400 (seconds in a day) is a multiple of 64 :)

Still, both slot += 1 and slot += SLOT_DURATION are ugly in this context.

@djrtwo
Copy link
Contributor Author

djrtwo commented Jan 28, 2019

what's ugly with slot += 1?? You have to tick this machine forward somehow...

@JustinDrake
Copy link
Collaborator

The reason is that, semantically, you're using seconds as the unit when setting genesis_slot = chainstart_time. (chainstart_time is a unix time denominated in seconds.) By doing slot += 1 you're adding slots to seconds.

Incrementing by SLOT_DURATION (as opposed to 1) would be the natural thing to do. It's also cool because slot would keep track of the unix timestamp. Unfortunately it's not future-proof because we may change SLOT_DURATION in the future.

@djrtwo
Copy link
Contributor Author

djrtwo commented Jan 28, 2019

ah, whoops. I meant to make the genesis epoch the number of epochs since unix time start.

Should be slot_to_epoch(chainstart_time // SLOT_DURATION // EPOCH_LENGTH) + 1.
This would make slots be unix slots :) -- the number of slots since unix start time.

This I think is cleaner but does not resolve what happens if we reduce SLOT_DURATION at a fork. If we just reduce slot duration, then we can skip forward to the new unix slot wrt the new slot time.

@djrtwo
Copy link
Contributor Author

djrtwo commented Jan 31, 2019

Potential alternative from @JustinDrake is to set initial slot to 2**63 such that you can always do state.slot << 1 to get the number of slots since genesis. Seems nice.

@djrtwo djrtwo changed the title Use chain_start unix time for first slot/epoch discuss initial epoch options Jan 31, 2019
@djrtwo djrtwo changed the title discuss initial epoch options initial epoch options Jan 31, 2019
@djrtwo
Copy link
Contributor Author

djrtwo commented Feb 7, 2019

Some conversation in the sharding gitter brought up that this most significant bit set to 1 can have typing issues with certain languages/frameworks. For example, Java does not have a built in uint type so setting the most significant bit will be treated natively as negative numbers.

Clean alternative is to set initial slot as 2**20 and render slots since genesis simply as slot - 2**20

@JustinDrake

@JustinDrake
Copy link
Collaborator

For example, Java

Are there other examples?

setting the most significant bit will be treated natively as negative numbers

I think we discussed that latest_penalized_balances is somewhat borderline using uint64s, hence somewhat more borderline using uint63s.

@djrtwo
Copy link
Contributor Author

djrtwo commented Feb 8, 2019

Someone on Nimbus ran into it with C types, but that was because they were mistakenly using signed rather than unsigned.

You're right on the latest_penalize_balances -- I think that is the most likely thing in the spec to overflow.

@JustinDrake
Copy link
Collaborator

JustinDrake commented Feb 13, 2019

One consideration is that we may want to preserve the invariant that state.slot * SECONDS_PER_SLOT % SECONDS_PER_DAY == 0 falls at 00:00 UTC. This would suggest making GENESIS_SLOT a multiple of 86400 / SECONDS_PER_SLOT = 14400.

@hwwhww
Copy link
Contributor

hwwhww commented Feb 14, 2019

In the current spec (not the proposed solution):

The node's Unix time is greater than or equal to state.genesis_time + block.slot * SLOT_DURATION. (Note that leap seconds mean that slots will occasionally last SLOT_DURATION + 1 or SLOT_DURATION - 1 seconds, possibly several times a year.)

Since block.slot is a huge number here (> 2**63), so the condition is incorrect now?
It seems to be: The node's Unix time is greater than or equal to state.genesis_time + (block.slot - GENESIS_SLOT) * SLOT_DURATION.

@JustinDrake
Copy link
Collaborator

Addressed by #655 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants