feat(cli): implement dkg

[iamquang95]

Should address: #314

This covers only the entry point of pluto dkg, not the full implementation of dkg.

[iamquang95]

@claude

[claude]

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

---

Code Review — feat(cli): implement dkg

• Gather context — reading changed files
• Compare with Go (Charon) implementation
• Review CLI wiring and command structure
• Review DKG entry point implementation
• Review eth1wrap changes
• Post final review

---

Summary

This PR wires up the pluto dkg CLI entry point — argument parsing, config construction, and a preflight stub that calls load_definition, check_clear_data_dir, and check_writes before hitting an unimplemented!() boundary. The scope is appropriately limited and clearly stated. The factoring of shared relay/DKG helpers into commands/common.rs is clean. Most parity gaps below are expected given the stub nature; a few low-risk items warrant attention.

---

Findings

• [Medium] Operation order in dkg::run deviates from Go
  Impact: Go calls loadDefinition → validateKeymanagerFlags; Rust reverses this. When the definition fails to load, Go users see a definition error, not a keymanager error. Failing fast on keymanager flags is arguably better UX, but it's an undocumented divergence.
  Evidence: crates/dkg/src/dkg.rs:171-181
  Go reference: charon/dkg/dkg.go:154-169
  Recommendation: Add a comment explaining the intentional reordering (early-exit on misconfiguration), or track it as a known parity deviation.

• [Medium] has_test_config() is incomplete relative to Go
  Impact: Go's HasTestConfig checks 4 fields (StoreKeysFunc, SyncCallback, Def, P2PNodeCallback). Rust only checks test_config.def.is_some(). When future backend callbacks are added to TestConfig, the guard against running preflight in tests (check_clear_data_dir skip) may not fire correctly.
  Evidence: crates/dkg/src/dkg.rs:139-142
  Go reference: charon/dkg/dkg.go:84-86
  Recommendation: Leave a // TODO comment in has_test_config referencing the other fields that will need adding as the backend is ported.

• [Medium] DEFAULT_RELAYS duplicated across two crates
  Impact: crates/cli/src/commands/common.rs:15 and crates/dkg/src/dkg.rs:9 define the same three relay URLs. If one is updated without the other, CLI defaults and programmatic defaults silently diverge.
  Evidence: crates/cli/src/commands/common.rs:15, crates/dkg/src/dkg.rs:9
  Recommendation: Re-export or reference one from the other, e.g. have dkg::DEFAULT_RELAYS re-use pluto_cli's constant, or move the canonical constant to a shared crate (core or p2p::config).

• [Medium] --log-format accepted but silently ignored
  Impact: DkgLogArgs.format is parsed from CLI and stored, but into_config() only passes level and color to build_console_tracing_config. A user passing --log-format=json gets console output with no warning.
  Evidence: crates/cli/src/commands/dkg.rs:127, crates/cli/src/commands/common.rs:34-51
  Recommendation: Either wire format into TracingConfig (if supported), or add a // TODO and a warn! if a non-console format is requested.

• [Low] warn_for_insecure_relays uses string prefix instead of parsed multiaddr protocol
  Impact: The relay command checks for HTTP via Protocol::Http on the parsed multiaddr (correct). The DKG command does a raw string prefix check on the original relay string. A relay expressed as an HTTP multiaddr not written as a URL (e.g. /ip4/1.2.3.4/tcp/80/http) would not trigger the DKG warning.
  Evidence: crates/cli/src/commands/dkg.rs:294-300, crates/cli/src/commands/relay.rs:43-52
  Recommendation: Mirror the relay approach: parse first via parse_relay_addr, then check the multiaddr for Protocol::Http. Fix this →

• [Low] validate_keymanager_flags trims whitespace; Go does not
  Impact: Rust treats " " (whitespace-only) as empty for both address and auth_token. Go would treat them as non-empty and proceed to URL parsing, yielding a different error. Minor behavioral divergence on edge-case input.
  Evidence: crates/dkg/src/dkg.rs:191-192
  Go reference: charon/dkg/dkg.go:1165-1184
  Recommendation: Document intentional difference, or align with Go by removing .trim().

