Skip to content

feat(cmd): implement run and unsafe run#503

Open
iamquang95 wants to merge 6 commits into
mainfrom
feat/cmd-run
Open

feat(cmd): implement run and unsafe run#503
iamquang95 wants to merge 6 commits into
mainfrom
feat/cmd-run

Conversation

@iamquang95

@iamquang95 iamquang95 commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Fix #403

@iamquang95

Copy link
Copy Markdown
Collaborator Author

@claude review this PR

@claude

claude Bot commented Jun 29, 2026

Copy link
Copy Markdown

Claude finished @iamquang95's task in 3m 7s —— View job


Review: feat(cmd): implement run and unsafe run

I read crates/cli/src/commands/run.rs, the CLI wiring, and compared against the Go source (charon/cmd/run.go, charon/cmd/cmd.go).

  • Gather context (read diff + Go source)
  • Review run.rs for functional equivalence with Charon
  • Review CLI wiring & supporting changes
  • Post review feedback

Overall this is a careful, high-quality port. Flag names, defaults, help text, env prefixes (CHARON_*), error strings, and the p2p-then-run validation order all match Go faithfully, and the test coverage is excellent (flag-set parity, defaults, every error string verbatim, the Loki lifecycle regression, the unsafe run / p2p-fuzz gating, and the stub panic). The Box<RunArgs> to avoid a large enum variant and the Loki drain-with-timeout in run() are nice touches.

A few findings, mostly minor:

1. Config logging doesn't replicate Go's redact() (parity / minor security) — run.rs:901

info!(config = ?config);

Go's printFlagsflagsToLogFieldsredact() (charon/cmd/cmd.go:213) redacts URL userinfo (passwords) for any flag whose name contains address (u.Redacted()), and replaces auth-token flag values with xxxxx. The single Debug dump of RunConfig here logs everything verbatim, so credentials embedded in e.g. --validator-api-address, --monitoring-address, --otlp-address (https://user:pass@host) are written in plaintext where Go would redact them. Worth either redacting address-type fields or noting this as a deliberate, tracked deviation. Fix this →

2. Hostname validation differs from Go's idna (low) — run.rs:824

Go uses idna.Lookup.ToASCII(config.ExternalHost); Pluto uses url::Host::parse. These are close in practice (the url crate uses IDNA internally), but one observable difference: Go's ExternalHost defaults to "" and idna.ToASCII("") succeeds, whereas an explicit --p2p-external-hostname "" here becomes Some("") and url::Host::parse("") errors. Pluto's Option<String> correctly skips the unset case, so this only bites the explicit-empty edge case. Worth a comment noting the intentional substitution.

3. parse_relay_addr is stricter than Go's url.Parse (low / likely intentional) — run.rs:701

Go's relay loop uses url.Parse, which almost never errors, so invalid relays only fail later at startup. parse_relay_addr (multiaddr-or-URL) rejects malformed relays up front with a hard error. This is arguably an improvement and is shared/pre-existing helper code, but it's a behavioral divergence from Go worth being aware of. The HTTP-vs-HTTPS warning is correctly preserved via the Protocol::Http check.

4. Default relay list differs (info, intentional) — run.rs:426

Go defaults to 3 relays (0.relay.obol.tech, 2.relay.obol.dev, 1.relay.obol.tech); Pluto uses pluto_p2p::config::DEFAULT_RELAYS (5 entries). This is a deliberate Pluto-wide default shared with other commands and is acknowledged in the tests — flagging only for the record.

Minor / nits

  • run.rs:50-52 — the doc comment claims --help lists flags "alphabetically regardless." clap v4 renders args in declaration order by default, not alphabetically; the comment may be inaccurate (Go's printFlags is what sorts alphabetically, in the log output). Harmless, but consider correcting.
  • simnet-validator-mock requires simnet-beacon-mock — not enforced in validation. This matches Go (run.go doesn't validate it in PreRunE either; it's checked at app start), so it's consistent for a stub. No change needed.
  • The lowercased error text in eth2util/helpers.rs correctly matches Go's helpers.go:32 ("http headers must be..."). 👍

Note

I reviewed statically and did not run cargo test/clippy/fmt in this session — the in-tree tests look comprehensive and should cover the parsing/validation surface.
feat/cmd-run

@iamquang95 iamquang95 marked this pull request as ready for review June 29, 2026 09:37
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.

Implement cmd/run — main node binary

1 participant