Start a round if we are at genesis

[ak88]

Fixes a special case where we are the very first proposer in XDC, and we would otherwise not propose anything.
Also make sure that we start a timeout timer.

[claude]

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

---

Code Review: Start a round if we are at genesis

• Read PR diff and changed files
• Analyze XdcHotStuff.Start() / IsBootstrapFirstProposer() logic
• Verify bootstrap state against QuorumCertificateManager.Initialize
• Review test coverage
• Post review

---

Summary

A clean, well-scoped fix. The previous guard in Start() (IsSynced() && CurrentRound != 0) prevented a freshly-bootstrapped node from ever proposing the first block, because at genesis IsBlockTree.IsSyncing() is still true. The new IsBootstrapFirstProposer() carve-out correctly recognizes the one-shot bootstrap state and lets the round-1 leader propose.

I verified the bootstrap state matches what QuorumCertificateManager.Initialize produces on a fresh chain: HighestQC = round-0 QC pointing at the head, and SetNewRound(1). The conditions in IsBootstrapFirstProposer (CurrentRound == 1, qc.Round == 0, qc.BlockNumber == head.Number, qc.Hash == head.Hash, masternodes present, IsMyTurn) line up exactly with that state.

I also traced the resulting RunRound path at genesis: Vote is correctly skipped (spec.SwitchBlock < head.Number is false at block 0, avoiding the InvalidOperationException for genesis having no QC data), both mine-period and QC-fallback gates short-circuit, and BuildAndProposeBlock fires. The non-genesis synced path is byte-for-byte unchanged, so no regression there.

No Critical / High / Medium issues found. Good test coverage with both a positive (leader proposes) and negative (non-leader stays quiet) case.

Findings (all Low)

1. Doc/scope of IsBootstrapFirstProposer (Low). The summary says "genesis", but Initialize produces the identical state at the V2 SwitchBlock too, so this also returns true at consensus activation. That is arguably the desired behavior (first V2 proposer), but the naming/comment implies genesis-only. Consider broadening the wording. (inline)

2. Null-check consistency (Low). Line 411 dereferences _xdcContext.HighestQC.ProposedBlockInfo directly, while line 214 defensively null-checks HighestQC. HighestQC is non-nullable and constructor-initialized, so no NRE in practice — just a style inconsistency. (inline)

3. Negative test timing (Low). TestStartDoesNotProposeFirstBlockWhenNotLeaderWhileSyncingAtGenesis asserts no proposal after a fixed await Task.Delay(500). Fixed-delay negative assertions can be a minor source of flakiness/slowness under CI load, though acceptable here. No change required.

One thing worth a sanity check (not a blocker): because the trigger only fires once in Start() and OnNewHeadBlock/OnNewRound remain gated on IsSynced(), please confirm that after the bootstrap block is produced the node reliably transitions to "synced" so subsequent rounds continue — i.e. that this genuinely bootstraps an ongoing chain rather than producing only block #1.
· branch fix/xdc-first-proposer

[claude]

Minor: the doc summary says "freshly bootstrapped chain where IsSynced is false only because genesis counts as syncing", but QuorumCertificateManager.Initialize produces the identical state (HighestQC = round-0 QC pointing at head, SetNewRound(1)) for both genesis and the V2 SwitchBlock. So this method also returns true at the consensus activation block, not only at genesis. That's arguably correct (first V2 proposer), but the summary/IsBootstrapFirstProposer naming implies genesis-only — consider broadening the wording to avoid future confusion.

Also a small consistency nit: line 411 dereferences _xdcContext.HighestQC.ProposedBlockInfo directly, whereas the rest of the class (e.g. line 214) defensively null-checks HighestQC before use. HighestQC is non-nullable and always constructor-initialized, so this won't NRE in practice — just inconsistent with the surrounding defensive style.

[ak88]

@claude again

[claude]

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

---

Re-review: Start a round if we are at genesis

• Fetch latest changes & diff vs origin/master
• Review XdcHotStuff.cs changes
• Check whether previous review points were addressed
• Review tests (MineModuleTests) and the new basefee / timeout changes

---

Since my last review (06-26) the branch grew three more commits: check basefee, more timeout logs, and a discv4 log-spam/spelling tidy. The core genesis-proposer logic is unchanged from what I already verified, so this pass focuses on what's new.

Core change — still correct ✅

Start() / IsBootstrapFirstProposer() (XdcHotStuff.cs:81-93, 406-418) is byte-for-byte the logic I traced previously: the round-1 leader on a freshly-bootstrapped chain proposes block #1 even though IsSynced() is false (genesis counts as syncing), while every other path stays gated on IsSynced(). No regression in the non-bootstrap synced path. Test coverage (positive + non-leader negative) is solid.

New: XdcBaseFeeCalculator (XdcBaseFeeCalculator.cs:16)

public UInt256 Calculate(BlockHeader parent, IEip1559Spec specFor1559) =>
    specFor1559.IsEip1559Enabled ? BaseFee : UInt256.Zero;

Returning 0 before EIP-1559 activation (rather than the constant 12.5 gwei) is the correct semantics for pre-fork blocks and is relevant to producing the very first block. Two observations (both Low):

• No regression test. This is a consensus-affecting value with a new branch and currently has no direct test. A two-case [TestCase] over XdcBaseFeeCalculator.Calculate (1559 on → BaseFee, off → 0) would lock it in cheaply and satisfies the repo's "always add a regression test when fixing a bug" rule.
• Worth confirming the XDC genesis/header validation expects BaseFeePerGas == 0 pre-fork — if any header builder or validator still assumes the constant, the two must agree.

New: TimeoutCertificateManager (TimeoutCertificateManager.cs)

Mostly added logging. The one behavioral change — capturing currentRound = _consensusContext.CurrentRound once and threading it into SendTimeout(currentRound) and the spec lookup — is a small improvement: it removes a re-read of the mutable CurrentRound property between sending the timeout and computing the spec, so the message and the spec can't disagree if the round advances mid-call. Good.

Previous Low nits — not yet addressed (still optional)

1. Doc/scope wording. The <summary> on IsBootstrapFirstProposer (XdcHotStuff.cs:402-405) and the log message still say "genesis", but QuorumCertificateManager.Initialize produces the identical state at the V2 SwitchBlock too, so this also fires at consensus activation. Behavior is arguably correct; the naming just implies genesis-only.
2. Null-check consistency. XdcHotStuff.cs:411 dereferences _xdcContext.HighestQC.ProposedBlockInfo directly while line 214 null-checks HighestQC. Non-nullable + constructor-initialized, so no NRE — pure style.

Verdict

No Critical / High / Medium issues. The core fix is sound and well-tested. The only thing I'd genuinely encourage before merge is a small regression test for the XdcBaseFeeCalculator branch. Add basefee regression test →

Also still worth a one-line confirmation (from my earlier review): after block #1 is produced via the bootstrap path, the node reliably flips to "synced" so OnNewHeadBlock/OnNewRound keep driving subsequent rounds — i.e. this bootstraps an ongoing chain, not just block #1.
· branch fix/xdc-first-proposer

[ak88]

@claude

[claude]

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

---

Re-review: Start a round if we are at genesis

• Fetch latest diff vs origin/master
• Review new commits since last review (basefee regression test, timeout reset)
• Verify previously raised nits status
• Post review

---

Since the last pass (06-30) the branch added two commits: make sure timeout is reset at genesis and synced (14a049a) and regression test XdcBaseFeeCalculatorTests (f08b99d). The core genesis-proposer logic is unchanged from what I verified in the previous two rounds, so this focuses on the new deltas.

✅ Basefee regression test added

The XdcBaseFeeCalculator.Calculate branch I asked to lock in now has a parameterized test (XdcBaseFeeCalculatorTests.cs:19-35) covering both IsEip1559Enabled on → BaseFee and off → 0. That closes the one item I'd genuinely encouraged before merge. 👍

✅ ResetTimeout refactor — correct

Start() now calls ResetTimeout(head, CurrentRound) after StartRoundTask, and OnNewRound was refactored to reuse the same helper (XdcHotStuff.cs:91, 156, 161-165). The extracted ResetTimeout is byte-for-byte the inline code it replaced, so OnNewRound is unchanged. The genuinely new behavior is that the timeout timer is now armed at startup (for both the synced path and the genesis-bootstrap path) rather than only after the first NewRound/NewHeadBlock event — a sensible fix so a node that starts up idle can still time out and drive round progression if the current leader is silent. No regression.

Spec/config changes (93e27f6, from prior review scope)

The nullable-with-?? ulong.MaxValue conversions for BlackListHFNumber, TipXDCX, TIPXDCXMinerDisable, DynamicGasLimitBlock (XdcChainSpecBasedSpecProvider.cs:78-83) change the absent-value default from "enabled at genesis" to "never enabled", which is the safer default and is now consistent with the surrounding TipUpgrade* handling. On the real XDC mainnet/testnet chains the values are explicitly set in the JSONs, so live behavior is config-driven. Looks intentional and correct.

Findings

Low — IsTIPXDCXReceiver is set but never read. XdcChainSpecBasedSpecProvider.cs:82 computes and assigns releaseSpec.IsTIPXDCXReceiver, and XdcReleaseSpec.cs:56 declares it, but nothing consumes it (the interface IXdcReleaseSpec doesn't even declare it, and no code reads it — cf. IsTIPXDCXMiner, which is read in XdcExtensions.Transactions.cs). It's harmless dead state today; if it's preparatory for receiver-side XDCX-disable logic, fine — otherwise consider dropping it until a consumer exists.

Low (still open, optional) — doc wording says "genesis". The <summary> on IsBootstrapFirstProposer (XdcHotStuff.cs:408-411) still says genesis-only, but the same bootstrap state also arises at the V2 SwitchBlock, so this also fires at consensus activation. Behavior is arguably desirable; just the naming/comment implies genesis-only.

Low (still open, optional) — null-check consistency. XdcHotStuff.cs:417 dereferences _xdcContext.HighestQC.ProposedBlockInfo directly while line 214 null-checks HighestQC. Non-nullable + constructor-initialized, so no NRE — pure style.

One thing still worth a one-line confirmation

Same as prior passes: after block #1 is produced via the bootstrap path (which only fires once in Start()), the node must reliably flip to IsSynced() == true so OnNewHeadBlock/OnNewRound keep driving subsequent rounds — i.e. this bootstraps an ongoing chain, not just block #1. Not a blocker, just confirm it holds.

Verdict

No Critical / High / Medium issues. The core fix is sound and now has the basefee regression test I'd asked for; the ResetTimeout change is a clean improvement. Remaining items are all Low/optional. Good to merge.
· branch fix/xdc-first-proposer

[claude]

Claude finished @ak88's task in 1m 18s —— View job

---

Re-review: Start a round if we are at genesis

• Fetch latest changes & diff vs origin/master
• Review new commits since last pass
• Confirm previous review points
• Post review

Since my last pass (06-30) two commits landed: make sure timeout is reset at genesis and synced (14a049a) and regression test XdcBaseFeeCalculatorTests (f08b99d). Both directly address the two follow-ups I raised.

✅ XdcBaseFeeCalculator regression test — added

XdcBaseFeeCalculatorTests.cs parameterizes exactly the branch I flagged: IsEip1559Enabled == true → BaseFee (12.5 gwei), false → 0. Clean, uses [TestCase] as the repo guidelines prefer, and locks in the consensus-relevant pre-fork value. 👍

✅ Timeout reset at genesis/synced — addresses my open question

This is the answer to the "does it keep going after block #1?" concern from my earlier reviews. The old Start() only called StartRoundTask; now it also calls the new ResetTimeout(head, CurrentRound) (XdcHotStuff.cs:90), and OnNewRound was refactored to reuse the same helper (XdcHotStuff.cs:156, 162-166). So on the bootstrap path the timeout timer is now armed too — if the round-1 proposal doesn't land, the timeout mechanism drives the round forward rather than the node stalling on block #1. Good de-duplication as well (single ResetTimeout helper instead of the inlined reset in two places).

One thing worth a quick sanity check (not a blocker): Start() now resets the timeout in the synced path too, which it didn't before. That's the intended widening per the commit message, and it looks correct — but please confirm re-arming the timeout on every Start() (e.g. a restart mid-chain) can't shorten/lengthen an in-flight round in a way the previous code deliberately avoided.

Core genesis-proposer logic — still correct

Start() / IsBootstrapFirstProposer() (XdcHotStuff.cs:81-93, 406-421) is unchanged from what I traced before: the round-1 leader on a freshly-bootstrapped chain proposes even though IsSynced() is false, all other paths stay gated on IsSynced().

Remaining Low nits (still optional, unchanged)

1. Doc/scope wording — the <summary> and log message still say "genesis", but QuorumCertificateManager.Initialize produces the identical state at the V2 SwitchBlock, so IsBootstrapFirstProposer() also returns true at consensus activation. Behavior is arguably correct; the wording just implies genesis-only.
2. Null-check consistency — XdcHotStuff.cs:411 dereferences _xdcContext.HighestQC.ProposedBlockInfo directly while other sites null-check HighestQC. Non-nullable + constructor-initialized, so no NRE — pure style.

Verdict

No Critical / High / Medium issues. Both follow-ups from my prior reviews are resolved and the fix is well-tested. Looks good to merge (the two Low nits are cosmetic and can be left as-is).
· branch fix/xdc-first-proposer

[ak88]

@claude

[claude]

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

---

Re-review: Start a round if we are at genesis

• Fetch latest diff vs origin/master
• Review new commit adbdde4 use XDCXReceiver correctly
• Confirm prior findings status
• Post review

Since the last pass (07-01) one new commit landed: adbdde4 use XDCXReceiver correctly. It addresses my earlier "IsTIPXDCXReceiver set but never read" Low finding by (a) adding IsTIPXDCXReceiver to the IXdcReleaseSpec interface and (b) switching the four XDCx trading/lending detectors in XdcExtensions.Transactions.cs from IsTIPXDCXMiner → IsTIPXDCXReceiver. This focuses on that commit.

🔴 Medium — a special-tx test still sets the old flag and will now fail

RequiresSpecialHandling → IsTradingTransaction / IsLendingTransaction / IsLendingFinalizedTradeTransaction / IsTradingStateTransaction now gate on spec.IsTIPXDCXReceiver (XdcExtensions.Transactions.cs:21-24). But SpecialTransactionsTests.Valid_SpecialTx_NotSign_Call_EmptyTx_Handler still enables the feature via the old flag:

// SpecialTransactionsTests.cs:771
spec.IsTIPXDCXMiner = true;   // IsTIPXDCXReceiver left at its default: false

That test sends txs to XDCXAddressBinary, XDCXLendingAddressBinary, TradingStateAddressBinary, XDCXLendingFinalizedTradeAddressBinary and asserts they take the special-handling path:

• finalNonce == initialNonce (special handling doesn't bump the nonce — IncrementNonce returns Ok early, XdcTransactionProcessor.cs:121-140)
• exactly one synthetic log at Logs[0].Address == address

With IsTIPXDCXReceiver == false, RequiresSpecialHandling now returns false for all four addresses, so base.Execute runs them as ordinary legacy txs → the nonce is incremented and no synthetic log is emitted. Both assertions break. This test isn't in the PR's changed-files, so it wasn't updated alongside the production change.

Fix: set the receiver flag in the test setup (keep the miner flag too if you want to prove they're independent):

spec.IsTIPXDCXMiner = true;
spec.IsTIPXDCXReceiver = true;

Fix this →

I couldn't run the test locally (test/build execution is blocked in this job), so please confirm on CI — but the code paths above make the failure deterministic.

🟡 Low — the unused flag just swapped identity

Before this commit IsTIPXDCXReceiver was set-but-never-read; now IsTIPXDCXMiner is (it's assigned in XdcChainSpecBasedSpecProvider.cs:81 and declared in XdcReleaseSpec.cs, but no production code reads it). If the miner flag is preparatory for future order-matching/block-production logic that's fine — otherwise the net effect is one dead flag traded for another. Worth a one-line confirmation that the receiver window (not the miner window) is the correct gate for tx acceptance/processing, given they differ on mainnet (TIPXDCXMinerDisable = 80370000 vs TIPXDCXReceiverDisable = 80370900, a 900-block window where receiver stays enabled after miner disables).

Core genesis-proposer logic — unchanged & still correct ✅

Start() / IsBootstrapFirstProposer() and the ResetTimeout refactor / basefee regression test are unchanged from the versions I verified in the prior passes. No new concerns there.

Verdict

One Medium: the new receiver-flag change breaks Valid_SpecialTx_NotSign_Call_EmptyTx_Handler because its setup still sets IsTIPXDCXMiner. Update the test to set IsTIPXDCXReceiver = true. Everything else is unchanged and good; the Low note about the swapped unused flag is optional.
· branch fix/xdc-first-proposer

[ak88]

@claude again

[claude]

Claude finished @ak88's task in 1m 15s —— View job

---

Re-review: Start a round if we are at genesis

• Fetch latest diff vs origin/master
• Review new commits since last pass
• Verify prior Medium finding (SpecialTransactionsTests) status
• Post review

Since the last pass (07-01 14:09, at adbdde4) three commits landed: 44d299b reduce log spam, 6606e02 ensure timestamp and wait period is correct, and c951bcc fix test. This focuses on those deltas.

✅ Prior Medium resolved — SpecialTransactionsTests now sets the receiver flag

c951bcc flips the test setup from IsTIPXDCXMiner = true → IsTIPXDCXReceiver = true (SpecialTransactionsTests.cs:771), which matches the production gate the earlier commit moved to (RequiresSpecialHandling → IsTIPXDCXReceiver). Valid_SpecialTx_NotSign_Call_EmptyTx_Handler will now recognize the trading/lending txs as special-handling again (nonce not bumped, synthetic log emitted). That closes the Medium I raised last pass. 👍

✅ ensure timestamp and wait period is correct — correct

Two changes in XdcHotStuff.cs:

1. Timestamp floor (XdcHotStuff.cs:272). The proposed block's timestamp is now Math.Max(now, parent.Timestamp + spec.MinePeriod) instead of just now. This is the right fix: header validation requires timestamp >= parent.Timestamp + MinePeriod, and at genesis (or whenever the Gate-1 wall-clock delay is skipped/truncated) now alone could fall below that floor and produce an invalid header. Because parent/spec here are the same proposalParent/proposalSpec used by the Gate-1 delay (:237), the floor is consistent with the wait — in steady state now wins, and the Max only bites in the bootstrap edge. Good.

2. ulong→TimeSpan refactor for the two delay gates (:236-252). Purely mechanical — the underflow-prone ulong - ulong arithmetic is replaced by TimeSpan subtraction, still guarded by the if (mineReadyAt > now) / if (fallbackReadyAt > now) checks, so no negative-delay risk. TimeSpan.FromSeconds(double) handles the range of realistic Unix timestamps without precision loss. Behavior-equivalent, slightly cleaner. No regression.

✅ reduce log spam — fine

RandomWalkKademliaDiscovery.cs: two Debug lookup log lines dropped to Trace. Cosmetic, no behavioral effect.

Core genesis-proposer logic — unchanged & still correct

Start() / IsBootstrapFirstProposer() and the ResetTimeout refactor / basefee regression test are byte-for-byte what I verified in prior passes.

Remaining Low nits (still open, optional — unchanged)

1. Doc wording says "genesis". IsBootstrapFirstProposer's <summary> still implies genesis-only, but the identical bootstrap state also arises at the V2 SwitchBlock, so it also fires at consensus activation. Behavior arguably correct; wording is narrow.
2. Null-check consistency. XdcHotStuff.cs dereferences _xdcContext.HighestQC.ProposedBlockInfo directly in one spot while null-checking HighestQC elsewhere. Non-nullable + constructor-initialized, so no NRE — pure style.

Verdict

No Critical / High / Medium issues. The prior Medium (test on the old flag) is fixed, and the timestamp-floor change is a genuine correctness improvement for the first-block path. Remaining items are the same cosmetic Lows. Good to merge.
· branch fix/xdc-first-proposer

[cicr99]

nit: we could cache IsSynced() in a local (like bootstrapFirstProposer) and simplify the inner condition to if (!isSynced), unless the second call was intentional. IsSynced() reads live block-tree state, so it could theoretically flip between the outer and inner checks.