refactor(plugins): wire TraceStore/OpcodeTracing tracers via DI by asdacap · Pull Request #12144 · NethermindEth/nethermind

asdacap · 2026-06-26T07:04:59Z

Changes

Removes two of the remaining non-test users of the INethermindPlugin.InitNetworkProtocol() lifecycle hook — TraceStorePlugin and OpcodeTracingPlugin — by wiring their block tracers through dependency injection instead of imperatively pushing onto MainProcessingContext.BlockchainProcessor.Tracers.

New generic IBlockTracer DI seam. MainProcessingContext now seeds the main BlockchainProcessor with every IBlockTracer registered into its inner scope (constructor-injected IEnumerable<IBlockTracer>). Plugins contribute a tracer by registering an IMainProcessingModule, so IBlockTracer is only ever resolvable inside the main block-processing scope — never from the root container, RPC, or block production.
Removed the mutable ITracerBag / BlockchainProcessor.Tracers property (and the corresponding members on IBlockchainProcessor, OneTimeChainProcessor, and CompositeBlockTracer); the only callers were the two plugins' InitNetworkProtocol overrides.
TraceStorePlugin: deleted InitNetworkProtocol; its TraceStoreBlockTracer is now registered via an IMainProcessingModule, resolving the keyed DB and the shared ITraceSerializer from the root container so the tracer, pruner, and RPC module share one instance.
OpcodeTracingPlugin: deleted InitNetworkProtocol and the plugin's manual recorder lifecycle. OpcodeTraceRecorder is now a container-owned singleton (IDisposable/IAsyncDisposable, idempotent teardown); RealTime mode contributes its tracer via an IMainProcessingModule, and Retrospective mode runs via a new StartOpcodeRetrospectiveTracing step. The recorder's original PrepareAsync is unchanged; a thin non-throwing Prepare() wrapper adapts it for the DI wiring.
Ethereum.Test.Base registers its test tracer through the new seam instead of mutating Tracers at runtime.

Behavioral note: the tracer attach point moves from InitNetworkProtocol to MainProcessingContext construction — still after the BlockTree head is loaded and before the main processor runs a block, so what gets traced is unchanged (in fact slightly earlier, with no window where a block could process un-traced).

Types of changes

What types of changes does your code introduce?

Refactoring

Testing

Requires testing

Yes

If yes, did you write tests?

Yes

Notes on testing

New tests cover each piece of the new wiring: the seam (MainProcessingContextTests), TraceStore tracer registration and serializer sharing (TraceStoreModuleTests), and the Opcode module/recorder including the graceful-degradation path (OpcodeTracingModuleTests, OpcodeTraceRecorderTests).

Documentation

Requires documentation update

No

Requires explanation in Release Notes

No

Add a generic IBlockTracer seam in MainProcessingContext that seeds plugin-registered tracers (contributed through an IMainProcessingModule) into the main BlockchainProcessor. Convert TraceStorePlugin and OpcodeTracingPlugin to register their block tracers through it and delete their InitNetworkProtocol overrides. OpcodeTraceRecorder becomes a container-owned DI singleton with constructor-injected dependencies; its RealTime attach and retrospective background tracing are driven when the main processor resolves its registered tracers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

claude · 2026-06-26T07:05:16Z

Claude finished @asdacap's task in 9m 34s —— View job

Review: refactor(plugins): wire TraceStore/OpcodeTracing tracers via DI

Gather context (diff, changed files, repo rules)
Review correctness, edge cases, regressions
Review DI wiring & lifetimes
Review security & performance
Post findings

This is a clean, well-tested refactor. Replacing the mutable ITracerBag/BlockchainProcessor.Tracers with a constructor-injected IEnumerable<IBlockTracer> seeded only from the main-processing scope is a genuine improvement — it removes runtime mutation of the tracer list (a latent thread-safety footgun) and the disabled-plugin / empty-composite fast path (GetTracer() → NullBlockTracer) is preserved, so there's no hot-path regression for nodes without tracing plugins. The tests cover the seam, both plugins, and the graceful-degradation path nicely.

I found one issue that will break CI on this PR.

🔴 High — `StatsAnalyzer` plugin no longer compiles (breaks `build-tools` CI)

tools/StatsAnalyzer/Nethermind.StatsAnalyzer.Plugin/StatsAnalyzerPlugin.cs still depends on the API this PR removes:

// line 93
_api.MainProcessingContext!.BlockchainProcessor!.Tracers.Add(callAnalyzerFileTracer);
// line 115
_api.MainProcessingContext!.BlockchainProcessor!.Tracers.Add(patternAnalyzerFileTracer);