• [Low] run_executes_preflight_before_reaching_backend_boundary asserts is_panic()
  Impact: The test spawns a task and asserts is_panic() to verify the unimplemented!() boundary is hit. This will need to be replaced when the backend is implemented; it's a brittle proxy for "preflight passed".
  Evidence: crates/dkg/src/dkg.rs:284-296
  Recommendation: Track this as a TODO comment: // TODO: replace with a real assertion once the backend is implemented.

---

Parity Matrix

Component	Go	Rust	Match	Notes
--data-dir flag	✓ CHARON_DATA_DIR	✓	✓
--definition-file flag	✓ CHARON_DEFINITION_FILE	✓	✓
--no-verify flag	✓ CHARON_NO_VERIFY	✓	✓
--timeout flag (default 1m)	✓	✓	✓
--publish-timeout (default 30s)	✓	✓	✓
--shutdown-delay (default 1s)	✓	✓	✓
--keymanager-address	✓	✓	✓
--keymanager-auth-token	✓	✓	✓
--p2p-relays	✓	✓	✓
--zipped flag	✓	✓	✓
--publish flag	✓	✓	✓
--execution-client-rpc-endpoint	✓	✓	✓
Keymanager validation error strings	✓	✓	✓	exact match
validateKeymanager: trim() behavior	strips nothing	strips whitespace	✗	minor edge case
Operation order: validate → load	load → validate	validate → load	✗	intentional early-exit
HasTestConfig guard	4 fields	1 field (def)	partial	other fields not yet ported
checkClearDataDir logic	✓	✓	✓
checkWrites logic	✓	✓	✓
Private key lock file	✓	✗	✗	expected stub gap
Definition version check	✓	✗	✗	expected stub gap
EthClient no-op on empty addr	✓ (via noop runner)	✓	✓

Tests

Tests observed in the diff — not executed locally (no cargo test run in this review):

• dkg_is_registered_as_top_level_subcommand — sanity-checks CLI wiring ✓
• dkg_defaults_match_go — validates all default values against Go ✓
• invalid_duration_fails_during_parse — clap parse-time validation ✓
• dkg_args_expose_expected_env_bindings — env var contract check ✓
• config_mapping_preserves_fields — round-trip CLI → Config ✓
• run_passes_config_and_token_to_runner — runner receives correct config ✓
• run_rejects_mismatched_keymanager_flags / run_executes_preflight_* / run_surfaces_data_dir_* — coverage of dkg::run preflight ✓
• empty_address_returns_noop_client — eth1wrap no-op path ✓

Test coverage is strong for the stub scope.

Open Questions / Assumptions

