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

Align fork epoch with the start of EPOCHS_PER_SYNC_COMMITTEE_PERIOD #2417

Closed
wants to merge 2 commits into from

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 17, 2021

Issue

  • Suggestion: align fork epoch with the start of EPOCHS_PER_SYNC_COMMITTEE_PERIOD.
  • Pros:
    1. More stable network at the starting epochs of the fork.
    2. Aesthetically align the time setup.
  • Cons: It would be more difficult to coordinate the suitable clock time.

If we do it, I'd like to add an invariant check to the pyspec builder. Therefore we have to adjust the temporary stub ALTAIR_FORK_EPOCH to the number that matches the assertion.

Proposal

  1. Update the TBD *_FORK_EPOCH stub to 2**63.
  2. Add ALTAIR_FORK_EPOCH % EPOCHS_PER_SYNC_COMMITTEE_PERIOD == 0 checks to pyspec builder.

@hwwhww hwwhww requested review from djrtwo and ralexstokes May 17, 2021 17:47
@@ -68,7 +68,7 @@ Warning: this configuration is not definitive.
| Name | Value |
| - | - |
| `MERGE_FORK_VERSION` | `Version('0x02000000')` |
| `MERGE_FORK_EPOCH` | `Epoch(18446744073709551615)` **TBD** |
| `MERGE_FORK_EPOCH` | `Epoch(9223372036854775808)` **TBD** |
Copy link
Member

Choose a reason for hiding this comment

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

the merge and sharding constants have to change as they are downstream of the altair fork so they need to have the alignment condition as well?

Copy link
Contributor Author

@hwwhww hwwhww May 18, 2021

Choose a reason for hiding this comment

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

That was only for simple alignment. Open to discussion! :)

@ralexstokes
Copy link
Member

ralexstokes commented May 17, 2021

i'm not opposed to this change but would say its more of a broader coordination issue across everyone who will be executing the fork -- the downside is that it constrains the timing of the fork (and raises the question how we want to handle future forks...) so that if e.g. we want to execute the fork at a given slot (bc the wall clock time is convenient for some reason) we may have to wait up to do ~48 hrs extra

may be worth raising on the implementers call thursday to get input from more sources before merging this

@protolambda
Copy link
Collaborator

Is the alignment of the stubs (which are not supposed to be used) really necessary? I think we can just add a special case for the stub value in the invariant check.

For non-stub values, I brought up the alignment issue during review in March here: #2201 (comment) A shorter period at the start seems fine to me if the test cannot avoid it. And yes, for mainnet we can align it nicely.

And I would prefer an unit test that checks alignment of all mainnet fork version values, to keep the spec builder simple.

@hwwhww
Copy link
Contributor Author

hwwhww commented May 18, 2021

@ralexstokes

its more of a broader coordination issue across everyone who will be executing the fork

Agreed!

This alignment is half for aesthetic improvement and half for technical improvement. It would look bad if we somehow set ALTAIR_FORK_EPOCH + 1 the period boundary, so I think some stability improvement is better than none.

@hwwhww
Copy link
Contributor Author

hwwhww commented May 18, 2021

@protolambda

I would prefer an unit test that checks alignment of all mainnet fork version values, to keep the spec builder simple.

Fair enough! I was thinking about putting invariant checks in the markdown file, but that was too messy. Moving to unit test should be good.

@hwwhww hwwhww changed the title Update fork epoch stub to Epoch(2**63) and add pyspec invariant check Align fork epoch with the start of EPOCHS_PER_SYNC_COMMITTEE_PERIOD May 18, 2021
@hwwhww
Copy link
Contributor Author

hwwhww commented May 20, 2021

Thanks for @ajsutton, @arnetheduck, @djrtwo, and @dankrad 's feedback on the call:

  1. Restrict sync committee period calculation boundaries #2406 ensures that the first two epochs have the same sync committee.
  2. In Teku's prototyping, a short period is not a big problem.
  3. Considering coordination overhead likely not worth it.

Other time period parameters:

  • EPOCHS_PER_HISTORICAL_VECTOR: ~0.8 years
  • SLOTS_PER_HISTORICAL_ROOT: ~27 hours
    ... are also too long to align with.

Leaving this PR open for a few days in case we get new feedback. If no, I'll close this issue. :)

edited: HISTORICAL_ROOTS_LIMIT SLOTS_PER_HISTORICAL_ROOT

@arnetheduck
Copy link
Contributor

During the meeting, I think the HISTORICAL_ROOTS_LIMIT was mentioned to constrain the alignment to a month (8k epochs) which indeed is very long.

I think1 day is fairly palatable however - should the proposal for #2417 be accepted, this alignment would help make the transition a lot easier.

@arnetheduck
Copy link
Contributor

Oops, wrong link, should be #2428

@djrtwo
Copy link
Contributor

djrtwo commented Jun 7, 2021

closing for now, as discussed two calls ago this does not buy us too much on simplification

@djrtwo djrtwo closed this Jun 7, 2021
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.

5 participants