Skip to content

docs(sync-service): trim heavy comments and inline single-use helpers - #1115

Merged
tcoratger merged 1 commit into
leanEthereum:mainfrom
tcoratger:refactor/sync-service-doc-trim
Jun 17, 2026
Merged

docs(sync-service): trim heavy comments and inline single-use helpers#1115
tcoratger merged 1 commit into
leanEthereum:mainfrom
tcoratger:refactor/sync-service-doc-trim

Conversation

@tcoratger

Copy link
Copy Markdown
Collaborator

Summary

Documentation and minor-structure cleanup of the sync orchestrator. No behavior change — process_block, the gossip handlers, replay, proof recovery, and the state machine are untouched in logic. The file drops 105 lines, almost all comment/docstring weight.

Changes

  • One-line docstrings for the retryable-attestation error, the publish-callback and block-aggregates fields, and the block proof-recovery method.
  • Proof-recovery comments trimmed from a ~50-line essay to one-sentence-per-line notes, keeping only the non-obvious reasoning (parent-state key resolution, the order-invariant fold, proof supersession).
  • Dropped the list() wrap around the block attestations — the SSZ list is a concrete sequence, iterated natively.
  • Removed banned markdown-bold Why: labels in the persistence prune comment; the genesis and finalized-root rationale is now plain prose.
  • Shortened the block-processing, recovery-call, and write-back comments to the load-bearing facts (keeping the no-await write-back invariant the review flagged).
  • Inlined two single-use helpers (component wiring into post-init; the block-aggregate publish loop into the gossip handler) and deleted them.

Considered and deliberately kept

The review weighed several more removals that were not applied, because they are multi-caller methods encapsulating a shared invariant (inlining would duplicate guard logic and risk drift):

  • _deconstruct_block_into_store stays a method on the orchestrator — relocating a 165-line isolated function to its own file is relocation, not decoupling.
  • _integrate_gossip_attestation / _integrate_gossip_aggregated_attestation — each has two callers (live + replay) and centralizes the unknown-block rejection remap.
  • _transition_to — two callers plus two tests pinning the invalid-transition guard.
  • has_root / head_slot — the read-only StoreView protocol implementation that backfill depends on.

Not addressed (flagged for a follow-up)

The aggregated-attestation gossip handler increments no valid/invalid metrics while the single-attestation handler does — likely an oversight, but a behavior change, so left out of this docs pass.

Testing

  • just check passes.
  • tests/node/sync/test_service.py passes (35 tests).

🤖 Generated with Claude Code

Cut the orchestrator's documentation to the project terseness bar and
remove two single-use helpers, with no behavior change.

- Collapse multi-paragraph docstrings to one line each: the retryable
  attestation error, the publish callback and block-aggregates fields,
  and the proof-recovery method.
- Trim the proof-recovery method's inline comments from a ~50-line essay
  to one-sentence-per-line notes, keeping only the non-obvious reasoning
  (parent-state key resolution, the order-invariant fold, supersession).
- Drop the list() wrap around the block attestations; the SSZ list is a
  concrete sequence and is iterated natively.
- Remove the banned markdown-bold "Why:" labels in the persistence prune
  comment and state the genesis and finalized-root rationale as prose.
- Shorten the block-processing, deconstruct-call, and write-back comments
  to the load-bearing facts, keeping the no-await write-back invariant.
- Inline the component wiring into post-init and the block-aggregate
  publish loop into the gossip handler, deleting both single-use helpers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tcoratger
tcoratger merged commit 456e046 into leanEthereum:main Jun 17, 2026
14 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.

1 participant