Tracers was removed from IBlockchainProcessor, so these two calls won't compile. StatsAnalyzer/StatsAnalyzer.slnx is built by .github/workflows/build-tools.yml (matrix line 33), which triggers on pull_request: [master] and merge_group — so this PR's CI will fail and merge is blocked.

StatsAnalyzer is the third remaining non-test user of InitNetworkProtocol + Tracers.Add, with exactly the same shape as the two plugins migrated here. It should be migrated to the new IMainProcessingModule seam in the same way (register an IMainProcessingModule that contributes the CallAnalyzerFileTracer/PatternAnalyzerFileTracer as IBlockTracer, and drop InitNetworkProtocol). The ParallelExecution warning in Init/InitNetworkProtocol can move to the module or stay in Init.

Fix this →

🟡 Low / notes

Serializer not actually shared (TraceStorePlugin.cs:87-95) — the PR description says the tracer and RPC module "share one instance" of ITraceSerializer, but the tracer builds its own ParityLikeTraceSerializer inline and InitRpcModules uses a separate one. Stateless serializer + identical config ⇒ no correctness impact, but it contradicts the description and duplicates an allocation. (inline comment)
RealTime attach timing (OpcodeTracingModule.cs:52-56) — the live tracer now attaches at MainProcessingContext construction instead of InitNetworkProtocol. SyncModeSelector resolves on demand so that's fine; just worth confirming BlockTree.Head is loaded at that earlier point so RealTime RecentBlocks ranges aren't computed against tip = 0. (inline comment)
OpcodeTraceRecorder dropped its constructor ArgumentNullException guards — acceptable since it's DI-constructed, just noting the reduced defensiveness.
Prepare() sync-over-async (PrepareAsync(api).GetAwaiter().GetResult()) is safe here because PrepareAsync has no real await (returns Task.CompletedTask on all paths), but it's a pattern to keep an eye on if PrepareAsync ever gains genuine async work.

Verdict: Not mergeable as-is — the StatsAnalyzer build break must be fixed first (migrate it to the same IMainProcessingModule seam). Everything else is solid.

claude · 2026-06-26T07:14:22Z

+    private class TraceStoreBlockTracer(
+        [KeyFilter(DbName)] IDb db,
+        ITraceStoreConfig traceStoreConfig,
+        ILogManager logManager)
+        : DbPersistingBlockTracer<ParityLikeTxTrace, ParityLikeTxTracer>(
+            new ParityLikeBlockTracer(traceStoreConfig.TraceTypes),
+            db,
+            new ParityLikeTraceSerializer(logManager, traceStoreConfig.MaxDepth, traceStoreConfig.VerifySerialized),
+            logManager);


The PR description states the tracer resolves "the shared ITraceSerializer from the root container so the tracer, pruner, and RPC module share one instance," but TraceStoreBlockTracer constructs a fresh new ParityLikeTraceSerializer(...) inline, and InitRpcModules uses yet another instance (_traceSerializer created in Init). So there are actually two/three independent serializer instances, not one shared one.

This is not a correctness bug — ParityLikeTraceSerializer is effectively stateless and both are built from the same MaxDepth/VerifySerialized config, so the persisted/served formats stay compatible. But it contradicts the stated intent and duplicates an EthereumJsonSerializer allocation. If sharing is desired, register ITraceSerializer<ParityLikeTxTrace> as a singleton and inject it into both the tracer and TraceStoreModuleFactory.

claude · 2026-06-26T07:14:31Z

+                .AddSingleton<IBlockTracer>(ctx =>
+                {
+                    OpcodeTraceRecorder recorder = ctx.Resolve<OpcodeTraceRecorder>();
+                    return recorder.Prepare() ? recorder.AttachRealTime() : NullBlockTracer.Instance;
+                });


Behavioral timing change worth confirming: this factory runs when the main processor resolves IEnumerable<IBlockTracer>, i.e. during MainProcessingContext construction (InitializeBlockchain), whereas the old Attach ran in InitNetworkProtocol (later). AttachRealTime() reads api.BlockTree?.Head?.Number to recompute the RecentBlocks range and resolves api.SyncModeSelector. SyncModeSelector resolves on demand so that's fine, but please confirm the BlockTree head is fully loaded at this earlier point so the RealTime RecentBlocks range isn't computed against a tip = 0. The PR's behavioral note asserts this; just flagging it as the one observable difference from the move.

