Skip to content

docs: add node-types page; trim duplicate ASCII diagrams from halt RCA - #70

Open
luishsr wants to merge 3 commits into
mainfrom
docs/node-types-architecture
Open

docs: add node-types page; trim duplicate ASCII diagrams from halt RCA#70
luishsr wants to merge 3 commits into
mainfrom
docs/node-types-architecture

Conversation

@luishsr

@luishsr luishsr commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds doc/node-types.md: Validator/Observer/Archive reference (role, config, why it matters), cross-linked from doc/README.md and doc/glossary.md.
  • In doc/incidents/2026-06-21-mainnet-halt.md, removes the characters-based pipeline/failure/fix diagrams, which duplicated the existing Mermaid diagrams — Mermaid is now the single source. The one unique data point (observed telemetry) is preserved as a plain note.

Test plan

  • Docs only — no code changes. Preview rendered Markdown/Mermaid on GitHub.

Adds doc/node-types.md (Validator/Observer/Archive reference) cross-linked
from README and glossary. In the mainnet-halt incident doc, removes the
characters-based pipeline/failure/fix diagrams now that Mermaid renders
of the same content exist, keeping the Mermaid versions as the single
source and folding the one unique data point (observed telemetry) into
a plain note.
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @luishsr's task in 2m 25s —— View job


PR Review

  • Gather context and read changed files
  • Review doc/node-types.md
  • Review doc/incidents/2026-06-21-mainnet-halt.md
  • Review doc/README.md and doc/glossary.md changes
  • Post final review

This is a well-grounded documentation PR. Code references are specific (file paths + line numbers), cross-links are accurate, and the "Archive is not a code concept" disclaimer in node-types.md:148-150 is exactly the right thing to say. Two issues need fixing before merge; the rest are observations.


Issues to fix

1. Stray code fence at end of incident file (doc/incidents/2026-06-21-mainnet-halt.md:108)

The file ends with a lone ``` on line 108 that has no matching opening fence. GitHub's renderer will treat the entire Evidence section's last bullet as being inside an unclosed code block, mangling its display. Fix this →

# line 107 (last bullet, correct)
- fix: `3a71a1a` — `consensus_bus.rs` (`executed_anchor`), ...
# line 108 (delete this)

**2. Mermaid `O --> A` edge implies data flow, but Archive is just a pruning config (`doc/node-types.md:35-36`)**

```mermaid
O -->|"run without --full / --minimal"| A["Archive<br/>..."]

This reads as "Observer streams to Archive," which contradicts the prose that correctly says Archive is an Observer with pruning disabled — not a downstream consumer. The edge direction and label mislead: someone skimming the diagram will infer there are two separate node processes (Observer feeding Archive), when there is actually one process run with different flags.

Suggested fix — use a style note or a subgraph to make Archive a configuration of Observer, or rephrase the edge: Fix this →


Observations (no action required, but worth knowing)

doc/incidents/ has no index entry in doc/README.md

The new doc/incidents/ directory isn't linked anywhere from the top-level README. This is the first post-mortem; if more will follow, adding a "Incidents / post-mortems" entry to doc/README.md now would be cleaner than retroactively adding it later. Low priority — fine to do as a follow-up.

node-types.md correctly disclaims "Archive" as a synthesis, not a prior convention

This page is the first place that names "Archive" as a distinct node type — there is no prior doc or code convention to defer to here (node-types.md:147-150)

This transparency is good and matches the stated goal of the PR. No action needed.

All code-path references verified: consensus_bus.rs, certifier.rs, primary.rs, proposer/mod.rs, consensus/state.rs, network/handler.rs, rpc_server_args.rs, persistence.rs, and batch-builder/src/lib.rs all exist at the cited paths. The glossary cross-link (glossary.md:74node-types.md) resolves correctly now that the file exists.


