diff --git a/iconservice/icon_service_engine.py b/iconservice/icon_service_engine.py index ba810bae0..630a58f31 100644 --- a/iconservice/icon_service_engine.py +++ b/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) diff --git a/iconservice/iconscore/icon_score_context.py b/iconservice/iconscore/icon_score_context.py index a3493c779..47d220a9a 100644 --- a/iconservice/iconscore/icon_score_context.py +++ b/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 diff --git a/iconservice/prep/penalty_imposer.py b/iconservice/prep/penalty_imposer.py index b2224e0fa..cd2e85dd8 100644 --- a/iconservice/prep/penalty_imposer.py +++ b/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 diff --git a/iconservice/prep/prep_address_converter.py b/iconservice/prep/prep_address_converter.py index 98933341e..156732b13 100644 --- a/iconservice/prep/prep_address_converter.py +++ b/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 diff --git a/tests/integrate_test/iiss/decentralized/test_preps_divide_node_address.py b/tests/integrate_test/iiss/decentralized/test_preps_divide_node_address.py index 181ce57be..239da14ef 100644 --- a/tests/integrate_test/iiss/decentralized/test_preps_divide_node_address.py +++ b/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}") diff --git a/tests/legacy_unittest/prep/test_prep_address_converter.py b/tests/legacy_unittest/prep/test_prep_address_converter.py index ebc1d00a8..72f48d5aa 100644 --- a/tests/legacy_unittest/prep/test_prep_address_converter.py +++ b/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)