Skip to content

refactor(consensus): move IBlockProductionPolicy to DI#12099

Merged
LukaszRozmej merged 5 commits into
masterfrom
remove-init-network-protocol
Jun 24, 2026
Merged

refactor(consensus): move IBlockProductionPolicy to DI#12099
LukaszRozmej merged 5 commits into
masterfrom
remove-init-network-protocol

Conversation

@asdacap

@asdacap asdacap commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Register IBlockProductionPolicy in DI (BlockProcessingModule) and apply the post-merge wrap via AddDecorator<IBlockProductionPolicy, MergeBlockProductionPolicy>() in MergePluginModule and AuRaMergeModule, instead of mutating _api.BlockProductionPolicy inside MergePlugin.InitNetworkProtocol.
  • Optimism and Taiko register their AlwaysStartBlockProductionPolicy / NeverStartBlockProductionPolicy in their own modules, dropping the InitializeBlockchain.CreateBlockProductionPolicy virtual and its overrides.
  • Remove the mutable IApiWithBlockchain.BlockProductionPolicy property (a DI registration plus a settable api field is rejected by FallbackToFieldFromApi); InitializeBlockProducer and StartBlockProducer now inject IBlockProductionPolicy.
  • MergePlugin.InitNetworkProtocol now only handles network capabilities and merge finalization.
  • Remove a duplicate MergeBlockProductionPolicy decorator in TestMergeModule that previously double-wrapped the policy.

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

Behaviour-preserving refactor covered by the existing merge/Optimism/Taiko suites. Extended MergePluginTests.Initializes_correctly to assert the resolved IBlockProductionPolicy is MergeBlockProductionPolicy. The full solution builds clean (0 warnings, 0 errors); Nethermind.Api.Test, Nethermind.Optimism.Test, Nethermind.Taiko.Test, Nethermind.Merge.Plugin.Test and Nethermind.Merge.AuRa.Test pass — the only failures observed were pre-existing on master (Runner.Test Arena_order_is_default test-state pollution) or contention-only block-improvement timing tests that pass in isolation.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

🤖 Generated with Claude Code

Register IBlockProductionPolicy in BlockProcessingModule and apply the
merge wrap via AddDecorator in MergePluginModule and AuRaMergeModule,
instead of mutating the api field in MergePlugin.InitNetworkProtocol.
Optimism and Taiko register their AlwaysStart/NeverStart policies in
their own modules, dropping the InitializeBlockchain.CreateBlockProductionPolicy
override. The mutable IApiWithBlockchain.BlockProductionPolicy property
is removed; InitializeBlockProducer and StartBlockProducer inject the
policy instead.

Also drops a duplicate MergeBlockProductionPolicy decorator in
TestMergeModule that previously double-wrapped the policy.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @asdacap's task in 6m 21s —— View job


Code Review

  • Gather context and read changed files
  • Read relevant rules files
  • Analyze correctness and potential regressions
  • Check DI wiring and decorator patterns
  • Post review findings

Overall this is a well-executed refactor. The design is sound and behaviour-preserving.

DI wiring correctness

Scenario Base policy Decorator Result
Standard merge BlockProductionPolicy (from BlockProcessingModule) MergeBlockProductionPolicy (from MergePluginModule)
AuRa merge BlockProductionPolicy MergeBlockProductionPolicy (from AuRaMergeModule)
Optimism AlwaysStartBlockProductionPolicy (overrides base) None
Taiko NeverStartBlockProductionPolicy (overrides base) None

The comment in MergePlugin.cs explaining why the decorator lives in MergePluginModule/AuRaMergeModule rather than BaseMergePluginModule is a nice addition — that's a genuinely non-obvious design choice.

IMergeBlockProductionPolicy runtime cast

MergeBlockProducerFactory does blockProductionPolicy as IMergeBlockProductionPolicy. Under the new wiring, the resolved IBlockProductionPolicy for Merge scenarios is a MergeBlockProductionPolicy instance (via AddDecorator), which implements IMergeBlockProductionPolicy. The cast continues to succeed. ✓