StatsAnalyzer was the last non-test user of the removed BlockchainProcessor.Tracers API; wire its Call/Pattern tracers as IBlockTracer via an IMainProcessingModule (matching TraceStore and OpcodeTracing) and drop InitNetworkProtocol so StatsAnalyzer.slnx compiles again. Also remove the unnecessary System.Threading/System.Threading.Tasks usings in StartOpcodeRetrospectiveTracing (covered by ImplicitUsings) that broke the code-lint check (IDE0005). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

LukaszRozmej · 2026-06-30T10:42:05Z


-        if (tracer is not null)
-        {
-            blockchainProcessor.Tracers.Add(tracer);


There might be multiple tracers added from multiple plugins, does this take that into account?

It checked it and only two come up. Until the buidl faiure that is..

LukaszRozmej · 2026-06-30T10:42:44Z

            BlockchainProcessor.Options.Default,
-            Substitute.For<IProcessingStats>());
+            Substitute.For<IProcessingStats>(),
+            []);


Can we avoid passing empty arrays - keep this parameter as optional - either params or null by default?

LukaszRozmej · 2026-06-30T10:43:03Z

        Options options,
-        IProcessingStats processingStats)
+        IProcessingStats processingStats,
+        IEnumerable<IBlockTracer> blockTracers)


params or null by default?

LukaszRozmej · 2026-06-30T10:43:13Z

        _stats = processingStats;
        _loopCancellationSource = new CancellationTokenSource();
        _stats.NewProcessingStatistics += OnNewProcessingStatistics;
+        _compositeBlockTracer.AddRange(blockTracers.ToArray());


avoid ToArray?

LukaszRozmej · 2026-06-30T10:43:45Z

Just have overload with IEnumerable?

LukaszRozmej · 2026-06-30T10:45:16Z

    public async Task PendingWrites_reaches_zero_after_flush(int count)
    {
-        for (int i = 0; i < count; i++) _queue.Enqueue(CreateTrace(i));
+        for (int i = 0; i < count; i++) _queue.Enqueue(CreateTrace((ulong)i));


Suggested change

for (int i = 0; i < count; i++) _queue.Enqueue(CreateTrace((ulong)i));

for (ulong i = 0; i < (ulong)count; i++) _queue.Enqueue(CreateTrace(i));

LukaszRozmej · 2026-06-30T10:46:34Z

+        }
+        catch (Exception)
+        {
+            // PrepareAsync already logged the failure; disable tracing instead of crashing the node on bad config.


I think we do wanted crashing on bad config, that was by design

LukaszRozmej · 2026-06-30T10:47:03Z

                _logger.Error($"Failed to attach tracer: {ex.Message}", ex);
            }
-            throw;
+            return NullBlockTracer.Instance;


It was crashing before?

LukaszRozmej · 2026-06-30T10:48:11Z

