feat(config): add Validate() method for startup validation#2763
Conversation
Add Config.Validate() to internal/config, checking the fully merged configuration (defaults, YAML, env, CLI flags) before any services start: mode enums, port ranges (0/privileged/out-of-range), the load-mode immutableDbPath requirement, a path-traversal guard on cardanoConfig, TLS cert/key pairing, mempool watermarks, block-producer credential paths, network identity, and duration/strategy strings that were previously only parsed at their point of use. All violations are aggregated into a single startup error naming each bad setting. Validation runs in PersistentPreRunE after CLI flags are applied, closing the gap where flag values bypassed LoadConfig's inline checks. The `dingo load <path>` positional argument is merged into ImmutableDbPath before validation so a config with runMode "load" plus a path argument does not fail spuriously. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
📝 WalkthroughWalkthroughThis PR adds comprehensive validation for merged configuration, including effective run-mode-specific listener checks, port bindability, path safety, TLS pairing, watermarks, credentials, network identity, durations, strategies, and Mithril backends. CLI mode detection now skips informational commands, merges Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/config/validate.go (2)
203-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWhitelist duplication risks drifting from the real parsers.
Both
chainsync.strategy(Line 206-213) andmithril.backend(Line 223-230) re-implement, as string literals, the accepted-value sets that live inchainsync.ParseHeaderSyncStrategyandcmd/dingo'sresolveMithrilBackend, respectively — a deliberate choice to avoid importing node-subsystem packages intointernal/config, per the comments. That's a reasonable tradeoff, but if either downstream parser's accepted set changes, this validator can silently start accepting/rejecting the wrong values, and a wrong config would then get pastValidate()only to fail later at startup — defeating some of the purpose of centralizing checks here. Consider adding a small parity test (e.g. in the layer that can import both) or a code comment cross-reference/TODO to keep them in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/validate.go` around lines 203 - 230, The accepted-value whitelists for chainsync.strategy and mithril.backend in Validate are duplicated from chainsync.ParseHeaderSyncStrategy and resolveMithrilBackend, so keep them from drifting. Add a parity safeguard in a test or a clear cross-reference/TODO near the switch statements in validate.go so any future parser changes are reflected here. Use the existing Validate, chainsync.ParseHeaderSyncStrategy, and resolveMithrilBackend symbols to locate the duplicated logic.
79-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider guarding against duplicate port assignments.
None of the configured ports (relay, private, metrics, debug, utxorpc, blockfrost, mesh, bark, midnight) are checked against each other for collisions. Two services accidentally sharing a port number would currently pass validation and only fail at bind time. A simple
map[uint]stringpass over the sameportsslice (skipping zero) would catch this cheaply while everything is already in scope here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/validate.go` around lines 79 - 98, The port validation loop in validate.go only checks each port individually and does not detect collisions between services. Add a duplicate-check pass over the existing ports slice in the validateConfig path, skipping zero-valued ports, and record any repeated port number across relayPort/privatePort/metricsPort/debugPort/utxorpcPort/blockfrostPort/meshPort/barkPort/Midnight.Port. Use a map[uint]string (or equivalent) to compare each port against previously seen settings and append a validation error when two configured services share the same port.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/validate.go`:
- Around line 79-98: The port validation loop in validateConfig is treating
relay/private/metrics listeners as required even for RunModeLoad, which makes
load-only configs fail unnecessarily. Update the port-checking logic around the
ports slice and validatePort call so those listener ports are skipped or allowed
to be 0 when c.RunMode is RunModeLoad, while keeping the existing required
behavior for normal modes.
- Around line 37-39: The privilege check in Config.Validate is using
os.Geteuid() directly, which misclassifies Windows and can incorrectly reject
low ports there. Update Validate to gate the privileged-port restriction by OS,
so it only treats Unix-like systems as requiring elevated privileges, and keep
the existing c.validate call path intact while adjusting the privilege flag
logic.
---
Nitpick comments:
In `@internal/config/validate.go`:
- Around line 203-230: The accepted-value whitelists for chainsync.strategy and
mithril.backend in Validate are duplicated from
chainsync.ParseHeaderSyncStrategy and resolveMithrilBackend, so keep them from
drifting. Add a parity safeguard in a test or a clear cross-reference/TODO near
the switch statements in validate.go so any future parser changes are reflected
here. Use the existing Validate, chainsync.ParseHeaderSyncStrategy, and
resolveMithrilBackend symbols to locate the duplicated logic.
- Around line 79-98: The port validation loop in validate.go only checks each
port individually and does not detect collisions between services. Add a
duplicate-check pass over the existing ports slice in the validateConfig path,
skipping zero-valued ports, and record any repeated port number across
relayPort/privatePort/metricsPort/debugPort/utxorpcPort/blockfrostPort/meshPort/barkPort/Midnight.Port.
Use a map[uint]string (or equivalent) to compare each port against previously
seen settings and append a validation error when two configured services share
the same port.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a172d546-cc9e-41af-bc49-22db40f3b84a
📒 Files selected for processing (4)
ARCHITECTURE.mdcmd/dingo/main.gointernal/config/validate.gointernal/config/validate_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Treat Windows as privileged for the sub-1024 port check; os.Geteuid returns -1 there, which otherwise rejected every low port on Windows. - Require the relay, private, and metrics listener ports only outside load mode, since `dingo load` starts none of those listeners. - Skip Validate() for the informational `version` and `list` subcommands so they still run against an otherwise-invalid config. - Detect two services assigned the same non-zero port, which would otherwise only fail at bind time. - Move the chainsync.strategy and mithril.backend accepted-value sets into exported slices and add a parity test in cmd/dingo (which can import both) so they cannot silently drift from the real parsers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
|
Pushed e56ab82 addressing the review feedback:
New test cases cover the load-mode/serve-mode port gating and duplicate-port detection; the parity tests cover the whitelists. On the Windows |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/dingo/config_parity_test.go">
<violation number="1" location="cmd/dingo/config_parity_test.go:30">
P3: Both parity tests only guard one direction of drift: they verify that every value the config's accepted list permits is also accepted by the downstream consumer. They do not guard the reverse direction — if someone adds a new valid value to `ParseHeaderSyncStrategy` or `resolveMithrilBackend` without updating the config's accepted list, neither test will catch it. Consider adding a second pass that iterates over the consumer's accepted values and verifies each one appears in the config's accepted list, so the test provides full bidirectional drift coverage.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The chainsync.strategy and mithril.backend parity tests only checked that every value config accepts is accepted by the downstream parser. They now also assert config's accepted set matches the parser's contract exactly, so a value added to ParseHeaderSyncStrategy or resolveMithrilBackend without updating config's list is caught too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
No-go for merge until the command-specific validation regression is fixed. The new validator applies serving-mode listener requirements based on cfg.RunMode, but several one-shot commands do not start those listeners and can still have the default runMode=serve. It may be cleanest to model this by normalizing an effective run mode before validation, or by adding command-specific run modes for load/sync-style invocations so Validate() can decide which listener set is actually required.
| // highest-precedence source for ImmutableDbPath; merge it before | ||
| // validation so a config with runMode "load" and no | ||
| // immutableDbPath doesn't fail spuriously. | ||
| if cmd.Name() == "load" && len(args) > 0 { |
There was a problem hiding this comment.
This only copies the positional path into ImmutableDbPath; it does not make validation treat the explicit dingo load <path> command as load mode. With the default runMode: serve, cfg.Validate() still requires relay/private/metrics ports even though loadRun does not start those listeners. Repro: go run ./cmd/dingo load /tmp --port 0 --private-port 0 --metrics-port 0 fails in validation before reaching the load command. A straightforward fix would be to normalize an effective run mode for this command before validation, or add a distinct run mode/validation profile for one-shot load commands.
| // Ports. The relay, private, and metrics listeners are required for | ||
| // the serving modes but not started by "load", so they may be unset | ||
| // (0) in a load-only config. | ||
| listenersRequired := c.RunMode != RunModeLoad |
There was a problem hiding this comment.
This conflates configured runMode with the actual Cobra command/service set being executed. Non-serving commands like dingo load <path> and dingo mithril sync can run with the default runMode: serve, but they do not start relay/private listeners, so listener-required validation rejects configs that are valid for those commands. This probably needs command/service-set aware validation, or additional/effective run modes so Validate() can determine which listeners are actually required.
The listener-port requirement keyed off cfg.RunMode, but the one-shot subcommands (load, sync, mithril) run a fixed operation and keep the default runMode=serve, so `dingo sync` and `dingo mithril ...` were wrongly required to have the relay/private/metrics listener ports set. cmd/dingo now derives an effective run mode from the invoked command and passes it to Validate(): the bare `dingo` process honors the configured runMode, `serve` is a serving mode, `load` requires an ImmutableDB source, and the one-shot utilities (sync, mithril) require neither listeners nor a source. RunMode.RequiresListeners centralizes the serving-mode set, and a new effective-only RunModeUtility models the one-shot commands. The version/list exemption and the load positional- arg merge now key off the top-level command so nested commands such as `dingo mithril list` are classified correctly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
|
@wolf31o2 fixed in 962a95b — thanks for catching this. You're right: the listener requirement keyed off Following the first of your suggestions (normalize an effective run mode before validation),
Verified end-to-end against the built binary:
New unit tests cover |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
effectiveRunMode passed an unrecognized configured runMode through to Validate, where RequiresListeners treats unknown modes as needing no listeners — so listener-port violations would not be aggregated with the invalid-runMode error. rootCmd dispatches an unrecognized mode to serve, so effectiveRunMode now does the same, keeping the serving-listener checks. (In practice runMode is already rejected at load/flag-apply time; this keeps the validator self-consistent regardless.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
Requesting changes for the listener validation regression. The new validation should only check ports for listeners that the effective invocation can actually start. Right now utility commands and core-mode serving configs can be rejected because they collide with configured API ports that are inactive for that path.
| {"privatePort", c.PrivatePort, listenersRequired}, | ||
| {"metricsPort", c.MetricsPort, listenersRequired}, | ||
| {"debugPort", c.DebugPort, false}, | ||
| {"utxorpcPort", c.UtxorpcPort, false}, |
There was a problem hiding this comment.
This port set is broader than the listeners that can actually bind for a given invocation. For example, dingo --metrics-port 8080 sync is rejected because metricsPort matches the default meshPort, but sync is classified as a utility and Mesh is not started there. The same shape exists for core-mode serving: UTxORPC/Blockfrost/Mesh/Midnight are gated on API storage mode, so their configured ports should not collide with active listeners when those APIs are inactive. Please derive the validated/deduped port set from the effective invocation mode plus storage mode, so we only reject ports that could actually bind together.
| return config.RunModeServe | ||
| case "load": | ||
| return config.RunModeLoad | ||
| default: |
There was a problem hiding this comment.
The catch-all RunModeUtility is probably too coarse once validation depends on listener surfaces. Please consider creating distinct effective run modes or listener profiles for load, sync, mithril, and serving modes instead of funneling all non-serve/load commands through one utility mode. That keeps load and sync from inheriting validations for listeners they do not start, while still allowing commands that do start metrics/debug or future utility listeners to validate those ports explicitly.
The startup validator range-checked, privilege-checked, and collision-checked a fixed set of nine ports regardless of whether a given invocation would actually bind them. That produced false rejections: `dingo --metrics-port 8080 sync` was rejected because the metrics port matched the default mesh port even though `sync` starts no Mesh listener, and core-mode serving configs could collide with the UTxORPC/Blockfrost/Mesh/Midnight ports that only bind under API storage. Derive the validated and deduplicated port set from the effective run mode plus the storage mode, mirroring the gating in (*dingo.Node).Start and cmd/dingo's node.Run/mithril paths: relay/private/bark bind only in serving modes; metrics/debug bind in serving modes and the sync/mithril utilities; UTxORPC/Blockfrost/Mesh/Midnight bind only under API storage (dev mode forces API on). A port for an inactive listener is skipped entirely, so it is neither range-checked nor counted toward a collision. Replace the coarse effective-only RunModeUtility with distinct RunModeSync and RunModeMithril so each one-shot command validates exactly the ports it starts (metrics/debug) without inheriting the relay/private/API checks it does not, and so a future utility listener can opt into its port checks explicitly. Update ARCHITECTURE.md and the validation tests accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
NO-GO: startup validation blocks mithril list / mithril show on metrics/debug port settings they do not use. Please split effective validation for Mithril read-only subcommands from Mithril sync, or skip auxiliary listener validation for list/show, and add a command-mode regression test.
| return config.RunModeLoad | ||
| case "sync": | ||
| return config.RunModeSync | ||
| case "mithril": |
There was a problem hiding this comment.
This maps every mithril subcommand to the mode that validates auxiliary listeners. mithril sync does start metrics/debug, but mithril list and mithril show only query the aggregator and never bind those ports. Repro on this branch: go run ./cmd/dingo --metrics-port 99999999 mithril list fails during config validation with invalid metricsPort, even though that command does not use the metrics listener. Please distinguish mithril sync from the read-only Mithril subcommands before validation.
The previous change mapped every `mithril` subcommand to the effective mode that validates the metrics and debug ports. But only `mithril sync` (and `sync --mithril`) starts those listeners; `mithril list` and `mithril show` just query the aggregator and bind nothing, so `dingo --metrics-port 99999999 mithril list` was rejected at startup for a port it never uses. Resolve the effective run mode from the invoked command rather than only its top-level name: `mithril sync` maps to RunModeSync (metrics/debug validated), while the read-only `mithril list`/`show` and bare `mithril` map to RunModeMithril, which now validates no listener ports. RunModeSync becomes the Mithril snapshot sync operation shared by `dingo sync --mithril` and `dingo mithril sync`. Add command-mode regression tests distinguishing the two, plus a validation test that read-only Mithril mode ignores metrics/debug ports. Update ARCHITECTURE.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
- Exempt cobra's built-in help and completion commands from config validation: they run the root PersistentPreRunE, so an invalid config blocked `dingo help` and shell-completion generation. - Validate the API listener ports when the configured runMode is "dev" even though `dingo serve` yields an effective serve mode: node.Run keys dev behavior (which forces API storage) off the configured mode, so those listeners bind. - Allow sub-1024 ports for non-root processes that can actually bind them: CAP_NET_BIND_SERVICE and a zeroed net.ipv4.ip_unprivileged_port_start (set inside containers by runtimes such as Docker) now pass the privileged-port check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Recognizing only a zeroed net.ipv4.ip_unprivileged_port_start missed deployments that lower the cutoff to a nonzero value (e.g. 80): the kernel permits binding such ports as non-root, but validation still rejected them. Thread the cutoff itself through the port checks — minBindablePort() returns 0 for root/Windows/CAP_NET_BIND_SERVICE and the sysctl value otherwise — and compare each active port to it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
No-go for merge on the current head. The earlier command-specific listener findings are addressed, and the merge-result tests and linters are clean, but the inline validation regressions below remain blocking.
| if path == "" { | ||
| return nil | ||
| } | ||
| cleaned := filepath.Clean(path) |
There was a problem hiding this comment.
Blocking: cleaning the path before checking it removes the traversal component being guarded against. For example, configs/../secret.json becomes secret.json and passes validation; the local CLI repro reports only an unrelated forced port error. The issue and ARCHITECTURE.md both promise no .. components. Inspect the original path components before cleaning (with appropriate platform separator handling), and change the test that currently accepts an inner dotdot.
| c.MempoolCapacity, | ||
| )) | ||
| } | ||
| if c.EvictionWatermark <= 0 || c.EvictionWatermark >= 1.0 { |
There was a problem hiding this comment.
Blocking: these comparisons accept NaN because every ordered comparison with NaN is false. --eviction-watermark NaN passes this validator and then reaches mempool threshold conversion/arithmetic. Reject non-finite values explicitly (or express the range check so NaN cannot satisfy it) for both watermarks, and add CLI/config regression coverage.
|
|
||
| // version and list are informational and start no services, so | ||
| // they must still run even when the merged config is invalid. | ||
| if !isInformationalCommand(top) { |
There was a problem hiding this comment.
Blocking: this exemption occurs after LoadConfig, so informational commands still fail on configuration loading/validation errors. Repro: dingo --config /tmp/does-not-exist version exits with “failed to load config” instead of printing the version. Classify and bypass runtime config loading before LoadConfig for version/list/help/completion, or narrow any loading to data those commands actually require.
| } | ||
|
|
||
| // Network identity | ||
| if c.Network == "" { |
There was a problem hiding this comment.
Blocking integration mismatch: this branch says a custom networkMagic is sufficient when network is empty, but startup cannot reach it. LoadConfig still unconditionally calls ValidateNetworkName(""), and ApplyFlags rejects --network= before Validate. A YAML config with network: "" and networkMagic: 42 fails in LoadConfig. Remove or reconcile the earlier fail-fast validation so the advertised contract and aggregation actually apply.
| } | ||
| // Two active listeners sharing a port only fails at bind time; catch it | ||
| // here. Zero ports are disabled or OS-assigned, so they don't clash. | ||
| seenPorts := make(map[uint]string, len(ports)) |
There was a problem hiding this comment.
Blocking false positive: port number alone is not a listener identity. Active listeners can legally share a port on distinct specific bind addresses, but this map rejects them. For example, debug on 127.0.0.1:13000 and Midnight on 127.0.0.2:13000 are bindable together yet are reported as a collision. Include the effective bind address (and wildcard overlap semantics) in collision detection, or leave bind conflicts to the existing synchronous listener startup paths.
- Reject any ".." path component in cardanoConfig by inspecting the original path: cleaning first erased inner components, letting traversal-shaped paths like "configs/../secret.json" through. - Check watermarks for NaN explicitly; every ordered comparison with NaN is false, so --eviction-watermark NaN passed the range checks and reached mempool threshold arithmetic. - Skip config loading (not just validation) for the informational version/list/help/completion commands so they work with a missing or unreadable config file. - Make the networkMagic-without-network contract reachable: LoadConfig no longer rejects an empty network name, the --network flag accepts an explicit empty value, and topology resolution returns an empty topology (as dev mode does) when there is no network name to resolve embedded topology or bootstrap peers from. Validate() enforces that network or networkMagic is set. - Make port-collision detection bind-address aware: listeners on distinct specific addresses (e.g. 127.0.0.1 and 127.0.0.2) may share a port; only equal or wildcard-overlapping bind addresses collide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/dingo/main.go (1)
445-453: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDefer all semantic validation until after
ApplyFlags.By the time Line 453 runs,
LoadConfigat Line 436 has already rejected invalid run modes, block-producer credentials, watermarks, and topology settings. Consequently, a higher-precedence CLI flag cannot repair an invalid YAML/env value, mode-dependent defaults may use the pre-CLI mode, and violations are not aggregated byConfig.Validate.Make
LoadConfigparse and merge only; perform semantic checks and mode-dependent defaulting after flags are applied.As per coding guidelines, configuration precedence must be CLI > env > YAML > defaults.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/dingo/main.go` around lines 445 - 453, Update the configuration flow around LoadConfig, ApplyFlags, and Config.Validate so LoadConfig only parses and merges defaults, YAML, and environment values without semantic validation or mode-dependent defaulting. Apply all CLI flags first, then derive mode-dependent defaults and run Config.Validate using the final effective configuration, preserving CLI > env > YAML > defaults precedence and allowing validation errors to be aggregated after overrides.Source: Coding guidelines
internal/config/validate_test.go (1)
64-77: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMove
RunMode/StartEravalidation intoConfig.Validate.
LoadConfigstill short-circuits on those fields, so startup can’t report both errors together. Add aLoadConfigregression for the combined failure case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/validate_test.go` around lines 64 - 77, Move the RunMode and StartEra checks from LoadConfig into Config.Validate, preserving their existing invalid-value errors and allowing validation to accumulate both failures. Add a LoadConfig regression test that supplies invalid values for both fields and asserts the combined error reports both issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/dingo/config_parity_test.go`:
- Around line 26-60: Replace the hand-maintained canonical slices in
TestChainsyncStrategyWhitelistParity and the corresponding Mithril parity test
with shared accepted-value tables. Define those tables in a package accessible
to both cmd/dingo and internal/config, then update
config.AcceptedChainsyncStrategies and config.AcceptedMithrilBackends and the
parity tests to reuse them while preserving the existing parser checks.
---
Outside diff comments:
In `@cmd/dingo/main.go`:
- Around line 445-453: Update the configuration flow around LoadConfig,
ApplyFlags, and Config.Validate so LoadConfig only parses and merges defaults,
YAML, and environment values without semantic validation or mode-dependent
defaulting. Apply all CLI flags first, then derive mode-dependent defaults and
run Config.Validate using the final effective configuration, preserving CLI >
env > YAML > defaults precedence and allowing validation errors to be aggregated
after overrides.
In `@internal/config/validate_test.go`:
- Around line 64-77: Move the RunMode and StartEra checks from LoadConfig into
Config.Validate, preserving their existing invalid-value errors and allowing
validation to accumulate both failures. Add a LoadConfig regression test that
supplies invalid values for both fields and asserts the combined error reports
both issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca4af416-36c5-4b8c-a9b9-b3d131158f3e
📒 Files selected for processing (9)
ARCHITECTURE.mdcmd/dingo/command_mode_test.gocmd/dingo/config_parity_test.gocmd/dingo/main.gointernal/config/config.gointernal/config/config_test.gointernal/config/flags.gointernal/config/validate.gointernal/config/validate_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ARCHITECTURE.md
| // internal/config cannot import chainsync or mithril without pulling | ||
| // node subsystems into the config package, so the accepted-value sets | ||
| // it validates against (config.AcceptedChainsyncStrategies and | ||
| // config.AcceptedMithrilBackends) are duplicated from those downstream | ||
| // parsers. These parity tests live in cmd/dingo, which can import all | ||
| // three, and guard against drift in both directions: | ||
| // | ||
| // - forward: every value config accepts must be accepted by the real | ||
| // parser, so a config never passes Validate() only to fail at | ||
| // startup; | ||
| // - reverse: config's accepted set must match the parser's contract | ||
| // exactly, so a value added to the parser without updating config | ||
| // (which would spuriously reject a valid config) is caught too. | ||
|
|
||
| // TestChainsyncStrategyWhitelistParity checks config.AcceptedChainsyncStrategies | ||
| // against chainsync.ParseHeaderSyncStrategy. | ||
| func TestChainsyncStrategyWhitelistParity(t *testing.T) { | ||
| // canonical is the contract of chainsync.ParseHeaderSyncStrategy; | ||
| // it mirrors that function's switch. The set-equality check below | ||
| // ties config's list to it, and the parse check ties it to the real | ||
| // parser, so a change to the parser's switch fails this test until | ||
| // both this list and config's list are updated. | ||
| canonical := []string{ | ||
| "", "primary", "parallel", "round-robin", "roundrobin", "round_robin", | ||
| } | ||
| assertWhitelistParity( | ||
| t, | ||
| "chainsync.strategy", | ||
| config.AcceptedChainsyncStrategies, | ||
| canonical, | ||
| func(v string) error { | ||
| _, err := chainsync.ParseHeaderSyncStrategy(v) | ||
| return err | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the parity test and the referenced parsers/contracts.
git ls-files cmd/dingo | sed -n '1,120p'
printf '\n--- config_parity_test.go ---\n'
cat -n cmd/dingo/config_parity_test.go | sed -n '1,220p'
printf '\n--- parser definitions ---\n'
rg -n "ParseHeaderSyncStrategy|AcceptedChainsyncStrategies|AcceptedMithrilBackends|Parse.*Mithril" -S .Repository: blinklabs-io/dingo
Length of output: 8149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map candidate files before reading implementation slices.
fd -a ".*(chainsync|mithril).*" .
fd -a "config.*\.go|.*parity.*\.go" cmd/dingoRepository: blinklabs-io/dingo
Length of output: 1892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n cmd/dingo/config_parity_test.go | sed -n '1,220p'Repository: blinklabs-io/dingo
Length of output: 5358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'pwd=%s\n' "$PWD"
git ls-files cmd/dingo/config_parity_test.go
cat -n cmd/dingo/config_parity_test.go | sed -n '1,220p'Repository: blinklabs-io/dingo
Length of output: 5413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "TestChainsyncStrategyWhitelistParity|assertWhitelistParity|canonical := \[\]string" cmd/dingo/config_parity_test.goRepository: blinklabs-io/dingo
Length of output: 547
Use a shared accepted-value table
Both parity tests still depend on hand-maintained canonical slices, so a new value accepted by chainsync.ParseHeaderSyncStrategy or resolveMithrilBackend can slip past this check until the list is updated manually. Define the accepted values once and reuse them here and in internal/config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/dingo/config_parity_test.go` around lines 26 - 60, Replace the
hand-maintained canonical slices in TestChainsyncStrategyWhitelistParity and the
corresponding Mithril parity test with shared accepted-value tables. Define
those tables in a package accessible to both cmd/dingo and internal/config, then
update config.AcceptedChainsyncStrategies and config.AcceptedMithrilBackends and
the parity tests to reuse them while preserving the existing parser checks.
The parity tests compared internal/config's accepted-value lists against hand-maintained canonical slices, so a value added to chainsync.ParseHeaderSyncStrategy or resolveMithrilBackend slipped past until someone remembered to update the test. Export the accepted sets from the parser packages and derive the parsers from them: chainsync.AcceptedHeaderSyncStrategyNames shares one name table with ParseHeaderSyncStrategy, and cmd/dingo's resolveMithrilBackend accepts mithril.AcceptedBackends plus the empty default. The parity tests now compare config's duplicated lists (still required by the import boundary) against those exported lists, so extending a parser fails the tests until config catches up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
Requesting changes on the current head. The earlier command-specific listener/path/NaN/networkMagic/bind-address findings are fixed, and the focused and conformance tests pass, but the startup flow still violates configuration precedence: LoadConfig performs semantic checks and mode-dependent defaulting before ApplyFlags. Please defer those operations until after flags, then validate/finalize the fully merged configuration. DevNet remains to be rerun after the fix.
| cfg.ImmutableDbPath = args[0] | ||
| } | ||
|
|
||
| if err := cfg.Validate(effectiveRunMode(cmd, cfg)); err != nil { |
There was a problem hiding this comment.
This validation call is too late to provide the advertised fully merged validation. LoadConfig has already rejected invalid runMode/startEra, block-producer credentials, and watermarks, and has already selected mode-dependent defaults before ApplyFlags. Confirmed repro: YAML runMode: bogus plus higher-precedence --run-mode serve fails inside LoadConfig; --run-mode leios can also retain the Praos mempool default selected earlier. Make LoadConfig parse/merge only, apply CLI flags, derive defaults from the final mode, then call Validate. Please add precedence, defaulting, and aggregation regression tests.
There was a problem hiding this comment.
Fixed in cb45d3b. LoadConfig now only parses and merges the YAML and environment sources — the runMode/startEra/watermark/block-producer fail-fasts and the mode-dependent MempoolCapacity defaulting are gone from it. A new Config.ApplyDefaults derives defaults from the fully merged configuration, and cmd/dingo calls it after ApplyFlags (and the load positional-arg merge), before Validate. Regression tests added in internal/config/flags_test.go run the full pipeline: an invalid YAML runMode overridden by --run-mode serve now validates cleanly (and is still rejected without the override), --run-mode leios gets the Leios mempool default, and multiple bad settings aggregate into a single Validate error.
LoadConfig rejected invalid runMode/startEra/watermark/block-producer values and chose the mode-dependent MempoolCapacity default before ApplyFlags ran, so a CLI flag could neither override an invalid YAML value (runMode: bogus + --run-mode serve failed in LoadConfig) nor influence a derived default (--run-mode leios kept the Praos mempool capacity chosen from the YAML-era mode). LoadConfig now only parses and merges YAML and environment sources. The new Config.ApplyDefaults fills in defaults derived from the merged configuration (runMode, mempool capacity, watermarks, forge slot thresholds, history-expiry frequency) and cmd/dingo calls it after ApplyFlags, before Validate, which already covers every semantic check LoadConfig used to fail fast on — and reports all violations together instead of stopping at the first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
|
DevNet rerun on cb45d3b ( |
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
Requesting changes on the current head. The prior CLI-precedence and mode-dependent-defaulting blocker is fixed, the branch merges cleanly with current main, and the focused race-enabled and conformance tests pass. One configuration safety issue remains: an explicitly negative history-expiry frequency is silently converted to the one-hour default, so a node can start pruning when the operator supplied a value they may reasonably expect to disable expiry or fail startup. Please preserve the negative value through defaulting, reject it in Validate, and cover YAML, environment, and CLI inputs. DevNet remains to be rerun after the fix.
| if c.ForgeStaleGapThresholdSlots == 0 { | ||
| c.ForgeStaleGapThresholdSlots = DefaultForgeStaleGapThresholdSlots | ||
| } | ||
| if c.HistoryExpiry.Frequency <= 0 { |
There was a problem hiding this comment.
This condition conflates an unset zero value with an explicitly negative duration. For example, --history-expiry-enabled --history-expiry-frequency=-1s reaches this code, is silently rewritten to 1h, passes validation, and starts the history-expiry worker. The resulting failure mode is particularly unsafe: instead of failing startup (or disabling expiry), the node periodically replaces stable historical block CBOR with expiry markers. Default only Frequency == 0; preserve negative values for Validate to reject.
There was a problem hiding this comment.
Fixed in b61c234: ApplyDefaults now only fills in an unset (zero) frequency, so an explicitly negative value survives defaulting for Validate to reject. Repro now fails startup: dingo --history-expiry-enabled --history-expiry-frequency=-1s ... → invalid configuration: invalid historyExpiry.frequency: -1s (must be positive). The pruner's own Frequency <= 0 guard in internal/historyexpiry remains as defense in depth for library callers that bypass Validate.
| "invalid %s %q: must be positive", d.setting, d.value, | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please validate historyExpiry.frequency as positive after defaults are applied. Currently ApplyDefaults erases a negative value before this validator runs, so the configuration error cannot be reported or aggregated. After changing defaulting to handle only zero, append an error when the frequency is negative (or non-positive if validation may be called without ApplyDefaults). Add regressions for YAML historyExpiry.frequency: -1s, DINGO_HISTORY_EXPIRY_FREQUENCY=-1s, and --history-expiry-frequency=-1s, especially with history expiry enabled, and assert startup fails rather than using the one-hour default.
There was a problem hiding this comment.
Done in b61c234: Validate rejects a non-positive historyExpiry.frequency (non-positive rather than just negative, so a caller that skips ApplyDefaults is also caught), aggregated with the other errors. Regression tests cover all three sources with expiry enabled — YAML historyExpiry.frequency: -1s, DINGO_HISTORY_EXPIRY_FREQUENCY=-1s, and --history-expiry-frequency=-1s — via the full LoadConfig→ApplyFlags→ApplyDefaults→Validate pipeline in flags_test.go, asserting validation fails and the negative value is preserved through defaulting.
ApplyDefaults rewrote any non-positive historyExpiry.frequency to the one-hour default, so an operator who configured a negative value — plausibly expecting expiry to be disabled or startup to fail — got a node that silently prunes historical block CBOR on a cadence they never chose. Only an unset (zero) frequency now takes the default; a negative value survives defaulting and Validate rejects it alongside any other configuration errors. Regression tests cover the YAML, environment, and CLI flag sources with history expiry enabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
|
DevNet rerun on b61c234: 8 of 10 scenarios pass, including TestSustainedConsensus and TestEpochBoundaryConsensus. TestChainGrowthRate fails with the same 0-blocks signature that reproduces on the upstream/main merge-base in this environment (see earlier comment), and TestBasicBlockForging failed this round with the same underlying forging stall (it passed on the previous run of cb45d3b). Both match the pre-existing local block-forging issue that is independent of this PR. Unit, conformance (via |
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
No-go for merge on the current head. The previously reported history-expiry issue is fixed, the branch merges cleanly with current main, and focused race, conformance, and import-boundary checks pass. However, topology is still loaded before CLI overrides and before the final aggregated validation pass, so the startup flow continues to violate CLI > env > YAML precedence for network and topology. Please defer topology resolution until the fully merged configuration has been defaulted and validated, and add regression coverage for CLI overrides of invalid YAML values.
| if err := ValidateNetworkName(globalConfig.Network); err != nil { | ||
| return nil, err | ||
| } | ||
| // LoadConfig only parses and merges configuration sources; it makes |
There was a problem hiding this comment.
[P1] LoadConfig still performs topology resolution below this comment (LoadTopologyConfig at line 1014), so it does more than parse and merge. This can reject a lower-precedence value before CLI flags are applied: I reproduced network: ../bad failing here with unknown network even when the invocation intends to supply --network preview. Remove topology loading from LoadConfig so a CLI override can repair YAML/env values as promised.
There was a problem hiding this comment.
Fixed in cfc226f: LoadConfig no longer resolves topology (or anything else derived from mergeable settings) — it parses and merges YAML/env only. Your repro now works: YAML network: ../bad with --network preview proceeds past config loading, validation, and topology resolution; without the override the value is rejected by Validate (invalid network name "../bad"), aggregated with any other errors.
| cfg.ImmutableDbPath = args[0] | ||
| } | ||
|
|
||
| // Every configuration source is merged at this point (defaults, |
There was a problem hiding this comment.
[P1] This is not yet the first point at which every configuration source has safely merged: both LoadConfig and ApplyFlags resolve topology before ApplyDefaults/Validate. I also reproduced a missing YAML topology file aborting before a valid --topology override can take effect. Resolve/load topology once after flags, defaults, and validation, and cover both the --network and --topology override cases.
There was a problem hiding this comment.
Done in cfc226f: the ApplyFlags topology reload is removed too, and topology is resolved exactly once in cmd/dingo after ApplyFlags → ApplyDefaults → Validate. Both override cases are covered by pipeline regression tests in flags_test.go (TestPipeline_NetworkFlagRepairsInvalidYAMLNetwork, TestPipeline_TopologyFlagRepairsMissingYAMLTopology), each also asserting the no-override invocation still fails — the bad network in Validate, the missing topology file at topology resolution. Verified against a real binary: --config badtopo.yaml --topology topo.json load ... starts, and without the flag fails with loading topology: failed to load topology file: ....
LoadConfig resolved topology as a side effect, and ApplyFlags resolved it again, both before ApplyDefaults and Validate ran. Because topology derives from the network and topology settings, a YAML or environment value could abort startup before the CLI flag that repairs it was ever consulted: a traversal-shaped YAML network failed topology resolution inside LoadConfig despite an explicit --network override, and a missing YAML topology file aborted despite a valid --topology override. Topology is now resolved exactly once, in cmd/dingo after ApplyFlags, ApplyDefaults, and Validate, so it always operates on the final, validated configuration. Pipeline regression tests cover both override cases and their no-override rejections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Chris Guiney <chris@guiney.net>
|
DevNet rerun on cfc226f: all three nodes come up healthy with the relocated topology resolution (Dingo loads its explicit topology file via the new post-validation path), and 8 of 10 scenarios pass including TestSustainedConsensus and TestEpochBoundaryConsensus. TestChainGrowthRate (2 blocks) and TestBasicBlockForging fail with the same forging-stall signature previously reproduced on the upstream/main merge-base in this environment — pre-existing, not from this PR. Full |
2 participants
Closes #1485
Adds
Config.Validate()tointernal/config, checking the fully merged configuration (defaults, YAML, env, CLI flags) before any services start. All violations are aggregated into a single startup error naming each bad setting, so operators can fix everything in one pass.Checks
runMode,startEra,storageModerunMode: loadrequiresimmutableDbPathport,privatePort,metricsPort) must be non-zero; optional API ports accept 0 as disabled; all must be ≤ 65535; ports below 1024 are rejected unless the process runs as rootcardanoConfig(no..components), matching the existing network-name guardnetworkornetworkMagicmust be setshutdownTimeout,ledgerCatchupTimeout,chainsync.stallTimeout,mithril.downloadIdleTimeout) parse at startup instead of failing at point of usechainsync.strategyandmithril.backendaccepted-value setsWire-up
Validation runs in
PersistentPreRunEafterApplyFlags. This also closes a gap where CLI flag values bypassedLoadConfig's inline checks entirely (flags are applied afterLoadConfigreturns, so e.g.--eviction-watermark 1.5was never validated). Thedingo load <path>positional argument is merged intoImmutableDbPathbefore validation so a config withrunMode: loadplus a path argument does not fail spuriously.Deliberately out of scope: file/dir existence checks (embedded-FS fallbacks and later-created paths make stat-at-validate wrong for several fields), and the closed issues referenced by the ticket (#276, #387, #390, #393).
Testing
make test(-race, includes conformance) passes; golangci-lint, modernize, and import-boundaries clean--cardano-config ../../etc/passwdeach fail with a clear named-setting error; multiple violations report togetherARCHITECTURE.md Configuration section updated; DATABASE.md not affected.
🤖 Generated with Claude Code
Summary by cubic
Adds
Config.Validate()for startup validation keyed to the command’s effective run mode, and defers defaulting/validation until all sources (YAML, env, flags, args) are merged. Also rejects a negativehistoryExpiry.frequencyinstead of silently defaulting.New Features
internal/config.Config.Validate()before any services start, using the effective mode (serve, load, sync, mithril).syncvalidates metrics/debug; read‑onlymithril list/showandloadstart none; API ports validate only underapistorage or when configuredrunModeisdev.internal/config.Config.ApplyDefaults()to fill derived defaults afterApplyFlags(runMode, mempoolCapacity, watermarks, forge thresholds, history‑expiry frequency) and then validate;LoadConfignow only parses/merges sources.chainsync.AcceptedHeaderSyncStrategyNamesandmithril.AcceptedBackends.internal/configduplicates are guarded by bidirectional parity tests incmd/dingo.Bug Fixes
version,list,help, andcompletionfrom config loading and validation.dingo load <path>merges the positional path before validation; load mode enforcesimmutableDbPath.cardanoConfigpath guard to reject any..; validate CLI flags, parse duration strings at startup, and validate mempool watermarks (including NaN) and block‑producer credentials.networkat load/flag time; validation still requires eithernetworkornetworkMagic.--run-mode leiossets the leios mempool capacity).historyExpiry.frequencyfrom any source and fail validation; only zero defaults to 1h.Written for commit b61c234. Summary will update on new commits.
Summary by CodeRabbit
dingo load <path>can supplyimmutableDbPatheven when omitted from YAML.--network "".