Turned slashed and initiated_exit into booleans

[vbuterin]

Cuts validator record size from 120 to 106 bytes, and arguably is a simplification

[hwwhww]

LGTM 👍

[JustinDrake]

Nice simplifications 👍 Looking at the validator state machine a bit closer there may be bugs:

1. is_active_validator returns validator.activation_epoch <= epoch < validator.exit_epoch. Because of the churn queuing mechanism validator.activation_epoch <= epoch does not mean that a validator has been activated. Maybe we need an activated flag. (Also if we do introduce an activated flag we may be able to reduce the Validator object by a further 31 bytes by merging activation_epoch and exit_epoch into activation_exit_epoch, mirroring ACTIVATION_EXIT_DELAY.)
2. It seems we are too eager with get_entry_exit_effect_epoch. Indeed, we set activation_epoch and exit_epoch to get_entry_exit_effect_epoch(get_current_epoch(state)), and then reuse get_entry_exit_effect_epoch in comparisons (e.g. validator.exit_epoch > get_entry_exit_effect_epoch(get_current_epoch(state))), negating the desired effect.

cc @djrtwo

[JustinDrake]

So I think the above bugs are due to confusing naming. (Specifically, activation_epoch is not the epoch at which a validator is activated, and same for exit_epoch.) My suggested refactor would be something either:

{
    'initiated_activation_epoch': uint64,
    'activated': bool,
    'initiated_exit_epoch': uint64,
    'exited': bool,
}

or

{
    'activatable_epoch': uint64,
    'activated': bool,
    'exitable_epoch': uint64,
    'exited': bool,
}

On the topic of naming, we probably ought to fix the "activated validator"/"active validator" confusion. My suggestion would be to renamed "activated" to "inducted".

[djrtwo]

(1) activation_epoch is when the validator is activated. They are in limbo until they come up in the queue/churn at which point their activation_epoch is set in the future. When activation_epoch is reached, they are active (as seen by is_active_validator).

(2) We only compare against get_entry_exit_effect_epoch when making sure that we have not already performed some operation. By checking validator.exit_epoch > get_entry_exit_effect_epoch(get_current_epoch(state)) we are checking that exit_epoch is still in the future and not yet been set. If we did validator.exit_epoch > get_current_epoch(state) the exit_epoch might have already been set in the future and we could accidentally perform a double op.

As for your renaming suggestions, activation_epoch and exit_epoch are in fact when these events occur so I see no need to modify and/or add booleans

[vbuterin]

1. Because of the churn queuing mechanism validator.activation_epoch <= epoch does not mean that a validator has been activated.

My understanding is that it does. There is no "queue" for activation; there's just a mechanism that scans left to right and grabs up the maximum number of eligible validators each time there is a validator set change. So activation_epoch really is the epoch you get activated.

[JustinDrake]

activation_epoch is when the validator is activated

My bad!

In update_validator_registry, I don't understand if validator.activation_epoch > get_entry_exit_effect_epoch(current_epoch). Should it be if current_epoch >= validator.activation_epoch?

[djrtwo]

In update_validator_registry, I don't understand if validator.activation_epoch > get_entry_exit_effect_epoch(current_epoch). Should it be if current_epoch >= validator.activation_epoch?

Say I have just been activated. That means my activation_epoch == get_entry_exit_effect_epoch(current_epoch) and I will become active in that many epochs from now. Now say there happens to be another operation in this epoch or even the next that is trying to activate me. if validator.activation_epoch > get_entry_exit_effect_epoch(current_epoch) is not longer true in this epoch because activation_epoch is == and it is no longer true in the next epoch because it is <. this prevents any operation from doubly activating me so it acts as a double op blocker.

same for the exit logic below it.

[JustinDrake]

Thanks @djrtwo for the call clarifying this :) As discussed and agreed, I clarified the use of FAR_FUTURE_EPOCH as a flag.