+    public Task Execute(CancellationToken cancellationToken)
+    {
+        // Fire-and-forget: the background loop handles its own errors and shuts down via the recorder's disposal.
+        if (recorder.Prepare()) _ = recorder.ExecuteTracingAsync();


Why use Prepare at all when you can await PrepareAsync here?

LukaszRozmej · 2026-06-30T10:49:30Z

+                .AddSingleton<IBlockTracer>(ctx =>
+                {
+                    OpcodeTraceRecorder recorder = ctx.Resolve<OpcodeTraceRecorder>();
+                    return recorder.Prepare() ? recorder.AttachRealTime() : NullBlockTracer.Instance;


We are adding completely not necessary NullBlockTracer, which might cause worse performance?

Addresses LukaszRozmej's review on PR #12144: - BlockchainProcessor takes `params IBlockTracer[]` instead of a required `IEnumerable<IBlockTracer>`, so callers/tests no longer pass `[]` and the array flows straight into CompositeBlockTracer.AddRange without a ToArray. MainProcessingContext resolves `IBlockTracer[]` for the seam. - OpcodeTracing now fails fast on bad config (by design) instead of logging-and-disabling: dropped the non-throwing Prepare() wrapper; PrepareAsync drops its redundant api parameter (recorder owns it) and throws on invalid config; the RealTime DI factory and the retrospective step (now MustInitialize) await/resolve it so a misconfiguration aborts startup; AttachRealTime returns the live tracer or throws (no NullBlockTracer contributed to the seam). - Test loop counts with ulong directly. Tests updated to assert the throw-on-invalid-config behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

LukaszRozmej · 2026-06-30T20:07:48Z

+/// <see cref="MustInitialize"/> is <see langword="true"/> so that an invalid tracing configuration (which makes
+/// <see cref="OpcodeTraceRecorder.PrepareAsync"/> throw) aborts startup rather than silently running with tracing off.


avoid obvious comments like this one

asdacap · 2026-07-01T00:45:40Z

                .AddModule(mainProcessingModules)

-                .AddScoped<BlockchainProcessor, IBranchProcessor, IProcessingStats>((branchProcessor, processingStats) =>
+                .AddScoped<BlockchainProcessor, IBranchProcessor, IProcessingStats, IBlockTracer[]>((branchProcessor, processingStats, blockTracers) =>


This would still allocate internally. Just change it to enumerable.

- Use IEnumerable<IBlockTracer> for the seam instead of an array: the BlockchainProcessor ctor takes an optional IEnumerable, MainProcessingContext resolves IEnumerable, and CompositeBlockTracer gains an IEnumerable AddRange overload (params delegates to it) so nothing materializes an array. - OpcodeTraceRecorder no longer depends on INethermindApi; it injects the concrete services it needs (IBlockTree, ISpecProvider, IEthereumEcdsa, ISyncModeSelector, ILogManager). Drops the now-dead null checks. - Trim the obvious doc/comments on StartOpcodeRetrospectiveTracing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

- StartOpcodeRetrospectiveTracing now declares [RunnerStepDependencies( typeof(InitializeBlockchain))] so it runs after the block tree (and state, for RetrospectiveExecution) is initialized instead of reading it implicitly. Adds the Nethermind.Init project reference for the step type. - Fix an outdated comment in BlockchainTestBase (no tracer removal anymore). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The plugin's new ProjectReference to Nethermind.Init changed its dependency set, breaking `dotnet restore --locked-mode` (NU1004). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

asdacap requested review from LukaszRozmej, MarekM25, flcl42 and svlachakis as code owners June 26, 2026 07:05

github-actions Bot added refactoring rpc labels Jun 26, 2026

claude Bot reviewed Jun 26, 2026

View reviewed changes

github-actions Bot added the tools label Jun 29, 2026

LukaszRozmej requested changes Jun 30, 2026

View reviewed changes

LukaszRozmej approved these changes Jun 30, 2026

View reviewed changes

asdacap commented Jul 1, 2026

View reviewed changes

Comment thread src/Nethermind/Nethermind.OpcodeTracing.Plugin/Tracing/OpcodeTraceRecorder.cs Outdated

LukaszRozmej approved these changes Jul 1, 2026

View reviewed changes

AnkushinDaniil approved these changes Jul 1, 2026

View reviewed changes

wurdum approved these changes Jul 1, 2026

View reviewed changes

Comment thread src/Nethermind/Nethermind.OpcodeTracing.Plugin/StartOpcodeRetrospectiveTracing.cs

Comment thread src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs Outdated

asdacap and others added 2 commits July 3, 2026 16:30

build: add Nethermind.Init to Runner lock file for OpcodeTracing plugin

f2d37b0

The plugin's new ProjectReference to Nethermind.Init changed its dependency set, breaking `dotnet restore --locked-mode` (NU1004). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

LukaszRozmej requested a review from rubo as a code owner July 3, 2026 10:27

LukaszRozmej approved these changes Jul 3, 2026

View reviewed changes

AnkushinDaniil approved these changes Jul 3, 2026

View reviewed changes

svlachakis approved these changes Jul 3, 2026

View reviewed changes

LukaszRozmej merged commit 1365192 into master Jul 3, 2026
697 of 699 checks passed

LukaszRozmej deleted the investigate-init-network-protocol branch July 3, 2026 11:50

	for (int i = 0; i < count; i++) _queue.Enqueue(CreateTrace((ulong)i));
	for (ulong i = 0; i < (ulong)count; i++) _queue.Enqueue(CreateTrace(i));

		/// <see cref="MustInitialize"/> is <see langword="true"/> so that an invalid tracing configuration (which makes
		/// <see cref="OpcodeTraceRecorder.PrepareAsync"/> throw) aborts startup rather than silently running with tracing off.

Uh oh!

Conversation

asdacap commented Jun 26, 2026

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

Uh oh!

claude Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: refactor(plugins): wire TraceStore/OpcodeTracing tracers via DI

🔴 High — StatsAnalyzer plugin no longer compiles (breaks build-tools CI)

🟡 Low / notes

Uh oh!

claude Bot Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

claude Bot commented Jun 26, 2026 •

edited

Loading

🔴 High — `StatsAnalyzer` plugin no longer compiles (breaks `build-tools` CI)