IS-1174: Fix PRepConverter Delete Logic (#491)

* Fix PRepCOnverter Delete Logic

iconservice/icon_service_engine.py

@@ -753,7 +753,7 @@ def _update_productivity(cls,
 
  for address, vote_state in validators:
  dirty_prep: Optional['PRep'] = context.get_prep(address, mutable=True)
- assert isinstance(dirty_prep, PRep)
+ assert isinstance(dirty_prep, PRep), f"dirty_prep: {address}"
 
  is_validator: bool = vote_state != BlockVoteStatus.NONE.value
 
@@ -776,7 +776,7 @@ def _update_last_generate_block_height(cls,
  return
 
  dirty_prep: 'PRep' = context.get_prep(prev_block_generator, mutable=True)
- assert isinstance(dirty_prep, PRep)
+ assert isinstance(dirty_prep, PRep), f"dirty_prep: {address}"
 
  dirty_prep.last_generate_block_height = context.block.height - 1
  context.put_dirty_prep(dirty_prep)

iconservice/iconscore/icon_score_context.py

@@ -242,7 +242,8 @@ def _update_prep_address_converter(self, dirty_prep: 'PRep'):
  prev_node=old_prep.node_address)
  elif dirty_prep.status != PRepStatus.ACTIVE:
  # unregisterPRep or disqualified by productivity penalty
- self._prep_address_converter.delete_node_address(node=dirty_prep.node_address)
+ self._prep_address_converter.delete_node_address(node=dirty_prep.node_address,
+ prep=dirty_prep.address)
 
  def _update_term(self, dirty_prep: 'PRep'):
  """Update term info with dirty_prep

iconservice/prep/penalty_imposer.py

@@ -54,11 +54,15 @@ def run(self,
 
  if self._check_block_validation_penalty(prep):
  Logger.info(f"PenaltyImposer statistics({PenaltyReason.BLOCK_VALIDATION}): "
+ f"prep: {prep.address} "
+ f"node_address: {prep.node_address} "
  f"prep_total_blocks: {prep.total_blocks} "
  f"prep_block_validation_proportion: {prep.block_validation_proportion}")
  reason = PenaltyReason.BLOCK_VALIDATION
  if self._check_low_productivity_penalty(prep):
  Logger.info(f"PenaltyImposer statistics({PenaltyReason.LOW_PRODUCTIVITY}): "
+ f"prep: {prep.address} "
+ f"node_address: {prep.node_address} "
  f"prep_total_blocks: {prep.total_blocks} "
  f"prep_unvalidated_sequence_blocks: {prep.unvalidated_sequence_blocks}")
  reason = PenaltyReason.LOW_PRODUCTIVITY

iconservice/prep/prep_address_converter.py

@@ -71,7 +71,11 @@ def add_node_address(self, node: 'Address', prep: 'Address'):
  raise InvalidParamsException(f"nodeAddress already in use: {node}")
  self._node_address_mapper[node] = prep
 
- def delete_node_address(self, node: 'Address'):
+ def delete_node_address(self, node: 'Address', prep: 'Address'):
+ self._add_prev_node_address(node=node, prep=prep)
+ self._delete_node_address(node)
+
+ def _delete_node_address(self, node: 'Address'):
  if node in self._node_address_mapper:
  del self._node_address_mapper[node]
 
@@ -81,7 +85,7 @@ def _add_prev_node_address(self, node: 'Address', prep: 'Address'):
 
  def replace_node_address(self, node: 'Address', prep: 'Address', prev_node: 'Address'):
  self._add_prev_node_address(node=prev_node, prep=prep)
- self.delete_node_address(node=prev_node)
+ self._delete_node_address(node=prev_node)
  self.add_node_address(node=node, prep=prep)
 
  def copy(self) -> 'PRepAddressConverter':
@@ -99,6 +103,7 @@ def validate_node_address(self,
 
  def get_prep_address_from_node_address(self,
  node_address: 'Address') -> 'Address':
-
- return self._prev_node_address_mapper.get(node_address,
- self._node_address_mapper.get(node_address, node_address))
+ ret: 'Address' = self._node_address_mapper.get(node_address)
+ if ret is None:
+ ret = self._prev_node_address_mapper.get(node_address, node_address)
+ return ret

tests/integrate_test/iiss/decentralized/test_preps_divide_node_address.py

@@ -150,10 +150,10 @@ def test_prep_set_node_address_check_generator(self):
  prev_block_generator = self._accounts[0].address
  prev_block_votes = [(x.address, True) for x in self._accounts[1:PREP_MAIN_PREPS]]
  block, tx_results, _, _, next_preps = self.debug_make_and_req_block(tx_list=[],
-  prev_block_generator=prev_block_generator,
-  prev_block_validators=None,
-  prev_block_votes=prev_block_votes,
-  block=None)
+ prev_block_generator=prev_block_generator,
+ prev_block_validators=None,
+ prev_block_votes=prev_block_votes,
+ block=None)
  self.assertEqual(tx_results[0].status, True)
  self.assertEqual(next_preps, None)
 
@@ -169,10 +169,10 @@ def test_prep_set_node_address_check_generator(self):
 
  prev_block_votes = [(x.address, True) for x in self._accounts[1:PREP_MAIN_PREPS]]
  block, tx_results, _, _, next_preps = self.debug_make_and_req_block(tx_list=[tx],
-  prev_block_generator=prev_block_generator,
-  prev_block_validators=None,
-  prev_block_votes=prev_block_votes,
-  block=None)
+ prev_block_generator=prev_block_generator,
+ prev_block_validators=None,
+ prev_block_votes=prev_block_votes,
+ block=None)
  self.assertEqual(tx_results[0].status, True)
  self.assertEqual(next_preps["preps"][0]["id"], dummy_node2)
  self._write_precommit_state(block)
@@ -207,10 +207,10 @@ def test_prep_set_node_address_check_votes(self):
  prev_block_generator = self._accounts[0].address
  prev_block_votes = [(x.address, True) for x in self._accounts[1:PREP_MAIN_PREPS]]
  block, tx_results, _, _, next_preps = self.debug_make_and_req_block(tx_list=[],
-  prev_block_generator=prev_block_generator,
-  prev_block_validators=None,
-  prev_block_votes=prev_block_votes,
-  block=None)
+ prev_block_generator=prev_block_generator,
+ prev_block_validators=None,
+ prev_block_votes=prev_block_votes,
+ block=None)
  self.assertEqual(tx_results[0].status, True)
  self.assertEqual(next_preps, None)
 
@@ -226,10 +226,10 @@ def test_prep_set_node_address_check_votes(self):
 
  prev_block_votes = [(x.address, True) for x in self._accounts[1:PREP_MAIN_PREPS]]
  block, tx_results, _, _, next_preps = self.debug_make_and_req_block(tx_list=[tx],
-  prev_block_generator=prev_block_generator,
-  prev_block_validators=None,
-  prev_block_votes=prev_block_votes,
-  block=None)
+ prev_block_generator=prev_block_generator,
+ prev_block_validators=None,
+ prev_block_votes=prev_block_votes,
+ block=None)
  self.assertEqual(tx_results[0].status, True)
  self.assertEqual(next_preps["preps"][1]["id"], dummy_node2)
  self._write_precommit_state(block)
@@ -474,3 +474,107 @@ def test_scenario6(self):
  # After calling write_precommit_state()
  ret: Dict[str, Union[str, int, bytes, 'Address']] = self.get_prep(prep_a)
  assert ret["nodeAddress"] == prep_a.address
+
+ def test_change_node_prep1(self):
+ # 1 block
+ # PRepA a ---- z
+ # penalty PRepA (low productivity)
+
+ self.set_revision(Revision.DIVIDE_NODE_ADDRESS.value)
+
+ self.distribute_icx(accounts=self._accounts[:PREP_MAIN_PREPS],
+ init_balance=1 * ICX_IN_LOOP)
+
+ # PRepA: 0
+ # PRepB: 1
+ prep_a: 'EOAAccount' = self._accounts[0]
+ node_address: 'Address' = create_address()
+
+ tx_list: list = [
+ self.create_set_prep_tx(
+ from_=prep_a,
+ set_data={
+ "nodeAddress": str(node_address)
+ }
+ )
+ ]
+
+ self.process_confirm_block_tx(
+ tx_list,
+ prev_block_generator=None,
+ prev_block_validators=None
+ )
+
+ PREV_PENALTY_GRACE_PERIOD = IconScoreContext.engine.prep._penalty_imposer._penalty_grace_period
+ PREV_LOW_PRODUCTIVITY_PENALTY_THRESHOLD = \
+ IconScoreContext.engine.prep._penalty_imposer._low_productivity_penalty_threshold
+
+ PENALTY_GRACE_PERIOD = 0
+ # enable low productivity
+ LOW_PRODUCTIVITY_PENALTY_THRESHOLD = 1
+
+ IconScoreContext.engine.prep._penalty_imposer._penalty_grace_period = PENALTY_GRACE_PERIOD
+ IconScoreContext.engine.prep._penalty_imposer._low_productivity_penalty_threshold = \
+ LOW_PRODUCTIVITY_PENALTY_THRESHOLD
+
+ votes = [[node_address, False]] + \
+ [[account.address, True] for account in self._accounts[2:PREP_MAIN_PREPS]]
+ tx_results = self.make_blocks(to=self._block_height + 2,
+ prev_block_generator=self._accounts[1].address,
+ prev_block_votes=votes)
+
+ # assert Error!
+ with self.assertRaises(AssertionError) as e:
+ self.make_blocks(
+ to=self._block_height + 1,
+ prev_block_generator=self._accounts[1].address,
+ prev_block_votes=votes)
+
+ self.assertEqual(e.exception.args[0], f"dirty_prep: {node_address}")
+
+ IconScoreContext.engine.prep._penalty_imposer._penalty_grace_period = PREV_PENALTY_GRACE_PERIOD
+ IconScoreContext.engine.prep._penalty_imposer._low_productivity_penalty_threshold = \
+ PREV_LOW_PRODUCTIVITY_PENALTY_THRESHOLD
+
+ def test_change_node_prep2(self):
+ # 1 block
+ # PRepA a ---- z
+ # unreg PRepA
+
+ self.set_revision(Revision.DIVIDE_NODE_ADDRESS.value)
+
+ self.distribute_icx(accounts=self._accounts[:PREP_MAIN_PREPS],
+ init_balance=1 * ICX_IN_LOOP)
+
+ # PRepA: 0
+ # PRepB: 1
+ prep_a: 'EOAAccount' = self._accounts[0]
+ node_address: 'Address' = create_address()
+
+ tx_list: list = [
+ self.create_set_prep_tx(
+ from_=prep_a,
+ set_data={
+ "nodeAddress": str(node_address)
+ }
+ )
+ ]
+
+ self.process_confirm_block_tx(tx_list)
+
+ self.unregister_prep(prep_a)
+
+ votes = [[node_address, False]] + \
+ [[account.address, True] for account in self._accounts[2:PREP_MAIN_PREPS]]
+ tx_results = self.make_blocks(to=self._block_height + 1,
+ prev_block_generator=self._accounts[1].address,
+ prev_block_votes=votes)
+
+ # assert Error!
+ with self.assertRaises(AssertionError) as e:
+ self.make_blocks(
+ to=self._block_height + 1,
+ prev_block_generator=self._accounts[1].address,
+ prev_block_votes=votes)
+
+ self.assertEqual(e.exception.args[0], f"dirty_prep: {node_address}")

tests/legacy_unittest/prep/test_prep_address_converter.py

@@ -46,7 +46,7 @@ def test_add_node_address(self):
  address = self.converter.get_prep_address_from_node_address(node_address)
  assert address == prep_address
 
- self.converter.delete_node_address(node_address)
+ self.converter._delete_node_address(node_address)
  address = self.converter.get_prep_address_from_node_address(node_address)
  assert address == node_address
  assert address != prep_address
@@ -81,6 +81,31 @@ def test_replace_node_address(self):
  assert len(converter._prev_node_address_mapper) == 1
  assert len(converter._node_address_mapper) == 1
 
+ def test_delete_node_address(self):
+ converter = self.converter
+ node_address = self.node_addresses[0]
+ prep_address = self.prep_addresses[0]
+
+ # Confirm that 2 addresses are different
+ assert node_address != prep_address
+
+ # Check whether old_node_address is not added to converter
+ address = self.converter.get_prep_address_from_node_address(node_address)
+ assert address == node_address
+
+ # Add old_node_address
+ converter.add_node_address(node_address, prep_address)
+ address = self.converter.get_prep_address_from_node_address(node_address)
+ assert address == prep_address
+
+ converter.delete_node_address(node_address, prep_address)
+ address = converter.get_prep_address_from_node_address(node_address)
+ assert address == prep_address
+ assert node_address in converter._prev_node_address_mapper
+ assert node_address not in converter._node_address_mapper
+ assert len(converter._prev_node_address_mapper) == 1
+ assert len(converter._node_address_mapper) == 0
+
  def test_copy(self):
  converter = self.converter
  old_node_address = self.node_addresses[0]
@@ -106,7 +131,7 @@ def test_copy(self):
  assert id(new_converter._prev_node_address_mapper) != id(converter._prev_node_address_mapper)
  assert id(new_converter._node_address_mapper) != id(converter._node_address_mapper)
 
- new_converter.delete_node_address(new_node_address)
+ new_converter._delete_node_address(new_node_address)
  assert new_node_address == new_converter.get_prep_address_from_node_address(new_node_address)
  assert prep_address == converter.get_prep_address_from_node_address(new_node_address)