fix: clamp DevMiner timestamp above tx parents

tests/test_dev_miner.py

@@ -28,6 +28,7 @@
 from txstratum.dev.block_miner import BlockMiner, solve_block
 from txstratum.dev.manager import DevMiningManager
 from txstratum.dev.tx_miner import solve_tx
+from txstratum.jobs import JobStatus, TxJob
 
 # Same TX1_DATA from test_api.py — a serialized transaction that we use as a
 # realistic input for PoW solving. The weight is ~32, so without
@@ -55,6 +56,60 @@ def update_timestamp(tx_bytes: bytes, *, delta: int = 0) -> bytes:
     return bytes(tx)
 
 
+class FakeResponse:
+    """Stand-in for an aiohttp response, usable as an async context manager."""
+
+    def __init__(self, status, payload):
+        self.status = status
+        self._payload = payload
+
+    async def json(self):
+        return self._payload
+
+    async def __aenter__(self):
+        return self
+
+    async def __aexit__(self, *exc_info):
+        return False
+
+
+class FakeSession:
+    """Stand-in for HathorClient._session, serving GET /transaction?id=...
+
+    The dev-miner reads parent timestamps straight off the fullnode, so the
+    tests need a session that answers that one endpoint. Requests are recorded
+    so tests can assert on caching.
+    """
+
+    def __init__(self, timestamps=None, *, status=200):
+        # Maps hex tx id -> timestamp. A hash absent from the map is answered
+        # the way the fullnode answers an unknown tx: success=False.
+        self.timestamps = timestamps or {}
+        self.status = status
+        self.requested_ids = []
+
+    def get(self, url, params=None):
+        tx_id = (params or {}).get("id")
+        self.requested_ids.append(tx_id)
+        timestamp = self.timestamps.get(tx_id)
+        if timestamp is None:
+            return FakeResponse(self.status, {"success": False})
+        return FakeResponse(
+            self.status, {"success": True, "tx": {"timestamp": timestamp}}
+        )
+
+
+def make_backend(parents=None, *, parent_timestamps=None, session_status=200):
+    """Build a mocked HathorClient whose parent lookups actually resolve."""
+    parents = parents if parents is not None else [b"\x00" * 32, b"\x01" * 32]
+    backend = MagicMock()
+    backend.get_tx_parents = AsyncMock(return_value=parents)
+    backend.push_tx_or_block = AsyncMock(return_value=True)
+    backend._session = FakeSession(parent_timestamps, status=session_status)
+    backend._get_url = lambda path: f"http://fullnode/v1a/{path}"
+    return backend
+
+
 # ---------------------------------------------------------------------------
 # Transaction PoW tests
 # ---------------------------------------------------------------------------
@@ -106,11 +161,7 @@ class TestDevMiningManager(AioHTTPTestCase):
     __test__ = True
 
     async def get_application(self):
-        self.backend = MagicMock()
-        self.backend.get_tx_parents = AsyncMock(
-            return_value=[b"\x00" * 32, b"\x01" * 32]
-        )
-        self.backend.push_tx_or_block = AsyncMock(return_value=True)
+        self.backend = make_backend()
         self.manager = DevMiningManager(backend=self.backend)
         await self.manager.start()
         self.healthcheck = MagicMock()
@@ -260,6 +311,118 @@ async def test_duplicate_job_submission(self):
 )
 
 
