Skip to content

feat(config): show non-default values on startup#11523

Open
obchain wants to merge 5 commits intoNethermindEth:masterfrom
obchain:feature/1462-show-non-default-config-on-startup
Open

feat(config): show non-default values on startup#11523
obchain wants to merge 5 commits intoNethermindEth:masterfrom
obchain:feature/1462-show-non-default-config-on-startup

Conversation

@obchain
Copy link
Copy Markdown

@obchain obchain commented May 8, 2026

Fixes #1462

Changes

  • Add IConfigProvider.GetNonDefaultValues() extension that yields (category, name, current, default) tuples for every config property whose runtime value differs from a freshly constructed implementation instance.
  • Log a non-default config summary at Info level on startup in Nethermind.Runner/Program.cs, alongside the existing Debug-level full dump. Operators see exactly which knobs they overrode.
  • Tests in ConfigProviderTests.cs cover the empty case, single override, and multi-category overrides.

Types of changes

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Startup logs now include a Configuration: N non-default value(s): summary listing each overridden setting; operators no longer need to enable Debug logging to see which values they have changed.

@LukaszRozmej
Copy link
Copy Markdown
Member

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 9, 2026

Claude finished @LukaszRozmej's task in 3m 22s —— View job


Code Review: feat(config): show non-default values on startup

  • Read agent rules and changed files
  • Review ConfigExtensions.cs
  • Review ConfigProviderTests.cs
  • Review Program.cs
  • Post findings

The feature itself is useful and the general approach is sound — one-time startup reflection against a fresh default instance is a reasonable strategy. However, there are several issues that need addressing before merge.


High

Sensitive values logged at Info level

ConfigExtensions.cs exposes all non-default config values including properties like Seq.ApiKey and KeyStore.Passwords (plaintext account passwords) — and Program.cs logs them at Info level. This is a real credential-exposure risk:

  • A Seq.ApiKey override would be logged into Seq itself — the very service the key authenticates against.
  • KeyStore.Passwords holds plaintext account unlock passwords. If an operator sets them to a non-default value they will appear in startup logs, log shippers, journalctl, and any centralized log store.

The existing Debug-level full dump already has this problem, but elevating specific overrides to Info makes it significantly worse: Info logs are far more likely to be shipped to external aggregators.

Recommendation: Add an IsSensitive (or similar) flag to ConfigItemAttribute and skip properties tagged as sensitive. At minimum, skip HiddenFromDocs properties; or introduce a dedicated [ConfigItem(IsSensitive = true)] on secrets before this lands.

Fix this →


Medium

Incomplete exception guard around Activator.CreateInstance (ConfigExtensions.cs:89)

Only MissingMethodException is caught, but Activator.CreateInstance can also throw TargetInvocationException (constructor body throws), TypeLoadException, and MethodAccessException. Any of these would propagate unhandled and crash startup — a harsh failure for what is essentially a diagnostic/logging feature.

// current
catch (MissingMethodException)
{
    continue;
}

// should be at minimum:
catch (Exception) when (/* TargetInvocationException | MissingMethodException | TypeLoadException */)
{
    continue;
}

Fix this →


LINQ where foreach suffices (ConfigExtensions.cs:69-71)

Per .agents/rules/coding-style.md: "No LINQ when a simple for/foreach works". The Where filter should be an early-continue:

// current
foreach (Type configInterface in TypeDiscovery
    .FindNethermindBasedTypes(typeof(IConfig))
    .Where(static t => t.IsInterface))

// should be
foreach (Type configInterface in TypeDiscovery.FindNethermindBasedTypes(typeof(IConfig)))
{
    if (!configInterface.IsInterface) continue;
    ...
}

Fix this →


Exception as control flow (ConfigExtensions.cs:79-84)

catch (ArgumentException) to detect an unregistered config type is exception-driven control flow. This is both a style concern and a subtle correctness risk — a bug elsewhere in GetConfig that throws ArgumentException for a different reason would be silently swallowed. If IConfigProvider doesn't expose a TryGetConfig/IsRegistered API, at minimum log or assert that the ArgumentException is for the expected reason.


Low

Category = string.Empty for INoCategoryConfig types (ConfigExtensions.cs:96)

For no-category config, GetCategoryName returns null and the code falls back to string.Empty. This yields log output like .PropertyName = ... (leading dot). Consider preserving null in the tuple so callers (including the logger in Program.cs) can render it cleanly.


var not used — good; but doc comment on private method

ValuesEqual is private and has no doc comment, which is consistent with the style guide. The large XML doc block on GetNonDefaultValues is good for a public API — no issue there.


Tests

  • [Parallelizable(ParallelScope.None)] on GetNonDefaultValuesTests is appropriate given shared TypeDiscovery state.
  • Returns_empty_when_no_overrides passes because types with default-valued instances compare equal; the test is sound.
  • Consider adding a test case that a sensitive property (once IsSensitive is introduced) is excluded from GetNonDefaultValues output.

