XDC - Fix subnet penalties and rewards by cicr99 · Pull Request #12080 · NethermindEth/nethermind

cicr99 · 2026-06-22T07:54:40Z

No description provided.

claude · 2026-06-22T07:56:15Z

Claude finished @cicr99's task in 8m 7s —— View job

PR Review: XDC - Fix subnet penalties and rewards

Tasks

Summary

This PR introduces a dedicated XdcSubnetRewardCalculator / XdcSubnetRewardCalculatorSource hierarchy so subnet nodes use a different masternode-selection path for reward calculation, fixes the subnet first-gap-snapshot logic, and overrides the penalty handler for subnets. The design is sound, but there are three issues that need addressing before merge and one low-priority note.

High — Null dereference in `SubnetPenaltyHandler.cs` line 51

PenaltiesAddress is ImmutableArray<Address>? (nullable value type). When the block at stopNumber + 1 hasn't had its Penalties bytes populated (which is the case for all non-gap blocks), PenaltiesAddress returns null. Evaluating foreach (Address penalty in (ImmutableArray<Address>?)null) applies the implicit Nullable<T> → T conversion, which throws InvalidOperationException at runtime. The companion PenaltyHandler.cs already guards this correctly with if (header?.PenaltiesAddress is null) return [].

Inline comment posted. Fix: foreach (Address penalty in parentHeader.PenaltiesAddress ?? []).

Medium — Duplicate `IPenaltyHandler` registration in `XdcModule.cs`

AddSingleton<IPenaltyHandler, PenaltyHandler>() appears at both line 88 and line 108 of XdcModule.cs. In Autofac the last-wins rule means runtime behaviour is unaffected, but the first registration is dead code that obscures intent.

Inline comment posted.

Medium — `XdcSubnetRewardCalculator.GetRewardMasternodes` silently returns empty set when `ValidatorsAddress` is null

When a checkpoint header has no validators encoded, the override returns []. This silently produces an epoch with zero rewards and zero diagnostics — a condition that is almost certainly a bug (corrupted header, decode failure, wrong block) rather than a valid empty-committee epoch. The failing-fast or at minimum logging path should be preferred.

Inline comment posted.

Medium — Wrong `FindBlock` overload mocked in `SubnetPenaltyTests`

SigningTxCache.GetSigningTransactions calls _blockTree.FindBlock(hash, BlockTreeLookupOptions, long?) (three parameters). The test mock in CreateMockedContext configures FindBlock(Hash256, long) (two parameters). NSubstitute resolves by exact signature, so the three-parameter call falls through to NSubstitute's default (null return), causing SigningTxCache to throw and the tests that rely on signing-tx removal of penalties (TestSigningTxRemovesPenalty, etc.) may be testing a different code path than intended.

Inline comment posted.

Low — `TestHookRewardV2` is not `using` the `XdcTestBlockchain`

TestHookRewardV2SplitReward (line 301) instantiates XdcTestBlockchain chain without a using statement, while the adjacent TestHookRewardV2 test (line 156) correctly uses using. If XdcTestBlockchain implements IDisposable, this leaks resources across the [NonParallelizable] fixture.

Observations (no action required)

