fix(feature-activation): fix get ancestor

hathor/feature_activation/feature_service.py

@@ -98,7 +98,7 @@ def get_state(self, *, block: 'Block', feature: Feature) -> FeatureState:
         offset_to_previous_boundary = offset_to_boundary or self._feature_settings.evaluation_interval
         previous_boundary_height = height - offset_to_previous_boundary
         assert previous_boundary_height >= 0
-        previous_boundary_block = self._get_ancestor_at_height(block=block, height=previous_boundary_height)
+        previous_boundary_block = self._get_ancestor_at_height(block=block, ancestor_height=previous_boundary_height)
         previous_boundary_state = self.get_state(block=previous_boundary_block, feature=feature)
 
         # We cache _and save_ the state of the previous boundary block that we just got.
@@ -198,32 +198,44 @@ def get_bits_description(self, *, block: 'Block') -> dict[Feature, FeatureDescri
             for feature, criteria in self._feature_settings.features.items()
         }
 
-    def _get_ancestor_at_height(self, *, block: 'Block', height: int) -> 'Block':
+    def _get_ancestor_at_height(self, *, block: 'Block', ancestor_height: int) -> 'Block':
         """
-        Given a block, returns its ancestor at a specific height.
+        Given a block, return its ancestor at a specific height.
         Uses the height index if the block is in the best blockchain, or search iteratively otherwise.
         """
-        assert height < block.get_height(), (
-            f"ancestor height must be lower than the block's height: {height} >= {block.get_height()}"
+        assert ancestor_height < block.get_height(), (
+            f"ancestor height must be lower than the block's height: {ancestor_height} >= {block.get_height()}"
         )
 
-        metadata = block.get_metadata()
+        # It's possible that this method is called before the consensus runs for this block, therefore we do not know
+        # if it's in the best blockchain. For this reason, we have to get the ancestor starting from our parent block.
+        parent_block = block.get_block_parent()
+        parent_metadata = parent_block.get_metadata()
+        assert parent_metadata.validation.is_fully_connected(), 'The parent should always be fully validated.'
 
-        if not metadata.voided_by and (ancestor := self._tx_storage.get_transaction_by_height(height)):
+        if parent_block.get_height() == ancestor_height:
+            return parent_block
+
+        if not parent_metadata.voided_by and (ancestor := self._tx_storage.get_transaction_by_height(ancestor_height)):
             from hathor.transaction import Block
             assert isinstance(ancestor, Block)
             return ancestor
 
-        return _get_ancestor_iteratively(block=block, ancestor_height=height)
-
+        return self._get_ancestor_iteratively(block=parent_block, ancestor_height=ancestor_height)
 
-def _get_ancestor_iteratively(*, block: 'Block', ancestor_height: int) -> 'Block':
-    """Given a block, returns its ancestor at a specific height by iterating over its ancestors. This is slow."""
-    # TODO: there are further optimizations to be done here, the latest common block height could be persisted in
-    #  metadata, so we could still use the height index if the requested height is before that height.
-    assert ancestor_height >= 0
-    ancestor = block
-    while ancestor.get_height() > ancestor_height:
-        ancestor = ancestor.get_block_parent()
+    def _get_ancestor_iteratively(self, *, block: 'Block', ancestor_height: int) -> 'Block':
+        """
+        Given a block, return its ancestor at a specific height by iterating over its ancestors.
+        This is slower than using the height index.
+        """
+        # TODO: there are further optimizations to be done here, the latest common block height could be persisted in
+        #  metadata, so we could still use the height index if the requested height is before that height.
+        assert ancestor_height >= 0
+        assert block.get_height() - ancestor_height <= self._feature_settings.evaluation_interval, (
+            'requested ancestor is deeper than the maximum allowed'
+        )
+        ancestor = block
+        while ancestor.get_height() > ancestor_height:
+            ancestor = ancestor.get_block_parent()
 
-    return ancestor
+        return ancestor

tests/feature_activation/test_feature_service.py

@@ -29,8 +29,9 @@
 from hathor.feature_activation.model.feature_description import FeatureDescription
 from hathor.feature_activation.model.feature_state import FeatureState
 from hathor.feature_activation.settings import Settings as FeatureSettings
-from hathor.transaction import Block
+from hathor.transaction import Block, TransactionMetadata
 from hathor.transaction.storage import TransactionStorage
+from hathor.transaction.validation_state import ValidationState
 
 
 def _get_blocks_and_storage() -> tuple[list[Block], TransactionStorage]:
@@ -72,19 +73,20 @@ def _get_blocks_and_storage() -> tuple[list[Block], TransactionStorage]:
         0b0000,
     ]
     storage = Mock()
-    storage.get_metadata = Mock(return_value=None)
 
-    for i, bits in enumerate(feature_activation_bits):
-        block_hash = genesis_hash if i == 0 else int.to_bytes(i, length=1, byteorder='big')
+    for height, bits in enumerate(feature_activation_bits):
+        block_hash = genesis_hash if height == 0 else int.to_bytes(height, length=1, byteorder='big')
         block = Block(hash=block_hash, storage=storage, signal_bits=bits)
         blocks.append(block)
-        parent_hash = blocks[i - 1].hash
+        parent_hash = blocks[height - 1].hash
         assert parent_hash is not None
         block.parents = [parent_hash]
+        block._metadata = TransactionMetadata(height=height)
+        block._metadata.validation = ValidationState.FULL
 
     block_by_hash = {block.hash: block for block in blocks}
     storage.get_transaction = Mock(side_effect=lambda hash_bytes: block_by_hash[hash_bytes])
