Skip to content

docs: expand the architecture doc - #581

Open
MegaRedHand wants to merge 9 commits into
mainfrom
docs/architecture
Open

docs: expand the architecture doc#581
MegaRedHand wants to merge 9 commits into
mainfrom
docs/architecture

Conversation

@MegaRedHand

Copy link
Copy Markdown
Collaborator

What

Expands docs/architecture.md from a four-paragraph sketch into a full page, and
wires it into the book. It was never listed in SUMMARY.md, so mdbook did not
render it.

Contents

  • Component diagram of the two actors, the swarm adapter, the aggregation worker
    and the shared Store, plus the actor-protocol message table.
  • BlockChainServer: the tick loop and its idempotency guard, the store-vs-actor
    split of per-interval duties, block import (state transition, state-root check,
    head recompute), aggregation off the message loop, reaggregation on import, the
    sync gate, pending-parent backfill and the chain-event bus.
  • P2PServer: why the swarm lives in its own task, gossip subscriptions, Status
    and range sync, BlocksByRoot retries.
  • Store: what a clone shares, which buffers are never persisted, the
    single-writer discipline.
  • HTTP API and the startup sequence, including the anchor decision and
    shutdown.

Cross-links to slots_and_intervals.md, 3sf_mini.md, lmd_ghost.md,
data_storage.md, checkpoint_sync.md and rpc.md.

Accuracy

Every claim was verified against the code it describes, then re-checked in a
second pass that corrected fifteen of them. Things worth knowing that the earlier
draft got wrong:

  • The aggregation worker holds each finished aggregate until the interval-2
    boundary, so an early session buys proving time and not earlier publication.
  • Only aggregators republish reaggregated votes; everyone else just folds them
    into the local pool for fork-choice weight.
  • pending_blocks is keyed by immediate parent; the deep missing ancestor lives
    in pending_block_parents.
  • Status goes out on the first connection to a peer, not on every connection.
  • The API, metrics and debug routers are separate, and merge onto one listener
    only when the ports match.

Also

  • New Design part in SUMMARY.md holding the page.
  • The introduction enumerated the book's parts but stopped at Operations, leaving
    Spec Deviations invisible from the intro. Both it and the new Design part are
    now listed.

mdbook build passes, including the linkcheck backend.

The page described the two actors in four paragraphs and was never listed in
SUMMARY.md, so mdbook did not render it at all.

Add a component diagram, the actor protocol table, and sections covering the
tick loop, block import, off-loop aggregation, the sync gate, pending-parent
backfill, chain events, the P2P swarm split, the Store and the startup
sequence. Every claim was checked against the code it describes.

Link the page from SUMMARY.md under a new Design part, and mention it in the
introduction.
The introduction enumerated the book's parts but stopped at Operations, so
Spec Deviations was reachable from the sidebar and invisible from the intro.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

This PR contains only documentation changes (no Rust code), so traditional code correctness and security vulnerability checks do not apply. Below is a review of the architectural documentation for clarity, consistency, and potential concerns.

Overall Quality
The documentation is well-structured and provides a clear overview of the dual-actor genserver architecture. The ASCII diagram effectively illustrates the message flow between BlockChainServer and P2PServer.

Specific Items

  1. XMSS State Persistence (docs/architecture.md:74)
    The document states that the actor "advances its XMSS signing keys on every tick" and catches up at spawn. For XMSS (a stateful signature scheme), key index advancement must be crash-safe. Consider adding a note clarifying whether this index is persisted atomically with state transitions, as key reuse would be catastrophic for security.

  2. Store Consistency Model (docs/architecture.md:165,182)
    The text states that cloning the Store shares "in-memory buffers that fork choice runs on" and that HTTP handlers read directly from the Store without actor messaging. While the document notes that only BlockChainServer writes, it is unclear how readers avoid observing partially-written state during a block import. Consider explicitly stating the synchronization mechanism (e.g., immutable data structures, versioning, or read-copy-update) to clarify the safety of concurrent reads.

  3. Resource Exhaustion Risk (docs/architecture.md:195)
    The document mentions raising file descriptor limits for "RocksDB's unbounded table cache." If the cache is truly unbounded, this poses a memory exhaustion risk on long-running nodes. If this is a documentation of current behavior rather than intended design, consider adding a warning about monitoring memory usage, or verify if the code actually imposes bounds.

  4. Weak Subjectivity (docs/architecture.md:200)
    The description of resuming a stale database without checkpoint sync ("resumed anyway, with a warning") could be dangerous if the database is sufficiently old (weak subjectivity violation). Consider strengthening the warning to indicate that this should only occur in trusted development environments, not production.

Minor

  • docs/architecture.md:30: The box-drawing diagram may render poorly on mobile viewers. Consider testing narrow-screen rendering.
  • docs/introduction.md: The reorganization from two to four parts is accurate and the new links are valid.

Summary
No blocking issues. The documentation accurately describes the node's architecture. Addressing the XMSS persistence and Store consistency clarifications would improve operational safety guidance.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Findings:

  1. docs/architecture.md says the two actors “never share memory,” but the implementation deliberately shares a cloned Store across actors and RPC, and that clone shares backend/cache/buffer state via Arc<Mutex<...>> (bin/ethlambda/src/main.rs, bin/ethlambda/src/main.rs, crates/storage/src/store.rs). That is a real architecture constraint, so the doc should say they do not share actor-owned mutable state directly, not that they share no memory.

  2. docs/architecture.md says subnet subscriptions are computed from the validator set and aggregator role. That is incomplete: the actual subscription set also depends on explicit aggregate_subnet_ids, and a validator-less aggregator falls back to subnet 0 (crates/net/p2p/src/lib.rs, bin/ethlambda/src/main.rs). Since subnet coverage affects attestation gossip and aggregation behavior, the architecture note should include that configuration input.

  3. docs/architecture.md says range sync “drops peers that fall behind the range.” The code only drops them from the current range-sync candidate set; it does not disconnect them from libp2p (crates/net/p2p/src/lib.rs, crates/net/p2p/src/req_resp/handlers.rs). That wording overstates the behavior and could mislead someone debugging peer retention.

No consensus/security/runtime code changes are in this PR, so I did not find functional regressions in fork choice, attestation processing, state transition, XMSS verification, or SSZ handling from the diff itself.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

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