review changes 3

Author: glevco

hathor/feature_activation/feature_service.py

@@ -12,7 +12,8 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 
-from typing import TYPE_CHECKING
+from dataclasses import dataclass
+from typing import TYPE_CHECKING, TypeAlias
 
 from hathor.feature_activation.feature import Feature
 from hathor.feature_activation.model.feature_description import FeatureDescription
@@ -24,6 +25,21 @@
     from hathor.transaction.storage import TransactionStorage
 
 
+@dataclass(frozen=True, slots=True)
+class BlockIsSignaling:
+    """Represent that a block is correctly signaling support for all currently mandatory features."""
+    pass
+
+
+@dataclass(frozen=True, slots=True)
+class BlockIsMissingSignal:
+    """Represent that a block is not signaling support for at least one currently mandatory feature."""
+    feature: Feature
+
+
+BlockSignalingState: TypeAlias = BlockIsSignaling | BlockIsMissingSignal
+
+
 class FeatureService:
     __slots__ = ('_feature_settings', '_tx_storage')
 
@@ -37,7 +53,7 @@ def is_feature_active(self, *, block: 'Block', feature: Feature) -> bool:
 
         return state == FeatureState.ACTIVE
 
-    def is_signaling_mandatory_features(self, block: 'Block') -> bool:
+    def is_signaling_mandatory_features(self, block: 'Block') -> BlockSignalingState:
         """
         Return whether a block is signaling features that are mandatory, that is, any feature currently in the
         MUST_SIGNAL phase.
@@ -60,9 +76,9 @@ def is_signaling_mandatory_features(self, block: 'Block') -> bool:
             missing_signals = threshold - count
 
             if missing_signals > remaining_blocks:
-                return False
+                return BlockIsMissingSignal(feature=feature)
 
-        return True
+        return BlockIsSignaling()
 
     def get_state(self, *, block: 'Block', feature: Feature) -> FeatureState:
         """Returns the state of a feature at a certain block. Uses block metadata to cache states."""

hathor/transaction/transaction_metadata.py

@@ -53,7 +53,8 @@ class TransactionMetadata:
     feature_activation_bit_counts: Optional[list[int]]
 
     # A dict of features in the feature activation process and their respective state. Must only be used by Blocks,
-    # is None otherwise.
+    # is None otherwise. This is only used for caching, so it can be safely cleared up, as it would be recalculated
+    # when necessary.
     feature_states: Optional[dict[Feature, FeatureState]] = None
     # It must be a weakref.
     _tx_ref: Optional['ReferenceType[BaseTransaction]']

hathor/verification/block_verifier.py

@@ -14,7 +14,7 @@
 
 from hathor import daa
 from hathor.conf.settings import HathorSettings
-from hathor.feature_activation.feature_service import FeatureService
+from hathor.feature_activation.feature_service import BlockIsMissingSignal, BlockIsSignaling, FeatureService
 from hathor.profiler import get_cpu_profiler
 from hathor.transaction import BaseTransaction, Block
 from hathor.transaction.exceptions import (
@@ -125,5 +125,15 @@ def verify_mandatory_signaling(self, block: Block) -> None:
 
         assert self._feature_service is not None
 
-        if not self._feature_service.is_signaling_mandatory_features(block):
-            raise BlockMustSignalError()
+        signaling_state = self._feature_service.is_signaling_mandatory_features(block)
+
+        match signaling_state:
+            case BlockIsSignaling():
+                return
+            case BlockIsMissingSignal(feature):
+                raise BlockMustSignalError(
+                    f"Block must signal support for feature '{feature.value}' during MUST_SIGNAL phase."
+                )
+            case _:
+                # TODO: This will be changed to assert_never() so mypy can check it.
+                raise NotImplementedError

tests/feature_activation/test_feature_service.py

@@ -19,7 +19,12 @@
 
 from hathor.conf import HathorSettings
 from hathor.feature_activation.feature import Feature
-from hathor.feature_activation.feature_service import FeatureService
+from hathor.feature_activation.feature_service import (
+    BlockIsMissingSignal,
+    BlockIsSignaling,
+    BlockSignalingState,
+    FeatureService,
+)
 from hathor.feature_activation.model.criteria import Criteria
 from hathor.feature_activation.model.feature_description import FeatureDescription
 from hathor.feature_activation.model.feature_state import FeatureState
@@ -655,29 +660,29 @@ def test_get_ancestor_at_height_voided(
 
 
 @pytest.mark.parametrize(
-    ['bit', 'threshold', 'block_height', 'is_signaling'],
+    ['bit', 'threshold', 'block_height', 'signaling_state'],
     [
-        (0, 4, 0, True),
-        (0, 4, 3, True),
-        (0, 4, 7, True),
-        (0, 4, 8, True),
-        (0, 4, 11, True),
-        (0, 4, 12, True),
-
-        (1, 4, 0, True),
-        (1, 4, 3, True),
-        (1, 4, 7, True),
-        (1, 4, 8, True),
-        (1, 4, 9, True),
-        (1, 4, 10, False),
-        (1, 4, 11, False),
-        (1, 4, 12, True),
-
-        (2, 2, 8, True),
-        (2, 2, 9, True),
-        (2, 2, 10, True),
-        (2, 2, 11, False),
-        (2, 2, 12, True),
+        (0, 4, 0, BlockIsSignaling()),
+        (0, 4, 3, BlockIsSignaling()),
+        (0, 4, 7, BlockIsSignaling()),
+        (0, 4, 8, BlockIsSignaling()),
+        (0, 4, 11, BlockIsSignaling()),
+        (0, 4, 12, BlockIsSignaling()),
+
+        (1, 4, 0, BlockIsSignaling()),
+        (1, 4, 3, BlockIsSignaling()),
+        (1, 4, 7, BlockIsSignaling()),
+        (1, 4, 8, BlockIsSignaling()),
+        (1, 4, 9, BlockIsSignaling()),
+        (1, 4, 10, BlockIsMissingSignal(feature=Feature.NOP_FEATURE_1)),
+        (1, 4, 11, BlockIsMissingSignal(feature=Feature.NOP_FEATURE_1)),
+        (1, 4, 12, BlockIsSignaling()),
+
+        (2, 2, 8, BlockIsSignaling()),
+        (2, 2, 9, BlockIsSignaling()),
+        (2, 2, 10, BlockIsSignaling()),
+        (2, 2, 11, BlockIsMissingSignal(feature=Feature.NOP_FEATURE_1)),
+        (2, 2, 12, BlockIsSignaling()),
     ]
 )
 def test_check_must_signal(
@@ -686,7 +691,7 @@ def test_check_must_signal(
     bit: int,
     threshold: int,
     block_height: int,
-    is_signaling: bool
+    signaling_state: BlockSignalingState
 ) -> None:
     feature_settings = FeatureSettings(
         evaluation_interval=4,
@@ -706,4 +711,4 @@ def test_check_must_signal(
 
     result = service.is_signaling_mandatory_features(block)
 
-    assert result is is_signaling
+    assert result == signaling_state

tests/tx/test_block.py

@@ -18,7 +18,8 @@
 
 from hathor.conf.get_settings import get_settings
 from hathor.conf.settings import HathorSettings
-from hathor.feature_activation.feature_service import FeatureService
+from hathor.feature_activation.feature import Feature
+from hathor.feature_activation.feature_service import BlockIsMissingSignal, BlockIsSignaling, FeatureService
 from hathor.transaction import Block, TransactionMetadata
 from hathor.transaction.exceptions import BlockMustSignalError
 from hathor.transaction.storage import TransactionMemoryStorage, TransactionStorage
@@ -137,7 +138,10 @@ def test_get_feature_activation_bit_value() -> None:
     assert block.get_feature_activation_bit_value(3) == 0
 
 
-@pytest.mark.parametrize('is_signaling_mandatory_features', [False, True])
+@pytest.mark.parametrize(
+    'is_signaling_mandatory_features',
+    [BlockIsSignaling(), BlockIsMissingSignal(feature=Feature.NOP_FEATURE_1)]
+)
 def test_verify_must_signal_when_feature_activation_is_disabled(is_signaling_mandatory_features: bool) -> None:
     settings = Mock(spec_set=HathorSettings)
     settings.FEATURE_ACTIVATION.enable_usage = False
@@ -153,19 +157,23 @@ def test_verify_must_signal() -> None:
     settings = Mock(spec_set=HathorSettings)
     settings.FEATURE_ACTIVATION.enable_usage = True
     feature_service = Mock(spec_set=FeatureService)
-    feature_service.is_signaling_mandatory_features = Mock(return_value=False)
+    feature_service.is_signaling_mandatory_features = Mock(
+        return_value=BlockIsMissingSignal(feature=Feature.NOP_FEATURE_1)
+    )
     verifier = BlockVerifier(settings=settings, feature_service=feature_service)
     block = Block()
 
-    with pytest.raises(BlockMustSignalError):
+    with pytest.raises(BlockMustSignalError) as e:
         verifier.verify_mandatory_signaling(block)
 
+    assert str(e.value) == "Block must signal support for feature 'NOP_FEATURE_1' during MUST_SIGNAL phase."
+
 
 def test_verify_must_not_signal() -> None:
     settings = Mock(spec_set=HathorSettings)
     settings.FEATURE_ACTIVATION.enable_usage = True
     feature_service = Mock(spec_set=FeatureService)
-    feature_service.is_signaling_mandatory_features = Mock(return_value=True)
+    feature_service.is_signaling_mandatory_features = Mock(return_value=BlockIsSignaling())
     verifier = BlockVerifier(settings=settings, feature_service=feature_service)
     block = Block()