XDC Fix mutable V2 config spec

[batrr]

Changes

• Fix mutable spec bug in XdcChainSpecBasedSpecProvider: previously GetXdcSpec called ApplyV2Config directly on the shared spec instance returned by GetSpec, so fetching with round Y would overwrite the V2 fields of a spec
  previously fetched with round X
• Introduce per-(blockIndex, roundIndex) spec cache GetXdcSpec now clones the base spec and caches an isolated copy per block transition × V2 config bucket, so concurrent or sequential calls with different rounds never mutate each
  other

Types of changes

• Bugfix (a non-breaking change that fixes an issue)
• Refactoring

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

[claude]

Claude finished @batrr's task in 6m 27s —— View job

---

PR Review: XDC Fix mutable V2 config spec

• Loading rules and context
• Reading changed files
• Analyzing correctness, thread safety, and performance
• Posting findings

---

Summary

The fix is conceptually correct and directly addresses the root cause: ApplyV2Config was mutating the shared XdcReleaseSpec instances stored in _blockTransitions, so any caller fetching a spec with round Y would silently corrupt the V2Config fields of specs fetched with round X.

The solution — cloning per (blockIndex, roundIndex) bucket and caching isolated copies in a ConcurrentDictionary — is sound. ApplyV2Config mutates only value-type fields (SwitchRound, MaxMasternodes, CertificateThreshold, TimeoutSyncThreshold, TimeoutPeriod, MinePeriod), all of which are properly isolated by MemberwiseClone. Thread safety is acceptable: _blockTransitions is written once in the constructor and only read afterwards; the ConcurrentDictionary handles concurrent cache access correctly.

---

Findings

Low — Shallow clone shares mutable reference-type collections

ReleaseSpec.Clone() uses MemberwiseClone() (line 186 of ReleaseSpec.cs), which is a shallow copy. All cached XdcReleaseSpec instances for a given block transition share references to the same V2Configs (List<V2ConfigParams>), BlackListedAddresses (HashSet<Address>), and GenesisMasterNodes (Address[]).

Currently no code path calls .Add() / .Remove() on these collections via a cached spec, so this is not an active bug. However, IXdcReleaseSpec exposes BlackListedAddresses with a public setter and it's a mutable HashSet<Address> — a future caller doing spec.BlackListedAddresses.Add(addr) (rather than assigning a new set) would silently corrupt every cached spec sharing that collection.

Fix this →

Low — Test bypasses CheckConfig validation via collection-initializer syntax

In XdcSpecProviderTests.cs:100-105:

V2Configs =
{
    new() { SwitchRound = 0 },
    new() { SwitchRound = 10 },
}

This uses the C# nested collection initializer, which calls V2Configs.Add(...) on the existing default list rather than invoking the property setter. As a result, CheckConfig and the sort step inside the setter are never executed. The test passes only because the items happen to be in sorted order and start with SwitchRound = 0. If a future test change adds items in wrong order, the missing validation won't catch it.

Use V2Configs = new() { new() {...}, new() {...} } or assign via a local variable to invoke the setter.

Fix this →

Low — Missing cache-hit (reference-equality) test

GetXdcSpec_ReturnsDifferentSpecInstances verifies that two different (block, round) pairs produce isolated specs. It does not verify that the same (block, round) pair returns the same cached instance on repeated calls. A ReferenceEquals assertion would catch regressions if caching is accidentally broken.

Low — GetBlockTransitionIndex/GetRoundConfigIndex silently return -1 on degenerate inputs

Both helpers return ~result - 1, which evaluates to -1 when the search key falls below the first element or the list is empty. If someone calls GetXdcSpec with blockNumber < 0 or with an empty V2Configs (bypassing CheckConfig), the -1 result propagates into _blockTransitions[-1] (IndexOutOfRangeException) or GetConfigAtRound([], round) (same). Adding a guard — or at minimum an ArgumentOutOfRangeException — would surface these issues with a clearer error.

---

Overall

No critical or high issues. The core fix is correct, the caching strategy is appropriate, and the test covers the main regression scenario. Addressing the shallow-clone note (even with a comment documenting the invariant that these collections must not be mutated in-place) would make the design intent clearer for future contributors.

[claude]

Claude finished @batrr's task in 4m 0s —— View job

---

PR Review: XDC Fix mutable V2 config spec

• Loading rules and context
• Reading changed files
• Analyzing correctness, thread safety, and performance
• Posting findings

---

Summary

The fix is correct and directly addresses the root cause: ApplyV2Config was mutating the shared XdcReleaseSpec instance stored in _blockTransitions, so any caller fetching a spec with round Y would silently overwrite V2 fields on specs previously fetched with round X.

