fix(eth): guard stalling-peer checks against missing local TD - #2523
fix(eth): guard stalling-peer checks against missing local TD#2523gzliudan wants to merge 1 commit into
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 Plus Run ID: 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 |
There was a problem hiding this comment.
Pull request overview
Prevents missing local total-difficulty entries from panicking Ethereum synchronization paths.
Changes:
- Adds nil-TD guards, one-time warnings, and metrics.
- Adds full, fast, and light synchronization coverage.
- Adds test helpers for legacy chaindata scenarios.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
eth/sync.go |
Guards protocol-manager TD comparisons. |
eth/sync_test.go |
Tests full and fast sync behavior. |
eth/metrics.go |
Adds protocol sync counter. |
eth/helper_test.go |
Adds passive and missing-TD test helpers. |
eth/handler.go |
Tracks one-time warning state. |
eth/downloader/metrics.go |
Adds downloader missing-TD counter. |
eth/downloader/downloader.go |
Guards stalling-peer TD comparisons. |
eth/downloader/downloader_test.go |
Tests downloader behavior across protocols and modes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if td == nil { | ||
| pm.warnMissingTd(currentBlock.Number.Uint64(), currentBlock.Hash()) | ||
| } else if pTd.Cmp(td) <= 0 { |
| if localTD == nil { | ||
| d.warnMissingTd(head.Number.Uint64(), head.Hash()) | ||
| return nil |
| head := blockchain.CurrentBlock() | ||
| rawdb.DeleteTd(db, head.Hash(), head.Number.Uint64()) |
…ises Legacy XDPoS chaindata predates the TD index, leaving the head without a total difficulty entry. Treating a missing TD as lower and entering the downloader was not panic-safe: a peer fork rooted below the current head passes the ErrUnknownAncestor check when the fork parent has a TD entry, and WriteHeader then compares externTd against a nil localTd. The full-block fork-choice paths dereference the current TD as well. Reconstruct the head TD before the sync threshold checks instead: BlockChain.RepairMissingTd walks the canonical parents down to the nearest ancestor with a known TD (or genesis) and persists the accumulated values in batches. synchronise repairs the block and snap heads, skipping the cycle if reconstruction fails. A still-missing local TD is treated as zero at the remaining fork-choice comparison sites as defense in depth. The downloader no longer skips the stalling-peer check when the local TD is missing: the unverifiable TD promise is conservatively treated as stalling, so a peer advertising an arbitrary TD without delivering headers is dropped instead of the node being marked synchronised. Add a missingtd_repaired counter and update the missing-head-TD tests to assert the repaired behavior.
|
This PR is incomplete. When completed, it will become complex and massive. It was replaced by #2524. |
Proposed changes
This PR solves the comment #2515 (comment).
GetTd returns nil when the TD entry is absent from the database (legacy XDPoS chaindata predating the TD index), which td.Cmp(nil) turns into a panic. Guard the stalling-peer checks in processHeaders and the sync threshold checks in ProtocolManager.synchronise: a missing TD skips the comparison and is reported via a counter metric, warning once per downloader lifetime.
The detection loss is bounded: header insertion rejects blocks whose parent TD is missing, so skipping the check cannot hide a peer that delivered an invalid chain.
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