Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/Nethermind/Nethermind.Config.Test/ConfigProviderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using FluentAssertions;
using Nethermind.Core.Extensions;
using Nethermind.JsonRpc;
Expand Down Expand Up @@ -102,4 +103,62 @@ public void Can_useExistingConfig()
configProvider.GetConfig<IBlocksConfig>().MinGasPrice.Should().Be(12345);
}
}

[TestFixture]
[Parallelizable(ParallelScope.None)]
public class GetNonDefaultValuesTests
{
[Test]
public void Returns_empty_when_no_overrides()
{
ConfigProvider configProvider = new();
configProvider.Initialize();

List<(string Category, string Name, object? CurrentValue, object? DefaultValue)> nonDefaults =
configProvider.GetNonDefaultValues().ToList();

Assert.That(nonDefaults, Is.Empty);
}

[Test]
public void Returns_overridden_value_with_current_and_default()
{
Dictionary<string, string> args = new()
{
{ "Network.DiscoveryPort", "12345" }
};
ConfigProvider configProvider = new();
configProvider.AddSource(new ArgsConfigSource(args));
configProvider.Initialize();

List<(string Category, string Name, object? CurrentValue, object? DefaultValue)> nonDefaults =
configProvider.GetNonDefaultValues()
.Where(static x => x.Category == "Network" && x.Name == nameof(INetworkConfig.DiscoveryPort))
.ToList();

Assert.That(nonDefaults, Has.Count.EqualTo(1));
Assert.That(nonDefaults[0].CurrentValue, Is.EqualTo(12345));
Assert.That(nonDefaults[0].DefaultValue, Is.EqualTo(30303));
}

[Test]
public void Surfaces_overrides_across_categories()
{
Dictionary<string, string> args = new()
{
{ "Network.DiscoveryPort", "12345" },
{ "JsonRpc.Enabled", "true" }
};
ConfigProvider configProvider = new();
configProvider.AddSource(new ArgsConfigSource(args));
configProvider.Initialize();

HashSet<string> keys = configProvider.GetNonDefaultValues()
.Select(static x => $"{x.Category}.{x.Name}")
.ToHashSet();

Assert.That(keys, Does.Contain("Network.DiscoveryPort"));
Assert.That(keys, Does.Contain("JsonRpc.Enabled"));
}
}
}
97 changes: 97 additions & 0 deletions src/Nethermind/Nethermind.Config/ConfigExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@
// SPDX-License-Identifier: LGPL-3.0-only

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.ComponentModel;
using System.Linq;
using System.Reflection;
using Nethermind.Core;
using Nethermind.Core.Extensions;

namespace Nethermind.Config;

Expand Down Expand Up @@ -41,4 +45,97 @@ public static T GetDefaultValue<T>(this IConfig config, string propertyName)
string? defaultValue = attribute.DefaultValue;
return (T)TypeDescriptor.GetConverter(typeof(T)).ConvertFrom(defaultValue!)!;
}

/// <summary>
/// Enumerates configuration properties whose current value differs from the
/// implementation's default. Useful for surfacing on startup which knobs the
/// operator has actually changed, rather than dumping every value.
/// </summary>
/// <remarks>
/// The default is taken from a freshly constructed instance of the
/// implementing type, so initializers and constructors are honoured exactly
/// as production wiring would do (no parsing of the <see cref="ConfigItemAttribute.DefaultValue"/> string).
/// </remarks>
/// <param name="configProvider">The provider to query.</param>
/// <returns>
/// Tuples of <c>(category, propertyName, currentValue, defaultValue)</c> for
/// every property whose current value is not equal to its default.
/// </returns>
public static IEnumerable<(string Category, string Name, object? CurrentValue, object? DefaultValue)>
GetNonDefaultValues(this IConfigProvider configProvider)
{
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;

.FindNethermindBasedTypes(typeof(IConfig))
.Where(static t => t.IsInterface))
{
Type? implementation = configInterface.GetDirectInterfaceImplementation();
if (implementation is null) continue;

IConfig current;
try
{
current = configProvider.GetConfig(configInterface);
}
catch (ArgumentException)
{
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)

try
{
fresh = (IConfig)Activator.CreateInstance(implementation)!;
}
catch (MissingMethodException)
{
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.

string category = GetCategoryName(configInterface) ?? string.Empty;

foreach (PropertyInfo property in configInterface.GetProperties())
{
if (!property.CanRead) continue;

object? actual = property.GetValue(current);
object? defaultValue = property.GetValue(fresh);

if (!ValuesEqual(actual, defaultValue))
{
yield return (category, property.Name, actual, defaultValue);
}
}
}
}

private static bool ValuesEqual(object? a, object? b)
{
if (a is null) return b is null;
if (b is null) return false;
if (a is string || b is string) return a.Equals(b);
if (a is IEnumerable enumerableA && b is IEnumerable enumerableB)
{
IEnumerator iteratorA = enumerableA.GetEnumerator();
IEnumerator iteratorB = enumerableB.GetEnumerator();
try
{
while (true)
{
bool hasA = iteratorA.MoveNext();
bool hasB = iteratorB.MoveNext();
if (hasA != hasB) return false;
if (!hasA) return true;
if (!ValuesEqual(iteratorA.Current, iteratorB.Current)) return false;
}
}
finally
{
(iteratorA as IDisposable)?.Dispose();
(iteratorB as IDisposable)?.Dispose();
}
}
return a.Equals(b);
}
}
18 changes: 18 additions & 0 deletions src/Nethermind/Nethermind.Runner/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,24 @@ async Task<int> RunAsync(ParseResult parseResult, PluginLoader pluginLoader, Can

EthereumJsonSerializer serializer = new();

if (logger.IsInfo)
{
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?

}

if (logger.IsDebug)
{
logger.Debug($"Nethermind configuration:\n{serializer.Serialize(initConfig, true)}");
Expand Down
Loading