The solution — cloning per (blockIndex, roundIndex) bucket and caching isolated copies in a ConcurrentDictionary — is sound. ApplyV2Config mutates only value-type fields (SwitchRound, MaxMasternodes, CertificateThreshold, TimeoutSyncThreshold, TimeoutPeriod, MinePeriod), all of which are isolated by MemberwiseClone. Thread safety is acceptable: _blockTransitions is written once in the base constructor and only read thereafter; ConcurrentDictionary handles concurrent cache access correctly.

The GetRoundConfigIndex + cache key design is correct: all rounds in the same V2Config bucket share one cache entry, and ApplyV2Config(round) always resolves to the same V2ConfigParams for any round in that bucket.

---

Findings

Low — Shallow clone shares mutable reference-type collections

(inline comment posted on XdcChainSpecBasedSpecProvider.cs:38)

ReleaseSpec.Clone() uses MemberwiseClone() — a shallow copy. All cached XdcReleaseSpec instances for a given block transition share the same V2Configs (List<V2ConfigParams>), BlackListedAddresses (HashSet<Address>), and GenesisMasterNodes (Address[]). ApplyV2Config only mutates value-type fields so there is no active bug. However, IXdcReleaseSpec exposes BlackListedAddresses with a public setter — a future in-place .Add(addr) call would silently corrupt all cached specs. At minimum add a comment documenting this invariant.

Low — Test collection initializer bypasses CheckConfig

(inline comment posted on XdcSpecProviderTests.cs:105)

V2Configs = { new() {...}, new() {...} } (without new List<> { ... }) calls .Add(...) on the existing backing field rather than the property setter. CheckConfig and the sort step are never exercised. The test passes only because items happen to be valid and sorted. Using V2Configs = new List<V2ConfigParams> { ... } would trigger the setter and provide true regression coverage.

Low — Missing cache-hit (reference-equality) test

GetXdcSpec_ReturnsDifferentSpecInstances verifies that two different (block, round) pairs produce isolated specs. It does not verify that repeated calls with the same (block, round) return the same cached instance (ReferenceEquals). A ReferenceEquals assertion would catch regressions if caching is accidentally broken.

Low — Index helpers silently return -1 on degenerate inputs

Both GetBlockTransitionIndex and GetRoundConfigIndex return ~result - 1 on a miss, which evaluates to -1 when the search key falls below the smallest element. With valid inputs (block numbers ≥ 0, V2Configs[0].SwitchRound == 0 enforced by CheckConfig) this cannot occur in practice. Adding a guard or ArgumentOutOfRangeException would surface the issue clearly if invariants are ever violated.

---

Overall

No critical or high issues. The core fix is correct, the caching strategy is appropriate, and the test covers the primary regression scenario. All findings are Low. The PR is mergeable.

[claude]

Low — shallow clone, shared mutable collections

MemberwiseClone() copies reference-type fields by reference. All cached XdcReleaseSpec instances for a given block transition share the same V2Configs (List<V2ConfigParams>), BlackListedAddresses (HashSet<Address>), and GenesisMasterNodes (Address[]).

ApplyV2Config only mutates value-type fields (SwitchRound, MaxMasternodes, etc.), so there is no active bug today. However, IXdcReleaseSpec exposes BlackListedAddresses with a public setter, and GetConfigAtRound reads V2Configs from the spec — any future in-place mutation (e.g., spec.BlackListedAddresses.Add(...)) would silently corrupt all cached specs sharing that collection.

At minimum, document the invariant with a comment; better, deep-copy the mutable collections after the clone:

Suggested change

	XdcReleaseSpec copy = (XdcReleaseSpec)baseSpec.Clone();
	XdcReleaseSpec copy = (XdcReleaseSpec)baseSpec.Clone();
	// V2Configs, BlackListedAddresses, and GenesisMasterNodes are shared with the base spec.
	// Callers must not mutate these collections in-place — only assign new instances.

Fix this →

[claude]

Low — collection initializer syntax bypasses CheckConfig

V2Configs =
{
    new() { SwitchRound = 0 },
    new() { SwitchRound = 10 },
}

This uses C#'s nested collection initializer, which calls .Add(...) on the existing backing list (_v2Configs = new()) rather than invoking the property setter. CheckConfig and the sort step inside the setter are never executed. The test passes only because the items are already in sorted order and start with SwitchRound = 0.

Use a property setter assignment to exercise the same validation path the production code goes through:

Suggested change

	}
	V2Configs = new List<V2ConfigParams>
	{
	new() { SwitchRound = 0 },
	new() { SwitchRound = 10 },
	},

Fix this →

[cicr99]

CI is not successful yet, but other than that looks good to me