fix: remove implicit fallback behavior#2759
Conversation
🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/config.go (1)
949-992: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject unknown top-level siblings in wrapped config files
internal/config/config.go:949-992still decodes the root mapping leniently whenconfig:is present, so a typo besideconfig:is dropped instead of failing fast. Validate top-level keys before entering the wrapped branch, or add coverage for this shape.🤖 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/config.go` around lines 949 - 992, Reject unknown top-level siblings in wrapped config files by validating the root mapping before the wrapped config path in the config loading logic. In the config unmarshalling flow around tempConfig, strictUnmarshalConfig, and removeMappingKeys, make sure any keys beside config/blob/metadata/database are rejected even when tempCfg.Config is present, rather than being silently ignored. Add or adjust coverage for the wrapped config case to confirm typos at the top level fail fast.
🤖 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 `@database/plugin/register.go`:
- Around line 105-127: Validate the plugin config keys in `register.go` before
mutating any destinations: in the `plugin.Options` handling, build the `known`
set and scan `pluginData` for unknown keys first, returning the `unknown config
key(s)` error before calling `option.ProcessConfig`. Then, only after validation
succeeds, run `option.ProcessConfig` for each option so `plugin.Options`,
`pluginData`, and `PluginTypeName(plugin.Type)` no longer allow partial
application when a typoed key is present.
---
Outside diff comments:
In `@internal/config/config.go`:
- Around line 949-992: Reject unknown top-level siblings in wrapped config files
by validating the root mapping before the wrapped config path in the config
loading logic. In the config unmarshalling flow around tempConfig,
strictUnmarshalConfig, and removeMappingKeys, make sure any keys beside
config/blob/metadata/database are rejected even when tempCfg.Config is present,
rather than being silently ignored. Add or adjust coverage for the wrapped
config case to confirm typos at the top level fail fast.
🪄 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: 7cf01290-d90b-422a-958a-c2a24ccbe730
📒 Files selected for processing (24)
api/utxorpc/submit.gochainselection/selector.goconnmanager/outbound.goconnmanager/outbound_test.godatabase/database.godatabase/default_plugin_log_test.godatabase/plugin/process_config_test.godatabase/plugin/register.godingo.yaml.exampleinternal/config/chainsync_strategy_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/flags.gointernal/config/logging_test.gointernal/config/mithril_backend_test.gointernal/config/strict_unmarshal_test.gointernal/config/validate_after_flags_test.goledger/chainsync.goledger/eras/conway.goledger/state.goledger/state_test.goledger/verify_header.goouroboros/leios_merged_test.goouroboros/leiosnotify.go
| known := make(map[string]struct{}, len(plugin.Options)) | ||
| for _, option := range plugin.Options { | ||
| known[option.Name] = struct{}{} | ||
| if err := option.ProcessConfig(pluginData); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| var unknown []string | ||
| for key := range pluginData { | ||
| if _, ok := known[key]; !ok { | ||
| unknown = append(unknown, key) | ||
| } | ||
| } | ||
| if len(unknown) > 0 { | ||
| slices.Sort(unknown) | ||
| return fmt.Errorf( | ||
| "unknown config key(s) %v for %s plugin %q", | ||
| unknown, | ||
| PluginTypeName(plugin.Type), | ||
| plugin.Name, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate unknown keys before applying known ones.
option.ProcessConfig (which mutates the plugin's Dest fields) runs inside the same loop that builds the known set, before the unknown-key scan executes. If a config block mixes valid and typo'd keys, the valid keys get applied to global/package state before the error is returned. Since this PR's goal is to avoid partial/implicit fallback behavior, validate all keys first, then apply.
🛠️ Proposed fix: validate before applying
known := make(map[string]struct{}, len(plugin.Options))
for _, option := range plugin.Options {
known[option.Name] = struct{}{}
- if err := option.ProcessConfig(pluginData); err != nil {
- return err
- }
}
var unknown []string
for key := range pluginData {
if _, ok := known[key]; !ok {
unknown = append(unknown, key)
}
}
if len(unknown) > 0 {
slices.Sort(unknown)
return fmt.Errorf(
"unknown config key(s) %v for %s plugin %q",
unknown,
PluginTypeName(plugin.Type),
plugin.Name,
)
}
+ for _, option := range plugin.Options {
+ if err := option.ProcessConfig(pluginData); err != nil {
+ return err
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| known := make(map[string]struct{}, len(plugin.Options)) | |
| for _, option := range plugin.Options { | |
| known[option.Name] = struct{}{} | |
| if err := option.ProcessConfig(pluginData); err != nil { | |
| return err | |
| } | |
| } | |
| var unknown []string | |
| for key := range pluginData { | |
| if _, ok := known[key]; !ok { | |
| unknown = append(unknown, key) | |
| } | |
| } | |
| if len(unknown) > 0 { | |
| slices.Sort(unknown) | |
| return fmt.Errorf( | |
| "unknown config key(s) %v for %s plugin %q", | |
| unknown, | |
| PluginTypeName(plugin.Type), | |
| plugin.Name, | |
| ) | |
| } | |
| } | |
| known := make(map[string]struct{}, len(plugin.Options)) | |
| for _, option := range plugin.Options { | |
| known[option.Name] = struct{}{} | |
| } | |
| var unknown []string | |
| for key := range pluginData { | |
| if _, ok := known[key]; !ok { | |
| unknown = append(unknown, key) | |
| } | |
| } | |
| if len(unknown) > 0 { | |
| slices.Sort(unknown) | |
| return fmt.Errorf( | |
| "unknown config key(s) %v for %s plugin %q", | |
| unknown, | |
| PluginTypeName(plugin.Type), | |
| plugin.Name, | |
| ) | |
| } | |
| for _, option := range plugin.Options { | |
| if err := option.ProcessConfig(pluginData); err != nil { | |
| return err | |
| } | |
| } | |
| } |
🤖 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 `@database/plugin/register.go` around lines 105 - 127, Validate the plugin
config keys in `register.go` before mutating any destinations: in the
`plugin.Options` handling, build the `known` set and scan `pluginData` for
unknown keys first, returning the `unknown config key(s)` error before calling
`option.ProcessConfig`. Then, only after validation succeeds, run
`option.ProcessConfig` for each option so `plugin.Options`, `pluginData`, and
`PluginTypeName(plugin.Type)` no longer allow partial application when a typoed
key is present.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 24 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
Thanks for tightening several config paths and adding observability. I am requesting changes because the PR currently says Closes #1649, but it is only a partial pass and still has at least one strictness hole that preserves silent fallback behavior.
The blocking issue is wrapped YAML config: when config: is present, unknown top-level siblings are still decoded leniently and ignored. That means a typo like databse: can silently drop database settings and fall back to defaults. That conflicts directly with the config parsing part of #1649 and the PR description.
I would also avoid closing #1649 from this PR. There are still explicit warn-and-continue or fallback paths that need either better error handling, an operator-controlled strict mode, or a clearly scoped follow-up: database plugin defaulting, invalid outbound source-port fallback, leader eligibility skip cases, tx validation slot-clock fallback, best-effort Leios drops, and recovered chain-selection panics. Some of these may be legitimate operational tradeoffs, but then the PR should say Refs #1649 rather than Closes #1649 and should track the remaining work.
Validation I ran locally: go test ./... and make test both passed.
| if tempCfg.Config != nil { | ||
| // Overlay config values onto existing defaults | ||
| configBytes, err := yaml.Marshal(tempCfg.Config) | ||
| if tempCfg.Config.Kind != 0 { |
There was a problem hiding this comment.
This branch still leaves the root document lenient when config: is present. yaml.Unmarshal(buf, &tempCfg) ignores unknown top-level siblings, and then this branch only strict-decodes tempCfg.Config. So a wrapped file with databse: instead of database: will load, silently drop the intended DB settings, and fall back to defaults. Please validate the root mapping in wrapped mode too, allowing only config, database, blob, and metadata, and add a regression test for an unknown top-level sibling next to a valid config: section.
| ) | ||
| dialer.LocalAddr = clientAddr | ||
| dialer.Control = socketControl | ||
| if resolveErr != nil { |
There was a problem hiding this comment.
This is now observable, but it is still a fallback: an invalid configured source port logs and then dials without source-port reuse. Since source-port reuse is required for peer sharing to be useful, I think this should fail during config validation/startup, or be gated behind an explicit tolerant mode. Logging and continuing is better than silent behavior, but it does not satisfy the fail-fast part of #1649.
| // slot coefficient is logged and skipped rather than rejecting, to tolerate | ||
| // early-chain bootstrap states where the genesis snapshot is not yet written. | ||
| // | ||
| // Reviewed for issue #1649 (fail-fast audit) and intentionally kept as |
There was a problem hiding this comment.
This comment makes sense as a risk note, but it also means #1649 is not complete here. If bootstrap needs a warn-and-continue window, we should distinguish expected bootstrap absence from unexpected post-bootstrap absence and fail after that boundary, with tests and DevNet coverage. Otherwise this PR should not close #1649.
| snapshotPParams := ls.currentPParams | ||
| snapshotPrevEraPParams := ls.prevEraPParams | ||
| ls.RUnlock() | ||
| // Reviewed for issue #1649 (fail-fast audit) and kept as graceful |
There was a problem hiding this comment.
Same concern here: this is still graceful degradation in transaction validation. Falling back to the snapshot tip slot may be a valid operational tradeoff, but it should either be controlled by an explicit strict/fail-fast option or called out as remaining #1649 work. As written, the PR documents that this fallback remains while claiming to close the audit.
| @@ -164,9 +164,21 @@ func New( | |||
| // Apply defaults for empty fields | |||
| if configCopy.BlobPlugin == "" { | |||
There was a problem hiding this comment.
This still applies implicit plugin defaults when the caller passes an empty plugin name. The debug log helps, but #1649 specifically calls out database plugin initialization. If defaulting is intended, I would prefer it happen once in config defaulting with a clear source of truth; the database constructor should probably reject an empty plugin after config has been resolved, or at least this should be listed as remaining fallback work rather than part of a closed audit.
|
Let me replace the "Closes" with "Refs" and address these reviews. Thanks for your time. |
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
No-go for merge. The wrapped-config blocker was addressed, and the focused tests pass, but there are still two merge-blocking issues: a remaining silent fallback path under database:, and stale README plugin docs that now describe rejected config keys.
Validation run locally: git diff --check origin/main...HEAD; go test ./internal/config ./database/plugin ./database ./connmanager ./ledger ./chainselection ./api/utxorpc ./ouroboros.
| if len(root.Content) > 0 { | ||
| removeMappingKeys( | ||
| root.Content[0], | ||
| []string{"blob", "metadata", "database"}, |
There was a problem hiding this comment.
This still leaves a silent fallback hole one level lower. Because database is stripped before strict Config decoding, unknown children under database: are only seen by the lenient tempConfig decode. A typo like database.blbo: or database.metadtaa: is silently ignored and the node falls back to the default plugin settings, which is the behavior this PR is trying to remove. Please validate the database mapping itself only contains blob and metadata before stripping/processing it, and add coverage for typoed database children.
| prefix: "" | ||
| # Project and credentials are always resolved via Application | ||
| # Default Credentials. There is no plugin config option for | ||
| # project ID or a bucket key prefix. |
There was a problem hiding this comment.
This removes the GCS project-id/prefix options from the example, but README.md still documents those as valid GCS options and still documents S3 static access-key-id/secret-access-key options. With the new strict plugin-key validation, following the README will now fail config load. Please update README.md to match the actual registered plugin options: GCS only bucket; S3 endpoint, bucket, region, and prefix, with credentials from the AWS default chain.
81554af to
685b0ef
Compare
There was a problem hiding this comment.
1 issue found across 16 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="dingo.yaml.example">
<violation number="1" location="dingo.yaml.example:372">
P2: Genesis/bootstrap can still reject blocks despite this option being false: missing stake snapshots are returned as errors before `StrictLeaderEligibility` is consulted, rather than logged and skipped as this example says. Align the implementation with the documented fallback or narrow the comment to the total-stake and slot-coefficient cases that the flag actually gates.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| strictUtxoValidation: false | ||
|
|
||
| # Reject a block (instead of logging a warning and skipping the check) when | ||
| # the active stake snapshot or active slot coefficient needed to verify Praos |
There was a problem hiding this comment.
P2: Genesis/bootstrap can still reject blocks despite this option being false: missing stake snapshots are returned as errors before StrictLeaderEligibility is consulted, rather than logged and skipped as this example says. Align the implementation with the documented fallback or narrow the comment to the total-stake and slot-coefficient cases that the flag actually gates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dingo.yaml.example, line 372:
<comment>Genesis/bootstrap can still reject blocks despite this option being false: missing stake snapshots are returned as errors before `StrictLeaderEligibility` is consulted, rather than logged and skipped as this example says. Align the implementation with the documented fallback or narrow the comment to the total-stake and slot-coefficient cases that the flag actually gates.</comment>
<file context>
@@ -368,6 +368,21 @@ validateHistorical: false
strictUtxoValidation: false
+# Reject a block (instead of logging a warning and skipping the check) when
+# the active stake snapshot or active slot coefficient needed to verify Praos
+# leader eligibility is unavailable. Leave disabled during genesis bootstrap,
+# where these are legitimately absent until the first stake snapshot is
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
No-go for merge at the current head. The earlier wrapped-config and plugin-documentation blockers are fixed, and the focused tests plus conformance pass, but duration validation still preserves an implicit fallback path. Please reject non-positive timeout values where they are not explicitly supported and add YAML/env/CLI coverage for zero and negative durations. Because this touches header and transaction validation, DevNet validation remains required after the fix.
| if d.value == "" { | ||
| continue | ||
| } | ||
| if _, err := time.ParseDuration(d.value); err != nil { |
There was a problem hiding this comment.
This only checks whether the value parses. time.ParseDuration accepts "0s" and negative durations, so shutdownTimeout and chainsync.stallTimeout accept values that their consumers later silently replace with defaults (configuredShutdownTimeout falls back to 30s for <= 0, and chain-sync only applies a configured timeout when it is > 0). That retains the implicit fallback behavior this PR is intended to remove. Please validate the parsed duration semantically—requiring > 0 for these timeout fields unless a documented non-positive value has an intentional meaning—and add zero/negative tests across YAML, environment, and CLI inputs.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
c14ecc6 to
7c8996a
Compare
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
Requesting changes on the current head. The previous non-positive timeout finding is fixed correctly, the affected race-enabled package tests and conformance suite pass, and the branch merges cleanly with current main. Two items remain: semantic validation currently runs before higher-precedence CLI flags are applied, and the externally visible storage-plugin configuration behavior needs the required DATABASE.md documentation. DevNet should be rerun after the consensus-affecting changes are ready.
| // --logging-level, --logging-format, --chainsync-strategy, or | ||
| // --mithril-backend flag reintroduce a value this check already | ||
| // rejects for YAML/env. | ||
| if err := validateRuntimeConfig(globalConfig); err != nil { |
There was a problem hiding this comment.
This semantic validation runs before ApplyFlags, so it violates the documented CLI > environment > YAML > defaults precedence whenever a lower layer contains a value that a valid CLI flag should replace. I reproduced this with:\n\nyaml\nshutdownTimeout: "0s"\n\n\nRunning dingo --config dingo.yaml --shutdown-timeout=30s load ... exits here with invalid shutdownTimeout "0s" before the valid CLI override is applied. The same ordering affects the other values checked by validateRuntimeConfig, such as logging level/format, ports, chainsync strategy, Mithril backend, and storage mode. Keep strict YAML shape/type decoding during load, but defer semantic validation until after flags have been applied in command startup; provide an explicit validation path for non-CLI callers. Please add a regression proving a valid CLI value overrides an invalid YAML/environment semantic value.
| PluginTypeMetadata: {"mysql": {}, "postgres": {}}, | ||
| } | ||
|
|
||
| // ProcessConfig applies plugin-specific config values from a parsed YAML |
There was a problem hiding this comment.
This changes the externally visible configuration contract for every storage plugin: unknown plugin names/keys now fail startup, while optional build-tag-gated plugin sections are treated specially. Under the repository documentation rules, storage-plugin behavior changes require a DATABASE.md update; updating only README and dingo.yaml.example does not satisfy that bar. Please document the strict plugin-name/key behavior and the build-tag-gated exception in DATABASE.md. For example, database.blob.badger.buckit now fails instead of being ignored, whereas an optional GCS/S3 section cannot be fully key-validated when that plugin is not compiled into the binary.
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
No-go for merge at the current head. The branch merges cleanly with current main and the affected race-enabled package tests plus conformance pass, but the new semantic validation still violates the documented CLI precedence for the special --debug override. Please apply that override before logging-level validation and add regression coverage. DevNet remains required after the fix because this PR changes header and transaction validation.
| cfg.RelayPort, | ||
| ) | ||
| } | ||
| switch strings.ToLower(strings.TrimSpace(cfg.Logging.Level)) { |
There was a problem hiding this comment.
P2: This rejects the configured logging level even when --debug is present, but cmd/dingo/main.go documents --debug as the highest-precedence override that forces debug level regardless of the configured value. I reproduced the precedence failure with DINGO_LOGGING_LEVEL=bogus go run ./cmd/dingo --debug version: startup exits here instead of running at debug level. Apply the --debug override before this semantic check (or otherwise exempt the superseded lower-priority level), and add an env/YAML + --debug regression test.
| // flag can override a semantically invalid YAML/env value instead of | ||
| // LoadConfig rejecting it beforehand. See ValidateRuntimeConfig's doc | ||
| // comment. | ||
| if err := ValidateRuntimeConfig(cfg); err != nil { |
There was a problem hiding this comment.
ApplyFlags has merged the flags declared in flagSpecs, but it has not applied the separate root-level --debug flag; that override is only consumed later by commonRun. Consequently this validation point is still too early for the effective logging level, despite the comment saying semantic validation occurs after CLI values are merged.
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: cryptodj413 <shinjirohara2@gmail.com>
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
No-go on the current head (741f68a0). I reproduced the PR against current main (ed882bb2) locally, and it no longer merges cleanly: internal/config/config.go conflicts with the new ApplyDefaults/Validate lifecycle, and ledger/state.go conflicts with the new loadStateSnapshots transaction-validation path. Please rebase and preserve both of those mainline refactors when integrating this work.
The two inline comments are also blocking because the new strict modes affect block acceptance and transaction validation, but only their configuration plumbing is tested. The full DevNet run passed with the defaults (false), so it does not exercise either enabled strict path.
The exact head is also not make golines clean; the target rewrites 22 changed files, including PR-added tests.
Before main advanced, the synthetic merge passed the affected race-enabled package tests, conformance, import boundaries, and the full DevNet suite including epoch-boundary consensus. The latest-base result cannot be tested until the conflicts are resolved. DATABASE.md and ARCHITECTURE.md were checked and their updates are appropriate, but should be rechecked after the rebase.
| // Skip rather than reject. | ||
| // Skip rather than reject, unless the operator opted into | ||
| // strict leader eligibility. | ||
| if ls.config.StrictLeaderEligibility { |
There was a problem hiding this comment.
Blocking: add ledger-level behavioral coverage for this strict mode. The only tests that mention StrictLeaderEligibility verify default/YAML/CLI plumbing; none exercises block verification. Please cover both true and false for zero total active stake and for a missing/non-positive active-slot coefficient, asserting hard rejection only in strict mode. This changes block acceptance, and the DevNet run uses the default false, so it cannot catch regressions in this branch.
| // instead of validating it against a stale reference slot (issue #1649). | ||
| currentSlot, currentSlotErr := ls.CurrentSlot() | ||
| if currentSlotErr != nil { | ||
| if ls.config.StrictSlotClock { |
There was a problem hiding this comment.
Blocking: exercise the CurrentSlot error behavior with StrictSlotClock both enabled and disabled. Existing coverage only proves that the flag can be configured; it does not verify that strict mode rejects the transaction or that tolerant mode uses the snapshot-tip fallback (and records the fallback metric). This is a transaction-validation behavior change that the default DevNet configuration does not execute.
2 participants
Refs #1649
Summary by cubic
Removes silent fallback behavior across config loading and runtime, adds opt‑in strict modes for leader eligibility and slot clock, and improves diagnostics; config load is now isolated and
--debugis registered early so validation logs are visible, refs #1649.Bug Fixes
config,database,blob,metadataare allowed (and underdatabase:onlyblob,metadata). Plugin config rejects unknown plugin names and keys; build‑tag‑only names (gcs,s3,mysql,postgres) are tolerated when not compiled. Storage mode is normalized. Config loading is isolated and early--debugregistration ensures debug logs during load/validation.logging.level/format,chainsync.strategy,mithril.backend,storageMode, out‑of‑range ports (incl.relayPort), malformed durations, and non‑positiveshutdownTimeout,ledgerCatchupTimeout, andchainsync.stallTimeout(mithril.downloadIdleTimeoutkeeps its documented semantics). Validation runs after flags so valid CLI values can override bad YAML/env.OutboundSourcePortcannot be resolved; unexpected event types are logged in UTxO RPC wait/watch, connection recycle, chain‑sync resync, andleios-notify/leios-fetch; unrecognizedleios-notifymessages are logged without closing the connection. Recovered panics include stack traces (chain selection, DB worker pool, Conway script‑purpose builder). Ledger view warns when bool‑only lookups hide DB errors; default blob/metadata plugin selection is debug‑logged.Migration
--strict-leader-eligibilityand--strict-slot-clock(default false). Leave off during genesis bootstrap; enable to fail fast on persistent issues.debug,info,warn,error]; format [text,json];chainsync.strategy(primary,parallel,round-robin);mithril.backend(v1,v2);storageMode(core,api); ports must be 0 or 1–65535. Durations must parse and be positive forshutdownTimeout,ledgerCatchupTimeout, andchainsync.stallTimeout(empty allowed).config,database,blob,metadataat the top level; underdatabase:allow onlyblobandmetadata. Plugin configs must use documented keys; GCS no longer acceptsproject-id/prefixand S3 no longer accepts static credential keys (use Application Default Credentials / AWS default chain).Written for commit 741f68a. Summary will update on new commits.
Summary by CodeRabbit