Add compute_cycle_transitions

[hwwhww]

What was wrong?

#1381

Before adding the whole cycle transition process, first, adding compute_cycle_transitions.

How was it fixed?

1. compute_cycle_transitions does two things:
   1. Check if block.slot_number >= crystallized_state.last_state_recalc + config.CYCLE_LENGTH; if yes:
      • Call compute_per_cycle_transition (a.k.a. initialize_new_cycle in beacon_chain repo) to compute new cycle and get updated states.
        • TODO in other PRs: compute_crosslinks will call compute_crosslinks and apply_rewards_and_penalties.
      • Check if it's ready for dynasty transition.
        • If yes, call compute_dynasty_transition
   2. Return the updated states.
2. Renamed total_deposits to total_balance as the latest spec.

Cute Animal Picture

[pipermerriam]

Making note that passing the config object around like this seems like an API that could be improved. IIUC the BeaconConfig should be both immutable and static within the context of a single fork. For this reason, does it make sense to make this a class property?

I'm starting to like this pattern and am considering what it would look like to port it over to the main Chain and VM APIs. Many of the constants would become config properties which would ideally reduce some of the reliance on inheritance by giving each VM it's own config object (think replacing VM.get_block_reward() with VM.config.block_reward.

[hwwhww]

@pipermerriam

Right! I just updated the code, made them all classmethod except for import_block. It did become clear! 👍

A flexible config is very useful for us to test different PoS parameters combinations. Now we use hybrid configurable setting + Danny's fixture parametrization:

py-evm/tests/beacon/state_machines/conftest.py

Lines 33 to 38 in 222b366

	@pytest.fixture
	def fixture_sm_class(config):
	return SerenityStateMachine.configure(
	__name__='SerenityStateMachineForTesting',
	config=config,
	)

Using VM.config.block_reward looks more concise to me!

[djrtwo]

This while loop will never end with the current stub of compute_per_cycle_transition.

Consider updating the stub to at least update new_crystallized_state.last_state_recalc += crystallized_state.last_state_recalc + CYCLE_LENGTH

[djrtwo]

and add a sanity check test that we can go through a cycle transition without it hanging in the while loop

[pipermerriam]

if it is possible to determine up-front how many iterations this will take (which I recognize may not be possible) I think it might be prudent to do that so that we can write this loop in a manner that is guaranteed to terminate.

Alternatively, can we compute a reasonable upper bound and set the loop to only run that long. The inner loop can use a break and then we can add an exception to the loop's else clause to prevent a possible infinite loop.

All of this assumes that the algorithms involved here are sufficiently complex that we won't be really sure this loop's exit condition will be met which currently seems to be the case.

[hwwhww]

if it is possible to determine up-front how many iterations this will take

I think yes, if no accident, it should be (block.slot_number - crystallized_state.last_state_recalc) // cls.config.CYCLE_LENGTH times since according to the spec, the crystallized_state.last_state_recalc will be updated to crystallized_state.last_state_recalc + cls.config.CYCLE_LENGTH at the end of compute_per_cycle_transition too.

[pipermerriam]

I see you pinged me for review but I don't see any of the suggested changes to this loop's structure. Is that being done separately?

[djrtwo]

Name might be better update_crosslinks

[hwwhww]

@pipermerriam @djrtwo ready for review again, thanks!

[pipermerriam]

@hwwhww this is mostly a rubber stamp. I don't know the protocol well enough to evaluate correctness, and there aren't many actual algorithms here so this is mostly just glue code stitching together existing functions (which I don't have a deep understanding of).

Maybe a second sign-off by @djrtwo would be good.

[djrtwo]

Approved