fix(cicd,cmd,consensus/XDPoS,eth): fix fast sync#2272
Conversation
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix fast sync on XDPoSChain by making fast sync pivot selection configurable (number/hash/root), adding “gap pivot” state sync + snapshot generation, and adjusting consensus header verification behavior to better support fast-sync header insertion.
Changes:
- Add fast-sync pivot configuration to ethconfig (TOML + CLI flags) and wire it into the downloader at node startup.
- Extend the downloader to support a fixed pivot, optional pivot hash verification, gap-pivot state syncs, and snapshot generation.
- Adjust XDPoS v1/v2 header verification and epoch-switch info lookup to support reduced verification modes used during header-chain validation.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| eth/ethconfig/config.go | Adds FastSyncPivot* fields to the eth configuration struct. |
| eth/ethconfig/gen_config.go | Updates TOML marshal/unmarshal for the new fast-sync pivot fields. |
| eth/downloader/statesync.go | Removes an outdated comment about fast sync usage. |
| eth/downloader/downloader.go | Implements configurable pivot/gap pivot logic and snapshot generation; changes fast-sync header verification constants/behavior. |
| eth/backend.go | Wires configured pivot settings into the downloader during backend initialization. |
| consensus/XDPoS/engines/engine_v2/verifyHeader.go | Adjusts verification flow to use header-provided validators/penalties when not full-verifying. |
| consensus/XDPoS/engines/engine_v2/utils.go | Updates getEpochSwitchInfo call sites for new signature. |
| consensus/XDPoS/engines/engine_v2/timeout.go | Updates getEpochSwitchInfo call sites for new signature. |
| consensus/XDPoS/engines/engine_v2/snapshot.go | Exports snapshot constructor/storage helpers (NewSnapshot/StoreSnapshot). |
| consensus/XDPoS/engines/engine_v2/snapshot_test.go | Updates tests to use exported snapshot helpers. |
| consensus/XDPoS/engines/engine_v2/epochSwitch.go | Refactors getEpochSwitchInfo to accept parent-header slices for VerifyHeaders optimization and softens snapshot failure handling. |
| consensus/XDPoS/engines/engine_v2/engine.go | Updates snapshot usage and verifyQC/getEpochSwitchInfo interactions for new signatures/exports. |
| consensus/XDPoS/engines/engine_v1/engine.go | Skips checkpoint signer checks when not doing full verification. |
| cmd/XDC/main.go | Registers new CLI flags for fast-sync pivot configuration. |
| cmd/utils/flags.go | Defines and applies new fast-sync pivot flags into ethconfig. |
| cicd/testnet/start.sh | Adds environment-driven fast-sync pivot args. |
| cicd/mainnet/start.sh | Adds environment-driven fast-sync pivot args. |
| cicd/local/start.sh | Adds environment-driven fast-sync pivot args. |
| cicd/devnet/start.sh | Adds environment-driven fast-sync pivot args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b04ac82 to
7b85d50
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
eth/downloader/downloader.go:523
- When a fixed pivot is configured, syncWithPeer does not validate that pivotNumber is <= the remote height. If pivotNumber is greater than the chain head, header fetching can terminate while d.committed stays 0, potentially stalling fast sync. Consider validating
d.pivotNumber <= height(and alsod.pivotNumber > 0) up front and returning a configuration error if it's out of range.
if d.pivotNumber != 0 {
// Use configured pivot block
log.Info("Using configured pivot block", "number", d.pivotNumber)
pivot = d.pivotNumber
if pivot <= origin {
origin = pivot - 1
}
} else if height <= uint64(fsMinFullBlocks) {
origin = 0
} else {
pivot = height - uint64(fsMinFullBlocks)
if pivot <= origin {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add new CLI flags to configure a fixed pivot block for fast sync: - --fastsyncpivotnumber: Pivot block number (0 = use default calculation) - --fastsyncpivothash: Pivot block hash for verification Changes: - Add FastSyncPivotNumber and FastSyncPivotHash to ethconfig.Config - Add pivotNumber and pivotHash fields to Downloader struct - Add SetPivotBlock() method to configure pivot before sync - Use configured pivot in syncWithPeer instead of dynamic calculation - Prevent pivot block from moving during sync when configured - Verify pivot block header hash after state sync completes - Add state root verification after state sync completes This allows operators to sync from a specific trusted checkpoint block instead of relying on the dynamic pivot calculation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Changes: - Add pivotGapNumbers field to Downloader struct - Calculate gap pivot numbers in SetPivotBlock() - Sync gap pivot states in processFastSyncContent() before primary pivot
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| */ | ||
| // Retrieve the snapshot needed to verify this header and cache it | ||
| snap, err := x.snapshot(chain, number-1, header.ParentHash, parents, nil) | ||
| if err != nil { |
| err = x.checkSignersOnCheckpoint(chain, header, signers) | ||
| if err == nil { | ||
| return x.verifySeal(chain, header, parents, fullVerify) | ||
| } |
There was a problem hiding this comment.
add error log, you can put info or debug level since you already handled it
| return x.verifySeal(chain, header, parents, fullVerify) | ||
| signers, err = x.getSignersFromContract(chain, header) | ||
| if err != nil { | ||
| return err |
| err = x.checkSignersOnCheckpoint(chain, header, signers) | ||
| if err == nil { | ||
| return x.verifySeal(chain, header, parents, fullVerify) | ||
| } |
There was a problem hiding this comment.
what if it is still err != nil?
| func (x *XDPoS_v2) getEpochSwitchInfo(chain consensus.ChainReader, header *types.Header, hash common.Hash) (*types.EpochSwitchInfo, error) { | ||
| // headers is allow to be nil. if not nil, headers contain header (as last item) and its parents if any, which can be used to avoid | ||
| // fetching from the database during recursive lookups (useful during VerifyHeaders). | ||
| func (x *XDPoS_v2) getEpochSwitchInfo(chain consensus.ChainReader, headers []*types.Header, hash common.Hash) (*types.EpochSwitchInfo, error) { |
There was a problem hiding this comment.
need to discuss the headers []*types.Header input
| standbynodes = common.RemoveItemFromArray(standbynodes, penalties) | ||
| snap, err := x.getSnapshot(chain, h.Number.Uint64(), false) | ||
| if err != nil { | ||
| log.Warn("[getEpochSwitchInfo] Adaptor v2 getSnapshot has error, cannot get standbynodes", "err", err) |
There was a problem hiding this comment.
We have moved this from error to warn, we won't know if snapshot has error, is it expected behaviour?
| d.pivotGapNumbers = nil | ||
| for i := uint64(0); ; i++ { | ||
| gapNumber := baseGap + epoch*i | ||
| if gapNumber >= number { |
There was a problem hiding this comment.
pivot block is excluded from gap snapshot generation when it itself a gap block
Suppose pivot number is 1350, for i=1 gapnumber=1350 and number=1350 so pivotGapNumbers won't include 1350 as 1350>=1350, so snapshot won't exist for this
Please let me know if I am understanding this correctly
Proposed changes
Fix fast sync. I'll add a doc about how to do fast sync. Tested on private net and devnet.
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