refactor(verification): reorganize min_height verification

Author: glevco

hathor/consensus/transaction_consensus.py

@@ -310,7 +310,7 @@ def check_conflicts(self, tx: Transaction) -> None:
 
         # If we got here, either it was a tie or we won.
         # So, let's void the conflict txs.
-        for conflict_tx in conflict_list:
+        for conflict_tx in sorted(conflict_list, key=lambda x: x.timestamp, reverse=True):
             self.mark_as_voided(conflict_tx)
 
         if not tie_list:

hathor/manager.py

@@ -914,6 +914,8 @@ def push_tx(self, tx: Transaction, allow_non_standard_script: bool = False,
         if not tx_from_lib.is_standard(max_output_script_size, not allow_non_standard_script):
             raise NonStandardTxError('Transaction is non standard.')
 
+        tx.update_reward_lock_metadata()
+
         # Validate tx.
         try:
             self.verification_service.verify(tx)

hathor/reward_lock/reward_lock.py

@@ -42,15 +42,16 @@ def get_spent_reward_locked_info(tx: 'Transaction', storage: 'VertexStorageProto
     """Check if any input block reward is locked, returning the locked information if any, or None if they are all
     unlocked."""
     from hathor.transaction.transaction import RewardLockedInfo
+    best_height = get_minimum_best_height(storage)
     for blk in iter_spent_rewards(tx, storage):
-        needed_height = _spent_reward_needed_height(blk, storage)
+        needed_height = _spent_reward_needed_height(blk, best_height)
         if needed_height > 0:
             return RewardLockedInfo(blk.hash, needed_height)
     return None
 
 
-def _spent_reward_needed_height(block: Block, storage: 'VertexStorageProtocol') -> int:
-    """ Returns height still needed to unlock this `block` reward: 0 means it's unlocked."""
+def get_minimum_best_height(storage: 'VertexStorageProtocol') -> int:
+    """Return the height of the current best block that shall be used for `min_height` verification."""
     import math
 
     # omitting timestamp to get the current best block, this will usually hit the cache instead of being slow
@@ -61,6 +62,11 @@ def _spent_reward_needed_height(block: Block, storage: 'VertexStorageProtocol')
         blk = storage.get_block(tip)
         best_height = min(best_height, blk.get_height())
     assert isinstance(best_height, int)
+    return best_height
+
+
+def _spent_reward_needed_height(block: Block, best_height: int) -> int:
+    """ Returns height still needed to unlock this `block` reward: 0 means it's unlocked."""
     spent_height = block.get_height()
     spend_blocks = best_height - spent_height
     settings = get_global_settings()

hathor/simulator/utils.py

@@ -20,15 +20,14 @@
 from hathor.types import Address, VertexId
 
 
-def gen_new_tx(manager: HathorManager, address: str, value: int, verify: bool = True) -> Transaction:
+def gen_new_tx(manager: HathorManager, address: str, value: int) -> Transaction:
     """
     Generate and return a new transaction.
 
     Args:
         manager: the HathorManager to generate the transaction for
         address: an address for the transaction's output
         value: a value for the transaction's output
-        verify: whether to verify the generated transaction
 
     Returns: the generated transaction.
     """
@@ -48,8 +47,6 @@ def gen_new_tx(manager: HathorManager, address: str, value: int, verify: bool =
     tx.weight = 1
     tx.parents = manager.get_new_tx_parents(tx.timestamp)
     manager.cpu_mining_service.resolve(tx)
-    if verify:
-        manager.verification_service.verify(tx)
     return tx
 
 
@@ -111,7 +108,6 @@ def add_new_block(
     if signal_bits is not None:
         block.signal_bits = signal_bits
     manager.cpu_mining_service.resolve(block)
-    manager.verification_service.validate_full(block)
     if propagate:
         manager.propagate_tx(block, fails_silently=False)
     if advance_clock:

hathor/transaction/base_transaction.py

@@ -624,13 +624,14 @@ def get_metadata(self, *, force_reload: bool = False, use_storage: bool = True)
             #        happens include generating new mining blocks and some tests
             height = self.calculate_height() if self.storage else None
             score = self.weight if self.is_genesis else 0
+            min_height = 0 if self.is_genesis else None
 
             metadata = TransactionMetadata(
                 hash=self._hash,
                 accumulated_weight=self.weight,
                 height=height,
                 score=score,
-                min_height=0,
+                min_height=min_height
             )
             self._metadata = metadata
         if not metadata.hash:
@@ -713,8 +714,9 @@ def update_initial_metadata(self, *, save: bool = True) -> None:
         """
         self._update_height_metadata()
         self._update_parents_children_metadata()
-        self._update_reward_lock_metadata()
+        self.update_reward_lock_metadata()
         self._update_feature_activation_bit_counts()
+        self._update_initial_accumulated_weight()
         if save:
             assert self.storage is not None
             self.storage.save_transaction(self, only_metadata=True)
@@ -724,10 +726,13 @@ def _update_height_metadata(self) -> None:
         meta = self.get_metadata()
         meta.height = self.calculate_height()
 
-    def _update_reward_lock_metadata(self) -> None:
+    def update_reward_lock_metadata(self) -> None:
         """Update the txs/block min_height metadata."""
         metadata = self.get_metadata()
-        metadata.min_height = self.calculate_min_height()
+        min_height = self.calculate_min_height()
+        if metadata.min_height is not None:
+            assert metadata.min_height == min_height
+        metadata.min_height = min_height
 
     def _update_parents_children_metadata(self) -> None:
         """Update the txs/block parent's children metadata."""
@@ -749,6 +754,11 @@ def _update_feature_activation_bit_counts(self) -> None:
         # This method lazily calculates and stores the value in metadata
         self.get_feature_activation_bit_counts()
 
+    def _update_initial_accumulated_weight(self) -> None:
+        """Update the vertex initial accumulated_weight."""
+        metadata = self.get_metadata()
+        metadata.accumulated_weight = self.weight
+
     def update_timestamp(self, now: int) -> None:
         """Update this tx's timestamp
 

hathor/transaction/storage/transaction_storage.py

@@ -31,6 +31,7 @@
 from hathor.pubsub import PubSubManager
 from hathor.transaction.base_transaction import BaseTransaction, TxOutput
 from hathor.transaction.block import Block
+from hathor.transaction.exceptions import RewardLocked
 from hathor.transaction.storage.exceptions import (
     TransactionDoesNotExist,
     TransactionIsNotABlock,
@@ -49,6 +50,7 @@
 from hathor.transaction.transaction import Transaction
 from hathor.transaction.transaction_metadata import TransactionMetadata
 from hathor.types import VertexId
+from hathor.verification.transaction_verifier import TransactionVerifier
 
 cpu = get_cpu_profiler()
 
@@ -1097,10 +1099,11 @@ def compute_transactions_that_became_invalid(self, new_best_height: int) -> list
         from hathor.transaction.validation_state import ValidationState
         to_remove: list[BaseTransaction] = []
         for tx in self.iter_mempool_from_best_index():
-            tx_min_height = tx.get_metadata().min_height
-            assert tx_min_height is not None
-            # We use +1 here because a tx is valid if it can be confirmed by the next block
-            if new_best_height + 1 < tx_min_height:
+            try:
+                TransactionVerifier.verify_reward_locked_for_height(
+                    tx, new_best_height, assert_min_height_verification=False
+                )
+            except RewardLocked:
                 tx.set_validation(ValidationState.INVALID)
                 to_remove.append(tx)
         return to_remove

hathor/verification/transaction_verifier.py

@@ -16,6 +16,7 @@
 from hathor.daa import DifficultyAdjustmentAlgorithm
 from hathor.profiler import get_cpu_profiler
 from hathor.reward_lock import get_spent_reward_locked_info
+from hathor.reward_lock.reward_lock import get_minimum_best_height
 from hathor.transaction import BaseTransaction, Transaction, TxInput
 from hathor.transaction.exceptions import (
     ConflictingInputs,
@@ -37,7 +38,6 @@
 from hathor.transaction.transaction import TokenInfo
 from hathor.transaction.util import get_deposit_amount, get_withdraw_amount
 from hathor.types import TokenUid, VertexId
-from hathor.util import not_none
 
 cpu = get_cpu_profiler()
 
@@ -142,12 +142,51 @@ def verify_script(self, *, tx: Transaction, input_tx: TxInput, spent_tx: BaseTra
             raise InvalidInputData(e) from e
 
     def verify_reward_locked(self, tx: Transaction) -> None:
-        """Will raise `RewardLocked` if any reward is spent before the best block height is enough, considering only
-        the block rewards spent by this tx itself, and not the inherited `min_height`."""
-        info = get_spent_reward_locked_info(tx, not_none(tx.storage))
+        """Will raise `RewardLocked` if any reward is spent before the best block height is enough, considering both
+        the block rewards spent by this tx itself, and the inherited `min_height`."""
+        assert tx.storage is not None
+        best_height = get_minimum_best_height(tx.storage)
+        self.verify_reward_locked_for_height(tx, best_height)
+
+    @staticmethod
+    def verify_reward_locked_for_height(
+        tx: Transaction,
+        best_height: int,
+        *,
+        assert_min_height_verification: bool = True
+    ) -> None:
+        """
+        Will raise `RewardLocked` if any reward is spent before the best block height is enough, considering both
+        the block rewards spent by this tx itself, and the inherited `min_height`.
+
+        Args:
+            tx: the transaction to be verified.
+            best_height: the height of the best chain to be used for verification.
+            assert_min_height_verification: whether the inherited `min_height` verification must pass.
+
+        Note: for verification of new transactions, `assert_min_height_verification` must be `True`. This
+        verification is always expected to pass for new txs, as a failure would mean one of its dependencies would
+        have failed too. So an `AssertionError` is raised if it fails.
+
+        However, when txs are being re-verified for Reward Lock during a reorg, it's possible that txs may fail
+        their inherited `min_height` verification. So in that case `assert_min_height_verification` is `False`,
+        and a normal `RewardLocked` exception is raised instead.
+        """
+        assert tx.storage is not None
+        info = get_spent_reward_locked_info(tx, tx.storage)
         if info is not None:
             raise RewardLocked(f'Reward {info.block_hash.hex()} still needs {info.blocks_needed} to be unlocked.')
 
+        meta = tx.get_metadata()
+        assert meta.min_height is not None
+        # We use +1 here because a tx is valid if it can be confirmed by the next block
+        if best_height + 1 < meta.min_height:
+            if assert_min_height_verification:
+                raise AssertionError('a new tx should never be invalid by its inherited min_height.')
+            raise RewardLocked(
+                f'Tx {tx.hash_hex} has min_height={meta.min_height}, but the best_height={best_height}.'
+            )
+
     def verify_number_of_inputs(self, tx: Transaction) -> None:
         """Verify number of inputs is in a valid range"""
         if len(tx.inputs) > self._settings.MAX_NUM_INPUTS:

hathor/vertex_handler/vertex_handler.py

@@ -150,6 +150,8 @@ def _validate_vertex(
             return False
 
         if not metadata.validation.is_fully_connected():
+            # TODO: Remove this from here after a refactor in metadata initialization
+            vertex.update_reward_lock_metadata()
             try:
                 self._verification_service.validate_full(vertex, reject_locked_reward=reject_locked_reward)
             except HathorError as e:

hathor/wallet/resources/send_tokens.py

@@ -127,6 +127,7 @@ def _render_POST_thread(self, values: dict[str, Any], request: Request) -> Union
             weight = self.manager.daa.minimum_tx_weight(tx)
         tx.weight = weight
         self.manager.cpu_mining_service.resolve(tx)
+        tx.update_reward_lock_metadata()
         self.manager.verification_service.verify(tx)
         return tx
 

hathor/wallet/resources/thin_wallet/send_tokens.py

@@ -270,6 +270,7 @@ def _should_stop():
         if context.should_stop_mining_thread:
             raise CancelledError()
         context.tx.update_hash()
+        context.tx.update_reward_lock_metadata()
         self.manager.verification_service.verify(context.tx)
         return context
 

tests/consensus/test_consensus.py

@@ -81,7 +81,6 @@ def test_revert_block_high_weight(self) -> None:
         b0 = tb0.generate_mining_block(manager.rng, storage=manager.tx_storage)
         b0.weight = 10
         manager.cpu_mining_service.resolve(b0)
-        manager.verification_service.verify(b0)
         manager.propagate_tx(b0, fails_silently=False)
 
         b1 = add_new_block(manager, advance_clock=15)
@@ -143,7 +142,6 @@ def test_dont_revert_block_low_weight(self) -> None:
         b0 = manager.generate_mining_block()
         b0.parents = [blocks[-1].hash, conflicting_tx.hash, conflicting_tx.parents[0]]
         manager.cpu_mining_service.resolve(b0)
-        manager.verification_service.verify(b0)
         manager.propagate_tx(b0, fails_silently=False)
 
         b1 = add_new_block(manager, advance_clock=15)
@@ -199,7 +197,6 @@ def test_dont_revert_block_high_weight_transaction_verify_other(self) -> None:
         b0 = tb0.generate_mining_block(manager.rng, storage=manager.tx_storage)
         b0.weight = 10
         manager.cpu_mining_service.resolve(b0)
-        manager.verification_service.verify(b0)
         manager.propagate_tx(b0, fails_silently=False)
 
         b1 = add_new_block(manager, advance_clock=15)
@@ -253,7 +250,6 @@ def test_dont_revert_block_high_weight_verify_both(self) -> None:
         b0.parents = [b0.parents[0], conflicting_tx.hash, conflicting_tx.parents[0]]
         b0.weight = 10
         manager.cpu_mining_service.resolve(b0)
-        manager.verification_service.verify(b0)
         manager.propagate_tx(b0, fails_silently=False)
 
         b1 = add_new_block(manager, advance_clock=15)

tests/event/test_event_reorg.py

@@ -36,7 +36,6 @@ def test_reorg_events(self) -> None:
         b0 = tb0.generate_mining_block(self.manager.rng, storage=self.manager.tx_storage, address=BURN_ADDRESS)
         b0.weight = 10
         self.manager.cpu_mining_service.resolve(b0)
-        self.manager.verification_service.verify(b0)
         self.manager.propagate_tx(b0, fails_silently=False)
         self.log.debug('reorg block propagated')
         self.run_to_completion()

tests/feature_activation/test_feature_simulation.py

@@ -228,6 +228,7 @@ def test_feature(self) -> None:
             non_signaling_block = manager.generate_mining_block()
             manager.cpu_mining_service.resolve(non_signaling_block)
             non_signaling_block.signal_bits = 0b10
+            non_signaling_block.update_reward_lock_metadata()
 
             with pytest.raises(BlockMustSignalError):
                 manager.verification_service.verify(non_signaling_block)

tests/p2p/test_sync.py

@@ -44,7 +44,6 @@ def _add_new_tx(self, address: str, value: int) -> Transaction:
         tx.weight = 10
         tx.parents = self.manager1.get_new_tx_parents()
         self.manager1.cpu_mining_service.resolve(tx)
-        self.manager1.verification_service.verify(tx)
         self.manager1.propagate_tx(tx)
         self.clock.advance(10)
         return tx
@@ -61,7 +60,6 @@ def _add_new_transactions(self, num_txs: int) -> list[Transaction]:
     def _add_new_block(self, propagate: bool = True) -> Block:
         block: Block = self.manager1.generate_mining_block()
         self.assertTrue(self.manager1.cpu_mining_service.resolve(block))
-        self.manager1.verification_service.verify(block)
         self.manager1.on_new_tx(block, propagate_to_peers=propagate)
         self.clock.advance(10)
         return block

tests/p2p/test_sync_mempool.py

@@ -36,7 +36,6 @@ def _add_new_tx(self, address: str, value: int) -> Transaction:
         tx.weight = 10
         tx.parents = self.manager1.get_new_tx_parents()
         self.manager1.cpu_mining_service.resolve(tx)
-        self.manager1.verification_service.verify(tx)
         self.manager1.propagate_tx(tx)
         self.clock.advance(10)
         return tx
@@ -53,7 +52,6 @@ def _add_new_transactions(self, num_txs: int) -> list[Transaction]:
     def _add_new_block(self, propagate: bool = True) -> Block:
         block: Block = self.manager1.generate_mining_block()
         self.assertTrue(self.manager1.cpu_mining_service.resolve(block))
-        self.manager1.verification_service.verify(block)
         self.manager1.on_new_tx(block, propagate_to_peers=propagate)
         self.clock.advance(10)
         return block

tests/p2p/test_sync_v2.py

@@ -276,7 +276,7 @@ def test_receiving_tips_limit(self) -> None:
         # Custom tx generator that generates tips
         parents = manager1.get_new_tx_parents(manager1.tx_storage.latest_timestamp)
 
-        def custom_gen_new_tx(manager: HathorManager, _address: str, value: int, verify: bool = True) -> Transaction:
+        def custom_gen_new_tx(manager: HathorManager, _address: str, value: int) -> Transaction:
             outputs = []
             # XXX: burn address guarantees that this output will not be used as input for any following transactions
             # XXX: reduce value to make sure we can generate more transactions, otherwise it will spend a linear random
@@ -294,8 +294,6 @@ def custom_gen_new_tx(manager: HathorManager, _address: str, value: int, verify:
             # XXX: fixed parents is the final requirement to make all the generated new tips
             tx.parents = parents
             manager.cpu_mining_service.resolve(tx)
-            if verify:
-                manager.verification_service.verify(tx)
             return tx
 
         # Generate 100 tx-tips in mempool.

tests/resources/transaction/test_mining.py

@@ -38,7 +38,7 @@ def test_get_block_template_with_address(self):
                 'accumulated_weight': 1.0,
                 'score': 0,
                 'height': 1,
-                'min_height': 0,
+                'min_height': None,
                 'first_block': None,
                 'feature_activation_bit_counts': None
             },
@@ -71,7 +71,7 @@ def test_get_block_template_without_address(self):
                 'accumulated_weight': 1.0,
                 'score': 0,
                 'height': 1,
-                'min_height': 0,
+                'min_height': None,
                 'first_block': None,
                 'feature_activation_bit_counts': None
             },