Use standard Microsoft logging in Kademlia

[flcl42]

Changes

• Replaced Nethermind.Kademlia's direct Nethermind.Logging dependency with Microsoft.Extensions.Logging.Abstractions and required ILoggerFactory injection.
• Registered no-op logging for generic Kademlia wiring by default, while discovery wiring supplies the Nethermind-backed Microsoft logger factory.
• Added no-log convenience constructors for direct Kademlia test construction so tests do not pass logger factories around.
• Preserved Nethermind logger-name conventions in the Microsoft logger adapter and forwarded exceptions from standard error logging calls.
• Updated the runner lock file for the discovery dependency change and aligned guarded log calls with the repo's one-line logging style.
• Removed the standalone NLog adapter regression test file per review direction.

Types of changes

What types of changes does your code introduce?

• 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

Notes on testing

• Main solution and benchmark solution builds passed with warnings as errors.
• Runner locked restore and solution locked restore now pass.
• Focused and full discovery tests passed locally.
• Affected NLog test project build and existing tests passed after removing the standalone adapter tests.
• Code-lint-shaped analyzer builds passed locally, and the GitHub Code Lint check passed on the pushed branch.
• Diff whitespace check passed.
• Verified Nethermind.Kademlia no longer has project references or Nethermind.Logging usage.
• Ethereum test solution build could not complete locally because external fixture files under src/tests are missing in this checkout.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[claude]

Claude finished @flcl42's task in 4m 8s —— View job

---

Review: PR #12170 — Use standard Microsoft logging in Kademlia

• Read the full diff
• Review logging adapter changes (NethermindLoggerFactory)
• Review Kademlia logging call-site changes
• Review DI / module wiring
• Review tests
• Post findings

---

Overall this is a clean, well-scoped refactor. Nethermind.Kademlia drops its Nethermind.Logging project reference in favor of Microsoft.Extensions.Logging.Abstractions, production wiring is correctly threaded through DiscoveryKademliaModuleBase (and inherited by both Discv4 and Discv5 KademliaModules), and the IsEnabled-guard-before-interpolation pattern is preserved at every call site, so the hot-path allocation behavior is unchanged. I verified that passing pre-interpolated $"…" strings with no args to LogDebug/LogTrace/etc. is safe — FormattedLogValues returns the message verbatim when the args array is empty, so there's no template-brace FormatException risk.

The two genuinely good fixes here are worth calling out: logger.Error(text, exception) now forwards the exception (previously dropped), and the adapter normalizes the category name to the Nethermind convention.

🟠 Medium — Kademlia unit/simulation test containers no longer wire logging

KademliaModule registers the Kademlia components with an optional ILoggerFactory? = null parameter. Only DiscoveryKademliaModuleBase registers ILoggerFactory. The two test containers build KademliaModule directly and register only ILogManager, never ILoggerFactory:

• Nethermind.Network.Discovery.Test/Kademlia/KademliaTests.cs:27 → new TestLogManager(LogLevel.Trace)
• Nethermind.Network.Discovery.Test/Kademlia/KademliaSimulation.cs:223 → new TestLogManager(LogLevel.Error)

Autofac treats the unresolved optional param as null, so the components silently fall back to NullLoggerFactory and emit no logs. Consequences:

1. Those TestLogManager registrations are now dead config (misleading — they look like they enable logging but don't).
2. The converted Kademlia logging paths (incl. Trace-level traversal logs the simulation used to exercise) lose all unit-test coverage; the new test only covers the adapter in isolation.

Mirror the production wiring in both test containers:

.AddSingleton<ILoggerFactory, ILogManager>(static lm => new NethermindLoggerFactory(lm))

Fix this →

🟢 Low — global Replace in category normalization

See inline comment on NethermindLoggerFactory.cs:15. Replace("Nethermind.", "") is a global replace, faithful to the old GetLoggerName, but the resulting logger-name format changes (clean <…> display name vs. the old `2[[…]] FullName). Only relevant if any log-filtering config keys on the old generic-type names.

🟢 Low / note — Error exception-forwarding has wider blast radius

logger.Error(formatter(...), exception) in NethermindLoggerFactory is a shared adapter also used by DotNetty (Program.cs) and Shutter. The change is strictly an improvement (exceptions were dropped before), just noting it affects more than Kademlia.

---

Verdict

No Critical/High issues. One Medium (test wiring / lost coverage) that should be addressed or explicitly acknowledged before merge; the Lows are informational.

[github-actions]

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: RETURNDATASIZE

No significant regressions or improvements detected.

[flcl42]

@claude review

[claude]

Claude finished @flcl42's task in 2m 37s —— View job

---

Review: PR #12170 — Use standard Microsoft logging in Kademlia

• Read the full diff
• Review logging adapter (NethermindLoggerFactory)
• Review Kademlia call-site changes
• Review DI / module wiring
• Review tests
• Post findings

---

This is a clean, well-scoped refactor. Nethermind.Kademlia drops its Nethermind.Logging project reference for Microsoft.Extensions.Logging.Abstractions, production wiring is correctly threaded through DiscoveryKademliaModuleBase (inherited by both Discv4/Discv5 modules), and the IsEnabled-guard-before-interpolation pattern is preserved at every call site, so hot-path allocation behavior is unchanged.

I verified the items that could have gone wrong in this kind of refactor:

• DI override ordering is correct. KademliaModule registers ILoggerFactory → NullLoggerFactory.Instance; DiscoveryKademliaModuleBase calls .AddModule(...) first and then .AddSingleton<ILoggerFactory, ILogManager>(... new NethermindLoggerFactory(lm)). The later registration wins, so production discovery gets real Nethermind-backed logging while generic/standalone Kademlia wiring safely no-ops. ✅
• No constructor-selection ambiguity. The new no-log convenience constructors don't shadow the ILoggerFactory ones — ILoggerFactory is always registered in every module path, so Autofac greedily picks the longer constructor. The parameterless-logger ctors are only used by direct-new tests. ✅
• No stale external call sites. No code outside Nethermind.Kademlia constructs these types with the old ILogManager signature (grep clean). ✅
• No orphaned ILogManager resolutions left in the two test containers after the registration removals (grep clean), so resolution won't fail. ✅
• logger.Error(text, exception) now forwards the exception (previously dropped) and the adapter normalizes the category to the Nethermind convention — both genuine improvements, covered by the new regression tests.

🟢 Low — lost trace/debug interpolation coverage in the Kademlia test containers

KademliaTests.cs and KademliaSimulation.cs previously registered TestLogManager (Trace / Error). Those registrations are now removed, and the containers resolve NullLoggerFactory from KademliaModule, so the converted log paths (incl. the Trace traversal logs the simulation exercised) no longer run under test — IsEnabled(...) short-circuits them. Removing the now-dead registrations was the right call (they no longer did anything), so this is purely a coverage note, not a correctness issue. If you want to keep exercising those interpolation paths, mirror production by adding to both containers:

.AddSingleton<ILoggerFactory, ILogManager>(static lm => new NethermindLoggerFactory(lm))

(requires the Nethermind.Logging.Microsoft reference in the test project). Optional.

🟢 Low — Replace("Nethermind.", string.Empty) is a global replace

In NethermindLoggerFactory.cs:15, for generic Kademlia types the category becomes the display name and Nethermind. is stripped from the type arguments too (e.g. Kademlia.KBucketTree<Stats.Model.Node, Core.Crypto.Hash256>). This is faithful to the old ILogManager.GetLoggerName (also a global replace), so the convention is preserved — only the format differs from the old backtick `2[[…]] FullName. No action unless any log-filtering config keys on the old generic-type names.

🟢 Note — Error exception-forwarding has wider blast radius

The NethermindLoggerFactory adapter is shared with DotNetty (Program.cs) and Shutter, so the (strictly-better) exception-forwarding change affects more than Kademlia. Worth being aware of, no action.

🟢 Nit — packages.lock.json gained a trailing newline

Nethermind.Runner/packages.lock.json now ends with a newline where it didn't before. Harmless, just unusual for a dotnet restore-generated file.

---

Verdict

No Critical / High / Medium findings. The earlier Medium (misleading dead TestLogManager config) has been resolved by the test-cleanup commits; the only residual is the optional coverage note above. Looks good to merge.
• branch feature/kad-std-log

[claude]

Claude finished @flcl42's task in 2m 31s —— View job

---

Review: PR #12170 — Use standard Microsoft logging in Kademlia

• Read the full diff (origin/master)
• Verify logging adapter (NethermindLoggerFactory) + its other consumers
• Verify Kademlia call-site conversions
• Verify DI / module wiring + constructor selection
• Verify tests / no orphaned resolutions
• Post findings

---

Confirming the conclusion of the two earlier passes after re-verifying against the current branch tip. This is a clean, well-scoped refactor: Nethermind.Kademlia drops its Nethermind.Logging project reference for Microsoft.Extensions.Logging.Abstractions, the IsEnabled-guard-before-interpolation pattern is preserved at every call site (hot-path allocation behavior unchanged), and the one-liner guard style now matches the repo convention.

Independently re-verified:

• DI override ordering is correct. KademliaModule registers ILoggerFactory → NullLoggerFactory.Instance (KademliaModule.cs:38); DiscoveryKademliaModuleBase adds the module first, then .AddSingleton<ILoggerFactory, ILogManager>(... new NethermindLoggerFactory(logManager)) (DiscoveryKademliaModuleBase.cs:25) — last registration wins, so production discovery gets real logging and generic/test wiring safely no-ops. ✅
• No constructor ambiguity / no broken call sites. Each converted type that needed direct-new construction got a no-log convenience ctor delegating to the ILoggerFactory one; Kademlia<> itself has no convenience ctor but is only ever resolved via DI (where ILoggerFactory is always registered). grep for new Kademlia< outside the module/config is clean. ✅
• No orphaned ILogManager resolutions remain in the two Kademlia test containers after the TestLogManager removals (grep clean), so resolution won't fail. ✅
• Replace("Nethermind.", "") blast radius is bounded. NethermindLoggerFactory is also used by Shutter (ShutterP2P.cs:74) and DotNetty (Program.cs:167), but their categories are third-party namespaces (libp2p/DotNetty) that don't contain Nethermind., so only the Kademlia category names are affected — and that strip is faithful to the old GetClassLogger convention. ✅
• logger.Error(text, exception) now forwards the exception (previously dropped) — a genuine improvement, shared by all three consumers above. ✅

🟢 Low — lost trace/debug interpolation coverage in Kademlia test containers

KademliaTests.cs / KademliaSimulation.cs now resolve NullLoggerFactory from KademliaModule, so the converted log paths (incl. the Trace traversal logs the simulation exercised) no longer execute under test. Removing the now-dead TestLogManager registrations was the right call; this is purely a coverage note. Optional: mirror production with .AddSingleton<ILoggerFactory, ILogManager>(static lm => new NethermindLoggerFactory(lm)) in both containers. [Acknowledged in prior comments — no action required.]

🟢 Nit — Nethermind.Runner/packages.lock.json gained a trailing newline

Harmless; just unusual for a dotnet restore-generated file.

---

Verdict

No Critical / High / Medium findings. The earlier Medium (misleading dead TestLogManager config) was resolved by the test-cleanup commits, and the residual coverage note is explicitly acknowledged. Looks good to merge.
• branch feature/kad-std-log

[LukaszRozmej]

Ok... but why?

[flcl42]

Ok... but why?

Want to reuse kademilia in libp2p where it can be included only as nuget package. Nethermind.Logging is the only dependency it has and I suspect publishing nethermind.kademlia will require publishing nethermind.logging. So this pr is an attempt to make it simpler to maintain(one nuget instead of two) and use by third parties. (I cannot include it as the source code at least because of the license clash + it would be ugly)