Skip to content

refactor(forks/lstar): split spec.py into per-concern mixins#817

Merged
tcoratger merged 1 commit into
leanEthereum:mainfrom
leolara:refactor/lstar-spec-mixins
Jun 1, 2026
Merged

refactor(forks/lstar): split spec.py into per-concern mixins#817
tcoratger merged 1 commit into
leanEthereum:mainfrom
leolara:refactor/lstar-spec-mixins

Conversation

@leolara
Copy link
Copy Markdown
Contributor

@leolara leolara commented Jun 1, 2026

refactor(forks/lstar): split spec.py into per-concern mixins

Summary

src/lean_spec/spec/forks/lstar/spec.py had grown to a single ~1875-line class,
LstarSpec, owning every concern of the fork: state transition, fork choice,
block production, signature verification, aggregation, interval ticking, and
validator duties. This PR splits that class into one file per concern, assembled
back into the same LstarSpec via mixins.

The split is a pure structural move: every method body is byte-for-byte
identical to before, and there are no changes to any external call site or type
annotation
. LstarSpec is still a single concrete class exposing the same
methods with the same signatures.

Motivation

  • One ~1875-line file is hard to navigate, review, and reason about. Fork choice
    and the state-transition function are large, independent subsystems that were
    interleaved in one class body.
  • The split makes each subsystem a self-contained, independently reviewable unit
    while preserving the exact public surface the node services and tests depend on.
  • It sets up the multi-fork direction (see "Extending a fork" below) without
    changing the agnostic ForkProtocol or the way callers use the spec.

What changed

LstarSpec is now assembled from seven mixins plus a small typing contract:

File Contents
_contract.py LstarSpecContract(ForkProtocol) — the only new construct. Declares the concrete container-factory types and the cross-mixin method surface. Also hosts the LstarStore alias.
state_transition.py StateTransitionMixin + the module function attestation_data_matches_chain.
signatures.py SignatureMixin.
block_production.py BlockProductionMixin.
fork_choice.py ForkChoiceMixin.
aggregation.py AggregationMixin.
timeline.py TimelineMixin.
validator_duties.py ValidatorDutiesMixin.
spec.py LstarSpec(...the seven mixins, LstarSpecContract) — identity fields, the *_class values, and the LstarStore re-export.

Two deliberate edits, everything else verbatim:

  1. The _attestation_data_matches_chain static method became a module-level
    function attestation_data_matches_chain (it never used self). Its two call
    sites now call it as a free function; block_production.py imports it from
    state_transition.py.
  2. The LstarStore = Store[State, Block] alias moved into _contract.py and is
    re-exported from spec.py, so from .spec import LstarStore (used by both
    __init__.py files) keeps working.

The typing contract

The mixins call each other through self (e.g. fork choice's block import calls
self.state_transition(...)). For the type checker to accept those cross-file
calls, each mixin needs to know the signatures of the siblings it calls and the
concrete types of the container factories it uses. LstarSpecContract provides
exactly that:

  • The nine *_class attributes, annotated with the concrete lstar types
    (type[State], not type[SpecStateType]), so self.state_class(...) calls
    type-check. No ClassVar (it would block subclass narrowing).
  • The ten methods that are actually called across file boundaries, as
    @abstractmethod signatures. (Methods only called within their own file are not
    in the contract.)

LstarSpecContract lives inside the lstar package, never in protocol.py,
because it references concrete lstar containers and ForkProtocol must stay
fork-agnostic (enforced by an existing AST import-guard test).

How callers are unaffected

ForkProtocol declares only three abstract methods (generate_genesis,
create_store, upgrade_state); every other method is called through the
concrete LstarSpec type — by the node services (spec: LstarSpec) and the
tests. Because the split keeps LstarSpec as one class exposing all those methods
unchanged, no caller, annotation, or import outside this package changes.

Extending a fork: descendants override methods directly, not mixins

This is the part most relevant to the multi-fork roadmap. The mixins are an
internal organization detail of lstar. A descendant fork does not need to
know they exist, and does not add or re-compose its own mixins.

A new fork subclasses the assembled class and overrides what it needs, directly on
the fork class:

class Devnet5Spec(LstarSpec):                       # inherits all seven mixins transitively
    block_class: type[Devnet5Block] = Devnet5Block  # swap one container type

    def upgrade_state(self, state: State) -> Devnet5State:
        ...                                          # the migration every fork overrides

    def process_attestations(self, state, attestations):  # OPTIONAL: override one method
        ...                                               # lives in StateTransitionMixin on lstar

Why overriding on the fork class is enough — late binding. Every sibling call
goes through self, so it is resolved against the runtime type on every call.
Lstar's inherited tick_interval (in the timeline mixin) calls
self.update_safe_target(...); when invoked on a Devnet5Spec instance, self
is Devnet5Spec, so it dispatches to Devnet5's override — even though
tick_interval was written long before Devnet5 existed and lives in a different
file. Change one method on the fork class, and every inherited caller picks it up.
This is identical to overriding a method on any ordinary subclass; the mixin
decomposition changes nothing about it.

# Minimal illustration: A in one mixin, B in another.
class StateTransitionMixin(LstarSpecContract):
    def A(self): return self.B() + 1     # cross-mixin call via self

