forked from ethereum/go-ethereum
-
Notifications
You must be signed in to change notification settings - Fork 70
fix(cicd,cmd,consensus/XDPoS,eth): fix fast sync #2272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
wgr523
wants to merge
23
commits into
dev-upgrade
Choose a base branch
from
fixfastsync-new
base: dev-upgrade
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
c0f15e9
feat: try to make full sync work. add log in syncState
wgr523 23fcba6
fix: when error getting snapshot, skip the error
wgr523 cee5d89
feat(downloader): add configurable pivot block for fast sync
wgr523 3724c25
chore: add fast sync args into docker entry start.sh
wgr523 e6bd9b1
style: more log about pivot
wgr523 144522d
feat: add FASTSYNC_PIVOT_ROOT, remove unnecessary verify
wgr523 0283f08
feat: re-add verify header for block after pivot
wgr523 7c90905
feat(downloader): sync multiple gap pivots during fast sync
wgr523 3493c0d
feat: move gap sync state after pivot sync state
wgr523 1d56bf3
feat: let processHeader wiat for state sync
wgr523 fbd8f9f
feat: add generateSnapshot in downloader for state sync
wgr523 ba6eb2d
feat: generate snapshot after state sync
wgr523 e55af5f
skip all header full verify during fast sync
wgr523 1845dab
chore: add fastsync args to devnet start.sh
wgr523 f499084
fix: statedb function, and better root choice in fast sync
wgr523 5b3204b
feat: when fullVerify=false, skip checkSignersOnCheckpoint
wgr523 d403c2b
fix: gen_config.go
wgr523 8f0705a
style: clean code comment
wgr523 4025516
feat: add fastsync args in start.sh
wgr523 5349447
Update cmd/utils/flags.go or better check on flags
wgr523 868149d
fix: use xdc_sort; better flags
wgr523 7b85d50
test: add fast sync pivot gap tests
wgr523 39d33bf
chore: require all three fastsync pivot params together in cicd start.sh
wgr523 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -179,6 +179,24 @@ var ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| Value: ethconfig.Defaults.SyncMode.String(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Category: flags.EthCategory, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FastSyncPivotNumberFlag = &cli.Uint64Flag{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Name: "fastsyncpivotnumber", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Usage: "Pivot block number for fast sync (0 = use default calculation)", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Value: 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Category: flags.EthCategory, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FastSyncPivotHashFlag = &cli.StringFlag{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Name: "fastsyncpivothash", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Usage: "Pivot block hash for fast sync verification (hex string, must be set if fastsyncpivotnumber is set)", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Value: "", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Category: flags.EthCategory, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FastSyncPivotRootFlag = &cli.StringFlag{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Name: "fastsyncpivotroot", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Usage: "State root of pivot block for fast sync state download (hex string, zero = use latest.Root)", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Value: "", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Category: flags.EthCategory, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| GCModeFlag = &cli.StringFlag{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Name: "gcmode", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Usage: `Blockchain garbage collection mode ("full", "archive")`, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1521,6 +1539,30 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| Fatalf("invalid --syncmode flag: %v", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pivotNumberSet := ctx.IsSet(FastSyncPivotNumberFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pivotHashSet := ctx.IsSet(FastSyncPivotHashFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pivotRootSet := ctx.IsSet(FastSyncPivotRootFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pivotHash := ctx.String(FastSyncPivotHashFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pivotRoot := ctx.String(FastSyncPivotRootFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if pivotNumberSet { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !pivotHashSet || pivotHash == "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Fatalf("--%s must be set if --%s is set", FastSyncPivotHashFlag.Name, FastSyncPivotNumberFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !pivotRootSet || pivotRoot == "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Fatalf("--%s must be set if --%s is set", FastSyncPivotRootFlag.Name, FastSyncPivotNumberFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cfg.FastSyncPivotNumber = ctx.Uint64(FastSyncPivotNumberFlag.Name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1549
to
+1555
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !pivotHashSet || pivotHash == "" { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotHashFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| if !pivotRootSet || pivotRoot == "" { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotRootFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| cfg.FastSyncPivotNumber = ctx.Uint64(FastSyncPivotNumberFlag.Name) | |
| pivotNumber := ctx.Uint64(FastSyncPivotNumberFlag.Name) | |
| if pivotNumber == 0 { | |
| Fatalf("--%s must be greater than 0 when explicitly set", FastSyncPivotNumberFlag.Name) | |
| } | |
| if !pivotHashSet || pivotHash == "" { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotHashFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| if !pivotRootSet || pivotRoot == "" { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotRootFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| cfg.FastSyncPivotNumber = pivotNumber |
Comment on lines
+1549
to
+1557
Copilot
AI
Apr 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pivot hash/root flags are parsed with common.HexToHash, which silently converts invalid hex strings into the zero hash (hex decoding errors are ignored in common.FromHex/Hex2Bytes). This can accidentally bypass pivot verification or state-root selection. Please validate the input as a 32-byte hex value (e.g., via hexutil.Decode/UnmarshalFixedText) and fail fast on parse/length errors; also reconcile the behavior/usage text that says the root can be zero with the current requirement that the flag must be non-empty.
Suggested change
| if !pivotHashSet || pivotHash == "" { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotHashFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| if !pivotRootSet || pivotRoot == "" { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotRootFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| cfg.FastSyncPivotNumber = ctx.Uint64(FastSyncPivotNumberFlag.Name) | |
| cfg.FastSyncPivotHash = common.HexToHash(pivotHash) | |
| cfg.FastSyncPivotRoot = common.HexToHash(pivotRoot) | |
| if !pivotHashSet { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotHashFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| if !pivotRootSet { | |
| Fatalf("--%s must be set if --%s is set", FastSyncPivotRootFlag.Name, FastSyncPivotNumberFlag.Name) | |
| } | |
| var parsedPivotHash common.Hash | |
| if err = parsedPivotHash.UnmarshalText([]byte(pivotHash)); err != nil { | |
| Fatalf("invalid --%s flag: %v", FastSyncPivotHashFlag.Name, err) | |
| } | |
| var parsedPivotRoot common.Hash | |
| if err = parsedPivotRoot.UnmarshalText([]byte(pivotRoot)); err != nil { | |
| Fatalf("invalid --%s flag: %v", FastSyncPivotRootFlag.Name, err) | |
| } | |
| cfg.FastSyncPivotNumber = ctx.Uint64(FastSyncPivotNumberFlag.Name) | |
| cfg.FastSyncPivotHash = parsedPivotHash | |
| cfg.FastSyncPivotRoot = parsedPivotRoot |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -243,32 +243,34 @@ func (x *XDPoS_v1) verifyCascadingFields(chain consensus.ChainReader, header *ty | |
| return x.verifySeal(chain, header, parents, fullVerify) | ||
| } | ||
|
|
||
| /* | ||
| BUG: snapshot returns wrong signers sometimes | ||
| when it happens we get the signers list by requesting smart contract | ||
| */ | ||
| // 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 { | ||
| return err | ||
| } | ||
| if fullVerify { | ||
| /* | ||
| BUG: snapshot returns wrong signers sometimes | ||
| when it happens we get the signers list by requesting smart contract | ||
| */ | ||
| // 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 { | ||
| return err | ||
| } | ||
|
|
||
| signers := snap.GetSigners() | ||
| err = x.checkSignersOnCheckpoint(chain, header, signers) | ||
| if err == nil { | ||
| return x.verifySeal(chain, header, parents, fullVerify) | ||
| } | ||
| signers := snap.GetSigners() | ||
| err = x.checkSignersOnCheckpoint(chain, header, signers) | ||
| if err == nil { | ||
| 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) | ||
| 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) | ||
| } | ||
| } | ||
|
|
||
| return err | ||
| return x.verifySeal(chain, header, parents, fullVerify) | ||
| } | ||
|
Comment on lines
+246
to
274
|
||
|
|
||
| func (x *XDPoS_v1) checkSignersOnCheckpoint(chain consensus.ChainReader, header *types.Header, signers []common.Address) error { | ||
|
|
||
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,33 +134,38 @@ func (x *XDPoS_v2) verifyHeader(chain consensus.ChainReader, header *types.Heade | |
| return utils.ErrInvalidCheckpointSigners | ||
| } | ||
|
|
||
| localMasterNodes, localPenalties, err := x.calcMasternodes(chain, header.Number, header.ParentHash, round) | ||
| masterNodes = localMasterNodes | ||
| if err != nil { | ||
| log.Error("[verifyHeader] Fail to calculate master nodes list with penalty", "Number", header.Number, "Hash", header.Hash()) | ||
| return err | ||
| } | ||
|
|
||
| validatorsAddress := common.ExtractAddressFromBytes(header.Validators) | ||
| if !utils.CompareSignersLists(localMasterNodes, validatorsAddress) { | ||
| for i, addr := range localMasterNodes { | ||
| log.Warn("[verifyHeader] localMasterNodes", "i", i, "addr", addr.Hex()) | ||
| // if fullVerify, verify masternodes and penalties; else use them inside header | ||
| if fullVerify { | ||
| localMasterNodes, localPenalties, err := x.calcMasternodes(chain, header.Number, header.ParentHash, round) | ||
| masterNodes = localMasterNodes | ||
| if err != nil { | ||
| log.Error("[verifyHeader] Fail to calculate master nodes list with penalty", "Number", header.Number, "Hash", header.Hash()) | ||
| return err | ||
| } | ||
| for i, addr := range validatorsAddress { | ||
| log.Warn("[verifyHeader] validatorsAddress", "i", i, "addr", addr.Hex()) | ||
| } | ||
| return utils.ErrValidatorsNotLegit | ||
| } | ||
|
|
||
| penaltiesAddress := common.ExtractAddressFromBytes(header.Penalties) | ||
| if !utils.CompareSignersLists(localPenalties, penaltiesAddress) { | ||
| for i, addr := range localPenalties { | ||
| log.Warn("[verifyHeader] localPenalties", "i", i, "addr", addr.Hex()) | ||
| validatorsAddress := common.ExtractAddressFromBytes(header.Validators) | ||
| if !utils.CompareSignersLists(localMasterNodes, validatorsAddress) { | ||
| for i, addr := range localMasterNodes { | ||
| log.Warn("[verifyHeader] localMasterNodes", "i", i, "addr", addr.Hex()) | ||
| } | ||
| for i, addr := range validatorsAddress { | ||
| log.Warn("[verifyHeader] validatorsAddress", "i", i, "addr", addr.Hex()) | ||
| } | ||
| return utils.ErrValidatorsNotLegit | ||
| } | ||
| for i, addr := range penaltiesAddress { | ||
| log.Warn("[verifyHeader] penaltiesAddress", "i", i, "addr", addr.Hex()) | ||
|
|
||
| penaltiesAddress := common.ExtractAddressFromBytes(header.Penalties) | ||
| if !utils.CompareSignersLists(localPenalties, penaltiesAddress) { | ||
| for i, addr := range localPenalties { | ||
| log.Warn("[verifyHeader] localPenalties", "i", i, "addr", addr.Hex()) | ||
| } | ||
| for i, addr := range penaltiesAddress { | ||
| log.Warn("[verifyHeader] penaltiesAddress", "i", i, "addr", addr.Hex()) | ||
| } | ||
| return utils.ErrPenaltiesNotLegit | ||
| } | ||
| return utils.ErrPenaltiesNotLegit | ||
| } else { | ||
| masterNodes = common.ExtractAddressFromBytes(header.Validators) | ||
| } | ||
|
Comment on lines
+137
to
169
|
||
| } else { | ||
| if len(header.Validators) != 0 { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both 3 parameters to be presented together? If yes we need to check them togethere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.