+class TestParentTimestampClamp(unittest.IsolatedAsyncioTestCase):
+    """The stamped timestamp must come out strictly above every parent's.
+
+    The fullnode enforces `tx.timestamp > parent.timestamp` at 1-second
+    granularity. Wall-clock time alone does not satisfy that when a parent was
+    stamped in the same second — the rejection reported in
+    HathorNetwork/tx-mining-service#172.
+    """
+
+    __test__ = True
+
+    # A fixed "now" so the expected timestamps are exact rather than windowed.
+    NOW = 1785951640
+
+    def setUp(self):
+        txstratum.time.set_time_function(lambda: float(self.NOW))
+
+    def tearDown(self):
+        txstratum.time.set_time_function(None)
+
+    async def test_uses_now_when_parents_are_older(self):
+        """The common case: nothing to clamp, so "now" stands unchanged."""
+        parent = b"\x0a" * 32
+        backend = make_backend(parent_timestamps={parent.hex(): self.NOW - 10})
+        manager = DevMiningManager(backend=backend)
+        self.assertEqual(self.NOW, await manager._get_timestamp_for([parent]))
+
+    async def test_clamps_past_a_same_second_parent(self):
+        """The reported bug: parent stamped this second, so we must exceed it."""
+        parent = b"\x0a" * 32
+        backend = make_backend(parent_timestamps={parent.hex(): self.NOW})
+        manager = DevMiningManager(backend=backend)
+        self.assertEqual(self.NOW + 1, await manager._get_timestamp_for([parent]))
+
+    async def test_clamps_past_the_newest_parent(self):
+        """With two parents, only the newest constrains the result."""
+        older, newer = b"\x0a" * 32, b"\x0b" * 32
+        backend = make_backend(
+            parent_timestamps={older.hex(): self.NOW - 5, newer.hex(): self.NOW}
+        )
+        manager = DevMiningManager(backend=backend)
+        self.assertEqual(self.NOW + 1, await manager._get_timestamp_for([older, newer]))
+
+    async def test_clamps_past_a_parent_already_ahead_of_now(self):
+        """A same-second chain, which is what defeats a blind `now + 1`.
+
+        The parent was itself clamped to NOW + 1 a moment ago. Stamping this tx
+        `now + 1` would tie with it and be rejected; reading the parent's real
+        timestamp is what makes the chain resolve.
+        """
+        parent = b"\x0a" * 32
+        backend = make_backend(parent_timestamps={parent.hex(): self.NOW + 1})
+        manager = DevMiningManager(backend=backend)
+        self.assertEqual(self.NOW + 2, await manager._get_timestamp_for([parent]))
+
+    async def test_falls_back_to_now_when_parent_is_unknown(self):
+        """An unreadable parent degrades to the old behavior, never an error."""
+        backend = make_backend(parent_timestamps={})
+        manager = DevMiningManager(backend=backend)
+        self.assertEqual(self.NOW, await manager._get_timestamp_for([b"\x0a" * 32]))
+
+    async def test_falls_back_to_now_on_http_error(self):
+        parent = b"\x0a" * 32
+        backend = make_backend(
+            parent_timestamps={parent.hex(): self.NOW}, session_status=500
+        )
+        manager = DevMiningManager(backend=backend)
+        self.assertEqual(self.NOW, await manager._get_timestamp_for([parent]))
+
+    async def test_no_parents_skips_the_lookup_entirely(self):
+        backend = make_backend()
+        manager = DevMiningManager(backend=backend)
+        self.assertEqual(self.NOW, await manager._get_timestamp_for([]))
+        self.assertEqual([], backend._session.requested_ids)
+
+    async def test_parent_timestamps_are_cached(self):
+        """A confirmed tx's timestamp is immutable, so one read per hash."""
+        parent = b"\x0a" * 32
+        backend = make_backend(parent_timestamps={parent.hex(): self.NOW})
+        manager = DevMiningManager(backend=backend)
+        await manager._get_timestamp_for([parent])
+        await manager._get_timestamp_for([parent])
+        self.assertEqual([parent.hex()], backend._session.requested_ids)
+
+    async def test_cache_does_not_grow_without_bound(self):
+        parents = [bytes([i]) * 32 for i in range(1, 11)]
+        backend = make_backend(
+            parent_timestamps={p.hex(): self.NOW - 100 for p in parents}
+        )
+        manager = DevMiningManager(backend=backend)
+        manager.PARENT_TIMESTAMP_CACHE_SIZE = 4
+        for parent in parents:
+            await manager._get_timestamp_for([parent])
+        self.assertLessEqual(len(manager._parent_timestamps), 4)
+
+    async def test_mined_tx_carries_the_clamped_timestamp(self):
+        """The clamp reaches the solved tx, not just the helper in isolation."""
+        tx = tx_or_block_from_bytes(TX1_DATA)
+        tx.weight = 1.0
+        backend = make_backend(
+            parent_timestamps={p.hex(): self.NOW for p in tx.parents}
+        )
+        manager = DevMiningManager(backend=backend)
+        job = TxJob(bytes(tx))
+
+        await manager._mine_job(job)
+
+        self.assertEqual(JobStatus.DONE, job.status)
+        self.assertEqual(self.NOW + 1, job.get_tx().timestamp)
+        self.assertTrue(job.get_tx().verify_pow())
+
+
 class TestSolveBlock(AioHTTPTestCase):
     """Test the block PoW solver (solve_block)."""
 

txstratum/dev/manager.py

