Merge pull request #747 from HathorNetwork/fix/compare-with-partial

fix(sync-v2): compare to local bytes broke in some cases

Author: jansegre

hathor/transaction/storage/transaction_storage.py

@@ -514,7 +514,9 @@ def transaction_exists(self, hash_bytes: bytes) -> bool:
     def compare_bytes_with_local_tx(self, tx: BaseTransaction) -> bool:
         """Compare byte-per-byte `tx` with the local transaction."""
         assert tx.hash is not None
-        local_tx = self.get_transaction(tx.hash)
+        # XXX: we have to accept any scope because we only want to know what bytes we have stored
+        with tx_allow_context(self, allow_scope=TxAllowScope.ALL):
+            local_tx = self.get_transaction(tx.hash)
         local_tx_bytes = bytes(local_tx)
         tx_bytes = bytes(tx)
         if tx_bytes == local_tx_bytes:

hathor/transaction/storage/tx_allow_scope.py

@@ -35,6 +35,7 @@ class TxAllowScope(Flag):
     VALID = auto()
     PARTIAL = auto()
     INVALID = auto()
+    ALL = VALID | PARTIAL | INVALID
 
     def is_allowed(self, tx: BaseTransaction) -> bool:
         """True means it is allowed to be used in the storage (as argument or as return), False means not allowed."""

tests/tx/test_tx.py

@@ -1137,6 +1137,60 @@ def test_sigops_input_multi_below_limit(self) -> None:
         tx.update_hash()
         tx.verify_sigops_input()
 
+    def test_compare_bytes_equal(self) -> None:
+        # create some block
+        [block1] = add_new_blocks(self.manager, 1, advance_clock=1)
+
+        # clone it to make sure we have a new instance
+        block2 = block1.clone()
+
+        # the storage already has block1 and should correctly return True
+        self.assertTrue(self.tx_storage.compare_bytes_with_local_tx(block2))
+
+    def test_compare_bytes_different(self) -> None:
+        # create some block
+        [block1] = add_new_blocks(self.manager, 1, advance_clock=1)
+
+        # clone it and change something, doesn't matter what it is
+        # XXX: note the hash is not being update on purpose, we expect a failure even if the hash hasn't changed
+        block2 = block1.clone()
+        block2.weight += 1
+
+        # the storage already has block1 and should correctly return False
+        self.assertFalse(self.tx_storage.compare_bytes_with_local_tx(block2))
+
+    def test_compare_bytes_partially_validated_equal(self) -> None:
+        from hathor.transaction.validation_state import ValidationState
+
+        # create some block, make it partially valid and save it
+        [block1] = add_new_blocks(self.manager, 1, advance_clock=1)
+        block1.set_validation(ValidationState.BASIC)
+        with self.tx_storage.allow_partially_validated_context():
+            self.tx_storage.save_transaction(block1)
+
+        # clone it to make sure we have a new instance
+        block2 = block1.clone()
+
+        # the storage already has block1 and should correctly return True
+        self.assertTrue(self.tx_storage.compare_bytes_with_local_tx(block2))
+
+    def test_compare_bytes_partially_validated_different(self) -> None:
+        from hathor.transaction.validation_state import ValidationState
+
+        # create some block, make it partially valid and save it
+        [block1] = add_new_blocks(self.manager, 1, advance_clock=1)
+        block1.set_validation(ValidationState.BASIC)
+        with self.tx_storage.allow_partially_validated_context():
+            self.tx_storage.save_transaction(block1)
+
+        # clone it and change something, doesn't matter what it is
+        # XXX: note the hash is not being update on purpose, we expect a failure even if the hash hasn't changed
+        block2 = block1.clone()
+        block2.weight += 1
+
+        # the storage already has block1 and should correctly return False
+        self.assertFalse(self.tx_storage.compare_bytes_with_local_tx(block2))
+
 
 class SyncV1TransactionTest(unittest.SyncV1Params, BaseTransactionTest):
     __test__ = True