-    storage.get_transaction_by_height = Mock(side_effect=lambda height: blocks[height])
+    storage.get_transaction_by_height = Mock(side_effect=lambda h: blocks[h])
 
     return blocks, storage
 
@@ -597,7 +599,7 @@ def test_get_ancestor_at_height_invalid(
     block = block_mocks[block_height]
 
     with pytest.raises(AssertionError) as e:
-        service._get_ancestor_at_height(block=block, height=ancestor_height)
+        service._get_ancestor_at_height(block=block, ancestor_height=ancestor_height)
 
     assert str(e.value) == (
         f"ancestor height must be lower than the block's height: {ancestor_height} >= {block_height}"
@@ -624,21 +626,22 @@ def test_get_ancestor_at_height(
 ) -> None:
     service = FeatureService(feature_settings=feature_settings, tx_storage=tx_storage)
     block = block_mocks[block_height]
-    result = service._get_ancestor_at_height(block=block, height=ancestor_height)
+    result = service._get_ancestor_at_height(block=block, ancestor_height=ancestor_height)
 
     assert result == block_mocks[ancestor_height]
     assert result.get_height() == ancestor_height
-    assert cast(Mock, tx_storage.get_transaction_by_height).call_count == 1
+    assert cast(Mock, tx_storage.get_transaction_by_height).call_count == (
+        0 if block_height - ancestor_height <= 1 else 1
+    ), 'this should only be called if the ancestor is deeper than one parent away'
 
 
 @pytest.mark.parametrize(
     ['block_height', 'ancestor_height'],
     [
         (21, 20),
-        (21, 10),
-        (21, 0),
+        (21, 18),
+        (15, 12),
         (15, 10),
-        (15, 0),
         (1, 0),
     ]
 )
@@ -651,8 +654,9 @@ def test_get_ancestor_at_height_voided(
 ) -> None:
     service = FeatureService(feature_settings=feature_settings, tx_storage=tx_storage)
     block = block_mocks[block_height]
-    block.get_metadata().voided_by = {b'some'}
-    result = service._get_ancestor_at_height(block=block, height=ancestor_height)
+    parent_block = block_mocks[block_height - 1]
+    parent_block.get_metadata().voided_by = {b'some'}
+    result = service._get_ancestor_at_height(block=block, ancestor_height=ancestor_height)
 
     assert result == block_mocks[ancestor_height]
     assert result.get_height() == ancestor_height

tests/feature_activation/test_feature_simulation.py

@@ -19,7 +19,6 @@
 
 from hathor.builder import Builder
 from hathor.conf.get_settings import get_global_settings
-from hathor.feature_activation import feature_service as feature_service_module
 from hathor.feature_activation.feature import Feature
 from hathor.feature_activation.feature_service import FeatureService
 from hathor.feature_activation.model.criteria import Criteria
@@ -88,11 +87,11 @@ def test_feature(self) -> None:
         web_client = StubSite(feature_resource)
 
         calculate_new_state_mock = Mock(wraps=feature_service._calculate_new_state)
-        get_ancestor_iteratively_mock = Mock(wraps=feature_service_module._get_ancestor_iteratively)
+        get_ancestor_iteratively_mock = Mock(wraps=feature_service._get_ancestor_iteratively)
 
         with (
             patch.object(FeatureService, '_calculate_new_state', calculate_new_state_mock),
-            patch.object(feature_service_module, '_get_ancestor_iteratively', get_ancestor_iteratively_mock)
+            patch.object(FeatureService, '_get_ancestor_iteratively', get_ancestor_iteratively_mock),
         ):
             # at the beginning, the feature is DEFINED:
             add_new_blocks(manager, 10)
@@ -578,11 +577,11 @@ def test_feature_from_existing_storage(self) -> None:
         web_client = StubSite(feature_resource)
 
         calculate_new_state_mock = Mock(wraps=feature_service1._calculate_new_state)
-        get_ancestor_iteratively_mock = Mock(wraps=feature_service_module._get_ancestor_iteratively)
+        get_ancestor_iteratively_mock = Mock(wraps=feature_service1._get_ancestor_iteratively)
 
         with (
             patch.object(FeatureService, '_calculate_new_state', calculate_new_state_mock),
-            patch.object(feature_service_module, '_get_ancestor_iteratively', get_ancestor_iteratively_mock)
+            patch.object(FeatureService, '_get_ancestor_iteratively', get_ancestor_iteratively_mock),
         ):
             assert artifacts1.tx_storage.get_vertices_count() == 3  # genesis vertices in the storage
 
@@ -631,11 +630,11 @@ def test_feature_from_existing_storage(self) -> None:
         web_client = StubSite(feature_resource)
 
         calculate_new_state_mock = Mock(wraps=feature_service._calculate_new_state)
-        get_ancestor_iteratively_mock = Mock(wraps=feature_service_module._get_ancestor_iteratively)
+        get_ancestor_iteratively_mock = Mock(wraps=feature_service._get_ancestor_iteratively)
 
         with (
             patch.object(FeatureService, '_calculate_new_state', calculate_new_state_mock),
-            patch.object(feature_service_module, '_get_ancestor_iteratively', get_ancestor_iteratively_mock)
+            patch.object(FeatureService, '_get_ancestor_iteratively', get_ancestor_iteratively_mock),
         ):
             # the new storage starts populated
             assert artifacts2.tx_storage.get_vertices_count() == 67