Summary: Approve after the stray ``` on line 108 is removed and the O --> A Mermaid edge is reworded to avoid implying data-flow between Observer and Archive. Everything else is solid.

@github-actions

Copy link
Copy Markdown
Contributor

Security Scan - Code

Severity: HIGH, CRITICAL

No vulnerabilities found

View scan results

Report Summary

┌─────────────────────────────────────────────┬────────────┬─────────────────┬───────────────────┐
│                   Target                    │    Type    │ Vulnerabilities │ Misconfigurations │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ Cargo.lock                                  │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/Cargo.lock      │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/fuzz/Cargo.lock │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/state-sum/Cargo.lock                    │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/tps/package-lock.json                   │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ rayls-contracts/package-lock.json           │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/chaos-network/Dockerfile                │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-network/Dockerfile               │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-replay/Dockerfile                │ dockerfile │        -        │         0         │
└─────────────────────────────────────────────┴────────────┴─────────────────┴───────────────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)

…diagram

Issue #48 asked the diagram to cover consensus participation, state, RPC
exposure, and public access. RPC exposure/public access were only in the
comparison table and prose, not the diagram graphic — add a per-node RPC
line so the diagram carries all four dimensions.
@luishsr

luishsr commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Outstanding from #48's Definition of Done: per-node machine sizing/cost is still open and needs SRE input — not addressed by this PR. `doc/node-types.md` flags it explicitly under "Open questions" rather than answering it, so it isn't lost, but it should be resolved before this doc is treated as complete guidance for provisioning.

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @luishsr's task in 1m 23s —— View job


PR Review — follow-up after commit 69ee8ab

  • Gather context and read changed files
  • Check if stray ``` on line 108 was removed (doc/incidents/2026-06-21-mainnet-halt.md)
  • Check if O --> A Mermaid edge was corrected (doc/node-types.md)
  • Review new commit content (RPC exposure/public-access diagram)
  • Post final review

New commit (69ee8ab) — good addition

Adding RPC exposure labels directly into the Mermaid diagram nodes is the right call: it makes the diagram self-contained across all four dimensions (committee membership, voting, state, RPC posture) without requiring readers to cross-reference the table. The per-node labels are accurate:

  • RPC: loopback only — not public on Validator matches the prose and code at rpc_server_args.rs:57-58
  • RPC: public — accepts tx submission on Observer and RPC: public — plus full history queries on Archive are both consistent with the operator guidance.

Issues still open from the previous review

Both items flagged earlier are unaddressed in this commit.

1. Stray ``` on line 108 of the incident file — still present

doc/incidents/2026-06-21-mainnet-halt.md:108 still ends with a lone ``` that has no matching opening fence:

107: - fix: `3a71a1a` — `consensus_bus.rs` (`executed_anchor`), ...
108: ```    ← delete this line

GitHub's renderer treats everything from the "Evidence" section's last bullet onward as inside an unclosed code block, making the evidence list unreadable. Fix this →

2. O --> A edge still implies data flow — still present

The diagram still has:

O -->|"run without --full / --minimal"| A["Archive..."]

This reads as "Observer streams to Archive," implying two separate node processes with a data pipe between them. The prose on node-types.md:159 correctly says "it is an Observer" — the diagram contradicts that. A reader skimming the diagram will infer a topology that doesn't exist.

Suggested fix — use a dotted line and reword the edge so it reads as a configuration variant, not a data-flow hop:

-    O -->|"run without --full / --minimal"| A["Archive<br/>Observer + pruning disabled<br/>full tx/log history<br/>RPC: public — plus full history queries"]
+    O -. "configured as (pruning flag omitted)" .-> A["Archive<br/>Observer + pruning disabled<br/>full tx/log history<br/>RPC: public — plus full history queries"]

Or, more explicitly, wrap Archive in a subgraph as a config variant of Observer. Fix this →


Summary: The new commit improves the diagram and addresses the Issue #48 completeness requirement. The two blocking issues (stray ``` and misleading edge direction) still need to land before merge.

@ncervaens

Copy link
Copy Markdown

This documentation helps a lot to understand globally Axyl. As I previously said its very technical with references to repo files so it belong here in .md files. Its worth that Limechain validates this docs by approving the PR.
But we need to have a more "newbies" documentation in our documentation website. We can create a new task for this.

@kstoykov

Copy link
Copy Markdown
Contributor

I would suggest to remove the link to fixed code lines in the .md files because the code changes quite faster than the .md files.

Also, do you think it could be good if we add a "Last modified date" or "Valid for commit #...". The idea is that these documents are always behind the code and it could be useful for a reader to know when exactly these docs were updated.

…rkers

Per review on #70 (kstoykov): pinpoint file:line citations rot as the
code moves faster than the docs. Switched node-types.md and glossary.md
to file-level references only, and added a "Last verified against
commit <sha>" note near the top of each so readers know how stale a
given read might be.

Left doc/incidents/2026-06-21-mainnet-halt.md's line numbers as-is —
those are a forensic record of the pre-fix code at incident time, not
a current pointer — but added an explicit note saying so, since the
same staleness concern applies if a reader assumes they're current.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@luishsr

luishsr commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

Done in d65bb87 — both suggestions:

  • Dropped line-pinned refs in node-types.md and glossary.md: everything is now a file-level pointer (crates/foo/bar.rs, not crates/foo/bar.rs:123), so a future refactor doesn't leave a misleading line number behind.
  • Added a freshness marker: both pages now open with "Last verified against commit <sha>" right under the title.

Left doc/incidents/2026-06-21-mainnet-halt.md as an exception — its file:line citations are a forensic record of the pre-fix code at the moment of the incident, not a current pointer, so removing them would lose real information. Added an explicit note there instead, so a reader doesn't mistake them for current line numbers either.

@ncervaens — noted on getting Limechain to review/approve, and agreed a separate "newbies" doc-website task makes sense as a follow-up rather than folding it into this page.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @luishsr's task in 1m 37s —— View job


PR Review — follow-up after commit d65bb87

  • Gather context
  • Read doc/incidents/2026-06-21-mainnet-halt.md — check stray ``` fix
  • Read doc/node-types.md — check O → A edge fix + freshness marker
  • Read doc/glossary.md — check freshness marker + file-level refs
  • Post final review

New commit (d65bb87) — changes are good

The two changes requested by @kstoykov are correctly implemented:

  • Freshness marker: both node-types.md and glossary.md now open with > Last verified against commit \69ee8ab`.` — the right call to make staleness visible at a glance.
  • File-level refs only: all file.rs:NNN line-pinned citations in both files have been replaced with file-path-only pointers. The incident file is correctly left as an exception, with an explicit forensic caveat at lines 99–101 so readers don't mistake the historical line numbers for current ones.

One minor note: the marker says "Last verified against 69ee8ab" while d65bb87 (the commit that wrote the marker) is HEAD. That's defensible — the code was verified in 69ee8ab; d65bb87 is docs-only. Still, a future reader running git log may find it briefly confusing. Not a blocker.


Two blocking issues still unaddressed

Both items flagged in the two previous reviews remain unresolved in d65bb87.

1. Stray lone code fence at the end of the incident file — still present

doc/incidents/2026-06-21-mainnet-halt.md:112 (was line 108 before the forensic note was inserted) ends with a lone ``` that has no matching opening fence:

111: - fix: `3a71a1a` — `consensus_bus.rs` (`executed_anchor`), ...
112: ```    ← delete this line

GitHub's renderer treats the last bullet and everything after it as being inside an unclosed code block, mangling the Evidence section. Fix this →

2. O --> A solid arrow still implies data flow — still present

doc/node-types.md:39 still reads:

O -->|"run without --full / --minimal"| A["Archive..."]

A solid --> arrow labelled with a flag-omission reads as "Observer emits data to Archive," implying two separate node processes with a pipe between them. The prose on line 161 correctly says "it is an Observer" — the diagram contradicts that.

Suggested one-line fix — switch to a dotted line and a config-variant label:

-    O -->|"run without --full / --minimal"| A["Archive<br/>Observer + pruning disabled<br/>full tx/log history<br/>RPC: public — plus full history queries"]
+    O -. "configured as (pruning flag omitted)" .-> A["Archive<br/>Observer + pruning disabled<br/>full tx/log history<br/>RPC: public — plus full history queries"]

Fix this →


Summary: d65bb87 correctly lands the freshness markers and file-level refs. The two items that have been flagged since the first review — the stray ``` and the misleading Mermaid edge — still need to be fixed before this is ready to merge.

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.

4 participants