Summary

The two blocking items are:

  1. Sensitive credential logging (High) — plaintext passwords and API keys will appear in Info logs
  2. Incomplete Activator.CreateInstance exception handling (Medium) — diagnostic code can crash startup

The LINQ style and exception-as-control-flow issues are straightforward to fix. The core idea of this feature is good; the sensitive-value handling is the design gap that needs to be addressed first.

Comment on lines +191 to +204
List<string> nonDefaultLines = [];
foreach ((string category, string name, object? currentValue, object? _) in configProvider.GetNonDefaultValues())
{
nonDefaultLines.Add($" {category}.{name} = {serializer.Serialize(currentValue)}");
}

if (nonDefaultLines.Count == 0)
{
logger.Info("Configuration: all values at defaults.");
}
else
{
logger.Info($"Configuration: {nonDefaultLines.Count} non-default value(s):\n{string.Join('\n', nonDefaultLines)}");
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is reflection based, it can slow down startup a bit, maybe worth it to run in Task.Run?

{
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] Sensitive data exposure

Properties like Seq.ApiKey and KeyStore.Passwords will be included here and logged at Info level in Program.cs. A Seq.ApiKey override would be logged into Seq itself; KeyStore.Passwords contains plaintext account unlock passwords.

Consider adding an IsSensitive flag to ConfigItemAttribute and skipping those properties:

Suggested change
// Skip sensitive properties to avoid logging credentials
foreach (PropertyInfo property in configInterface.GetProperties()
.Where(static p => p.CanRead && p.GetCustomAttribute<ConfigItemAttribute>()?.IsSensitive != true))

Or at a minimum filter out properties where the attribute is not present or HiddenFromDocs is true.

continue;
}

IConfig fresh;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] Incomplete exception handling

Activator.CreateInstance can also throw TargetInvocationException (if the constructor body throws), TypeLoadException, and MethodAccessException. Any of these would propagate and crash startup for what is a diagnostic-only feature. Broaden the catch:

Suggested change
IConfig fresh;
catch (Exception e) when (e is MissingMethodException or TargetInvocationException or TypeLoadException or MethodAccessException)

{
ArgumentNullException.ThrowIfNull(configProvider);

foreach (Type configInterface in TypeDiscovery
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] LINQ where foreach suffices

Per repo coding style (coding-style.md): "No LINQ when a simple for/foreach works". Replace with an early-continue:

Suggested change
foreach (Type configInterface in TypeDiscovery
foreach (Type configInterface in TypeDiscovery.FindNethermindBasedTypes(typeof(IConfig)))
{
if (!configInterface.IsInterface) continue;

LukaszRozmej and others added 2 commits May 10, 2026 08:43
- Add IsSensitive flag to ConfigItemAttribute and mark Seq.ApiKey,
  KeyStore.Passwords, KeyStore.TestNodeKey, EthStats.Secret so their
  values never reach startup logs via GetNonDefaultValues.
- Catch TargetInvocationException, TypeLoadException, and
  MethodAccessException alongside MissingMethodException so a single
  reflection-construction failure cannot crash startup.
- Replace LINQ Where filter with an early-continue foreach per repo
  coding style.
- Skip the redundant GetDirectInterfaceImplementation TypeDiscovery
  rescan; reuse the populated instance's concrete type for the fresh
  default.
- Run the non-default scan via Task.Run so the reflection cost no
  longer blocks the rest of startup; warn (not crash) on failure.
- Add a regression test covering sensitive-property suppression.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Run the non-default scan synchronously before the runner can mutate
  config (was a fire-and-forget Task.Run that raced ResolveDataDirectory,
  BlockTree.Initializer, plugin load, etc., serialised mutating graphs,
  and emitted resolved absolute paths as "non-default" overrides).
- Make the helper resilient: a single failing config interface
  (provider lookup, ctor, or property getter) now skips that one
  interface only via an Action<Type, Exception>? callback the caller
  uses for logging. Previously a single throw would short-circuit the
  yield iterator and silently drop every later config.
- Replace the hand-rolled IEnumerator-based ValuesEqual with
  StructuralComparisons.StructuralEqualityComparer.Equals. Configs only
  use scalars / strings / arrays — all handled correctly — and the
  recursion-cycle hazard goes away.
- Mark KeyStore identity fields sensitive (UnlockAccounts,
  BlockAuthorAccount, EnodeAccount, EnodeKeyFile) so account addresses
  and key-file paths never reach startup logs.
- Add a regression test where one IConfigProvider.GetConfig call
  throws: enumeration must continue and surface other categories'
  overrides while the callback receives the failing type.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configuration display on startup - only non-default

2 participants