refactor(lstar): polish block production, consolidate its tests (leanEthereum#831)

Improve the proposer-side block builder and bring its unit tests up to
reference-spec quality.

block_production.py:
- Collapse the genesis justified-checkpoint branch into a ternary.
- Rename vague locals to descriptive names (selected -> selected_proofs,
  current_justified -> current_justified_checkpoint, found_entries ->
  found_new_entries, proof_groups -> signatures_by_attestation_data).
- Rewrite the docstring and inline comments to the documentation rules:
  one sentence per line, no backticks, no identifier names in prose,
  phase-labeled steps with the load-bearing rationale preserved.

CLAUDE.md:
- Add a CRITICAL rule requiring descriptive, self-documenting names,
  distinct from the existing no-abbreviations rule.

Tests:
- Add tests/lean_spec/spec/forks/lstar/test_block_production.py as the
  canonical home (mirror rule), with eight decoupled scenarios covering
  every build_block path, full-object assertions, and line-by-line docs.
- Remove the migrated build_block tests from state/test_state_aggregation.py,
  leaving only its aggregate() tests.
- Delete the slow monolithic justification-gap test; its mechanism is
  covered by the fast unit fixed-point test and the consensus filler.

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

Author: tcoratger

CLAUDE.md

@@ -57,6 +57,24 @@ subspecifications that the Lean Ethereum protocol relies on.
     or any on-the-wire string. Rename the Python identifier, never the serialized contract.
   - When a fully-expanded name becomes unwieldy, prefer a shorter but still complete phrasing
     (drop redundant words) rather than re-introducing an abbreviation.
+- **CRITICAL - DESCRIPTIVE, SELF-DOCUMENTING NAMES**: This is a STRICT requirement, separate from
+  the no-abbreviations rule above. Every identifier must let the reader understand what it holds by
+  reading it alone, without scanning the surrounding code. A name that is fully spelled out but
+  still vague is NOT acceptable. This applies to source, tests, and `packages/`.
+  - BAN vague placeholder names that describe nothing: `selected`, `result`, `data`, `value`,
+    `item`, `temp`, `obj`, `info`, `payload` (when unqualified), `current`, `entry`, `thing`,
+    `out`, `ret`, single-letter names (except a conventional math index `i`/`j` in a tight numeric
+    loop). Name the THING, not its role: `selected` → `selected_proofs`, `result` →
+    `post_state` / `merged_signature`, `current` → `current_justified_checkpoint`.
+  - Encode the type or domain meaning when a bare word is ambiguous. A variable holding a
+    `Checkpoint` is `justified_checkpoint`, not `justified`; a bitfield of justified slots is
+    `justified_slots`, not `slots`; a boolean is a predicate phrase (`found_new_entries`,
+    `is_genesis_self_vote`), never a noun.
+  - NEVER reuse one vague name for two different things in the same scope. If an inner loop holds
+    something different from an outer variable of the same name, rename it (e.g. payload proofs vs.
+    grouped signatures → `proofs` and `grouped_signatures`).
+  - The bar: a reviewer reading any single line in isolation should know what each name refers to.
+    If they would have to scroll up to find out, the name is wrong.
 - **CRITICAL - TEST STRUCTURE MIRRORS SOURCE STRUCTURE**: This is a STRICT requirement. The
   test tree under `tests/lean_spec/` mirrors the source tree under `src/lean_spec/` one-to-one.
   A source module `src/lean_spec/<path>/<name>.py` has its unit tests in

src/lean_spec/spec/forks/lstar/block_production.py

@@ -37,119 +37,155 @@ def build_block(
         """
         Build a valid block on top of the given pre-state.
 
-        Computes the post-state and creates a block with the correct state root.
+        # Overview
 
-        Uses a fixed-point algorithm: finds attestation_data entries whose source
-        matches the current justified checkpoint, greedily selects proofs maximizing
-        new validator coverage, then applies the STF. If justification advances,
-        repeats with the new checkpoint.
+        A proposer packs attestations into a block and records the post-state root.
+        A vote is eligible only if the point it builds from is already justified.
+        Including a vote can justify a new checkpoint.
+        That, in turn, makes further votes eligible.
+
+        # Algorithm
+
+        Selection runs as a fixed point:
+
+        1. Anchor on the checkpoint this chain currently treats as justified.
+        2. Greedily pick proofs covering the most new validators.
+        3. Apply the state transition to a trial block.
+        4. If justification or finalization advanced, repeat from the new checkpoint.
+        5. Stop when a full pass adds nothing.
+
+        The loop is bounded: justification and finalization only move forward,
+        and the set of chosen entries only grows.
+
+        Args:
+            state: Pre-state the block builds on.
+            slot: Slot the new block occupies.
+            proposer_index: Validator proposing the block.
+            parent_root: Root of the parent block.
+            known_block_roots: Block roots the proposer has seen and may vote on.
+            aggregated_payloads: Candidate proofs grouped by the data they attest to.
+
+        Returns:
+            The final block, its post-state, the included attestations,
+            and the merged proof backing each one.
         """
         aggregated_attestations: list[AggregatedAttestation] = []
         aggregated_signatures: list[SingleMessageAggregate] = []
 
         if aggregated_payloads:
-            # Fixed-point loop: find attestation_data entries matching the current
-            # justified checkpoint and greedily select proofs. Processing attestations
-            # may advance justification, unlocking more entries.
-            # When building on top of genesis (slot 0), process_block_header
-            # updates the justified root to parent_root. Apply the same
-            # derivation here so attestation sources match.
-            if state.latest_block_header.slot == Slot(0):
-                current_justified = Checkpoint(slot=Slot(0), root=parent_root)
-            else:
-                current_justified = state.latest_justified
-
-            # Track the justified-slot bitfield to skip already-justified targets.
+            # Anchor on the checkpoint this chain treats as justified.
             #
-            # Extend the bitfield to cover every slot we might query.
-            # The range runs from the finalized boundary up to slot - 1 inclusive.
+            # Building directly on genesis is special.
+            # Header processing justifies the parent at slot 0.
+            # Anchor on that same checkpoint so eligible sources match.
+            current_justified_checkpoint = (
+                Checkpoint(slot=Slot(0), root=parent_root)
+                if state.latest_block_header.slot == Slot(0)
+                else state.latest_justified
+            )
+
+            # Track which slots are already justified.
+            #
+            # Extend the window so every slot the loop may query is covered.
+            # It spans the finalized boundary up to the slot before this block.
             current_finalized_slot = state.latest_finalized.slot
             current_justified_slots = state.justified_slots.extend_to_slot(
                 current_finalized_slot, slot - Slot(1)
             )
 
-            # Build the chain view as it will appear on the candidate block.
+            # Assemble the chain as it will look once this block is applied.
             #
-            # The view is the recorded history up to the parent.
-            # Then comes the parent root at the parent's slot.
-            # Then zero-hash entries for any skipped slots up to the new block.
-            # The chain-match helper uses this view to validate source and target roots.
+            # 1. History up to the parent.
+            # 2. The parent root at its own slot.
+            # 3. A zero hash for each slot skipped before this block.
+            # 4. Source and target roots are validated against this view.
             num_empty_slots = int(slot - state.latest_block_header.slot - Slot(1))
             extended_historical_block_hashes: list[Bytes32] = (
                 list(state.historical_block_hashes) + [parent_root] + [ZERO_HASH] * num_empty_slots
             )
 
             processed_attestation_data: set[AttestationData] = set()
 
+            # Fixed-point selection.
+            #
+            # - Each pass scans every candidate once, in target-slot order.
+            # - Accepting an entry may advance justification and unlock more.
+            # - Re-scan until a pass finds nothing new.
             while True:
-                found_entries = False
+                found_new_entries = False
 
+                # Visit candidates in target-slot order.
+                # Earlier targets justify first and unlock later ones.
                 for attestation_data, proofs in sorted(
                     aggregated_payloads.items(), key=lambda item: item[0].target.slot
                 ):
                     if attestation_data in processed_attestation_data:
                         continue
 
+                    # Stop once the block holds the maximum distinct data entries.
+                    # This cap is a proposer-side budget, not a consensus rule.
                     if Uint8(len(processed_attestation_data)) >= MAX_ATTESTATIONS_DATA:
                         break
 
+                    # Skip votes whose head block the proposer has not seen.
                     if attestation_data.head.root not in known_block_roots:
                         continue
 
-                    # Chain-match runs first.
+                    # Reject votes that do not match this chain.
                     #
-                    # It rejects checkpoints whose slot is past the chain view.
-                    # That prevents the bounded queries below from indexing out of range.
+                    # This also rejects any checkpoint past the chain view.
+                    # That keeps the bounded lookups below in range.
                     if not attestation_data_matches_chain(
                         attestation_data, extended_historical_block_hashes
                     ):
                         continue
 
-                    # The source slot must already be justified on this chain.
+                    # A vote may only build from an already-justified source.
                     if not current_justified_slots.is_slot_justified(
                         current_finalized_slot, attestation_data.source.slot
                     ):
                         continue
 
-                    # Genesis-anchored votes have source.slot = target.slot = 0.
+                    # Genesis self-votes have source and target both at slot 0.
                     #
-                    # They cannot advance justification: the state transition drops them.
-                    # They still carry head-vote weight for fork choice.
-                    # Including them in the body propagates them into peers' payload pool.
-                    # The bypass below keeps them past the target-already-justified check,
-                    # since slot 0 is implicitly justified and would otherwise filter them.
+                    # - The state transition drops them: they justify nothing.
+                    # - They still carry head weight for fork choice.
+                    # - Including them propagates them to peers.
+                    # - Slot 0 counts as justified, so the next check would drop them.
+                    # - This flag lets them through.
                     is_genesis_self_vote = attestation_data.source.slot == Slot(0) and (
                         attestation_data.target.slot == Slot(0)
                     )
 
-                    # Skip attestations whose target slot is already justified.
+                    # Skip votes whose target slot is already justified.
                     #
-                    # Justification adds nothing for them.
-                    # Entries the state transition will later drop are still kept here.
-                    # They carry head-vote weight for fork choice.
+                    # A justified target gains nothing from more votes.
+                    # Genesis self-votes are exempt, kept for their head weight.
                     if not is_genesis_self_vote and current_justified_slots.is_slot_justified(
                         current_finalized_slot, attestation_data.target.slot
                     ):
                         continue
 
                     processed_attestation_data.add(attestation_data)
+                    found_new_entries = True
 
-                    found_entries = True
-
-                    selected, _ = select_proofs_for_coverage(proofs)
-                    aggregated_signatures.extend(selected)
-                    for proof in selected:
+                    # Choose proofs covering the most validators.
+                    # Emit one attestation per chosen proof.
+                    selected_proofs, _ = select_proofs_for_coverage(proofs)
+                    aggregated_signatures.extend(selected_proofs)
+                    for proof in selected_proofs:
                         aggregated_attestations.append(
                             self.aggregated_attestation_class(
                                 aggregation_bits=proof.participants,
                                 data=attestation_data,
                             )
                         )
 
-                if not found_entries:
+                if not found_new_entries:
                     break
 
-                # Build candidate block and check if justification changed.
+                # Apply the state transition to a trial block.
+                # Its post-state reveals whether this pass advanced justification.
                 candidate_block = self.block_class(
                     slot=slot,
                     proposer_index=proposer_index,
@@ -163,43 +199,48 @@ def build_block(
                 )
                 post_state = self.process_block(self.process_slots(state, slot), candidate_block)
 
-                # Re-run the filter when justification or finalization advanced.
+                # Repeat only if justification or finalization moved.
                 #
-                # Both quantities are monotonic in 3SF-mini, so the loop is bounded.
-                # Finalization advancement shifts the justified window forward.
-                # That can unlock attestations whose target slot was outside it before.
+                # - Both advance monotonically, so the loop is bounded.
+                # - A finalization step slides the justified window forward.
+                # - That can make previously out-of-range targets eligible.
                 if (
-                    post_state.latest_justified != current_justified
+                    post_state.latest_justified != current_justified_checkpoint
                     or post_state.latest_finalized.slot != current_finalized_slot
                 ):
-                    current_justified = post_state.latest_justified
+                    current_justified_checkpoint = post_state.latest_justified
                     current_justified_slots = post_state.justified_slots
                     current_finalized_slot = post_state.latest_finalized.slot
+                    # The chain view never changes between passes.
+                    # Earlier block hashes are fixed once written.
+                    # Attestation processing does not rewrite them.
                     continue
 
                 break
 
-            # Compact: merge all proofs sharing the same AttestationData into one
-            # using recursive children aggregation.
+            # Collapse each attestation data down to a single proof.
             #
-            # During the fixed-point loop above, multiple proofs may have been
-            # selected for the same AttestationData across iterations. Group them
-            # and merge each group into a single recursive proof.
-            proof_groups: dict[AttestationData, list[SingleMessageAggregate]] = {}
+            # - The coverage picker may emit several proofs for one data in a pass.
+            # - A block must carry one attestation per data, over the union of voters.
+
+            # Group every proof under the data it attests to.
+            # Strict pairing guards against the two lists drifting out of sync.
+            signatures_by_attestation_data: dict[AttestationData, list[SingleMessageAggregate]] = {}
             for attestation, signature in zip(
                 aggregated_attestations, aggregated_signatures, strict=True
             ):
-                proof_groups.setdefault(attestation.data, []).append(signature)
+                signatures_by_attestation_data.setdefault(attestation.data, []).append(signature)
 
+            # Rebuild the output lists, one entry per distinct data.
             aggregated_attestations = []
             aggregated_signatures = []
-            for attestation_data, proofs in proof_groups.items():
-                if len(proofs) == 1:
-                    signature = proofs[0]
+            for attestation_data, grouped_signatures in signatures_by_attestation_data.items():
+                if len(grouped_signatures) == 1:
+                    # One proof already covers this data, so use it as-is.
+                    signature = grouped_signatures[0]
                 else:
-                    # Multiple proofs for the same data were aggregated separately.
-                    # Merge them into one recursive proof using children-only
-                    # aggregation (no new raw signatures).
+                    # Fold the proofs into one, each kept as a child.
+                    # Verifying a child needs the public keys of the voters it covers.
                     children = [
                         (
                             proof,
@@ -208,22 +249,24 @@ def build_block(
                                 for validator_index in proof.participants.to_validator_indices()
                             ],
                         )
-                        for proof in proofs
+                        for proof in grouped_signatures
                     ]
+                    # Merge over the union of voters; no new raw signatures are added.
                     signature = SingleMessageAggregate.aggregate(
                         children=children,
                         raw_xmss=[],
                         message=hash_tree_root(attestation_data),
                         slot=attestation_data.slot,
                     )
+
                 aggregated_signatures.append(signature)
                 aggregated_attestations.append(
                     self.aggregated_attestation_class(
                         aggregation_bits=signature.participants, data=attestation_data
                     )
                 )
 
-        # Create the final block with selected attestations.
+        # Assemble the block carrying the chosen attestations.
         final_block = self.block_class(
             slot=slot,
             proposer_index=proposer_index,
@@ -234,7 +277,10 @@ def build_block(
             ),
         )
 
-        # Recompute state from the final block.
+        # Recompute the post-state to obtain the state root.
+        #
+        # Merging proofs keeps the same voters, so the post-state is unchanged.
+        # Only the body's shape differs, so just the root is needed.
         post_state = self.process_block(self.process_slots(state, slot), final_block)
         final_block.state_root = hash_tree_root(post_state)