Merge pull request #1514 from ethereum/queue-fix-on-finality

Fix queue rate and only activate upon finality

specs/core/0_beacon-chain.md

@@ -61,6 +61,8 @@
             - [`bls_aggregate_pubkeys`](#bls_aggregate_pubkeys)
         - [Predicates](#predicates)
             - [`is_active_validator`](#is_active_validator)
+            - [`is_eligible_for_activation_queue`](#is_eligible_for_activation_queue)
+            - [`is_eligible_for_activation`](#is_eligible_for_activation)
             - [`is_slashable_validator`](#is_slashable_validator)
             - [`is_slashable_attestation_data`](#is_slashable_attestation_data)
             - [`is_valid_indexed_attestation`](#is_valid_indexed_attestation)
@@ -591,6 +593,34 @@ def is_active_validator(validator: Validator, epoch: Epoch) -> bool:
     return validator.activation_epoch <= epoch < validator.exit_epoch
 ```
 
+#### `is_eligible_for_activation_queue`
+
+```python
+def is_eligible_for_activation_queue(validator: Validator) -> bool:
+    """
+    Check if ``validator`` is eligible to be placed into the activation queue.
+    """
+    return (
+        validator.activation_eligibility_epoch == FAR_FUTURE_EPOCH
+        and validator.effective_balance == MAX_EFFECTIVE_BALANCE
+    )
+```
+
+#### `is_eligible_for_activation`
+
+```python
+def is_eligible_for_activation(state: BeaconState, validator: Validator) -> bool:
+    """
+    Check if ``validator`` is eligible for activation.
+    """
+    return (
+        # Placement in queue is finalized
+        validator.activation_eligibility_epoch <= state.finalized_checkpoint.epoch
+        # Has not yet been activated
+        and validator.activation_epoch == FAR_FUTURE_EPOCH
+    )
+```
+
 #### `is_slashable_validator`
 
 ```python
@@ -1302,26 +1332,22 @@ def process_rewards_and_penalties(state: BeaconState) -> None:
 def process_registry_updates(state: BeaconState) -> None:
     # Process activation eligibility and ejections
     for index, validator in enumerate(state.validators):
-        if (
+        if is_eligible_for_activation_queue(validator):
-            validator.activation_eligibility_epoch == FAR_FUTURE_EPOCH
+            validator.activation_eligibility_epoch = get_current_epoch(state) + 1
-            and validator.effective_balance == MAX_EFFECTIVE_BALANCE
-        ):
-            validator.activation_eligibility_epoch = get_current_epoch(state)
 
         if is_active_validator(validator, get_current_epoch(state)) and validator.effective_balance <= EJECTION_BALANCE:
             initiate_validator_exit(state, ValidatorIndex(index))
 
-    # Queue validators eligible for activation and not dequeued for activation prior to finalized epoch
+    # Queue validators eligible for activation and not yet dequeued for activation
     activation_queue = sorted([
         index for index, validator in enumerate(state.validators)
-        if validator.activation_eligibility_epoch != FAR_FUTURE_EPOCH
+        if is_eligible_for_activation(state, validator)
-        and validator.activation_epoch >= compute_activation_exit_epoch(state.finalized_checkpoint.epoch)
+        # Order by the sequence of activation_eligibility_epoch setting and then index
-    ], key=lambda index: state.validators[index].activation_eligibility_epoch)
+    ], key=lambda index: (state.validators[index].activation_eligibility_epoch, index))
-    # Dequeued validators for activation up to churn limit (without resetting activation epoch)
+    # Dequeued validators for activation up to churn limit
     for index in activation_queue[:get_validator_churn_limit(state)]:
         validator = state.validators[index]
-        if validator.activation_epoch == FAR_FUTURE_EPOCH:
+        validator.activation_epoch = compute_activation_exit_epoch(get_current_epoch(state))
-            validator.activation_epoch = compute_activation_exit_epoch(get_current_epoch(state))
 ```
 
 #### Slashings

test_libs/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_registry_updates.py

@@ -17,24 +17,80 @@ def mock_deposit(spec, state, index):
 
 @with_all_phases
 @spec_state_test
-def test_activation(spec, state):
+def test_add_to_activation_queue(spec, state):
+    # move past first two irregular epochs wrt finality
+    next_epoch(spec, state)
+    next_epoch(spec, state)
+
     index = 0
     mock_deposit(spec, state, index)
 
-    for _ in range(spec.MAX_SEED_LOOKAHEAD + 1):
+    yield from run_process_registry_updates(spec, state)
-        next_epoch(spec, state)
+
+    # validator moved into queue
+    assert state.validators[index].activation_eligibility_epoch != spec.FAR_FUTURE_EPOCH
+    assert state.validators[index].activation_epoch == spec.FAR_FUTURE_EPOCH
+    assert not spec.is_active_validator(state.validators[index], spec.get_current_epoch(state))
+
+
+@with_all_phases
+@spec_state_test
+def test_activation_queue_to_activated_if_finalized(spec, state):
+    # move past first two irregular epochs wrt finality
+    next_epoch(spec, state)
+    next_epoch(spec, state)
+
+    index = 0
+    mock_deposit(spec, state, index)
+
+    # mock validator as having been in queue since latest finalized
+    state.finalized_checkpoint.epoch = spec.get_current_epoch(state) - 1
+    state.validators[index].activation_eligibility_epoch = state.finalized_checkpoint.epoch
+
+    assert not spec.is_active_validator(state.validators[index], spec.get_current_epoch(state))
 
     yield from run_process_registry_updates(spec, state)
 
+    # validator activated for future epoch
     assert state.validators[index].activation_eligibility_epoch != spec.FAR_FUTURE_EPOCH
     assert state.validators[index].activation_epoch != spec.FAR_FUTURE_EPOCH
-    assert spec.is_active_validator(state.validators[index], spec.get_current_epoch(state))
+    assert not spec.is_active_validator(state.validators[index], spec.get_current_epoch(state))
+    assert spec.is_active_validator(
+        state.validators[index],
+        spec.compute_activation_exit_epoch(spec.get_current_epoch(state))
+    )
+
+
+@with_all_phases
+@spec_state_test
+def test_activation_queue_no_activation_no_finality(spec, state):
+    # move past first two irregular epochs wrt finality
+    next_epoch(spec, state)
+    next_epoch(spec, state)
+
+    index = 0
+    mock_deposit(spec, state, index)
+
+    # mock validator as having been in queue only after latest finalized
+    state.finalized_checkpoint.epoch = spec.get_current_epoch(state) - 1
+    state.validators[index].activation_eligibility_epoch = state.finalized_checkpoint.epoch + 1
+
+    assert not spec.is_active_validator(state.validators[index], spec.get_current_epoch(state))
+
+    yield from run_process_registry_updates(spec, state)
+
+    # validator not activated
+    assert state.validators[index].activation_eligibility_epoch != spec.FAR_FUTURE_EPOCH
+    assert state.validators[index].activation_epoch == spec.FAR_FUTURE_EPOCH
 
 
 @with_all_phases
 @spec_state_test
 def test_activation_queue_sorting(spec, state):
-    mock_activations = 10
+    churn_limit = spec.get_validator_churn_limit(state)
+
+    # try to activate more than the per-epoch churn linmit
+    mock_activations = churn_limit * 2
 
     epoch = spec.get_current_epoch(state)
     for i in range(mock_activations):
@@ -44,9 +100,9 @@ def test_activation_queue_sorting(spec, state):
     # give the last priority over the others
     state.validators[mock_activations - 1].activation_eligibility_epoch = epoch
 
-    # make sure we are hitting the churn
+    # move state forward and finalize to allow for activations
-    churn_limit = spec.get_validator_churn_limit(state)
+    state.slot += spec.SLOTS_PER_EPOCH * 3
-    assert mock_activations > churn_limit
+    state.finalized_checkpoint.epoch = epoch + 1
 
     yield from run_process_registry_updates(spec, state)
 
@@ -63,6 +119,38 @@ def test_activation_queue_sorting(spec, state):
     assert state.validators[churn_limit - 2].activation_epoch != spec.FAR_FUTURE_EPOCH
 
 
+@with_all_phases
+@spec_state_test
+def test_activation_queue_efficiency(spec, state):
+    churn_limit = spec.get_validator_churn_limit(state)
+    mock_activations = churn_limit * 2
+
+    epoch = spec.get_current_epoch(state)
+    for i in range(mock_activations):
+        mock_deposit(spec, state, i)
+        state.validators[i].activation_eligibility_epoch = epoch + 1
+
+    # move state forward and finalize to allow for activations
+    state.slot += spec.SLOTS_PER_EPOCH * 3
+    state.finalized_checkpoint.epoch = epoch + 1
+
+    # Run first registry update. Do not yield test vectors
+    for _ in run_process_registry_updates(spec, state):
+        pass
+
+    # Half should churn in first run of registry update
+    for i in range(mock_activations):
+        if i < mock_activations // 2:
+            assert state.validators[i].activation_epoch < spec.FAR_FUTURE_EPOCH
+        else:
+            assert state.validators[i].activation_epoch == spec.FAR_FUTURE_EPOCH
+
+    # Second half should churn in second run of registry update
+    yield from run_process_registry_updates(spec, state)
+    for i in range(mock_activations):
+        assert state.validators[i].activation_epoch < spec.FAR_FUTURE_EPOCH
+
+
 @with_all_phases
 @spec_state_test
 def test_ejection(spec, state):
@@ -73,13 +161,87 @@ def test_ejection(spec, state):
     # Mock an ejection
     state.validators[index].effective_balance = spec.EJECTION_BALANCE
 
-    for _ in range(spec.MAX_SEED_LOOKAHEAD + 1):
-        next_epoch(spec, state)
-
     yield from run_process_registry_updates(spec, state)
 
     assert state.validators[index].exit_epoch != spec.FAR_FUTURE_EPOCH
+    assert spec.is_active_validator(state.validators[index], spec.get_current_epoch(state))
     assert not spec.is_active_validator(
         state.validators[index],
-        spec.get_current_epoch(state),
+        spec.compute_activation_exit_epoch(spec.get_current_epoch(state))
+    )
+
+
+@with_all_phases
+@spec_state_test
+def test_ejection_past_churn_limit(spec, state):
+    churn_limit = spec.get_validator_churn_limit(state)
+
+    # try to eject more than per-epoch churn limit
+    mock_ejections = churn_limit * 3
+
+    for i in range(mock_ejections):
+        state.validators[i].effective_balance = spec.EJECTION_BALANCE
+
+    expected_ejection_epoch = spec.compute_activation_exit_epoch(spec.get_current_epoch(state))
+
+    yield from run_process_registry_updates(spec, state)
+
+    for i in range(mock_ejections):
+        # first third ejected in normal speed
+        if i < mock_ejections // 3:
+            assert state.validators[i].exit_epoch == expected_ejection_epoch
+        # second thirdgets delayed by 1 epoch
+        elif mock_ejections // 3 <= i < mock_ejections * 2 // 3:
+            assert state.validators[i].exit_epoch == expected_ejection_epoch + 1
+        # second thirdgets delayed by 2 epochs
+        else:
+            assert state.validators[i].exit_epoch == expected_ejection_epoch + 2
+
+
+@with_all_phases
+@spec_state_test
+def test_activation_queue_activation_and_ejection(spec, state):
+    # move past first two irregular epochs wrt finality
+    next_epoch(spec, state)
+    next_epoch(spec, state)
+
+    # ready for entrance into activation queue
+    activation_queue_index = 0
+    mock_deposit(spec, state, activation_queue_index)
+
+    # ready for activation
+    activation_index = 1
+    mock_deposit(spec, state, activation_index)
+    state.finalized_checkpoint.epoch = spec.get_current_epoch(state) - 1
+    state.validators[activation_index].activation_eligibility_epoch = state.finalized_checkpoint.epoch
+
+    # ready for ejection
+    ejection_index = 2
+    state.validators[ejection_index].effective_balance = spec.EJECTION_BALANCE
+
+    yield from run_process_registry_updates(spec, state)
+
+    # validator moved into activation queue
+    validator = state.validators[activation_queue_index]
+    assert validator.activation_eligibility_epoch != spec.FAR_FUTURE_EPOCH
+    assert validator.activation_epoch == spec.FAR_FUTURE_EPOCH
+    assert not spec.is_active_validator(validator, spec.get_current_epoch(state))
+
+    # validator activated for future epoch
+    validator = state.validators[activation_index]
+    assert validator.activation_eligibility_epoch != spec.FAR_FUTURE_EPOCH
+    assert validator.activation_epoch != spec.FAR_FUTURE_EPOCH
+    assert not spec.is_active_validator(validator, spec.get_current_epoch(state))
+    assert spec.is_active_validator(
+        validator,
+        spec.compute_activation_exit_epoch(spec.get_current_epoch(state))
+    )
+
+    # validator ejected for future epoch
+    validator = state.validators[ejection_index]
+    assert validator.exit_epoch != spec.FAR_FUTURE_EPOCH
+    assert spec.is_active_validator(validator, spec.get_current_epoch(state))
+    assert not spec.is_active_validator(
+        validator,
+        spec.compute_activation_exit_epoch(spec.get_current_epoch(state))
     )