1. The log-format argument (console / logfmt / json) — is there a planned issue to wire this into TracingConfig? Both relay and DKG have the same TODO.
2. AppendConfig (Go's merge-two-DKG-ceremonies feature) — expected to remain absent until that feature is ported?
3. When adding the backend, will TestConfig be extended with callbacks (StoreKeysFunc, SyncCallback, etc.) at that point, or earlier?

[iamquang95]

@claude

[claude]

Claude finished @iamquang95's task in 2m 53s —— View job

---

Code Review — feat(cli): implement dkg

• Read changed files
• Compare with Go (Charon) implementation
• Review CLI wiring and command structure
• Review DKG entry point implementation
• Review eth1wrap and p2p changes
• Post final review

---

Summary

This is an updated review reflecting the state of the branch after the developer addressed several findings from the first pass. The scope — a preflight stub that wires CLI arguments into Config and calls load_definition, check_clear_data_dir, and check_writes before hitting unimplemented!() — is appropriate. The factoring of DEFAULT_RELAYS and parse_relay_addr into shared locations is clean.

---

Changes Since Previous Review

The following issues from the first review have been resolved:

Issue	Resolution
Operation order deviation (load_definition vs validate_keymanager_flags)	Fixed — now matches Go order
DEFAULT_RELAYS duplicated across two crates	Fixed — all callers reference pluto_p2p::config::DEFAULT_RELAYS
warn_for_insecure_relays used raw string prefix instead of parsed multiaddr	Fixed — now parses via parse_relay_addr and checks Protocol::Http
validate_keymanager_flags trimmed whitespace (Go does not)	Fixed — .trim() removed
has_test_config() missing TODO for future fields	Fixed — TODO comment added

---

Remaining Findings

• [Medium] --log-format silently ignored
  DkgLogArgs.format is parsed from the CLI and stored, but into_config() only passes level and color to build_console_tracing_config. A user passing --log-format=json gets console output with no warning. This affects the relay command equally (same TODO in build_console_tracing_config).
  Evidence: crates/cli/src/commands/dkg.rs:126, crates/cli/src/commands/common.rs:39-41
  Recommendation: Wire format into TracingConfig when supported, or emit a warn! if a non-console format is requested. Track via an issue if not planned for this PR.

• [Medium, New] Missing keymanager connectivity verification
  Go calls cl.VerifyConnection(ctx) when KeymanagerAddr != "" (Go: dkg.go:174-179) to confirm the keymanager is reachable before proceeding. The Rust preflight only validates the URL format (url::Url::parse). pluto_eth2util::keymanager::Client::verify_connection() already exists (crates/eth2util/src/keymanager.rs:111) — it does a 2-second TCP dial to the keymanager host:port. This is a silent behavioral divergence: Go fails fast with a clear "verify keymanager address" error; Rust proceeds into DKG and fails later.
  Recommendation: Add verify_connection() to the preflight after validate_keymanager_flags, or add a // TODO with a reference to the Go code if deferring. Fix this →

• [Low] run_executes_preflight_before_reaching_backend_boundary asserts is_panic()
  The test spawns a task and asserts .is_panic() to verify the unimplemented!() boundary is reached. This will need to be replaced or removed when the backend is implemented. A // TODO comment would signal this is intentionally temporary.
  Evidence: crates/dkg/src/dkg.rs:287-289

• [Low] Triple relay parsing in into_config
  Relay strings are parsed three times: once in validate_p2p_args, once inside warn_for_insecure_relays (both called from into_config before the relay loop), and once in the relay-building loop at line 130-133. The expect in warn_for_insecure_relays is sound because validate_p2p_args runs first, but the redundancy is a minor maintenance hazard.
  Evidence: crates/cli/src/commands/dkg.rs:123-133
  Recommendation: Pass the already-parsed Vec<Multiaddr> to warn_for_insecure_relays instead of re-parsing from strings. This matches how relay.rs does it (parse once in the relay loop, warn inline).

---

Parity Matrix (updated)

Component	Go	Rust	Match	Notes
--data-dir, --definition-file, --no-verify	✓	✓	✓
--timeout, --publish-timeout, --shutdown-delay	✓	✓	✓
--keymanager-address, --keymanager-auth-token	✓	✓	✓
--p2p-relays (shared constant)	✓	✓	✓
--zipped, --publish, --execution-client-rpc-endpoint	✓	✓	✓
Keymanager validation error strings	exact match	exact match	✓
validateKeymanager trim behavior	no trim	no trim	✓	fixed
Operation order: loadDefinition → validateKeymanager	✓	✓	✓	fixed
HasTestConfig guard on checkClearDataDir	✓	✓	✓
checkClearDataDir / checkWrites logic	✓	✓	✓
Keymanager VerifyConnection check	✓ (before preflight)	✗	✗	method exists, not called
--log-format wired into tracing	✓	✗	✗	silently ignored
HasTestConfig completeness	4 fields	1 field (def)	partial	TODO comment added
Private key lock file	✓	✗	✗	expected stub gap
Definition version check (v1.6.0+)	✓	✗	✗	expected stub gap
AppendConfig merge	✓	✗	✗	expected stub gap

Overall

The preflight stub is clean and well-tested for its scope. The two medium items — --log-format and missing verify_connection — are the most notable gaps relative to Go behavior. The rest are low-priority polish. Happy to approve once those are tracked (either fixed or captured as TODOs/issues).