Feature/subnet plugin

[ak88]

Plugin to be used for XDC subnets

[LukaszRozmej]

Can we put those in plugins? They are just strings.

[ak88]

@claude code review

[claude]

Claude finished @ak88's task in 2m 48s —— View job

---

Code Review: Feature/subnet plugin

• Read repository rules and guidelines
• Read changed files
• Analyze code and provide review feedback

---

Summary

The PR introduces a XdcSubnetPlugin and supporting infrastructure (XdcSubnetModule, SubnetSnapshotManager, BaseSnapshotManager) for XDC subnet support. The refactoring to extract BaseSnapshotManager<TSnapshot> is clean and the overall architecture is sound, but there are two bugs that will cause runtime failures, plus several design and code-quality issues.

---

Critical Bugs

1. _xdcPlugin.Init() never called — NullReferenceException at runtime

src/Nethermind/Nethermind.Xdc/XdcSubnetPlugin.cs:25

XdcSubnetPlugin.Init() returns Task.CompletedTask without forwarding nethermindApi to _xdcPlugin. As a result, XdcPlugin._nethermindApi is always null. All three delegating methods will throw a NullReferenceException at runtime:

// Current — broken
public Task Init(INethermindApi nethermindApi) => Task.CompletedTask;

// Fix
public Task Init(INethermindApi nethermindApi) => _xdcPlugin.Init(nethermindApi);

InitNetworkProtocol() calls _nethermindApi.ProtocolsManager!.RemoveSupportedCapability(...), InitBlockProducer() calls _nethermindApi.Context.Resolve<StartXdcBlockProducer>(), and InitBlockProducerRunner() calls _nethermindApi.Context.Resolve<IBlockTree>() — all will NPE. Fix this →

2. BaseSnapshotManager subscribes to BlockAddedToMain without ever unsubscribing — event handler leak

src/Nethermind/Nethermind.Xdc/BaseSnapshotManager.cs:39

_blockTree.BlockAddedToMain += OnBlockAddedToMain;  // subscribed in ctor
// ... no Dispose / no -= anywhere

This prevents BaseSnapshotManager (and its IBlockTree) from being garbage collected. The class must implement IDisposable and unsubscribe in Dispose(). Compare with ExpCountDown and TimeoutTimer in the same project which correctly implement IDisposable. Fix this →

---

Design Issues

3. Duplicate ISnapshotManager registration in XdcSubnetModule

src/Nethermind/Nethermind.Xdc/XdcSubnetModule.cs:18-22

base.Load(builder) (i.e. XdcModule.Load) already registers ISnapshotManager as SnapshotManager. XdcSubnetModule then registers it again as SubnetSnapshotManager. Autofac resolves the last registration, so the SnapshotManager entry is dead weight. Consider adding a protected virtual RegisterSnapshotManager method to XdcModule so subclasses override only that part without triggering a redundant base registration.

4. SealEngineType.XDPoSSubnet belongs in the plugin, not core

(Already flagged by @LukaszRozmej — the same applies to XDPoS if that can be moved, but at minimum the new XDPoSSubnet should live in the plugin namespace, not Nethermind.Core.)

5. DI factory delegate in XdcSubnetModule — unavoidable but note the pattern

src/Nethermind/Nethermind.Xdc/XdcSubnetModule.cs:20

The DI rules discourage the factory-delegate form (.AddSingleton<IFoo, Dep1, Dep2, ...>(factory)). It is acceptable here only because [KeyFilter] on the IDb parameter requires manual resolution — confirm this is intentional and add a comment explaining why the preferred .AddSingleton<ISnapshotManager, SubnetSnapshotManager>() form cannot be used.

---

Code Quality

6. SubnetSnapshot.NextEpochPenalties exposes a public setter it doesn't need

src/Nethermind/Nethermind.Xdc/Types/SubnetSnapshot.cs:11

public Address[] NextEpochPenalties { get; set; }  // mutable after construction

The property is only ever assigned in the constructor. Use init; (or remove the setter entirely) to make the intent clear and prevent accidental mutation.

7. Redundant null-guard in SubnetSnapshot constructor

src/Nethermind/Nethermind.Xdc/Types/SubnetSnapshot.cs:19

NextEpochPenalties = penalties ?? [];

