Skip to content

feat(tooling): add multi-server-devnet skill - #565

Open
MegaRedHand wants to merge 7 commits into
mainfrom
feat/multi-server-devnet-skill
Open

feat(tooling): add multi-server-devnet skill#565
MegaRedHand wants to merge 7 commits into
mainfrom
feat/multi-server-devnet-skill

Conversation

@MegaRedHand

Copy link
Copy Markdown
Collaborator

Adds the multi-server-devnet skill: the operational knowledge for the
long-lived devnets that run as detached docker containers on remote hosts, one
independent single-host devnet per server, federated into one Grafana.

Nothing in it is deployment-specific. Hosts, node counts, subnet counts, central
prometheus/loki urls and Grafana ids all come from a gitignored
scripts/devnet.env (template committed as devnet.env.example), and every
dashboard picks its datasource through a template variable rather than a pinned
uid, so the JSON drops into any Grafana unedited.

Contents

Area What's here
Topology One devnet per server, kept apart by pointing every ENR at 127.0.0.1; three-layer naming (identity node_N / container <client>_N / index N)
Genesis make-genesis.sh, merge-keyshards.py (hash-sig-cli has no --start-index), subnet-align-validators.py
Lifecycle start-devnet.sh, cs-restart.sh, agg-restart.sh, convert.sh (per-client CLI shapes for zeam/ream/qlean/grandine/gean/lantern)
Observability prometheus-config.sh, promtail-config.sh, start-promtail.sh, the finality Slack alert, and four dashboards (client, finality, logs, resources)
Audit sweep.sh for per-devnet head/justified/finalized + client mix
References operations.md, clients.md, node-health.md (per-node "is this node working" checklist with a query and a log grep per item)

The golden rules in SKILL.md each came out of an outage: always pass a
checkpoint-sync URL on restart, wait 60s between stop and start so the node
rejoins the gossip meshes, one stable GENESIS_TIME per devnet, and keep the
swap + per-container memory guards so one client's leak OOMs its own container
instead of starving the host.

Client dashboard start-time panels

The two follow-up commits rework how the client dashboard reports node starts,
since both panels answered a question nobody asks:

  • "Latest start time" → "Oldest node start". It used max(), i.e. the
    newest process start, so one restarted node made a fleet that had been up
    for days look fresh. Now min(), rendered relative ("6 days ago"), which is
    the devnet's uninterrupted age.
  • "Start time" → "Time since node start". It plotted
    changes(lean_node_start_time_seconds[1m]), which drew flat zeros when
    nothing had restarted and, worse, hid restarts that had happened: the spike
    is only as wide as the range window, so Grafana's step steps over it at any
    dashboard range past about an hour. Verified on live data over 7 days — 2
    non-zero points at step=60s, zero at step=300s. Replaced with one tile per
    node showing age since start, coloured red < 15m / orange < 1h / yellow < 6h /
    dark green beyond, so a restart lights up and decays back to calm.

Both panels note that only ethlambda exports
lean_node_start_time_seconds, so converted canaries never appear in them.

Testing

The skill's dashboards are deployed and serving on the central Grafana; the
committed client-dashboard.json is byte-identical to the live copy there.
Panel queries were checked against live Prometheus across devnet-eth2 /
devnet-eth35 / devnet-eth4.

Captures the operating procedure for the federated devnets (one independent
single-host devnet per server, federated into one Grafana), which until now
lived only on the hosts: genesis generation, per-host prometheus/promtail
config emitters, client conversion, checkpoint-sync restarts, finality alert.

Vendors the four provisioned Grafana dashboards so the repo is their source of
truth. Their directory is bind-mounted read-only into the container, so UI
edits get reverted by the provisioner and nothing syncs server-side changes
back; resources-dashboard.json had already drifted three panels behind the
deployed copy.

Nothing about a particular deployment is committed. Hosts, urls and Grafana
ids live in a gitignored devnet.env (devnet.env.example documents every key);
the dashboards resolve their datasource through a template variable instead of
a pinned uid, so they drop into any Grafana unedited; and the finality alert's
Slack webhook, Grafana base url and prometheus datasource uid are substituted
at deploy time, since a provisioned alert rule cannot use a datasource
variable. webhook.txt and devnet.env are gitignored so the webhook can't be
committed by accident.
Halve the client dashboard's Overview stat and slot-graph rows so the
finality-delay graphs sit closer to the fold, and reframe both start-time
panels around the question they are actually asked: how long has this been
running?

- "Latest start time" -> "Oldest node start". max() reported the *newest*
  process start, so a single restarted node masked a fleet that had been up
  for days; min() gives the devnet's uninterrupted age instead. Rendered with
  dateTimeFromNow ("6 days ago") rather than an ISO timestamp the reader has
  to subtract by hand.

- "Start time" -> "Node start times", timeseries -> table. changes(...[1m])
  drew flat zeros whenever nothing had restarted recently, and its spike is
  only as wide as the range window, so Grafana's step aliased the event away
  at any dashboard range past ~1h: the panel read "no restarts" whether or
  not restarts had occurred. A per-node table of start times, newest first,
  cannot hide one. Also replaces that panel's description, which was a
  copy-paste of the processed-slots text.

Both panels carry a note that only ethlambda exports
lean_node_start_time_seconds, so converted canaries never appear.
Replace the per-node start-time table with a stat panel that draws one tile
per node, so a restart is visible at a glance instead of requiring the reader
to compare timestamps down a column.

The tile value is an age (time() - lean_node_start_time_seconds) rather than
the start timestamp: thresholds compare a field's own value, so a tile holding
an epoch timestamp could only be coloured against a fixed calendar date, which
goes stale immediately. Age makes the bands meaningful and permanent -- red
under 15m, orange under 1h, yellow under 6h, dark green beyond -- so a
restarted node lights up and decays back to calm over six hours.

Trade-off: Grafana cannot sort stat tiles, so ordering follows the query's
series order. Colour, not position, is what surfaces the event.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🤖 Kimi Code Review

⚠️ Warning: Diff was truncated from 12990 to 10000 lines. Some changes were not reviewed.

This PR adds operational tooling (documentation, shell scripts, Grafana dashboards) for managing multi-server devnets. Since this is infrastructure code rather than core consensus logic, the review focuses on operational safety, script correctness, and documentation maintainability.

Critical Issues

1. Fragile Line Number References in Documentation

Files: node-health.md, operations.md

Specific line number references to Rust source files will become stale when code changes, leading to operational confusion:

  • node-health.md:119: `validate_attestation_data`, `store.rs:172`
  • node-health.md:124: `produce_attestation_data` (`store.rs:759`)
  • node-health.md:126: `get_attestation_target` (`store.rs:646`)
  • operations.md:78: `crates/blockchain/src/store.rs:566 .expect("safe target exists")`

Recommendation: Replace line numbers with function names and module paths only (e.g., crates/blockchain/src/store.rs in function validate_attestation_data). If specific line references are needed for debugging, include the git commit hash the reference was verified against.

2. Unsafe File Deletion in agg-restart.sh

File: scripts/agg-restart.sh:32

sudo rm -rf "$DATA/$name"/* 2>/dev/null

Issues:

  • The glob * expands before sudo executes, potentially causing permission errors or partial deletion if the user running the script lacks read access to $DATA/$name
  • 2>/dev/null suppresses all errors, masking disk failures or permission issues
  • No verification that $DATA/$name is actually a devnet data directory (could be empty string or /)

Recommendation:

# Validate path before deletion
if [[ -d "$DATA/$name" && "$name" == node_* ]]; then
    sudo find "$DATA/$name" -mindepth 1 -delete
else
    log "ERROR: Invalid data directory $DATA/$name"
    exit 1
fi

3. Unquoted Variables in Subshell Commands

File: scripts/agg-restart.sh:28

[ -n "$cid" ] && sudo sh -c "docker logs $cid > '$CRASH/node_${n}-${TS}.log' 2>&1"

$cid is not quoted inside the sh -c string. While container IDs shouldn't contain spaces, this is brittle.

Recommendation:

[ -n "$cid" ] && sudo sh -c "docker logs \"$cid\" > '$CRASH/node_${n}-${TS}.log' 2>&1"

Or better, avoid the subshell:

[ -n "$cid" ] && sudo docker logs "$cid" > "$CRASH/node_${n}-${TS}.log" 2>&1

Security & Safety Concerns

4. Missing Error Handling in agg-restart.sh

File: scripts/agg-restart.sh

  • Uses set -u but not set -e (continues on error)
  • docker run output redirected to /dev/null (line 39), hiding startup failures
  • No validation that CS_PORT (checkpoint sync source) is actually healthy before restarting nodes
  • No validation that $IMAGE exists locally or in registry before stopping the old container

Recommendation: Add health checks:

# Verify checkpoint source is healthy before proceeding
if ! curl -sf "http://127.0.0.1:$CS/lean/v0/states/finalized" >/dev/null; then
    log "ERROR: Checkpoint sync source on port $CS is not healthy"
    exit 1
fi

5. Input Validation Missing

File: scripts/agg-restart.sh:14

CS=$1; shift; NODES="$*"

No validation that CS is a number, or that nodes exist, or that $IMAGE is set to a valid value.

Recommendation: Add validation:

if ! [[ "$CS" =~ ^[0-9]+$ ]]; then
    echo "ERROR: CS_PORT must be a number"
    exit 1
fi
if [[ -z "${IMAGE:-}" ]]; then
    echo "ERROR: IMAGE environment variable required"
    exit 1
fi

Operational Risks

6. Gossip Backoff Timing Edge Case

File: scripts/agg-restart.sh:30-31

The 60s sleep only occurs if existed=yes, determined by checking if container ID existed. However, if the container exists but is in a crash loop or unhealthy state, the script still waits 60s unnecessarily, or conversely, if Docker daemon is slow to report the container, it might skip the backoff.

Recommendation: Check container state explicitly:

if [ "$existed" = yes ] && sudo docker inspect -f '{{.State.Running}}' "$cid" 2>/dev/null | grep -q "true"; then
    log "stopped node_$n container; 60s gossip backoff"
    sleep 60
fi

7. Dashboard JSON Maintenance

File: scripts/client-dashboard.json

The dashboard contains hardcoded metric names (e.g., lean_attestation_committee_subnet, lean_pq_sig_attestation_signing_time_seconds). If these metric names change in the Rust code, the dashboards will break silently.

Recommendation: Add a comment block at the top of the JSON indicating which version of ethlambda metrics this dashboard was built against, or include a test in CI that validates metric names against the dashboard JSON.

Minor Improvements

8. Prometheus Query Efficiency

File: node-health.md:218-219

histogram_quantile(0.9, sum by (le) (rate(lean_tick_interval_duration_seconds_bucket{network="$NET"}[10m])))

High-cardinality histograms over 10m can be expensive at scale. Consider if 5m is sufficient for the health check use case.

9. Inconsistent Use of sudo

File: scripts/agg-restart.sh

The script mixes sudo docker commands. Since the script is designed to run on the host (as noted in comments), it should either require root privileges at the start or use sudo consistently for all Docker commands.

Recommendation: Add a check at the start:

if ! sudo -n docker ps >/dev/null 2>&1; then
    echo "ERROR: Requires passwordless sudo access to docker"
    exit 1
fi

10. Documentation Clarity on Finality Threshold

File: SKILL.md:37, references/clients.md:21

The finality threshold is mentioned as ceil(2/3 · NODES), but in Ethereum consensus, the threshold is typically floor(2/3 * NODES) + 1 or strictly greater than 2/3. Verify this is the intended behavior for this lean consensus variant.

Positive Aspects

  • Excellent operational detail: The documentation captures hard-won operational knowledge (60s gossip backoff, genesis_time landmine, young-devnet bootstrap behavior) that would otherwise be tribal knowledge
  • Safety warnings: Clear warnings about destructive operations (genesis regeneration, data wipes) with confirmation prompts suggested
  • Observability: Comprehensive Grafana dashboards with proper template variables for multi-tenancy ($network, $job)
  • Skill structure: The .claude/skills/ structure properly separates triggers, workflows, and references

Summary

This PR provides valuable operational tooling but needs hardening against script failures and documentation rot. The critical fixes needed are:

  1. Remove or version-specific line number references to source code
  2. Fix the unsafe rm -rf pattern in agg-restart.sh
  3. Add health checks before destructive operations
  4. Quote variables properly in subshell commands

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

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

🤖 Codex Code Review

  1. convert.sh, cs-restart.sh, agg-restart.sh, and to a lesser extent start-devnet.sh do not fail the script when docker run fails. In convert.sh and both restart scripts, success is logged unconditionally after the launch attempt; in start-devnet.sh, per-node failures are logged but the script still exits 0 and ends with “all N nodes launched”. A bad image tag, CLI mismatch, or checkpoint-sync failure can therefore silently leave the devnet below quorum while the operator believes the rollout succeeded. These scripts should return nonzero on any failed launch.

  2. convert.sh omits $LOGOPT on the zeam path even though it also enables --console_log_level info. Every other client path caps Docker json logs, but this one reintroduces unbounded log growth on the noisiest conversion path. That is a direct disk-exhaustion regression.

  3. start-promtail.sh mounts /var/run/docker.sock into the promtail container. The :ro bind does not make the Docker API read-only; access to the Unix socket is effectively host-root-equivalent. If this is intentional, it should be treated as a major trust-boundary expansion and documented as such, or replaced with a collection path that does not expose the daemon socket.

No consensus Rust paths were changed in this PR, so I did not find fork-choice / state-transition / SSZ / XMSS regressions in the client itself. The meaningful risk here is operational correctness and host security.


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

@greptile-apps

greptile-apps Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds an operational skill for running isolated single-host devnets across multiple servers, with lifecycle, genesis, client-conversion, observability, alerting, and audit tooling. One shared configuration-loader defect currently truncates documented multi-host values and can reinterpret configuration as shell syntax.

  • Adds scripts for genesis generation, validator alignment, startup, checkpoint restart, and multi-client conversion.
  • Adds Prometheus, Promtail, Loki, Grafana dashboard, and finality-alert configuration.
  • Adds operational references covering topology, client compatibility, node health, and incident recovery.

Confidence Score: 4/5

The configuration loader must be fixed before merging because documented multi-server values are truncated, causing fleet operations to silently omit hosts.

Removing configuration quotes and evaluating the resulting text changes a space-separated server list into multiple shell words, leaving only the first hostname assigned and also allowing values to be interpreted as shell syntax.

Files Needing Attention: .claude/skills/multi-server-devnet/scripts/devnet-env.sh

Important Files Changed

Filename Overview
.claude/skills/multi-server-devnet/scripts/devnet-env.sh Adds shared deployment configuration loading, but eval reparses and truncates documented space-containing values.
.claude/skills/multi-server-devnet/scripts/start-devnet.sh Launches isolated devnet nodes with per-client command shapes, aggregator roles, resource limits, and bounded logs.
.claude/skills/multi-server-devnet/scripts/make-genesis.sh Builds per-host genesis artifacts and delegates multi-validator subnet alignment to the new Python helper.
.claude/skills/multi-server-devnet/scripts/convert.sh Implements rolling checkpoint-synced conversion across six alternative clients while retaining node identity.
.claude/skills/multi-server-devnet/scripts/promtail-config.sh Generates Docker-discovered log shipping with normalized labels, backlog filtering, multiline handling, and structured metadata.
.claude/skills/multi-server-devnet/scripts/deploy-finality-alert.sh Renders and remotely provisions the centralized Grafana finality alert from deployment-specific settings.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Env[devnet.env] --> Loader[devnet_load_env]
  Loader --> Lifecycle[Lifecycle and audit scripts]
  Lifecycle --> Hosts[Independent host devnets]
  Hosts --> Metrics[Per-host Prometheus and Promtail]
  Metrics --> Central[Central Prometheus, Loki, and Grafana]
  Central --> Dashboards[Dashboards and finality alerts]
Loading
Prompt To Fix All With AI
### Issue 1
.claude/skills/multi-server-devnet/scripts/devnet-env.sh:39
**Eval breaks environment values**

When the documented `SERVERS="host-a host-b"` value is loaded, stripping its quotes and reparsing it through `eval` leaves only the first host assigned, so fleet operations silently skip subsequent devnets; the same reparsing also executes shell substitutions or separators embedded in values.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "feat(tooling): flag recent devnet node r..." | Re-trigger Greptile

Comment thread .claude/skills/multi-server-devnet/scripts/devnet-env.sh Outdated
Restarting a node silently dropped its aggregator role. A node that comes
back without --is-aggregator stops storing gossip signatures, so its
subnet's votes are never aggregated: attestations still verify, every log
line looks healthy, and the devnet quietly stops finalizing with
attestation_count=0. The role now travels with the node, read off the
container being replaced, with AGG=auto|<id>|off to set it deliberately.
The separate aggregator restart script is gone: with one script per role
the role handling drifts out of whichever one gets used every day, which
is how it got lost in the first place.

Launch and convert now validate before destroying anything, since a typo
used to surface only after the container and its data were already gone,
leaving the node down. start-devnet.sh checks SUBNETS against the
genesis' ATTESTATION_COMMITTEE_COUNT (a mismatch makes nodes compute a
different subnet map than the aggregators listen on, so votes vanish) and
refuses to start on leftover node data, since resuming an old DB against
a new genesis forks that node onto its own chain. convert.sh reads ACC
from the genesis on disk instead of defaulting to 2, and warns when a
conversion takes away a subnet's only aggregator.

Adds host-check.sh, the local counterpart to sweep.sh: it reads only
127.0.0.1, so it still answers when the central Prometheus or this host's
remote_write is the thing that broke. Adds start-observability.sh to
relaunch a host's scrapers.
@MegaRedHand
MegaRedHand force-pushed the feat/multi-server-devnet-skill branch from d441c34 to bce946e Compare August 4, 2026 18:28
…silently

The env loader was correct but unreadable: `eval "export $key=\$val"` escapes
the value so it expands after eval has parsed, and an `export name=value`
assignment suppresses word splitting, so multi-word values did arrive whole.
Nobody should have to know that to trust the line, and a reviewer already
read it as the interpolated form that would break SERVERS="host-a host-b".
Bash indirect expansion plus one quoted export word says the same thing with
nothing left to reparse.

While in there, the header's promise that comments and malformed lines are
skipped was only half true. A `# note` after an unquoted value landed inside
the value, so SERVERS picked up a host called '#' for sweep.sh to ssh to;
quoted values now delimit themselves, keeping a '#' that belongs to the value
(http://x/y#frag). A bare word with no '=' became a variable assigned its own
name. A leading-digit name reached `export` and produced its error instead of
ours. Indented lines were dropped outright. Names that can't be assigned are
now reported on stderr, because a config line that goes unread is how you
deploy against the wrong deployment.
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