refactor(node/sync): tidy block cache and fix internal-node eviction (leanEthereum#1110)

Clarity, modern Python, and one correctness fix in BlockCache.

Documentation:
- Trim the banner-essay module docstring, the PendingBlock class and field
  docstrings, and the verbose method Args/Returns to the project rules,
  keeping the genuine rationale (the >= capacity reason, the empty-set
  deletion, the slot-order example, FIFO attack-resistance).

Structure:
- Inline the single-use _evict_oldest into add and drop unmark_orphan,
  whose only caller was remove (folded its discard inline). mark_orphan
  stays: it has callers in two other modules plus internal use and guards
  the orphan set against phantom entries.
- Make PendingBlock frozen; nothing mutates it.

Eviction correctness fix:
- Evicting an internal node left its cached children stranded: still cached,
  no longer linked to a present parent, never re-marked as orphans, so
  backfill never refetched the parent and orphan_count under-reported (which
  could flip the node to synced with stuck blocks). Eviction now re-marks the
  evicted block's still-cached children as orphans, with a regression test.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: tcoratger

src/lean_spec/node/sync/block_cache.py

@@ -1,41 +1,4 @@
-"""
-Block cache for downloaded blocks awaiting parent resolution.
-
-Why Cache Blocks?
-
-In an ideal world, blocks arrive in perfect order: parent before child, always.
-Reality differs. Network latency, parallel downloads, and gossip propagation
-mean blocks often arrive before their parents are known.
-
-Without caching, we would have two bad choices:
-
-1. **Drop the block**: Wasteful. We will need it later.
-2. **Re-request later**: Slow. Network round-trips add latency.
-
-The block cache provides a third option: hold the block until its parent
-arrives, then process both.
-
-How It Works
-
-The cache maintains three data structures:
-
-1. **Block storage**: Maps block root to PendingBlock (the block + metadata)
-2. **Orphan set**: Roots of blocks whose parents are completely unknown
-3. **Parent index**: Maps parent_root to child roots for descendant lookup
-
-When a parent arrives:
-
-1. Look up children via the parent index
-2. Check if those children can now be processed
-3. Process children in slot order (ensuring parent-before-child)
-4. Recursively check if processed children have their own waiting children
-
-Memory Safety
-
-The cache is bounded by MAX_CACHED_BLOCKS (1024). When full, FIFO eviction
-removes the oldest blocks. This prevents memory exhaustion from attacks or
-prolonged network partitions that could otherwise grow the cache unboundedly.
-"""
+"""Block cache for downloaded blocks awaiting parent resolution."""
 
 from __future__ import annotations
 
@@ -49,89 +12,39 @@
 from lean_spec.spec.ssz import Bytes32
 
 
-@dataclass(slots=True)
+@dataclass(frozen=True, slots=True)
 class PendingBlock:
-    """
-    A block awaiting integration into the Store.
-
-    PendingBlock wraps a SignedBlock with metadata needed for
-    cache management. This metadata answers key questions:
-
-    - **Who sent it?** For peer scoring when we determine validity
-    - **When did it arrive?** For timeout and staleness decisions
-    - **How deep is the backfill?** To enforce MAX_BACKFILL_DEPTH limit
-
-    Blocks remain pending until:
-
-    1. Their parent arrives and they can be processed (success)
-    2. They are evicted due to cache capacity limits (dropped)
-    3. They are determined to be invalid (rejected)
-    """
+    """A cached block plus the metadata needed to resolve and score it later."""
 
     block: SignedBlock
     """The complete signed block."""
 
     root: Bytes32
-    """
-    The SSZ hash tree root of the block.
-
-    Computed once at cache insertion for efficiency. All subsequent lookups
-    and comparisons use this cached value rather than recomputing.
-    """
+    """Hash tree root of the block, computed once at insertion to avoid recomputing."""
 
     parent_root: Bytes32
-    """
-    Root of the parent block.
-
-    Stored separately for quick parent relationship lookups without
-    deserializing the full block. This is the key to efficient descendant
-    processing.
-    """
+    """Root of the parent, stored separately so lookups need not deserialize the block."""
 
     slot: Slot
-    """
-    Slot of the block.
-
-    Used for ordering during batch processing. We must process blocks in
-    slot order to ensure parents are processed before children.
-    """
+    """Slot of the block, used to order batch processing so parents precede children."""
 
     received_from: PeerId | None
-    """
-    Peer that sent this block.
-
-    Essential for peer scoring.
-        - If the block is valid, the peer gets credit.
-        - If invalid, they get penalized.
-
-    This creates incentives for good behavior.
-    None for self-produced blocks.
-    """
+    """Peer that sent the block, for later scoring. None for self-produced blocks."""
 
     backfill_depth: int = 0
     """
-    Depth of backfill chain from original request.
+    How many ancestors deep this block was fetched while chasing a missing parent.
 
-    When fetching missing parents recursively, this tracks how deep we are.
-        - A block at depth 0 came directly from gossip or an explicit request.
-        - A block at depth 5 is the 5th ancestor we fetched while backfilling.
-
-    Blocks with depth >= MAX_BACKFILL_DEPTH trigger backfill termination
-    to prevent unbounded recursion during attacks or deep forks.
+    Bounds recursion so deep forks or attacks cannot backfill without limit.
     """
 
 
 @dataclass(slots=True)
 class BlockCache:
-    """
-    Cache for blocks awaiting parent resolution.
-
-    Holds blocks that cannot be immediately processed because their parent
-    blocks are not yet in the Store.
-    """
+    """Cache for blocks that cannot be processed yet because their parent is unknown."""
 
     _blocks: OrderedDict[Bytes32, PendingBlock] = field(default_factory=OrderedDict)
-    """Block storage ordered by insertion time for FIFO eviction."""
+    """Blocks ordered by insertion time, so the oldest can be evicted first."""
 
     _orphans: set[Bytes32] = field(default_factory=set)
     """Roots of blocks whose parents are completely unknown."""
@@ -154,82 +67,66 @@ def add(
         backfill_depth: int = 0,
     ) -> PendingBlock:
         """
-        Add a block to the cache.
-
-        This is the primary entry point for caching blocks. The method handles:
-
-        1. Deduplication (same block added twice returns existing entry)
-        2. Capacity management (evicts oldest if full)
-        3. Index maintenance (updates parent->children mapping)
+        Cache a block, evicting the oldest entry first if at capacity.
 
-        Args:
-            block: The signed block to cache.
-            peer: The peer that sent this block (for later scoring).
-            backfill_depth: How deep in the backfill chain (0 = direct request).
-
-        Returns:
-            The PendingBlock wrapper, either newly created or existing.
+        Returns the existing entry if the block is already cached.
         """
         block_inner = block.block
         root = hash_tree_root(block_inner)
 
-        # Deduplication: if we already have this block, return the existing entry.
+        # The same block can arrive from several peers, or be re-requested while pending.
         #
-        # This can happen when multiple peers send the same block via gossip,
-        # or when a block is requested while already pending.
+        # Return the existing entry instead of caching a duplicate.
         if root in self._blocks:
             return self._blocks[root]
 
-        # Capacity management: evict before adding to ensure we stay within bounds.
-        #
-        # We check >= rather than > because we are about to add one more.
+        # Use >= here, not >: we are about to add one entry, so evict at the limit.
         if len(self._blocks) >= MAX_CACHED_BLOCKS:
-            self._evict_oldest()
-
-        parent_root = block_inner.parent_root
+            # Evict the oldest block (FIFO).
+            #
+            # Oldest-first means an attacker cannot keep a block cached by re-sending it.
+            evicted_root, evicted = self._blocks.popitem(last=False)
+            self._orphans.discard(evicted_root)
+
+            # Detach the evicted block from its parent's child set.
+            siblings = self._by_parent.get(evicted.parent_root)
+            if siblings:
+                siblings.discard(evicted_root)
+                if not siblings:
+                    del self._by_parent[evicted.parent_root]
+
+            # Its cached children just lost their parent, so they are orphans again.
+            for child_root in self._by_parent.get(evicted_root, set()):
+                self.mark_orphan(child_root)
 
         pending = PendingBlock(
             block=block,
             root=root,
-            parent_root=parent_root,
+            parent_root=block_inner.parent_root,
             slot=block_inner.slot,
             received_from=peer,
             backfill_depth=backfill_depth,
         )
-
-        # Insert into primary storage.
         self._blocks[root] = pending
 
-        # Update parent index so we can find this block when its parent arrives.
-        self._by_parent[parent_root].add(root)
-
+        # Index by parent so this block is found when its parent is processed.
+        self._by_parent[block_inner.parent_root].add(root)
         return pending
 
     def remove(self, root: Bytes32) -> PendingBlock | None:
         """
-        Remove a block from the cache.
+        Remove a block and clean up its orphan and parent-index entries.
 
-        Called after a block has been successfully processed or determined
-        invalid. Maintains all index consistency automatically.
-
-        Args:
-            root: The block root to remove.
-
-        Returns:
-            The removed PendingBlock if it existed, None otherwise.
+        Returns the removed block, or None if it was not cached.
         """
         pending = self._blocks.pop(root, None)
         if pending is None:
             return None
 
-        # Clean up orphan tracking.
-        self.unmark_orphan(root)
+        self._orphans.discard(root)
 
-        # Clean up parent index.
-        #
-        # We must remove this block from its parent's child set. If that
-        # leaves the parent with no children, remove the parent entry entirely
-        # to avoid memory leaks from empty sets accumulating.
+        # Drop this child from its parent's set.
+        # Delete the parent entry once empty, so empty sets do not accumulate.
         children = self._by_parent.get(pending.parent_root)
         if children:
             children.discard(root)
@@ -240,95 +137,21 @@ def remove(self, root: Bytes32) -> PendingBlock | None:
 
     def mark_orphan(self, root: Bytes32) -> None:
         """
-        Mark a block as an orphan (parent not in Store or cache).
-
-        An orphan is a block whose parent is completely unknown. This differs
-        from a block whose parent is simply pending: orphans need backfill,
-        pending blocks just need to wait.
+        Mark a block as an orphan: its parent is in neither the store nor the cache.
 
-        Typical flow:
-        1. Block arrives
-        2. Check if parent in Store -> no
-        3. Check if parent in cache -> no
-        4. Mark as orphan, trigger backfill for parent
-
-        Args:
-            root: The block root to mark as orphan.
+        Unlike a block waiting on a pending parent, an orphan needs its parent backfilled.
         """
         if root in self._blocks:
             self._orphans.add(root)
 
-    def unmark_orphan(self, root: Bytes32) -> None:
-        """
-        Remove orphan status from a block.
-
-        Called when a block's parent has been received. The block is no longer
-        an orphan because its parent now exists (in cache or Store).
-
-        Args:
-            root: The block root to unmark.
-        """
-        self._orphans.discard(root)
-
     def get_children(self, parent_root: Bytes32) -> list[PendingBlock]:
-        """
-        Get all cached children of a given parent root.
-
-        This is the core of descendant processing. When a parent block is
-        successfully processed, call this method to find children that were
-        waiting for it. Those children can now be processed.
-
-        Results are sorted by slot to ensure parent-before-child ordering when
-        processing a chain of descendants.
-
-        Args:
-            parent_root: The root of the parent block that was just processed.
-
-        Returns:
-            List of pending child blocks, sorted by slot (earliest first).
-        """
+        """Return the cached children of a parent, sorted by slot so parents process first."""
         child_roots = self._by_parent.get(parent_root, set())
-        children = [self._blocks[r] for r in child_roots if r in self._blocks]
-
-        # Sort by slot ensures correct processing order.
-        #
-        # If block A at slot 100 and block B at slot 101 both waited for
-        # parent P, we must process A before B.
-        return sorted(children, key=lambda p: p.slot)
+        children = [self._blocks[root] for root in child_roots if root in self._blocks]
+        # If two blocks at slots 100 and 101 share a parent, 100 must process first.
+        return sorted(children, key=lambda pending: pending.slot)
 
     @property
     def orphan_count(self) -> int:
         """Number of orphan blocks in the cache."""
         return len(self._orphans)
-
-    def _evict_oldest(self) -> None:
-        """
-        Evict the oldest block to make room for new entries.
-
-        FIFO (First-In-First-Out) eviction is used because:
-
-        1. **Simplicity**: No scoring or prioritization needed
-        2. **Fairness**: Old blocks had their chance; new ones deserve theirs
-        3. **Attack resistance**: Attackers cannot keep malicious blocks cached
-           by refreshing them
-
-        More sophisticated strategies (LRU, priority queues) add complexity
-        without clear benefits for this use case.
-        """
-        if not self._blocks:
-            return
-
-        # popitem(last=False) removes the first (oldest) entry.
-        #
-        # This is O(1) due to OrderedDict's doubly-linked list implementation.
-        oldest_root, oldest_block = self._blocks.popitem(last=False)
-
-        # Clean up orphan tracking.
-        self._orphans.discard(oldest_root)
-
-        # Clean up parent index to prevent memory leaks.
-        children = self._by_parent.get(oldest_block.parent_root)
-        if children:
-            children.discard(oldest_root)
-            if not children:
-                del self._by_parent[oldest_block.parent_root]