The coding style says "Trust null annotations, don't add redundant null checks." Since the penalties parameter is typed as Address[] (non-nullable), the ?? guard is noise unless HandlePenalties is known to return null (it doesn't — it returns penalties.ToArray() or []). Remove the guard, or change the parameter type to Address[]? if null is genuinely possible.

8. header.ParentHash may be null for non-genesis blocks

src/Nethermind/Nethermind.Xdc/SubnetSnapshotManager.cs:32

Address[] penalties = header.IsGenesis ? [] : penaltyHandler.HandlePenalties(header.Number, header.ParentHash, candidates);

BlockHeader.ParentHash is typed Hash256?. For non-genesis blocks a null ParentHash would be silently passed to HandlePenalties, which does not validate it. Add an assertion or guard (e.g. header.ParentHash! with a comment, or an explicit null check).

9. Incomplete test name

src/Nethermind/Nethermind.Xdc.Test/SubnetSnapshotManagerTests.cs:53

GetSnapshot_ShouldRetrieveFromIfFound is missing context — presumably "from cache". Rename to GetSnapshot_ShouldRetrieveFromCacheIfFound.

10. Test field _xdcPlugin typed as IConsensusPlugin loses strong typing

src/Nethermind/Nethermind.Xdc/XdcSubnetPlugin.cs:15

private IConsensusPlugin _xdcPlugin = new XdcPlugin(chainSpec);

Typing as IConsensusPlugin rather than XdcPlugin means any future extension of XdcPlugin's interface requires a cast. Use XdcPlugin directly for the field type.

---

[ak88]

code review @claude

[claude]

Claude finished @ak88's task in 3m 45s —— View job

---

Code Review: Feature/subnet plugin

• Read repository rules and guidelines
• Read changed files and git diff
• Analyze code and provide review feedback

---

Summary

This PR adds XdcSubnetPlugin, XdcSubnetModule, SubnetSnapshotManager, SubnetPenaltyHandler, and BaseSnapshotManager<TSnapshot> for XDC subnet support. The refactoring to extract BaseSnapshotManager<TSnapshot> is clean, and the previous critical bug (Init not forwarding to _xdcPlugin) is now fixed. However, there are two new critical bugs plus several carryover and code quality issues.

---

Critical Bugs

1. SubnetPenaltyHandler is never registered in DI — wrong handler used at runtime

src/Nethermind/Nethermind.Xdc/XdcSubnetModule.cs

XdcSubnetModule.Load calls base.Load(builder), which registers IPenaltyHandler as PenaltyHandler. XdcSubnetModule never overrides this with SubnetPenaltyHandler. As a result, SubnetSnapshotManager (which takes IPenaltyHandler from DI) will receive PenaltyHandler at runtime — the wrong implementation.

PenaltyHandler.HandlePenalties hard-casts to XdcBlockHeader, while SubnetPenaltyHandler.HandlePenalties correctly casts to XdcSubnetBlockHeader. Using PenaltyHandler in a subnet context will throw InvalidCastException on the first non-genesis snapshot creation.

Fix: add .AddSingleton<IPenaltyHandler, SubnetPenaltyHandler>() to XdcSubnetModule.Load after base.Load(builder). Fix this →

2. Duplicate IPenaltyHandler registration in XdcModule — redundant registration

src/Nethermind/Nethermind.Xdc/XdcModule.cs:80 and :104

IPenaltyHandler is registered twice as PenaltyHandler (once at line 80 under "penalty handler" and again at line 104). This is dead code introduced in this PR. Remove the duplicate. Fix this →

3. BaseSnapshotManager event handler leak (carryover from previous review)

src/Nethermind/Nethermind.Xdc/BaseSnapshotManager.cs:39

_blockTree.BlockAddedToMain += OnBlockAddedToMain; // subscribed, never unsubscribed

The class has no Dispose method and does not implement IDisposable. This prevents BaseSnapshotManager and IBlockTree from being garbage-collected. Add IDisposable and unsubscribe in Dispose(). Fix this →

---

Design Issues

4. Double ISnapshotManager registration

src/Nethermind/Nethermind.Xdc/XdcSubnetModule.cs:18-25

base.Load(builder) registers ISnapshotManager as SnapshotManager. XdcSubnetModule then registers it again as SubnetSnapshotManager. Autofac resolves the last registration, so the SnapshotManager entry is never used in the subnet context. Consider adding a protected virtual void RegisterSnapshotManager(ContainerBuilder builder) hook in XdcModule so subclasses only override that part.

5. CertThreshold → CertificateThreshold rename is a breaking change

src/Nethermind/Nethermind.Xdc/Spec/XdcChainSpecEngineParameters.cs

V2ConfigParams.CertThreshold was renamed to CertificateThreshold. Any existing chain spec JSON files that use "certThreshold" (or whatever the JSON key was) will silently fail to deserialize — the field will default to 0.0. Verify that the JSON key name either has a [JsonPropertyName("certThreshold")] alias or confirm no chain spec files use the old name.

---

Code Quality

6. header.ParentHash nullable mismatch in SubnetSnapshotManager

src/Nethermind/Nethermind.Xdc/SubnetSnapshotManager.cs:32

Address[] penalties = header.IsGenesis ? [] : penaltyHandler.HandlePenalties(header.Number, header.ParentHash, candidates);

BlockHeader.ParentHash is Hash256? but IPenaltyHandler.HandlePenalties takes Hash256 (non-nullable). A null-annotated mismatch: use header.ParentHash! with a brief comment, or add an explicit guard. Fix this →

7. SubnetSnapshot.NextEpochPenalties exposes a public setter

src/Nethermind/Nethermind.Xdc/Types/SubnetSnapshot.cs:11

public Address[] NextEpochPenalties { get; set; }  // only assigned in constructor

Use init; to communicate intent and prevent accidental mutation. Fix this →

8. Redundant null-guard in SubnetSnapshot constructor

src/Nethermind/Nethermind.Xdc/Types/SubnetSnapshot.cs:15

NextEpochPenalties = penalties ?? [];

penalties is typed as Address[] (non-nullable). Per the coding style, redundant null checks are noise. Remove the ?? [] guard — or change the parameter to Address[]? if null is genuinely possible (it isn't from SubnetSnapshotManager). Fix this →

9. Inconsistent hex prefix in XdcConstants

src/Nethermind/Nethermind.Xdc/XdcConstants.cs:47-53

SetSecret and SetOpening use bare hex ("34d38600"), while VoteMethod, UnvoteMethod, ProposeMethod, ResignMethod, and SignMethod all use 0x-prefixed hex ("0x6dd7d8ea"). Standardize to one convention — prefer the 0x-prefix form which is consistent with the rest of the Nethermind codebase.

10. Incomplete test name

src/Nethermind/Nethermind.Xdc.Test/SubnetSnapshotManagerTests.cs:54

GetSnapshot_ShouldRetrieveFromIfFound is missing context — presumably "from cache". Rename to GetSnapshot_ShouldRetrieveFromCacheIfFound. Fix this →

---