XdcRewardCalculatorSource unsealing: Making it internal class (non-sealed) to enable the XdcSubnetRewardCalculatorSource subclass is correct; the accessibility boundary keeps this from being a public API concern.
Burn-accumulation scope in GetSigningTxCount: Only blocks from epochHeader.Number - 1 down to the first epoch switch contribute to burnedInOneEpoch. This matches the intent (current epoch's burns for the minted-record contract) and the TestHookRewardAfterUpgrade test validates the accounting.
GetCandidatesByStakeForReward uses epochHeader (current checkpoint): The comment explains the Go-equivalent ordering rationale. This is intentional and well-documented.

claude · 2026-06-22T08:03:07Z

-            if (parentNumber == minBlockNumber + 1)
+            if (parentNumber == stopNumber + 1)
            {
                foreach (Address penalty in parentHeader.PenaltiesAddress)


High — Potential null dereference on PenaltiesAddress

PenaltiesAddress is ImmutableArray<Address>? (nullable struct). When C# evaluates foreach (T x in nullableStruct) on a null nullable value, it applies an implicit conversion from Nullable<T> → T, which throws InvalidOperationException if the value is null.

Blocks that are not gap/checkpoint blocks will have Penalties bytes as null, making PenaltiesAddress return null. If stopNumber + 1 does not correspond to a block with encoded penalties, this line will throw at runtime.

The fix from PenaltyHandler.cs (line 34) already shows the correct null-guard idiom:

foreach (Address penalty in parentHeader.PenaltiesAddress ?? []) { penalties.Add(penalty); }

Fix this →

Maybe better throw in this case?
We are expecting the field to be set when this is called.

claude · 2026-06-22T08:03:13Z

@@ -87,9 +87,6 @@ protected override void Load(ContainerBuilder builder)
            // penalty handler
            .AddSingleton<IPenaltyHandler, PenaltyHandler>()


Medium — Duplicate IPenaltyHandler registration

IPenaltyHandler is registered as PenaltyHandler twice: here at line 88 and again at line 108. In Autofac the last registration wins, so behaviour is correct at runtime, but the first registration is dead code and creates confusion about intent. One of the two should be removed.

claude · 2026-06-22T08:03:22Z

+        transactionProcessor,
+        rewardsStore)
+{
+    protected internal override HashSet<Address> GetRewardMasternodes(XdcBlockHeader checkpointHeader, IXdcReleaseSpec spec) =>


Medium — Silent empty-set on null validators

When ValidatorsAddress is null, this returns [], which means the entire epoch reward is silently zeroed. The base class implementation at the same override point throws or falls back to V1 extra-data parsing. For the subnet, an empty validator set is almost certainly a bug (corrupt header, missing decode, unexpected state). Consider throwing here — or at minimum logging a warning — so the issue surfaces during debugging rather than producing a chain with no rewards and no diagnostics.

protected internal override HashSet<Address> GetRewardMasternodes(XdcBlockHeader checkpointHeader, IXdcReleaseSpec spec) => checkpointHeader.ValidatorsAddress is { } validators ? [.. validators] : throw new InvalidOperationException( $"ValidatorsAddress is null on subnet checkpoint header {checkpointHeader.Number} ({checkpointHeader.Hash})");

claude · 2026-06-22T08:03:33Z

-        blockTree.FindHeader(Arg.Any<Hash256>(), Arg.Any<long>())
+        blockTree.FindHeader(Arg.Any<Hash256>(), Arg.Any<BlockTreeLookupOptions>(), Arg.Any<long>())
            .Returns(ci => hashToHeader.TryGetValue(ci.ArgAt<Hash256>(0), out XdcSubnetBlockHeader? h) ? h : null);
        blockTree.FindBlock(Arg.Any<Hash256>(), Arg.Any<long>())


Medium — Mock intercepts the wrong FindBlock overload

SigningTxCache.GetSigningTransactions calls _blockTree.FindBlock(blockHash, SigningTxLookupOptions, blockNumber) — the three-parameter overload Block? FindBlock(Hash256, BlockTreeLookupOptions, long?).

The mock configured here matches Block? FindBlock(Hash256, long?) (two parameters), which is a separate interface method. NSubstitute resolves by exact method signature, so the 3-parameter call made by SigningTxCache falls through to NSubstitute's default (returns null), causing SigningTxCache to throw InvalidOperationException("Expected block … to exist in block tree").

This means tests like TestSigningTxRemovesPenalty are not correctly exercising the code path they describe. Fix by also mocking the three-parameter overload:

blockTree.FindBlock(Arg.Any<Hash256>(), Arg.Any<BlockTreeLookupOptions>(), Arg.Any<long?>()) .Returns(ci => hashToBlock.TryGetValue(ci.ArgAt<Hash256>(0), out Block? block) ? block : null);

ak88 · 2026-06-24T08:38:31Z

-            if (parentNumber == minBlockNumber + 1)
+            if (parentNumber == stopNumber + 1)
            {
                foreach (Address penalty in parentHeader.PenaltiesAddress)


Maybe better throw in this case?
We are expecting the field to be set when this is called.

ak88 · 2026-06-24T08:40:36Z


    public void SetSigningTransactions(Hash256 blockHash, Transaction[] transactions) => _signingTxsCache.Set(blockHash, transactions);

+    public bool TryGetHeader(Hash256 blockHash, out XdcBlockHeader? header) => _headersCache.TryGet(blockHash, out header);


what is this for ?
Dont think you need this or the header cache.

ak88 · 2026-06-24T10:11:47Z

+        StoreSnapshotNumber(snapshot);
+    }
+
+    private TSnapshot? GetSnapshotByStoredGapNumber(long gapNumber)


What is the reason exactly for this method?
When fetching from BlockTree we have a certain guarantee that we are looking at the canonical header.
What will happen during a reorg? Can we end up looking at the wrong header?

@claude check this

cicr99 added 3 commits June 22, 2026 08:28

fix for subnet first gap snapshot

9a76dad

preserve signing headers during block processing

1cc9b29

fix subnet reward calculation

ef1e230

cicr99 requested a review from ak88 as a code owner June 22, 2026 07:54

github-actions Bot added the xdc label Jun 22, 2026

claude Bot reviewed Jun 22, 2026

View reviewed changes

ak88 reviewed Jun 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XDC - Fix subnet penalties and rewards#12080

XDC - Fix subnet penalties and rewards#12080
cicr99 wants to merge 3 commits into
masterfrom
fix/xdc-subnet-gap-snapshot

cicr99 commented Jun 22, 2026

Uh oh!

claude Bot commented Jun 22, 2026 •

edited

Loading

Uh oh!

claude Bot Jun 22, 2026

Uh oh!

ak88 Jun 24, 2026

Uh oh!

claude Bot Jun 22, 2026

Uh oh!

claude Bot Jun 22, 2026

Uh oh!

claude Bot Jun 22, 2026

Uh oh!

ak88 Jun 24, 2026

Uh oh!

ak88 Jun 24, 2026

Uh oh!

ak88 Jun 24, 2026

Uh oh!

ak88 Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -87,9 +87,6 @@ protected override void Load(ContainerBuilder builder)
		// penalty handler
		.AddSingleton<IPenaltyHandler, PenaltyHandler>()


		public void SetSigningTransactions(Hash256 blockHash, Transaction[] transactions) => _signingTxsCache.Set(blockHash, transactions);

		public bool TryGetHeader(Hash256 blockHash, out XdcBlockHeader? header) => _headersCache.TryGet(blockHash, out header);

Conversation

cicr99 commented Jun 22, 2026

Uh oh!

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

Uh oh!

PR Review: XDC - Fix subnet penalties and rewards

Summary

High — Null dereference in SubnetPenaltyHandler.cs line 51

Medium — Duplicate IPenaltyHandler registration in XdcModule.cs

Medium — XdcSubnetRewardCalculator.GetRewardMasternodes silently returns empty set when ValidatorsAddress is null

Medium — Wrong FindBlock overload mocked in SubnetPenaltyTests

Low — TestHookRewardV2 is not using the XdcTestBlockchain

Observations (no action required)

Uh oh!

claude Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude Bot commented Jun 22, 2026 •

edited

Loading

High — Null dereference in `SubnetPenaltyHandler.cs` line 51

Medium — Duplicate `IPenaltyHandler` registration in `XdcModule.cs`

Medium — `XdcSubnetRewardCalculator.GetRewardMasternodes` silently returns empty set when `ValidatorsAddress` is null

Medium — Wrong `FindBlock` overload mocked in `SubnetPenaltyTests`

Low — `TestHookRewardV2` is not `using` the `XdcTestBlockchain`