XDC subnet snapshot manager

[batrr]

Changes

• Introduce base and subnet snapshot managers

Relevant links
https://github.com/XinFinOrg/XDC-Subnet/blob/master/consensus/XDPoS/engines/engine_v2/snapshot.go#L14-L23
https://github.com/XinFinOrg/XDC-Subnet/blob/master/consensus/XDPoS/engines/engine_v2/engine.go#L1044
https://github.com/XinFinOrg/XDC-Subnet/blob/master/consensus/XDPoS/engines/engine_v2/engine.go#L520-L529

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

[Copilot]

Pull request overview

This PR introduces a refactoring to support subnet-specific snapshot management alongside the existing main chain snapshot management. It extracts common snapshot manager functionality into a generic base class and creates specialized implementations for main chain and subnet operations.

Changes:

• Refactored SnapshotManager to inherit from a new generic BaseSnapshotManager<TSnapshot> base class, extracting common snapshot storage, retrieval, and caching logic
• Added SubnetSnapshotManager which stores penalties in snapshots (calculated at snapshot creation time) rather than computing them dynamically
• Introduced SubnetEpochSwitchManager with an overridden penalty resolution method that retrieves penalties from SubnetSnapshot instances
• Modified EpochSwitchManager to add a virtual ResolvePenalties method, enabling subnet-specific penalty handling while maintaining backward compatibility

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
BaseSnapshotManager.cs	New generic base class containing common snapshot management logic including caching, database operations, and the NewHeadBlock event handler
SubnetSnapshotManager.cs	Subnet-specific snapshot manager that stores penalties in snapshots at creation time
SnapshotManager.cs	Refactored to inherit from BaseSnapshotManager, preserving existing functionality for main chain
SubnetEpochSwitchManager.cs	Subnet-specific epoch switch manager that retrieves penalties from SubnetSnapshot
EpochSwitchManager.cs	Added virtual ResolvePenalties method to support different penalty resolution strategies
SubnetSnapshotManagerTests.cs	Test suite for SubnetSnapshotManager covering basic snapshot operations

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[batrr]

@claude review

[claude]

Claude finished @batrr's task in 3m 10s —— View job

---

Code Review: XDC Subnet Snapshot Manager

• Read repository rules (coding-style, di-patterns, test-infrastructure, robustness)
• Read changed files
• Analyze code and formulate review
• Post review feedback

---

Overview

The PR cleanly extracts common snapshot logic into BaseSnapshotManager<TSnapshot> and adds a SubnetSnapshotManager that stores pre-computed penalties inside each snapshot. The overall structure is sound; my findings are below in rough priority order.

---

Issues

1. Event handler subscription leak — BaseSnapshotManager.cs:39

_blockTree.BlockAddedToMain += OnBlockAddedToMain;

The subscription is never unsubscribed. BaseSnapshotManager will be kept alive by the block tree's event handler table for the entire node lifetime, and if the manager is ever replaced or garbage-collected the event will fire on a dead object. The class should implement IDisposable and unsubscribe in Dispose():

public void Dispose() => _blockTree.BlockAddedToMain -= OnBlockAddedToMain;

This pattern was already present in the pre-refactor SnapshotManager, but the refactor is a good opportunity to fix it. Fix this →

2. Silent behavior change in GetXdcSpec call — BaseSnapshotManager.cs:119

The original SnapshotManager.UpdateMasterNodes called:

IXdcReleaseSpec spec = specProvider.GetXdcSpec(header, round);

The refactored base drops round:

IXdcReleaseSpec spec = _specProvider.GetXdcSpec(header);

The old code even carried a comment: "Could consider dropping the round parameter here, since the consensus parameters used here should not change". The comment has been removed in the refactor, and the round-less overload is now used, which is fine — but both these facts should be confirmed: (a) the overload actually resolves via block number not round, and (b) ApplyV2Config is only used with round 0 in XdcChainSpecBasedSpecProvider.CreateReleaseSpec. If that assumption breaks in a future V2 config that activates at a specific round, this will silently stop working. A brief comment explaining why round is intentionally omitted here would prevent future regressions.

3. Redundant intermediate variable — BaseSnapshotManager.cs:86–88

TSnapshot decoded = _snapshotDecoder.Decode(value);
snapshot = decoded;

decoded is used only once immediately after assignment. Simplify to:

snapshot = _snapshotDecoder.Decode(value);

Fix this →

4. SwitchBlock + 1 special case not handled in SubnetSnapshotManager — SubnetSnapshotManager.cs:29–35

MasternodesCalculator.CalculateNextEpochMasternodes has an explicit guard:

if (blockNumber == spec.SwitchBlock + 1)
{
    // no penalty filtering, just cap
    return (candidates, []);
}

SubnetSnapshotManager.CreateSnapshot has no equivalent. Either this is intentional (subnet semantics differ at SwitchBlock + 1) or it's an oversight. Either way a comment is needed; if it's an oversight, the guard should be added. Fix this →

5. Genesis block receives penalty computation — SubnetSnapshotManager.cs:32

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

This runs even when header.IsGenesis == true (i.e., header.ParentHash is the zero hash). Whether invoking HandlePenalties on the genesis block is meaningful depends on the implementation, but it's surprising and should be gated or documented.

6. Incomplete test method name — SubnetSnapshotManagerTests.cs:82

public void NewHeadBlock_(int gapNumber)

The trailing underscore suggests an incomplete rename. A descriptive name like NewHeadBlock_ShouldCreateSubnetSnapshotWithPenalties matches the naming convention used in other test methods in this file. Fix this →

7. Missing test coverage

• Penalty filtering and max-cap enforcement: CreateSnapshot filters penalties and (should) enforce MaxMasternodes. Neither path has a test case.
• Genesis snapshot creation: No test verifies what happens when header.IsGenesis == true.
• StoreSnapshot idempotency: The skip-if-key-exists guard in BaseSnapshotManager.StoreSnapshot:102 has no test.

---

Nits

• SubnetSnapshot.cs:20 — penalties ?? [] uses a null-check on a non-nullable Address[]. Per the coding style, trust null annotations; drop the ?? [].
• BaseSnapshotDecoder.cs:19 — return null from a non-nullable generic method. The caller in BaseSnapshotManager already handles null via the type-test result, but this will produce a compiler warning and can be made explicit with return null! or a nullable annotation.

---

Summary

The refactoring is well-structured and the base-class extraction is clean. The blocking items before merge are: the SwitchBlock + 1 handling (intentional or needs the guard), the missing IDisposable / event unsubscription, and the incomplete test name. The other points are lower-priority but worth addressing before shipping.

[alexb5dh]

All comments are minor