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

Merge transition process with computed transition total difficulty #2462

Merged
merged 11 commits into from
Jun 8, 2021

Conversation

mkalinin
Copy link
Collaborator

@mkalinin mkalinin commented Jun 1, 2021

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 formula transition_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 in TransitionStore.

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 that state.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 corresponding state.eth1_data.

Why TransitionStore instead of Store.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 keep transition_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.

@hwwhww hwwhww added the Bellatrix CL+EL Merge label Jun 1, 2021
specs/merge/fork-choice.md Outdated Show resolved Hide resolved
specs/merge/fork-choice.md Outdated Show resolved Hide resolved
specs/merge/fork-choice.md Outdated Show resolved Hide resolved
specs/merge/fork-choice.md Outdated Show resolved Hide resolved
specs/merge/fork-choice.md Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a 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

specs/merge/fork.md Show resolved Hide resolved
specs/merge/fork.md Show resolved Hide resolved
}


def run_fork_test(post_spec, pre_state):
Copy link
Contributor

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])
Copy link
Contributor

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 )

Comment on lines +25 to +28
@with_phases(phases=[PHASE0], other_phases=[MERGE])
@spec_test
@with_state
@with_meta_tags(MERGE_FORK_TEST_META_TAGS)
Copy link
Collaborator

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.

specs/merge/fork.md Outdated Show resolved Hide resolved
specs/merge/fork.md Outdated Show resolved Hide resolved
@mkalinin mkalinin force-pushed the merge-transition-with-dynamic-ttd branch from 29a87d5 to 6350e27 Compare June 8, 2021 11:56
Copy link
Contributor

@djrtwo djrtwo left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bellatrix CL+EL Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants