Repository navigation
Conversation
On a fresh chain nothing has initialised the v2 engine yet, so the first proposal initialises it lazily through verifyHeader while the bft goroutine concurrently verifies the votes for that same block. VerifyVoteMessage takes no lock, so it can reach getSnapshot before initial() has called StoreSnapshot for the V2 switch gap block. The lookup misses, every vote for block 1 is dropped, no QC ever forms, and the chain stalls at height 1 with an empty highestQuorumCert -- a state restarting cannot repair, because block 1 is by then committed without a QC and is never re-proposed. getSnapshot now rebuilds that one snapshot from the V2 switch block header, the same source initial() reads, then stores and returns it. Scope is narrow: only the gap block initial() owns is rebuilt, an empty masternode set is refused rather than stored (an empty snapshot loads back fine and would mask the hole permanently), and every other missing snapshot still returns an error. Measured over 5 fresh 3-node bootstraps: unpatched stalled 1/5, exactly the run that logged a snapshot miss; patched passed 5/5, with one run logging the rebuild -- the race fired and was absorbed without dropping a vote. Reverting snapshot.go makes TestGetSnapshotRebuildsBootstrapSnapshot fail with the production error, "getSnapshot() = not found". Ported to dev-upgrade: when the rebuild does not apply, getSnapshot still reports the miss as consensus.ErrGapSnapshotUnavailable. TestInitialFirstV2Block asserted that no snapshot exists before initial(), which this change deliberately reverses; it now expects the rebuilt gap 450 snapshot. (cherry picked from commit fc838407115741a8616c2c3e97851d46abd22889) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wgr523
requested review from
anunay-xin,
benjamin202410,
gzliudan and
wanwiset25
October 11, 2026 17:42
There was a problem hiding this comment.
🔵 Needs a closer look
The change affects concurrent consensus bootstrap behavior and warrants final human validation despite its focused tests.
0 open findings
What changed in this PR
Fixes a startup race that could prevent V2 consensus from forming the first quorum certificate.
Changes:
- Rebuilds the missing bootstrap snapshot from the V2 switch header.
- Rejects empty masternode snapshots.
- Adds unit and integration coverage for pre-initialization lookup behavior.
| File | Description |
|---|---|
consensus/XDPoS/engines/engine_v2/snapshot.go |
Adds bootstrap snapshot recovery. |
consensus/XDPoS/engines/engine_v2/snapshot_test.go |
Tests recovery boundaries and validation. |
consensus/tests/engine_v2_tests/initial_test.go |
Verifies on-demand recovery before initialization. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gzliudan
approved these changes
Oct 11, 2026
benjamin202410
approved these changes
Oct 13, 2026
anunay-xin
approved these changes
Oct 13, 2026
wanwiset25
approved these changes
Oct 15, 2026
wanwiset25
approved these changes
Oct 15, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
On a fresh chain the v2 engine is initialised lazily by the first proposal (via
verifyHeader), while the bft goroutine concurrently verifies votes for that block.VerifyVoteMessagetakes no lock, so it can callgetSnapshotbeforeinitial()has stored the V2 switch gap snapshot. The lookup misses, every vote for block 1 is dropped, no QC forms, and the chain stalls at height 1 permanently.getSnapshotnow rebuilds that one snapshot from the V2 switch block header (the same sourceinitial()uses), stores it and returns it. Scope is narrow:initial()owns is rebuilt; every other missing snapshot still errorsMeasured over 5 fresh 3-node bootstraps (private local net): unpatched stalled 1/5 (the run that logged a snapshot miss); patched passed 5/5, with one run logging the rebuild.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that