fix: fix justifications unpacking/repacking logic to handle finalized slot boundaries

[GrapeBaBa]

🗒️ Description

🔗 Related Issues or PRs

✅ Checklist

• Ran tox checks to avoid unnecessary CI fails:

  uvx tox

• Considered adding appropriate tests for the changes.
• Considered updating the online docs in the ./docs/ directory.

[tcoratger]

Just left couple of small comments.

Thanks for finding the bug. Any reason why this draft PR is not converted to a real PR? I think this makes sense to merge is as soon as possible no?

[tcoratger]

I recommend implementing this directly into Slot to avoid overloading the state as something like:

    def justified_index_after(self, finalized_slot: Slot) -> int | None:
        """
        Return the bitfield index of this slot relative to a finalized slot.

        This index is used to look up justification status in the `justified_slots`
        bitfield. The bitfield starts tracking at `finalized_slot + 1`.

        Logic:
          - If self <= finalized_slot: It is implicitly justified (part of history).
            Returns None.
          - If self > finalized_slot: It maps to index (self - finalized_slot - 1).
            Example: finalized=100.
                     slot=101 -> index 0
                     slot=102 -> index 1

        Args:
            finalized_slot: The anchor slot representing the finalized history.

        Returns:
            The 0-based index for the bitfield, or None if implicitly justified.
        """
        if self <= finalized_slot:
            return None
        
        # Calculate distance and adjust for 0-based indexing starting at finalized+1
        return int(self - finalized_slot) - 1

[tcoratger]

Again not to overload the state, I think that it makes sense to move this to JustifiedSlots implementation as:

def is_slot_justified(self, finalized_slot: Slot, target_slot: Slot) -> bool:
        """
        Check if a target slot is justified.
        
        This handles the logic of:
        1. Implicit justification (slots <= finalized).
        2. Relative indexing into the bitfield.
        3. Bounds checking.
        """
        # Use the new method on Slot to get the relative index
        idx = target_slot.justified_index_after(finalized_slot)

        # Case 1: The slot is part of finalized history (implicitly justified)
        if idx is None:
            return True

        # Case 2: The slot is in the future relative to our tracked bits
        if idx >= len(self):
            raise IndexError(
                f"Slot {target_slot} is out of bounds for justification tracking "
                f"(max trackable slot: {finalized_slot + Slot(len(self))})"
            )

        # Case 3: Check the specific bit in our collection
        return bool(self[idx])

[tcoratger]

I think that you can move this as well to JustifiedSlots as

def set_justified(self, finalized_slot: Slot, target_slot: Slot, value: bool | Boolean) -> None:
        """
        Set the justified status for a target slot.
        
        This handles the relative indexing logic internally.
        
        Args:
            finalized_slot: The anchor slot representing finalized history.
            target_slot: The slot to update.
            value: The boolean status to set.
        """
        # 1. Calculate the relative index
        idx = target_slot.justified_index_after(finalized_slot)

        # 2. Handle implicit justification (No-op)
        # If the slot is already finalized, we typically cannot "un-justify" it,
        # and it is implicitly justified anyway. We effectively ignore writes here.
        if idx is None:
            return

        # 3. Check bounds
        if idx >= len(self):
            raise IndexError(
                f"Cannot set justification for slot {target_slot}: index {idx} "
                f"out of bounds (len={len(self)})"
            )

        # 4. Update the bit
        # We explicitly cast to Boolean to ensure SSZ type safety if required
        self[idx] = Boolean(value)

[tcoratger]

Thanks a lot for finding the bug, maybe for clarity something like this would be better no?

Suggested change

	base_slot = finalized_slot + Slot(1)
	next_slot = Slot(int(base_slot) + len(self.justified_slots))
	new_bits = list(self.justified_slots.data)
	for slot_i in range(int(next_slot), int(block.slot)):
	slot = Slot(slot_i)
	if slot == parent_header.slot:
	new_bits.append(Boolean(is_genesis_parent))
	else:
	new_bits.append(Boolean(False))
	new_justified_slots_data = JustifiedSlots(data=new_bits)
	# 1. Calculate the slot where our current list ends (the "frontier").
	# Formula: finalized + 1 (start offset) + length
	frontier_slot = finalized_slot + Slot(1) + Slot(len(self))
	# 2. Calculate how many empty slots exist between the frontier and the target.
	# If target is 100 and frontier is 98, we need to fill slots 98 and 99.
	# Gap = 100 - 98 = 2.
	gap_size = int(target_slot - frontier_slot)
	# 3. If no gap, return self unchanged.
	if gap_size <= 0:
	return self
	# 4. Fill the gap with False (unjustified) entries.
	return JustifiedSlots(data=self.data + [Boolean(False)] * gap_size)

[GrapeBaBa]

Just left couple of small comments.

Thanks for finding the bug. Any reason why this draft PR is not converted to a real PR? I think this makes sense to merge is as soon as possible no?

Thanks @tcoratger , I was going to let @g11tech pre review it first. I will fix the comments and make it ready to review soon.

[Copilot]

Pull request overview

This pull request fixes the justifications unpacking/repacking logic to correctly handle finalized slot boundaries. The main change is to make the justified_slots bitfield relative to the finalized boundary, rather than using absolute slot indices.

Changes:

• Introduced finalized-relative storage for justified_slots, where the bitfield stores slots starting from (latest_finalized.slot + 1)
• Added methods to JustifiedSlots: is_slot_justified, set_justified, extend_to_slot, and shift_window to manage the relative bitfield
• Updated process_block_header to use extend_to_slot for materializing slots
• Updated process_attestations to use relative slot access and to rebase the bitfield when finalization advances
• Added comprehensive tests for the new behavior

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
tests/lean_spec/subspecs/containers/test_state_justified_slots.py	New test file covering finalized-relative storage invariants and boundary conditions
src/lean_spec/subspecs/containers/state/types.py	Added helper methods to JustifiedSlots for relative slot access and window management
src/lean_spec/subspecs/containers/state/state.py	Updated block header and attestation processing to use finalized-relative indexing
src/lean_spec/subspecs/containers/slot.py	Added justified_index_after method to compute relative bitfield indices

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment mentions "every newly materialized slot in [parent_header.slot, block.slot)" but the function extend_to_slot doesn't reference parent_header.slot. The function only knows about the target slot and extends to make slots up to (but not including) target_slot addressable. Consider revising the comment to more accurately reflect the function's behavior, such as: "Extend tracking to make slots [finalized_slot+1, block.slot) addressable."

Suggested change

	# Here we append flags for every newly materialized slot in:
	# [parent_header.slot, block.slot)
	# but only for slots >= (latest_finalized.slot + 1).
	# Here we extend tracking so that all slots in:
	# [latest_finalized.slot + 1, block.slot)
	# are addressable in `justified_slots`.

[Copilot]

The set_justified method mutates the bitfield in place, which breaks immutability. At line 416, justified_slots is assigned to self.justified_slots, creating a reference to the original object. When set_justified is called at line 512, it mutates this original object, affecting the state before the update. This could lead to unintended side effects where both the old and new state share the same mutated bitfield object. Consider making set_justified return a new JustifiedSlots object instead of mutating in place, similar to how extend_to_slot and shift_window work.

[tcoratger]

Overall this looks good, just left couple of suggestions for your consideration but it's not a blocker for me for merging except the fact that we should use immutability and fix the comments of copilot.