-
Notifications
You must be signed in to change notification settings - Fork 997
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
Merge transition process with computed transition total difficulty #2462
Merge transition process with computed transition total difficulty #2462
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Slight preference for vitaliks calculation based on difficultly because what we choose >= 2 weeks prior is almost certainly less accurate. But just a slight preference
} | ||
|
||
|
||
def run_fork_test(post_spec, pre_state): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth abstracting this into a standard run_fork_test(post_spec, pre_state, stable_fields, modified fields, fork_specific_checks_fn=None)
and have run_merge_fork_test
call it. But... doesn't matter too much for this PR
) | ||
|
||
|
||
@with_phases(phases=[PHASE0], other_phases=[MERGE]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bummer these are all copied over.. Good for now, but we might consider a more re-usable approach (cc: @ralexstokes, @protolambda )
@with_phases(phases=[PHASE0], other_phases=[MERGE]) | ||
@spec_test | ||
@with_state | ||
@with_meta_tags(MERGE_FORK_TEST_META_TAGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just define:
def composed(*decs):
def deco(f):
for dec in reversed(decs):
f = dec(f)
return f
return deco
merge_test = composed(
with_phases(phases=[PHASE0], other_phases=[MERGE]),
spec_test,
with_state,
with_meta_tags(MERGE_FORK_TEST_META_TAGS),
)
@merge_test
def test_something_fun(spec, phases, state):
...
Lots of other places where we can combine decorators as well.
Co-authored-by: vbuterin <v@buterin.com>
29a87d5
to
6350e27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent!
plenty of iteration to do but I'm going to get this merged and released for alpha.7
Introduces the Merge fork (
upgrade_to_merge
) with the version of transition process that uses dynamically computed transition total difficulty.The computation happens during the Merge fork, i.e. next to
upgrade_to_merge
call. Total difficulty is computed with the following formulatransition_total_difficulty = get_pow_block(state.eth1_data.block_hash).total_difficulty + TRANSITION_TOTAL_DIFFICULTY_OFFSET
. And then it is stored to a specific storage slot inTransitionStore
.Rationale
Dynamically computed total difficulty makes the transition process more predictable.
For example, we expect
MERGE_FORK_EPOCH
two weeks after updated client software has been deployed and we also take a couple of extra days after the Merge fork to be able to react accordingly if something went wrong during the fork itself.OTOH, we can compute transition total difficulty at the time of the fork and remove period before the fork (2 weeks in our case) from our model. If we double total difficulty estimation to protect from difficulty fluctuations then we can reduce the time between software deployment and the actual transition by at least two weeks in our case. Which, in its turn, makes estimated time of the transition more accurate.
Implementation details
What if
state.eth1_data
is too old?If
TRANSITION_TOTAL_DIFFICULTY_OFFSET
is set to 1 week worth of difficulty then it will be enough time to handle this case properly. It's hard to imagine thatstate.eth1_data
is older than 3 days (more than 10 voting periods failed in a row).What if
state.eth1_data
is forked out?We may set
MERGE_FORK_EPOCH
to the beginning of the voting period to reduce the possibility of changing eth1 data in state due to chain re-orgs. Also,TransitionStore
should be initialized each time the fork happens in the client's code and seeded with correspondingstate.eth1_data
.Why
TransitionStore
instead ofStore.transition_total_difficulty
?To keep implementation and testing simple and weaken dependency of the fork choice tests and transition process. We can opt for re-using
Store
to keeptransition_total_difficulty
if we want.Testing
Merge fork tests are copied from Altair ones. There is no tests for the transition process itself, we better work on them in a separate PR once we settled down with particular approach to the transition.
Transition total difficulty announcement
We might want to communicate transition total difficulty offchain once it's computed. So, validators will be able to check it and act accordingly if computation went wrong on their end. This would require a corresponding change in beacon node API.