hww feedback for finality rewards fix

specs/phase0/beacon-chain.md

@@ -1368,7 +1368,7 @@ def get_finality_delay(state: BeaconState) -> uint64:
 
 
 ```python
-def in_inactivity_leak(state: BeaconState) -> bool:
+def is_in_inactivity_leak(state: BeaconState) -> bool:
     return get_finality_delay(state) > MIN_EPOCHS_TO_INACTIVITY_PENALTY
 ```
 
@@ -1397,8 +1397,9 @@ def get_attestation_component_deltas(state: BeaconState,
     for index in get_eligible_validator_indices(state):
         if index in unslashed_attesting_indices:
             increment = EFFECTIVE_BALANCE_INCREMENT  # Factored out from balance totals to avoid uint64 overflow
-            if in_inactivity_leak(state):
+            if is_in_inactivity_leak(state):
-                # Full base reward will be cancelled out by inactivity penalty deltas
+                # Since full base reward will be canceled out by inactivity penalty deltas,
+                # optimal participation receives full base reward compensation here.
                 rewards[index] += get_base_reward(state, index)
             else:
                 reward_numerator = get_base_reward(state, index) * (attesting_balance // increment)
@@ -1464,7 +1465,7 @@ def get_inactivity_penalty_deltas(state: BeaconState) -> Tuple[Sequence[Gwei], S
     Return inactivity reward/penalty deltas for each validator.
     """
     penalties = [Gwei(0) for _ in range(len(state.validators))]
-    if in_inactivity_leak(state):
+    if is_in_inactivity_leak(state):
         matching_target_attestations = get_matching_target_attestations(state, get_previous_epoch(state))
         matching_target_attesting_indices = get_unslashed_attesting_indices(state, matching_target_attestations)
         for index in get_eligible_validator_indices(state):

tests/core/pyspec/eth2spec/test/helpers/rewards.py

@@ -151,7 +151,6 @@ def run_get_inactivity_penalty_deltas(spec, state):
     matching_attestations = spec.get_matching_target_attestations(state, spec.get_previous_epoch(state))
     matching_attesting_indices = spec.get_unslashed_attesting_indices(state, matching_attestations)
 
-    finality_delay = spec.get_previous_epoch(state) - state.finalized_checkpoint.epoch
     eligible_indices = spec.get_eligible_validator_indices(state)
     for index in range(len(state.validators)):
         assert rewards[index] == 0
@@ -159,7 +158,7 @@ def run_get_inactivity_penalty_deltas(spec, state):
             assert penalties[index] == 0
             continue
 
-        if finality_delay > spec.MIN_EPOCHS_TO_INACTIVITY_PENALTY:
+        if spec.is_in_inactivity_leak(state):
             base_reward = spec.get_base_reward(state, index)
             base_penalty = spec.BASE_REWARDS_PER_EPOCH * base_reward - spec.get_proposer_reward(state, index)
             if not has_enough_for_reward(spec, state, index):

tests/core/pyspec/eth2spec/test/phase_0/epoch_processing/test_process_rewards_and_penalties.py

@@ -63,74 +63,6 @@ def test_genesis_epoch_full_attestations_no_rewards(spec, state):
         assert state.balances[index] == pre_state.balances[index]
 
 
-@with_all_phases
-@spec_state_test
-def test_full_attestations(spec, state):
-    attestations = prepare_state_with_attestations(spec, state)
-
-    pre_state = state.copy()
-
-    yield from run_process_rewards_and_penalties(spec, state)
-
-    attesting_indices = spec.get_unslashed_attesting_indices(state, attestations)
-    assert len(attesting_indices) == len(pre_state.validators)
-    for index in range(len(pre_state.validators)):
-        if index in attesting_indices:
-            assert state.balances[index] > pre_state.balances[index]
-        else:
-            assert state.balances[index] < pre_state.balances[index]
-
-
-@with_all_phases
-@spec_state_test
-@leaking()
-def test_full_attestations_with_leak(spec, state):
-    attestations = prepare_state_with_attestations(spec, state)
-
-    proposer_indices = [a.proposer_index for a in state.previous_epoch_attestations]
-    pre_state = state.copy()
-
-    yield from run_process_rewards_and_penalties(spec, state)
-
-    attesting_indices = spec.get_unslashed_attesting_indices(state, attestations)
-    assert len(attesting_indices) == len(pre_state.validators)
-    for index in range(len(pre_state.validators)):
-        # Proposers can still make money during a leak
-        if index in proposer_indices:
-            assert state.balances[index] > pre_state.balances[index]
-        # If not proposer but participated optimally, should have exactly neutral balance
-        elif index in attesting_indices:
-            assert state.balances[index] == pre_state.balances[index]
-        else:
-            assert state.balances[index] < pre_state.balances[index]
-
-
-@with_all_phases
-@spec_state_test
-@leaking()
-def test_partial_attestations_with_leak(spec, state):
-    attestations = prepare_state_with_attestations(spec, state)
-
-    attestations = attestations[:len(attestations) // 2]
-    state.previous_epoch_attestations = state.previous_epoch_attestations[:len(attestations)]
-    proposer_indices = [a.proposer_index for a in state.previous_epoch_attestations]
-    pre_state = state.copy()
-
-    yield from run_process_rewards_and_penalties(spec, state)
-
-    attesting_indices = spec.get_unslashed_attesting_indices(state, attestations)
-    assert len(attesting_indices) < len(pre_state.validators)
-    for index in range(len(pre_state.validators)):
-        # Proposers can still make money during a leak
-        if index in proposer_indices and index in attesting_indices:
-            assert state.balances[index] > pre_state.balances[index]
-        # If not proposer but participated optimally, should have exactly neutral balance
-        elif index in attesting_indices:
-            assert state.balances[index] == pre_state.balances[index]
-        else:
-            assert state.balances[index] < pre_state.balances[index]
-
-
 @with_all_phases
 @spec_state_test
 def test_full_attestations_random_incorrect_fields(spec, state):
@@ -224,6 +156,7 @@ def run_with_participation(spec, state, participation_fn):
         return att_participants
 
     attestations = prepare_state_with_attestations(spec, state, participation_fn=participation_tracker)
+    proposer_indices = [a.proposer_index for a in state.previous_epoch_attestations]
 
     pre_state = state.copy()
 
@@ -233,10 +166,20 @@ def run_with_participation(spec, state, participation_fn):
     assert len(attesting_indices) == len(participated)
 
     for index in range(len(pre_state.validators)):
-        if index in participated:
+        if spec.is_in_inactivity_leak(state):
-            assert state.balances[index] > pre_state.balances[index]
+            # Proposers can still make money during a leak
+            if index in proposer_indices and index in participated:
+                assert state.balances[index] > pre_state.balances[index]
+            # If not proposer but participated optimally, should have exactly neutral balance
+            elif index in attesting_indices:
+                assert state.balances[index] == pre_state.balances[index]
+            else:
+                assert state.balances[index] < pre_state.balances[index]
         else:
-            assert state.balances[index] < pre_state.balances[index]
+            if index in participated:
+                assert state.balances[index] > pre_state.balances[index]
+            else:
+                assert state.balances[index] < pre_state.balances[index]
 
 
 @with_all_phases
@@ -246,6 +189,14 @@ def test_almost_empty_attestations(spec, state):
     yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, 1))
 
 
+@with_all_phases
+@spec_state_test
+@leaking()
+def test_almost_empty_attestations_with_leak(spec, state):
+    rng = Random(1234)
+    yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, 1))
+
+
 @with_all_phases
 @spec_state_test
 def test_random_fill_attestations(spec, state):
@@ -253,6 +204,14 @@ def test_random_fill_attestations(spec, state):
     yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, len(comm) // 3))
 
 
+@with_all_phases
+@spec_state_test
+@leaking()
+def test_random_fill_attestations_with_leak(spec, state):
+    rng = Random(4567)
+    yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, len(comm) // 3))
+
+
 @with_all_phases
 @spec_state_test
 def test_almost_full_attestations(spec, state):
@@ -260,12 +219,27 @@ def test_almost_full_attestations(spec, state):
     yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, len(comm) - 1))
 
 
+@with_all_phases
+@spec_state_test
+@leaking()
+def test_almost_full_attestations_with_leak(spec, state):
+    rng = Random(8901)
+    yield from run_with_participation(spec, state, lambda slot, comm_index, comm: rng.sample(comm, len(comm) - 1))
+
+
 @with_all_phases
 @spec_state_test
 def test_full_attestation_participation(spec, state):
     yield from run_with_participation(spec, state, lambda slot, comm_index, comm: comm)
 
 
+@with_all_phases
+@spec_state_test
+@leaking()
+def test_full_attestation_participation_with_leak(spec, state):
+    yield from run_with_participation(spec, state, lambda slot, comm_index, comm: comm)
+
+
 @with_all_phases
 @spec_state_test
 def test_duplicate_attestation(spec, state):