XDC Fix signing old blocks

[batrr]

Changes

XDC masternodes send a "sign" tx every MergeSignRange (15) blocks. This is triggered by SignTransactionManager.OnBlockAddedToMain, which runs for every block added to the chain — including old blocks processed while the node is
catching up. So a node that was offline for a while would send sign txs for lots of old blocks when it syncs back.

XDC Go client doesn't have this problem: only the head path signs, and catch-up runs through the downloader, which never signs. We could check "are we behind the peers' head?" using the sync layer, but pulling
that into the XDC sign logic isn't worth it.

So we check the block's age instead: only sign if the block's timestamp is within MergeSignRange * MinePeriod * 2 seconds of now, about the last 2 signing rounds (~30 blocks / ~1 minute on XDC).

Types of changes

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

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

[claude]

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

---

Review

• Load repo rules and context
• Read changed files
• Review SignTransactionManager.cs
• Review XdcConstants.cs
• Review test files
• Post findings

---

The fix is correct and well-scoped. The core logic — skip signing if block.Timestamp + window < now — is sound, the boundary is right (≤60 s signs, >60 s skips), and ITimestamper is properly injected via Autofac (NethermindModule registers it as a singleton). The check is placed after spec lookup but before the snapshot call, so stale blocks don't trigger the more expensive snapshot fetch.

No Critical / High / Medium findings. Three Low notes posted as inline comments:

#	Severity	Location	Issue
1	Low	XdcConstants.cs:65	Comment says "during sync" — IsSyncing() already handles active sync; this guards post-downtime restart backlog
2	Low	SignTransactionManager.cs:83	window > 0 guard is correct and load-bearing (prevents disabling signing when MinePeriod = 0) but undocumented
3	Low	SignTransactionManagerTests.cs:55	FindBestSuggestedHeader().Returns(header) is a no-op for an NSubstitute mock; the comment implies a behavioral dependency that doesn't exist

All three are nits — none block merge.

[claude]

Low — the comment says "during sync" but the IsSyncing() check on line 70 of SignTransactionManager already handles in-progress sync. This constant guards the subtler case: a node that was offline for a while restarts, IsSyncing() settles to false, but BlockAddedToMain fires for the trailing backlog of historical blocks. Suggest:

Suggested change

	public const int MaxSignableBlockPeriods = 2;
	// Guard against signing stale blocks replayed on restart (IsSyncing() handles active sync separately).
	public const int MaxSignableBlockPeriods = 2;

[claude]

Low — the window > 0 guard is correct and load-bearing: without it, window == 0 would make timestamp + 0 < now true for every past block, silently disabling signing entirely when MinePeriod or MergeSignRange is zero. Worth adding a brief comment to document this intent so a future reader doesn't simplify it away:

Suggested change

	if (window > 0 && (long)xdcHeader.Timestamp + window < _timestamper.UnixTime.SecondsLong)
	if (window > 0 && (long)xdcHeader.Timestamp + window < _timestamper.UnixTime.SecondsLong) // window==0 disables the guard

[claude]

Low — since blockTree is an NSubstitute mock, IsSyncing() returns default (i.e. isSyncing: false) without any explicit setup, so the FindBestSuggestedHeader().Returns(header) call doesn't actually influence the test outcome. The comment // bestSuggested == head => not syncing implies a behavioral dependency that doesn't exist for a mock. The line could be removed, or if you want to document the intent, add a comment explaining IsSyncing() returns false by default on a mock.

[batrr]

@claude re review

[claude]

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

---

Re-review

• Load repo rules
• Read changed files
• Review SignTransactionManager.cs
• Review XdcConstants.cs
• Review test files
• Post findings

---

The fix is correct and the approach is sound. The timestamp guard correctly identifies stale catch-up blocks, the window formula (15 × 2 × 2 = 60 s ≈ 2 signing rounds) matches the description, and ITimestamper is properly injected. The XdcTestBlockchain change (ctx.Resolve<ManualTimestamper>()) is the right fix — TestEnvironmentModule registers ManualTimestamper as a singleton, so this resolves the same instance used elsewhere in the test harness.

Correcting a prior review error: my previous comment on SignTransactionManagerTests.cs:55 wrongly said FindBestSuggestedHeader().Returns(header) was a no-op. It is load-bearing — IsSyncing() reads FindBestSuggestedHeader()?.Number ?? 0, and without the stub that returns 0, tripping the bestSuggestedNumber == 0 branch and marking the node as syncing. The test comment is accurate.

No Critical / High / Medium findings.

#	Severity	Location	Issue
1	Low	SignTransactionManager.cs:82	No window > 0 guard: if MinePeriod or MergeSignRange is ever 0, signing is silently disabled for all blocks. Not a live risk on XDC mainnet, but worth a one-line guard or a comment.
2	Low	SignTransactionManagerTests.cs:55	Correction to previous inline comment — the FindBestSuggestedHeader().Returns(header) stub is necessary, not a no-op.

Mergeable — both findings are nits; neither blocks merge.

[claude]

Low — window == 0 is a silent failure mode: if either MergeSignRange or MinePeriod is ever 0, timestamp + 0 < now is true for every past block and signing is silently disabled. Both are always positive on XDC mainnet so this is not a production risk, but worth a brief guard or a comment to protect against future misconfiguration:

Suggested change

	long window = spec.MergeSignRange * spec.MinePeriod * XdcConstants.MaxSignableBlockPeriods;
	// Sign only recent head blocks; older ones are replayed during catch-up.
	// window > 0 guard: if MinePeriod or MergeSignRange is 0, skip would match everything.
	long window = spec.MergeSignRange * spec.MinePeriod * XdcConstants.MaxSignableBlockPeriods;
	if (window > 0 && (long)xdcHeader.Timestamp + window < _timestamper.UnixTime.SecondsLong)
	return;

[claude]

Correction to previous review — the previous comment called this a no-op, but it is load-bearing. IBlockTree.IsSyncing() is a static extension method that calls blockTree.FindBestSuggestedHeader()?.Number ?? 0. Without this stub, the mock returns null, giving bestSuggestedNumber = 0, and isSyncing = (0 == 0) = true — all shouldSign = true cases would fail. The comment here is accurate; the stub should stay.

[ak88]

Think we can probably extend the window here. All headers in current epoch should be signable.