@@ -25,6 +25,7 @@
 """
 
 import asyncio
+from collections import OrderedDict
 from typing import TYPE_CHECKING, Any, Dict, List, Optional
 
 from structlog import get_logger
@@ -55,6 +56,11 @@ class DevMiningManager:
 
     TX_CLEAN_UP_INTERVAL = 300.0  # seconds
 
+    # Upper bound for the parent-timestamp cache. A confirmed transaction's
+    # timestamp never changes, so a cached entry cannot go stale; the bound is
+    # only here to keep the dict from growing without limit over a long run.
+    PARENT_TIMESTAMP_CACHE_SIZE = 1024
+
     def __init__(self, backend: "HathorClient"):
         """Initialize the dev mining manager."""
         self.log = logger.new()
@@ -63,6 +69,7 @@ def __init__(self, backend: "HathorClient"):
         self.tx_jobs: Dict[bytes, TxJob] = {}
         self._tasks: Dict[bytes, asyncio.Task[None]] = {}
         self.refuse_new_jobs = False
+        self._parent_timestamps: "OrderedDict[bytes, int]" = OrderedDict()
 
         # Statistics — same fields as TxMiningManager for API compatibility.
         self.txs_solved: int = 0
@@ -182,6 +189,83 @@ async def _mine_with_parents(self, job: TxJob) -> None:
         job.set_parents(parents)
         await self._mine_job(job)
 
+    async def _fetch_tx_timestamp(self, tx_hash: bytes) -> Optional[int]:
+        """Read one transaction's timestamp from the fullnode, or None.
+
+        hathorlib's HathorClient has no "get transaction by hash", and this fix
+        is deliberately confined to this repo, so the client's session and URL
+        builder are used directly. Both are private, which is exactly why this
+        is the only place that touches them: if hathorlib later grows a proper
+        accessor (or the parents endpoint starts returning timestamps, as
+        HathorNetwork/tx-mining-service#172 sketches), this body is the only
+        thing that has to change.
+
+        Returns None on any failure — a timestamp we cannot read is one we
+        cannot clamp against, and degrading to plain wall-clock time is far
+        better than failing the job.
+        """
+        cached = self._parent_timestamps.get(tx_hash)
+        if cached is not None:
+            self._parent_timestamps.move_to_end(tx_hash)
+            return cached
+
+        session = self.backend._session
+        if session is None:
+            return None
+
+        try:
+            url = self.backend._get_url("transaction")
+            async with session.get(url, params={"id": tx_hash.hex()}) as resp:
+                if resp.status != 200:
+                    return None
+                data = await resp.json()
+        except Exception as e:
+            self.log.warn(
+                "Could not read parent timestamp",
+                parent=tx_hash.hex(),
+                error=f"{type(e).__name__}: {e}",
+            )
+            return None
+
+        if not data.get("success"):
+            return None
+        timestamp = data.get("tx", {}).get("timestamp")
+        if not isinstance(timestamp, int):
+            return None
+
+        self._parent_timestamps[tx_hash] = timestamp
+        if len(self._parent_timestamps) > self.PARENT_TIMESTAMP_CACHE_SIZE:
+            self._parent_timestamps.popitem(last=False)
+        return timestamp
+
+    async def _get_timestamp_for(self, parents: List[bytes]) -> int:
+        """Return a timestamp that is strictly greater than every parent's.
+
+        The fullnode requires `tx.timestamp > parent.timestamp` at 1-second
+        granularity, but wall-clock time alone does not guarantee that: two txs
+        broadcast in the same second, where one is picked as the other's
+        parent, produce a rejection the client cannot prevent or repair (both
+        fields are assigned here, under the nonce).
+
+        Clamping stays well inside consensus rules — hathor-core tolerates
+        timestamps up to MAX_FUTURE_TIMESTAMP_ALLOWED (300s) in the future, and
+        this exceeds "now" by at most one second per same-second parent chain.
+        """
+        now = int(txstratum.time.time())
+        if not parents:
+            return now
+
+        timestamps: List[Optional[int]] = list(
+            await asyncio.gather(
+                *(self._fetch_tx_timestamp(parent) for parent in parents)
+            )
+        )
+        known = [ts for ts in timestamps if ts is not None]
+        if not known:
+            return now
+
+        return max(now, max(known) + 1)
+
     async def _mine_job(self, job: TxJob) -> None:
         """Solve PoW for a transaction job.
 
@@ -196,8 +280,9 @@ async def _mine_job(self, job: TxJob) -> None:
 
         tx = job.get_tx()
         # Update timestamp to current time — the fullnode validates that tx
-        # timestamps are within an acceptable delta of the current time.
-        tx.timestamp = int(txstratum.time.time())
+        # timestamps are within an acceptable delta of the current time — and
+        # push it past the parents when they were stamped in this same second.
+        tx.timestamp = await self._get_timestamp_for(tx.parents)
 
         loop = asyncio.get_event_loop()
         solved = await loop.run_in_executor(None, solve_tx, tx)