class ForkChoiceMixin(LstarSpecContract):
    def B(self): return 10

class LstarSpec(StateTransitionMixin, ForkChoiceMixin, LstarSpecContract): ...

class Devnet5Spec(LstarSpec):
    def B(self): return 100              # override B only; leave A inherited

Devnet5Spec().A()   # -> 101  (inherited A used the overridden B)
LstarSpec().A()     # -> 11

The only condition: a method must call its sibling as self.B(...) (which the
split preserves everywhere). To extend rather than replace, use super():

class Devnet5Spec(LstarSpec):
    def update_safe_target(self, store):
        store = super().update_safe_target(store)   # run lstar's version
        ...                                          # then add behavior

And if a fork changes only a container (not logic), it overrides no method at all:
the inherited bodies already build via self.block_class(...), which resolves to
the fork's container at runtime.

So: subclass LstarSpec, override the attributes/methods that differ, done. No
new mixins, no new contract base.

When a mixin is not the right tool (and you want components instead)

Mixins are excellent for method-level reuse and override, which is what the
roadmap's "swap a container, inherit the logic, occasionally tweak a method" needs.
They are a poor fit for subsystem-level swapping, and that is the signal to
reach for a different structure (composed components — "Option 3" in the design
notes).

Reach for a component, not a mixin override, when any of these hold:

  • A fork replaces a whole subsystem, not a method or two — e.g. an entirely
    different fork-choice rule. Overriding a dozen interrelated methods scattered
    across the MRO is fragile; a replacement still has to silently honor the implicit
    contract the other mixins rely on, with no enforced interface.
  • Two implementations must coexist or be selected at runtime/config time.
    Mixins bind one implementation per class via the MRO; you cannot hold two. A
    component is an object you can choose, inject, or swap.
  • The subsystem needs its own state or dependencies. A mixin shares the single
    self namespace and cannot cleanly own its own fields or collaborators; a
    component can.
  • You want a real, enforced interface boundary (so the type checker verifies a
    replacement is complete). The mixin contract is an internal convenience, not a
    public API; a component exposes an explicit protocol.
  • You find yourself writing per-fork mixins and re-ordering base classes to
    assemble a fork. That re-composition is the smell: the concern wants to be a
    swappable object, not a class fragment glued by inheritance order.

When that day comes, the migration is incremental: each mixin already encapsulates
one concern and can be promoted to a held collaborator (e.g. self.fork_choice)
exposing a typed interface, without disturbing the other concerns.

Rule of thumb: tweak a method → override on the fork subclass (mixins are
invisible). Replace a subsystem or need two of something → make it a component.

Verification

  • Verbatim: an AST diff of all 30 methods (the 29 now on the class plus the
    relocated chain-match helper) against their new homes confirms byte-identical
    bodies, modulo the two intended edits above.
  • Contract parity: the 10 abstract signatures match their implementations
    exactly; an independent recomputation of the cross-file call graph confirms the
    contract covers exactly those 10 methods (none missing, none unused).
  • Runtime: LstarSpec() instantiates; the MRO is the expected linear chain;
    __abstractmethods__ is empty.
  • just check passes (ruff lint + format, ty type check, codespell, mdformat,
    lock).
  • just test passes (3084 tests, ~93% coverage); the consensus fixture path runs
    through fill against the refactored spec.
  • A multi-dimension adversarial review of the split (verbatim, imports/cycles,
    contract/typing, API compatibility, docs/style, plan adherence) found no
    blocker or major issues
    . After rebasing onto current main, the split was
    re-extracted from the updated spec.py (which had gained the shared
    ancestor-weight-walk helper and dropped a vestigial parameter) and re-verified
    with the same verbatim, contract-parity, type-check, and full-test gates.

Compatibility notes

  • No backward-compatibility shims were added (per repo policy). LstarStore is
    relocated, not aliased twice; the only re-export is the pre-existing public path.
  • ForkProtocol / protocol.py is untouched and stays fork-agnostic.
  • Pyright (not the repo's checker) reports the *_class narrowing as an invariant
    override; this is pre-existing — the original LstarSpec narrowed the same
    attributes — and accepted by ty, which is the project's type checker.

Follow-ups (out of scope)

  • One pre-existing banner-style separator comment carried over verbatim inside the
    aggregation logic. It violates the comment-style rule but predates this change;
    leaving it keeps this PR a pure move. Worth a separate doc-cleanup pass.

Break the single large LstarSpec class into one file per concern (state
transition, fork choice, block production, signatures, aggregation, timeline,
validator duties), assembled back into LstarSpec via mixins over a shared
typing contract (LstarSpecContract).

Pure structural move: every method body is byte-for-byte identical, with no
change to any external call site or type annotation. The only edits are
relocating the chain-match static helper to a module-level function and
moving the LstarStore alias into the contract module (still re-exported from
spec.py).
@tcoratger tcoratger force-pushed the refactor/lstar-spec-mixins branch from c93eed2 to e3072b0 Compare June 1, 2026 20:02
@tcoratger tcoratger marked this pull request as ready for review June 1, 2026 21:22
@tcoratger tcoratger merged commit 12c8820 into leanEthereum:main Jun 1, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants