From 3bc25c41ceb6573250f8a87ddf90eacf7d5058c8 Mon Sep 17 00:00:00 2001
From: Chiwon Cho <goldworm@icon.foundation>
Date: Mon, 2 Dec 2019 18:43:01 +0900
Subject: [PATCH] IS-929: Fix a bug on
 PRepEngine._reset_block_validation_penalty (#392)

* Change the target prep list to reset the block validation penalty from engine.preps to context.preps
* Change the condition to pick up the P-Reps to reset the block validation penalty
* Rename PRepEngine._release_block_validation_penalty() to PRepEngine._reset_block_validation_penalty()
* Add a unittest to prep/test_engine.py
---
 iconservice/prep/engine.py | 13 ++++----
 tests/prep/test_engine.py  | 68 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/iconservice/prep/engine.py b/iconservice/prep/engine.py
index 5f291371e..7d0943473 100644
--- a/iconservice/prep/engine.py
+++ b/iconservice/prep/engine.py
@@ -226,7 +226,7 @@ def _on_term_ended(self, context: 'IconScoreContext') -> Tuple[dict, 'Term']:
         context.storage.meta.put_last_main_preps(context, main_preps)
 
         # All block validation penalties are released
-        self._release_block_validation_penalty(context)
+        self._reset_block_validation_penalty(context)
 
         # Create a term with context.preps whose grades are up-to-date
         new_term: 'Term' = self._create_next_term(context, self.term)
@@ -348,17 +348,16 @@ def _update_prep_grades(cls,
 
         Logger.debug(tag=_TAG, msg="_update_prep_grades() end")
 
-    def _release_block_validation_penalty(self, context: 'IconScoreContext'):
-        """Release block validation penalty every term end
+    @classmethod
+    def _reset_block_validation_penalty(cls, context: 'IconScoreContext'):
+        """Reset block validation penalty in the end of every term
 
         :param context:
         :return:
         """
 
-        old_preps = self.preps
-
-        for prep in old_preps:
-            if prep.penalty == PenaltyReason.BLOCK_VALIDATION:
+        for prep in context.preps:
+            if prep.penalty == PenaltyReason.BLOCK_VALIDATION and prep.status == PRepStatus.ACTIVE:
                 dirty_prep = context.get_prep(prep.address, mutable=True)
                 dirty_prep.reset_block_validation_penalty()
                 context.put_dirty_prep(dirty_prep)
diff --git a/tests/prep/test_engine.py b/tests/prep/test_engine.py
index 517e3448d..d93b5b8cf 100644
--- a/tests/prep/test_engine.py
+++ b/tests/prep/test_engine.py
@@ -453,11 +453,20 @@ def test_handle_get_prep_term_with_penalized_preps(self):
             assert prep_item["status"] == PRepStatus.ACTIVE.value
             assert prep_item["penalty"] == PenaltyReason.NONE.value
 
+        # The P-Reps which got penalized for consecutive 660 block validation failure
+        # are located at the end of the P-Rep list
+        prev_delegated = -1
         for i, prep_item in enumerate(prep_list[-5:]):
             assert prep_item["address"] == Address.from_prefix_and_int(AddressPrefix.EOA, i)
             assert prep_item["status"] == PRepStatus.ACTIVE.value
             assert prep_item["penalty"] == PenaltyReason.BLOCK_VALIDATION.value
 
+            delegated: int = prep_item["delegated"]
+            if prev_delegated >= 0:
+                assert prev_delegated >= delegated
+
+            prev_delegated = delegated
+
     def test_handle_get_inactive_preps(self):
         expected_block_height = 1234
 
@@ -516,3 +525,62 @@ def test_handle_get_inactive_preps(self):
         assert len(expected_preps) == len(inactive_preps)
         assert expected_block_height == response["blockHeight"]
         assert expected_total_delegated == response["totalDelegated"]
+
+    def test__reset_block_validation_penalty(self):
+        engine = PRepEngine()
+        engine.term = self.term
+        engine.preps = self.preps
+
+        self.term.freeze()
+        self.preps.freeze()
+
+        assert engine.term.is_frozen()
+        assert engine.preps.is_frozen()
+
+        IconScoreContext.engine = Mock()
+        IconScoreContext.storage = Mock()
+        IconScoreContext.engine.prep = engine
+        context = _create_context()
+
+        assert engine.preps.size(active_prep_only=False) == context.preps.size(active_prep_only=False)
+        for i in range(engine.preps.size(active_prep_only=True)):
+            assert engine.preps.get_by_index(i) == context._preps.get_by_index(i)
+
+        # Impose block validation penalty on 5 Main P-Reps
+        indices = set()
+        for _ in range(5):
+            index = random.randint(0, self.main_prep_count - 1)
+            indices.add(index)
+
+        # Impose the block validation penalty on randomly chosen 5 or less than P-Reps
+        for i in indices:
+            prep = context.preps.get_by_index(i)
+            dirty_prep = context.get_prep(prep.address, mutable=True)
+            assert not dirty_prep.is_frozen()
+
+            dirty_prep.penalty = PenaltyReason.BLOCK_VALIDATION
+            dirty_prep.grade = PRepGrade.CANDIDATE
+
+            context.put_dirty_prep(dirty_prep)
+
+        context.update_dirty_prep_batch()
+
+        for i in indices:
+            prep = context.preps.get_by_index(i)
+            assert prep.is_frozen()
+            assert prep.status == PRepStatus.ACTIVE
+            assert prep.penalty == PenaltyReason.BLOCK_VALIDATION
+            assert prep.grade == PRepGrade.CANDIDATE
+
+        engine._reset_block_validation_penalty(context)
+
+        # The penalties of P-Reps in context should be reset
+        # to PenaltyReason.NONE at the last block of the current term
+        for prep in context.preps:
+            assert prep.status == PRepStatus.ACTIVE
+            assert prep.penalty == PenaltyReason.NONE
+
+        for i in indices:
+            prep = context.preps.get_by_index(i)
+            assert prep.status == PRepStatus.ACTIVE
+            assert prep.penalty == PenaltyReason.NONE