Findings

Medium – pre-existing double-decoration of IBlockProducerFactory in TestMergeModule (inline comment left)

TestMergeModule calls .AddModule(new MergePluginModule()) which already registers .AddDecorator<IBlockProducerFactory, MergeBlockProducerFactory>(), and then adds a second identical .AddDecorator<IBlockProducerFactory, MergeBlockProducerFactory>() in its own body. Autofac stacks decorators, so tests using this module get MergeBlockProducerFactory(MergeBlockProducerFactory(base)) — the outer pre-merge producer is itself a MergeBlockProducer. The Is.InstanceOf<MergeBlockProducer>() assertion doesn't catch this. This is pre-existing (not introduced by this PR), but since the PR is already cleaning up duplicate decorators from this same file, it's worth addressing in the same pass or in a follow-up.

Low – no issues

Everything else looks clean: the virtual CreateBlockProductionPolicy is fully gone (no remaining overrides), Runner.Test/Build.cs correctly drops the now-removed property assignment, and the auto-mock NSubstituteRegistrationSource in that test file will auto-create a mock for IBlockProductionPolicy when needed.

// Block production related.
.AddDecorator<IBlockProductionPolicy, MergeBlockProductionPolicy>()
.AddScoped<PostMergeBlockProducerFactory>()
.AddDecorator<IBlockProducerFactory, MergeBlockProducerFactory>()

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 – pre-existing double-decoration of IBlockProducerFactory

This PR correctly removes the two duplicate AddDecorator<IBlockProductionPolicy, MergeBlockProductionPolicy>() lines, but there's still a double-decoration for IBlockProducerFactory:

  1. MergePluginModule (loaded above via .AddModule(new MergePluginModule())) already registers .AddDecorator<IBlockProducerFactory, MergeBlockProducerFactory>().
  2. This line adds it again.

Result in Autofac: MergeBlockProducerFactory(MergeBlockProducerFactory(base)) — the outer factory's baseBlockProducerFactory is another MergeBlockProducerFactory, so InitBlockProducer() produces a MergeBlockProducer whose pre-merge producer is itself a MergeBlockProducer. This isn't caught by Is.InstanceOf<MergeBlockProducer>() but is wrong behaviour.

This was pre-existing before this PR; this review just flags it since the PR is already tidying up similar duplicate decorators. Consider dropping this line (the one from MergePluginModule is sufficient), or filing a follow-up issue.

new PostMergeBlockProducerFactory(specProvider, sealEngine, timestamper, blocksConfig, logManager))
.AddDecorator<IBlockProducerFactory, MergeBlockProducerFactory>()
.AddDecorator<IBlockProducerRunnerFactory, MergeBlockProducerRunnerFactory>()
// Wraps the base policy. Kept here (and in AuRaMergeModule) rather than BaseMergePluginModule

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.

No need in comment

asdacap and others added 4 commits June 23, 2026 20:21
- Drop the explanatory comment on the IBlockProductionPolicy decorator (per @flcl42).
- Remove the duplicate IBlockProducerFactory decorator in TestMergeModule, which
  double-wrapped the factory already provided by MergePluginModule (per @claude[bot]).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-protocol

# Conflicts:
#	src/Nethermind/Nethermind.Api/IApiWithBlockchain.cs
#	src/Nethermind/Nethermind.Api/NethermindApi.cs
Clears IDE0005 flagged by the code-lint CI after merging master:
- InitializeBlockchain.cs: using Nethermind.Consensus
- TestMergeModule.cs: using Nethermind.Consensus.Producers

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-protocol

# Conflicts:
#	src/Nethermind/Nethermind.Merge.Plugin/MergePlugin.cs
@LukaszRozmej LukaszRozmej merged commit 6867ed9 into master Jun 24, 2026
558 checks passed
@LukaszRozmej LukaszRozmej deleted the remove-init-network-protocol branch June 24, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimism refactoring taiko related to the taiko alethia rollup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants